diff mbox series

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

Message ID 20230412094938.2693890-3-luca.fancellu@arm.com (mailing list archive)
State Superseded
Headers show
Series SVE feature for arm guests | expand

Commit Message

Luca Fancellu April 12, 2023, 9:49 a.m. UTC
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(-)

Comments

Bertrand Marquis April 13, 2023, 12:47 p.m. UTC | #1
Hi Luca,

> On 12 Apr 2023, at 11:49, Luca Fancellu <Luca.Fancellu@arm.com> 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.
> 

Please mention in the commit message that you are bumping
domctl interface version.

> 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>

With the commit message fixed:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> 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;
> +        }
> +    }
> +
>     /* 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;
> +
>     /* 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.
> -- 
> 2.34.1
>
Julien Grall April 13, 2023, 12:57 p.m. UTC | #2
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,
Luca Fancellu April 13, 2023, 1:24 p.m. UTC | #3
Hi Julien,


>>  @@ -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.

No SVE is not supported for 32 bit guests, here I think we will get “SVE is unsupported on this machine” because get_sys_vl_len() will return 0.

I can however put everything inside #ifdef CONFIG_ARM64_SVE or CONFIG_ARM_64 if you prefer

>> 
>> 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?

Yes, I’ll move it and protect with CONFIG_ARM_64, is it ok for you if I move it after:

/* Monitor options */
struct {
    uint8_t privileged_call_enabled : 1;
} monitor;
Julien Grall April 13, 2023, 1:30 p.m. UTC | #4
On 13/04/2023 14:24, Luca Fancellu wrote:
> Hi Julien,

Hi Luca,

>>>   @@ -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.
> 
> No SVE is not supported for 32 bit guests, here I think we will get “SVE is unsupported on this machine” because get_sys_vl_len() will return 0.

 From my understanding, get_sys_vl_len() will return the len supported 
by the hosts. So if you run a 32-bit guest on top of a 64-bit hosts, 
then I believe get_sys_vl_len() will be non-zero.

>> 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?
> 
> Yes, I’ll move it and protect with CONFIG_ARM_64, is it ok for you if I move it after:
> 
> /* Monitor options */
> struct {
>      uint8_t privileged_call_enabled : 1;
> } monitor;

Please check the padding with "pahole". If possible, it would be better 
to re-use an existing one.

Cheers,
Luca Fancellu April 13, 2023, 2:05 p.m. UTC | #5
> On 13 Apr 2023, at 14:30, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 13/04/2023 14:24, Luca Fancellu wrote:
>> Hi Julien,
> 
> Hi Luca,
> 
>>>>  @@ -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.
>> No SVE is not supported for 32 bit guests, here I think we will get “SVE is unsupported on this machine” because get_sys_vl_len() will return 0.
> 
> From my understanding, get_sys_vl_len() will return the len supported by the hosts. So if you run a 32-bit guest on top of a 64-bit hosts, then I believe get_sys_vl_len() will be non-zero.

Yes you are right, I realise that I need the domain type information and I can’t have it in arch_sanitise_domain_config, however they might have sense there, and I can do a check
like this afterwards:

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c1f0d1d78431..ce1235c25769 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3694,6 +3694,12 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
         return -EINVAL;
     }
 
+    if ( d->arch.sve_vl && (kinfo->type == DOMAIN_32BIT) )
+    {
+        printk("SVE is not available for 32-bit domain\n");
+        return -EINVAL;
+    }
+
     if ( is_64bit_domain(d) )
         vcpu_switch_to_aarch64_mode(v);

Would it be ok for you?


> 
>>> 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?
>> Yes, I’ll move it and protect with CONFIG_ARM_64, is it ok for you if I move it after:
>> /* Monitor options */
>> struct {
>>     uint8_t privileged_call_enabled : 1;
>> } monitor;
> 
> Please check the padding with "pahole". If possible, it would be better to re-use an existing one.

Ok I’ll try to use the tool

> 
> Cheers,
> 
> -- 
> Julien Grall
Luca Fancellu April 13, 2023, 4:10 p.m. UTC | #6
> 
> 
>> 
>>>> 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?
>>> Yes, I’ll move it and protect with CONFIG_ARM_64, is it ok for you if I move it after:
>>> /* Monitor options */
>>> struct {
>>>    uint8_t privileged_call_enabled : 1;
>>> } monitor;
>> 
>> Please check the padding with "pahole". If possible, it would be better to re-use an existing one.
> 
> Ok I’ll try to use the tool

I’ve managed to use the tool, the field seems already in a good spot:

struct arch_domain {
	enum domain_type           type;                 /*     0     4 */
	uint8_t                    sve_vl;               /*     4     1 */

	/* XXX 3 bytes hole, try to pack */

	struct p2m_domain          p2m;                  /*     8   328 */
	/* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
	struct hvm_domain          hvm;                  /*   336   312 */
	/* --- cacheline 10 boundary (640 bytes) was 8 bytes ago --- */
	struct paging_domain       paging;               /*   648    32 */
	struct vmmio               vmmio;                /*   680    32 */
	/* --- cacheline 11 boundary (704 bytes) was 8 bytes ago --- */
	unsigned int               rel_priv;             /*   712     4 */

	/* XXX 4 bytes hole, try to pack */

	struct {
		uint64_t           offset;               /*   720     8 */
		s_time_t           nanoseconds;          /*   728     8 */
	} virt_timer_base;                               /*   720    16 */
	struct vgic_dist           vgic;                 /*   736   200 */

	/* XXX last struct has 2 bytes of padding */

	/* --- cacheline 14 boundary (896 bytes) was 40 bytes ago --- */
	struct vuart               vuart;                /*   936    32 */
	/* --- cacheline 15 boundary (960 bytes) was 8 bytes ago --- */
	unsigned int               evtchn_irq;           /*   968     4 */
	struct {
		uint8_t            privileged_call_enabled:1; /*   972: 0  1 */
	} monitor;                                       /*   972     1 */

	/* XXX 3 bytes hole, try to pack */

	struct vpl011              vpl011;               /*   976    72 */

	/* size: 1152, cachelines: 18, members: 13 */
	/* sum members: 1038, holes: 3, sum holes: 10 */
	/* padding: 104 */
	/* paddings: 1, sum paddings: 2 */
} __attribute__((__aligned__(128)));

> 
>> 
>> Cheers,
>> 
>> -- 
>> Julien Grall
Julien Grall April 13, 2023, 7:52 p.m. UTC | #7
Hi Luca,

On 13/04/2023 15:05, Luca Fancellu wrote:
> 
> 
>> On 13 Apr 2023, at 14:30, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 13/04/2023 14:24, Luca Fancellu wrote:
>>> Hi Julien,
>>
>> Hi Luca,
>>
>>>>>   @@ -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.
>>> No SVE is not supported for 32 bit guests, here I think we will get “SVE is unsupported on this machine” because get_sys_vl_len() will return 0.
>>
>>  From my understanding, get_sys_vl_len() will return the len supported by the hosts. So if you run a 32-bit guest on top of a 64-bit hosts, then I believe get_sys_vl_len() will be non-zero.
> 
> Yes you are right, I realise that I need the domain type information and I can’t have it in arch_sanitise_domain_config, however they might have sense there, and I can do a check
> like this afterwards:
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c1f0d1d78431..ce1235c25769 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3694,6 +3694,12 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
>           return -EINVAL;
>       }
>   
> +    if ( d->arch.sve_vl && (kinfo->type == DOMAIN_32BIT) )
> +    {
> +        printk("SVE is not available for 32-bit domain\n");
> +        return -EINVAL;
> +    }
> +
>       if ( is_64bit_domain(d) )
>           vcpu_switch_to_aarch64_mode(v);
> 
> Would it be ok for you?

construct_domain() is only going to be used for domains created by Xen. 
You would need the same check for the ones created by the toolstack.

Do you need to know the SVE length when the domain is created? If not, 
then I would suggest to create a new domctl that would be called after 
we switch the domain to 32-bit.

Cheers,
Julien Grall April 13, 2023, 7:53 p.m. UTC | #8
Hi Luca,

On 13/04/2023 17:10, Luca Fancellu wrote:
>>
>>
>>>
>>>>> 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?
>>>> Yes, I’ll move it and protect with CONFIG_ARM_64, is it ok for you if I move it after:
>>>> /* Monitor options */
>>>> struct {
>>>>     uint8_t privileged_call_enabled : 1;
>>>> } monitor;
>>>
>>> Please check the padding with "pahole". If possible, it would be better to re-use an existing one.
>>
>> Ok I’ll try to use the tool
> 
> I’ve managed to use the tool, the field seems already in a good spot:

ok. Thanks for checking.

Cheers,
Luca Fancellu April 14, 2023, 11:07 a.m. UTC | #9
> On 13 Apr 2023, at 20:52, Julien Grall <julien@xen.org> wrote:
> 
> Hi Luca,
> 
> On 13/04/2023 15:05, Luca Fancellu wrote:
>>> On 13 Apr 2023, at 14:30, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 13/04/2023 14:24, Luca Fancellu wrote:
>>>> Hi Julien,
>>> 
>>> Hi Luca,
>>> 
>>>>>>  @@ -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.
>>>> No SVE is not supported for 32 bit guests, here I think we will get “SVE is unsupported on this machine” because get_sys_vl_len() will return 0.
>>> 
>>> From my understanding, get_sys_vl_len() will return the len supported by the hosts. So if you run a 32-bit guest on top of a 64-bit hosts, then I believe get_sys_vl_len() will be non-zero.
>> Yes you are right, I realise that I need the domain type information and I can’t have it in arch_sanitise_domain_config, however they might have sense there, and I can do a check
>> like this afterwards:
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index c1f0d1d78431..ce1235c25769 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3694,6 +3694,12 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
>>          return -EINVAL;
>>      }
>>  +    if ( d->arch.sve_vl && (kinfo->type == DOMAIN_32BIT) )
>> +    {
>> +        printk("SVE is not available for 32-bit domain\n");
>> +        return -EINVAL;
>> +    }
>> +
>>      if ( is_64bit_domain(d) )
>>          vcpu_switch_to_aarch64_mode(v);
>> Would it be ok for you?
> 
> construct_domain() is only going to be used for domains created by Xen. You would need the same check for the ones created by the toolstack.
> 
> Do you need to know the SVE length when the domain is created? If not, then I would suggest to create a new domctl that would be called after we switch the domain to 32-bit.

Hi Julien,

Yes that’s true, we would like to prevent who is using hyper calls to have guests with SVE but 32 bits, I think that with this check it’s possible to avoid them:

diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
index 0de89b42c448..b7189e8dbbb5 100644
--- a/xen/arch/arm/arm64/domctl.c
+++ b/xen/arch/arm/arm64/domctl.c
@@ -43,6 +43,9 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
         case 32:
             if ( !cpu_has_el1_32 )
                 return -EINVAL;
+            /* SVE is not supported for 32 bit domain */
+            if ( is_sve_domain(d) )
+                return -EOPNOTSUPP;
             return switch_mode(d, DOMAIN_32BIT);
         case 64:
             return switch_mode(d, DOMAIN_64BIT);

It’s a bit late in the guest creation, but we don’t have the domain type information before, this check together with the check above in construct_domain would
be enough.

What do you think?

> 
> Cheers,
> 
> -- 
> Julien Grall
Julien Grall April 14, 2023, 5:16 p.m. UTC | #10
Hi Luca,

On 14/04/2023 12:07, Luca Fancellu wrote:
> 
> 
>> On 13 Apr 2023, at 20:52, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Luca,
>>
>> On 13/04/2023 15:05, Luca Fancellu wrote:
>>>> On 13 Apr 2023, at 14:30, Julien Grall <julien@xen.org> wrote:
>>>>
>>>>
>>>>
>>>> On 13/04/2023 14:24, Luca Fancellu wrote:
>>>>> Hi Julien,
>>>>
>>>> Hi Luca,
>>>>
>>>>>>>   @@ -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.
>>>>> No SVE is not supported for 32 bit guests, here I think we will get “SVE is unsupported on this machine” because get_sys_vl_len() will return 0.
>>>>
>>>>  From my understanding, get_sys_vl_len() will return the len supported by the hosts. So if you run a 32-bit guest on top of a 64-bit hosts, then I believe get_sys_vl_len() will be non-zero.
>>> Yes you are right, I realise that I need the domain type information and I can’t have it in arch_sanitise_domain_config, however they might have sense there, and I can do a check
>>> like this afterwards:
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index c1f0d1d78431..ce1235c25769 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -3694,6 +3694,12 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
>>>           return -EINVAL;
>>>       }
>>>   +    if ( d->arch.sve_vl && (kinfo->type == DOMAIN_32BIT) )
>>> +    {
>>> +        printk("SVE is not available for 32-bit domain\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>>       if ( is_64bit_domain(d) )
>>>           vcpu_switch_to_aarch64_mode(v);
>>> Would it be ok for you?
>>
>> construct_domain() is only going to be used for domains created by Xen. You would need the same check for the ones created by the toolstack.
>>
>> Do you need to know the SVE length when the domain is created? If not, then I would suggest to create a new domctl that would be called after we switch the domain to 32-bit.
> 
> Hi Julien,
> 
> Yes that’s true, we would like to prevent who is using hyper calls to have guests with SVE but 32 bits, I think that with this check it’s possible to avoid them:
> 
> diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> index 0de89b42c448..b7189e8dbbb5 100644
> --- a/xen/arch/arm/arm64/domctl.c
> +++ b/xen/arch/arm/arm64/domctl.c
> @@ -43,6 +43,9 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>           case 32:
>               if ( !cpu_has_el1_32 )
>                   return -EINVAL;
> +            /* SVE is not supported for 32 bit domain */
> +            if ( is_sve_domain(d) )
> +                return -EOPNOTSUPP;
>               return switch_mode(d, DOMAIN_32BIT);
>           case 64:
>               return switch_mode(d, DOMAIN_64BIT);
> 
> It’s a bit late in the guest creation, but we don’t have the domain type information before, this check together with the check above in construct_domain would
> be enough.
> 
> What do you think?

I would be OK with this approach for now. In the longer term, we 
probably want to consider to set the mode when the domain is created 
because this can't change at runtime.

Cheers,
Jan Beulich April 17, 2023, 9:20 a.m. UTC | #11
On 13.04.2023 14:47, Bertrand Marquis wrote:
>> On 12 Apr 2023, at 11:49, Luca Fancellu <Luca.Fancellu@arm.com> 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.
>>
> 
> Please mention in the commit message that you are bumping
> domctl interface version.

In which case please not just "that", but also why.

Jan
diff mbox series

Patch

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;
+        }
+    }
+
     /* 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;
+
     /* 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.