Re: [XEN][PATCH 2/8] xen/arm: move vcpu_switch_to_aarch64_mode() in arch_vcpu_create()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 23/07/2025 11:19, Grygorii Strashko wrote:
> On 23.07.25 12:16, Julien Grall wrote:
> > On 23/07/2025 08:58, Grygorii Strashko wrote:
> > > From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> > >
> > > Move vcpu_switch_to_aarch64_mode() in arch_vcpu_create() callback 
> > > instead
> > > of calling it manually from few different places after vcpu_create().
> > >
> > > Before doing above ensure vcpu0 is created after kernel_probe() is 
> > > done and
> > > domain's guest execution mode (32-bit/64-bit) is set for dom0 and 
> > > dom0less
> > > domains.
> > The commit message doesn't mention anything about domains created by 
> > the toolstack. In this case, from my understanding, the switch to 64- 
> > bit domain happens *after* the vCPUs are created.
> > At the moment, I think this is probably ok to call...
> >
> > > Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> > > ---
> > >   xen/arch/arm/domain.c                    |  3 +++
> > >   xen/arch/arm/domain_build.c              | 13 +++++--------
> > >   xen/common/device-tree/dom0less-build.c  |  6 +++---
> > >   xen/include/asm-generic/dom0less-build.h |  2 +-
> > >   4 files changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > index 79a144e61be9..bbd4a764c696 100644
> > > --- a/xen/arch/arm/domain.c
> > > +++ b/xen/arch/arm/domain.c
> > > @@ -586,6 +586,9 @@ int arch_vcpu_create(struct vcpu *v)
> > >       if ( get_ssbd_state() == ARM_SSBD_RUNTIME )
> > >           v->arch.cpu_info->flags |= CPUINFO_WORKAROUND_2_FLAG;
> > > +    if ( is_64bit_domain(v->domain) )
> > > +        vcpu_switch_to_aarch64_mode(v);
> > ... this function here because I *think* it would be NOP. But this 
> > feels really fragile.
> The toolstack configures domain and vcpus through 
> XEN_DOMCTL_set_address_size on Arm64:
> - toolstack creates domain and parses kernel binary: domain created with 
> DOMAIN_32BIT mode by default
> - toolstack creates vcpus (still 32 bit mode): libxl__build_pre()- 
>  >xc_domain_max_vcpus()
> - toolstack switches domain mode depending on kernel binary type: 
> libxl__build_dom()->xc_dom_boot_mem_init(),
>    which triggers XEN_DOMCTL_set_address_size hypercall.
>    Xen: arm64: switches domain mode and re-configures vcpus: 
> subarch_do_domctl()->set_address_size()
> So, this patch does not affect toolstack path, only optimizes Xen boots 
> a bit. 
Thanks for providing more detaisl. I am not sure what you mean by 
optimize. It reduces the number of places where 
vcpu_switch_to_aarch64_mode() is called, but there should be no 
difference in term of boot time.
> Also, during Xen boot or by toolstack - the domain is always created 
> before it's type is even known, which, in turn,
> is based on guest binary which is parsed later during domain 
> configuration stage.
What you are describing is the current situation. But this doesn't tell 
me *why* we can't provide the type when the domain is created.
> I can add note in commit message "This patch doesn't affect on the 
> toolstack Arm64 domain creation path as toolstack always
> re-configures domain mode and vcpus through XEN_DOMCTL_set_address_size 
> hypercall during domain configuration stage"
Well, as I wrote before, I find this code extremely fragile. And you so 
far, you don't seem to have address this concern in your reply. In fact...
> > If the desire is to make 32-bit domain optional on Arm64. Then I think 
> > it would be better to pass the domain type when the domain
> > is created (IOW add an extra flags to XEN_DOMCTL_createdomain). This 
> > will require more work, but it will be a lot more robust.
... I proposed what I think is a better alternative. Did you consider it?

Cheers,

--
Julien Grall