Re: [PATCH v5 02/12] xen/arm: add SVE vector length field to the domain

[Julien Grall <julien@xen.org>]

Hi,

On 12/04/2023 10:49, Luca Fancellu wrote:
> Add sve_vl field to arch_domain and xen_arch_domainconfig struct,
> to allow the domain to have an information about the SVE feature
> and the number of SVE register bits that are allowed for this
> domain.
> 
> sve_vl field is the vector length in bits divided by 128, this
> allows to use less space in the structures.
> 
> The field is used also to allow or forbid a domain to use SVE,
> because a value equal to zero means the guest is not allowed to
> use the feature.
> 
> Check that the requested vector length is lower or equal to the
> platform supported vector length, otherwise fail on domain
> creation.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> Changes from v4:
>   - Return 0 in get_sys_vl_len() if sve is not supported, code style fix,
>     removed else if since the conditions can't fallthrough, removed not
>     needed condition checking for VL bits validity because it's already
>     covered, so delete is_vl_valid() function. (Jan)
> Changes from v3:
>   - don't use fixed types when not needed, use encoded value also in
>     arch_domain so rename sve_vl_bits in sve_vl. (Jan)
>   - rename domainconfig_decode_vl to sve_decode_vl because it will now
>     be used also to decode from arch_domain value
>   - change sve_vl from uint16_t to uint8_t and move it after "type" field
>     to optimize space.
> Changes from v2:
>   - rename field in xen_arch_domainconfig from "sve_vl_bits" to
>     "sve_vl" and use the implicit padding after gic_version to
>     store it, now this field is the VL/128. (Jan)
>   - Created domainconfig_decode_vl() function to decode the sve_vl
>     field and use it as plain bits value inside arch_domain.
>   - Changed commit message reflecting the changes
> Changes from v1:
>   - no changes
> Changes from RFC:
>   - restore zcr_el2 in sve_restore_state, that will be introduced
>     later in this serie, so remove zcr_el2 related code from this
>     patch and move everything to the later patch (Julien)
>   - add explicit padding into struct xen_arch_domainconfig (Julien)
>   - Don't lower down the vector length, just fail to create the
>     domain. (Julien)
> ---
>   xen/arch/arm/arm64/sve.c             | 12 ++++++++++++
>   xen/arch/arm/domain.c                | 27 +++++++++++++++++++++++++++
>   xen/arch/arm/include/asm/arm64/sve.h | 12 ++++++++++++
>   xen/arch/arm/include/asm/domain.h    |  5 +++++
>   xen/include/public/arch-arm.h        |  2 ++
>   xen/include/public/domctl.h          |  2 +-
>   6 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
> index 6f3fb368c59b..78f7482619da 100644
> --- a/xen/arch/arm/arm64/sve.c
> +++ b/xen/arch/arm/arm64/sve.c
> @@ -6,6 +6,7 @@
>    */
>   
>   #include <xen/types.h>
> +#include <asm/cpufeature.h>
>   #include <asm/arm64/sve.h>
>   #include <asm/arm64/sysregs.h>
>   #include <asm/processor.h>
> @@ -48,3 +49,14 @@ register_t vl_to_zcr(unsigned int vl)
>       ASSERT(vl > 0);
>       return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK;
>   }
> +
> +/* Get the system sanitized value for VL in bits */
> +unsigned int get_sys_vl_len(void)
> +{
> +    if ( !cpu_has_sve )
> +        return 0;
> +
> +    /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */
> +    return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) *
> +            SVE_VL_MULTIPLE_VAL;
> +}
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index adb6ace2e24d..769fae8fe25e 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -13,6 +13,7 @@
>   #include <xen/wait.h>
>   
>   #include <asm/alternative.h>
> +#include <asm/arm64/sve.h>
>   #include <asm/cpuerrata.h>
>   #include <asm/cpufeature.h>
>   #include <asm/current.h>
> @@ -550,6 +551,8 @@ int arch_vcpu_create(struct vcpu *v)
>       v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);
>   
>       v->arch.cptr_el2 = get_default_cptr_flags();
> +    if ( is_sve_domain(v->domain) )
> +        v->arch.cptr_el2 &= ~HCPTR_CP(8);
>   
>       v->arch.hcr_el2 = get_default_hcr_flags();
>   
> @@ -594,6 +597,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>       unsigned int max_vcpus;
>       unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap);
>       unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu);
> +    unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl);
>   
>       if ( (config->flags & ~flags_optional) != flags_required )
>       {
> @@ -602,6 +606,26 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>           return -EINVAL;
>       }
>   
> +    /* Check feature flags */
> +    if ( sve_vl_bits > 0 )
> +    {
> +        unsigned int zcr_max_bits = get_sys_vl_len();
> +
> +        if ( !zcr_max_bits )
> +        {
> +            dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n");
> +            return -EINVAL;
> +        }
> +
> +        if ( sve_vl_bits > zcr_max_bits )
> +        {
> +            dprintk(XENLOG_INFO,
> +                    "Requested SVE vector length (%u) > supported length (%u)\n",
> +                    sve_vl_bits, zcr_max_bits);
> +            return -EINVAL;
> +        }

Is SVE supported for 32-bit guest? If not, then you should had a check 
here to prevent the creation of the domain if sve_vl_bits is set.

> +    }
> +
>       /* The P2M table must always be shared between the CPU and the IOMMU */
>       if ( config->iommu_opts & XEN_DOMCTL_IOMMU_no_sharept )
>       {
> @@ -744,6 +768,9 @@ int arch_domain_create(struct domain *d,
>       if ( (rc = domain_vpci_init(d)) != 0 )
>           goto fail;
>   
> +    /* Copy the encoded vector length sve_vl from the domain configuration */
> +    d->arch.sve_vl = config->arch.sve_vl;
> +
>       return 0;
>   
>   fail:
> diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h
> index 144d2b1cc485..a4c53e3e8e2e 100644
> --- a/xen/arch/arm/include/asm/arm64/sve.h
> +++ b/xen/arch/arm/include/asm/arm64/sve.h
> @@ -13,10 +13,17 @@
>   /* Vector length must be multiple of 128 */
>   #define SVE_VL_MULTIPLE_VAL (128U)
>   
> +static inline unsigned int sve_decode_vl(unsigned int sve_vl)
> +{
> +    /* SVE vector length is stored as VL/128 in xen_arch_domainconfig */
> +    return sve_vl * SVE_VL_MULTIPLE_VAL;
> +}
> +
>   #ifdef CONFIG_ARM64_SVE
>   
>   register_t compute_max_zcr(void);
>   register_t vl_to_zcr(unsigned int vl);
> +unsigned int get_sys_vl_len(void);
>   
>   #else /* !CONFIG_ARM64_SVE */
>   
> @@ -30,6 +37,11 @@ static inline register_t vl_to_zcr(unsigned int vl)
>       return 0;
>   }
>   
> +static inline unsigned int get_sys_vl_len(void)
> +{
> +    return 0;
> +}
> +
>   #endif /* CONFIG_ARM64_SVE */
>   
>   #endif /* _ARM_ARM64_SVE_H */
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index e776ee704b7d..78cc2da3d4e5 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -31,6 +31,8 @@ enum domain_type {
>   
>   #define is_domain_direct_mapped(d) ((d)->cdf & CDF_directmap)
>   
> +#define is_sve_domain(d) ((d)->arch.sve_vl > 0)
> +
>   /*
>    * Is the domain using the host memory layout?
>    *
> @@ -67,6 +69,9 @@ struct arch_domain
>       enum domain_type type;
>   #endif
>   
> +    /* max SVE encoded vector length */
> +    uint8_t sve_vl;
> +
Can we move this somewhere else to avoid adding extra padding? Also 
shouldn't this be protected with #ifdef CONFIG_ARM_64 to make clear this 
is not supported on Xen 32-bit?

>       /* Virtual MMU */
>       struct p2m_domain p2m;
>   
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 1528ced5097a..38311f559581 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -300,6 +300,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>   struct xen_arch_domainconfig {
>       /* IN/OUT */
>       uint8_t gic_version;
> +    /* IN - Contains SVE vector length divided by 128 */
> +    uint8_t sve_vl;
>       /* IN */
>       uint16_t tee_type;
>       /* IN */
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 529801c89ba3..e2e22cb534d6 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -21,7 +21,7 @@
>   #include "hvm/save.h"
>   #include "memory.h"
>   
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016
>   
>   /*
>    * NB. xen_domctl.domain is an IN/OUT parameter for this operation.

Cheers,

-- 
Julien Grall