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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 20 Mar 2023, at 09:09, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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?

At the beginning I thought it was more useful the plain format, but probably
It’s better to keep that structure small. I’ll change it.

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

Ok I’ll change them

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

Ack

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