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


> 
>>> 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;
> 
> Please check the padding with "pahole". If possible, it would be better to 
> re-use an existing one.

Ok I’ll try to use the tool

> 
> Cheers,
> 
> -- 
> Julien Grall