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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

Given that everything else looks fine:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


>      return 0;
>  
>  fail:
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 6b5a5f8..4a01f8b 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
>       *
>       */
>      uint32_t clock_frequency;
> +    /*
> +     * OUT
> +     * Guest physical address space size
> +     */
> +    uint8_t gpaddr_bits;
>  };
>  #endif /* __XEN__ || __XEN_TOOLS__ */
>  
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 96696e3..f37586e 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
>  
>  /*
>   * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> -- 
> 2.7.4
>