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