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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 13 Apr 2023, at 20:52, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> On 13/04/2023 15:05, Luca Fancellu wrote:
>>> On 13 Apr 2023, at 14:30, Julien Grall <julien@xxxxxxx> 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