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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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;