Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Oleksandr <olekstysh@xxxxxxxxx>]

On 07.10.21 03:49, Stefano Stabellini wrote:


Hi Stefano

> On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > We need to pass info about maximum supported guest physical
> > address space size to the toolstack on Arm in order to properly
> > calculate the base and size of the extended region (safe range)
> > for the guest. The extended region is unused address space which
> > could be safely used by domain for foreign/grant mappings on Arm.
> > The extended region itself will be handled by the subsequent
> > patch.
> >
> > Currently the same guest physical address space size is used
> > for all guests.
> >
> > As we add new field to the structure bump the interface version.
> >
> > Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > ---
> > Changes RFC -> V2:
> >     - update patch subject/description
> >     - replace arch-specific sub-struct with common gpaddr_bits
> >       field and update code to reflect that
> >
> > Changes V2 -> V3:
> >     - make the field uint8_t and add uint8_t pad[7] after
> >     - remove leading blanks in libxl.h
> >
> > Changes V3 -> V4:
> >     - also print gpaddr_bits from output_physinfo()
> >     - add Michal's R-b
> >
> > Changes V4 -> V5:
> >     - update patch subject and description
> >     - drop Michal's R-b
> >     - pass gpaddr_bits via createdomain domctl
> >       (struct xen_arch_domainconfig)
> > ---
> >   tools/include/libxl.h            | 5 +++++
> >   tools/libs/light/libxl_arm.c     | 2 ++
> >   tools/libs/light/libxl_types.idl | 1 +
> >   xen/arch/arm/domain.c            | 6 ++++++
> >   xen/include/public/arch-arm.h    | 5 +++++
> >   xen/include/public/domctl.h      | 2 +-
> >   6 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> > index b9ba16d..33b4bfb 100644
> > --- a/tools/include/libxl.h
> > +++ b/tools/include/libxl.h
> > @@ -279,6 +279,11 @@
> >   #define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1
> >   
> >   /*
> > + * libxl_domain_build_info has the gpaddr_bits field.
> > + */
> > +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_GPADDR_BITS 1
> > +
> > +/*
> >    * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing
> >    * 'soft reset' for domains and there is 'soft_reset' shutdown reason
> >    * in enum libxl_shutdown_reason.
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index e3140a6..45e0386 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -123,6 +123,8 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
> >   
> >       state->clock_frequency = config->arch.clock_frequency;
> >   
> > +    d_config->b_info.arch_arm.gpaddr_bits = config->arch.gpaddr_bits;
> > +
> >       return 0;
> >   }
> >   
> > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> > index 3f9fff6..39482db 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >   
> >       ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> >                                  ("vuart", libxl_vuart_type),
> > +                               ("gpaddr_bits", uint8),
> >                                 ])),
> >       ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> >                                 ])),
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 19c756a..dfecc45 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -767,6 +767,12 @@ int arch_domain_create(struct domain *d,
> >       if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
> >           goto fail;
> >   
> > +    /*
> > +     * Pass maximum IPA bits to the toolstack, currently the same guest
> > +     * physical address space size is used for all guests.
> > +     */
> > +    config->arch.gpaddr_bits = p2m_ipa_bits;
> This could also be set in arch_sanitise_domain_config together with
> config->arch.gic_version. I prefer if it was done in
> arch_sanitise_domain_config but also here is OK I think.
I don't mind, being honest I had an idea to place this in 
arch_sanitise_domain_config(), but couldn't convince myself.

> Given that everything else looks fine:
>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Thanks!

Sadly, according to the recent discussion most likely this version is 
also no-go.

[snip]

--
Regards,

Oleksandr Tyshchenko