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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.03.2023 14:19, Luca Fancellu wrote:
> @@ -744,6 +773,9 @@ int arch_domain_create(struct domain *d,
>      if ( (rc = domain_vpci_init(d)) != 0 )
>          goto fail;
>  
> +    /* Copy and decode sve_vl from the domain configuration */
> +    d->arch.sve_vl_bits = domainconfig_decode_vl(config->arch.sve_vl);

Considering that you now "encode" and "decode" the value when coming in /
going out for a hypercall, wouldn't it make sense to also have in internally
stored value in the same more compact format?

> --- a/xen/arch/arm/include/asm/arm64/sve.h
> +++ b/xen/arch/arm/include/asm/arm64/sve.h
> @@ -13,10 +13,23 @@
>  /* Vector length must be multiple of 128 */
>  #define SVE_VL_MULTIPLE_VAL (128U)
>  
> +static inline bool is_vl_valid(uint16_t vl)
> +{
> +    /* SVE vector length is multiple of 128 and maximum 2048 */
> +    return ((vl % SVE_VL_MULTIPLE_VAL) == 0) && (vl <= SVE_VL_MAX_BITS);
> +}
> +
> +static inline uint16_t domainconfig_decode_vl(uint8_t 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(uint16_t vl);
> +uint16_t get_sys_vl_len(void);
>  
>  #else /* !CONFIG_ARM64_SVE */
>  
> @@ -30,6 +43,11 @@ static inline register_t vl_to_zcr(uint16_t vl)
>      return 0;
>  }
>  
> +static inline uint16_t get_sys_vl_len(void)
> +{
> +    return 0;
> +}

Throughout here: Style - please limit the use of fixed width types to
cases where they're actually necessary to use to achieve a certain
effect (see ./CODING_STYLE). None of the cases above look to match that
criteria, merely ...

> @@ -114,6 +116,9 @@ struct arch_domain
>      void *tee;
>  #endif
>  
> +    /* max SVE vector length in bits */
> +    uint16_t sve_vl_bits;

... this may be justified (for space efficiency), while ...

> +
>  }  __cacheline_aligned;

(nit: stray insertion of a blank line)

> --- 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 the public interface it's of course mandatory to use.

Jan