> On 20 Mar 2023, at 09:09, Jan Beulich <jbeul...@suse.com> 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
