Re: [PATCH v5 07/12] xen: enable Dom0 to use SVE feature

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 20 Apr 2023, at 13:29, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> On 20/04/2023 09:46, Luca Fancellu wrote:
>>>>>>> +int __init sve_sanitize_vl_param(int val, unsigned int *out)
>>>>>>> +{
>>>>>>> +    /*
>>>>>>> +     * Negative SVE parameter value means to use the maximum supported
>>>>>>> +     * vector length, otherwise if a positive value is provided, check 
>>>>>>> if the
>>>>>>> +     * vector length is a multiple of 128 and not bigger than the 
>>>>>>> maximum value
>>>>>>> +     * 2048
>>>>>>> +     */
>>>>>>> +    if ( val < 0 )
>>>>>>> +        *out = get_sys_vl_len();
>>>>>>> +    else if ( ((val % SVE_VL_MULTIPLE_VAL) == 0) && (val <= 
>>>>>>> SVE_VL_MAX_BITS) )
>>>>>>> +        *out = val;
>>>>>> 
>>>>>> Shouldn't you also check if it is not greater than the maximum vector 
>>>>>> length ?
>>>>> 
>>>>> I don’t understand, I am checking that the value is below or equal to 
>>>>> SVE_VL_MAX_BITS,
>>>>> If you mean if it should be checked also against the maximum supported 
>>>>> length by the platform,
>>>>> I think this is not the right place, the check is already in 
>>>>> arch_sanitise_domain_config(), introduced
>>>>> in patch #2
>>>> 
>>>> If this is not the right place to check it then why checking the rest here 
>>>> ?
>>>> 
>>>> From a user or a developer point of view I would expect the validity of 
>>>> the input to be checked only
>>>> in one place.
>>>> If here is not the place for that it is ok but then i would check 
>>>> everything in arch_sanitise_domain_config
>>>> (multiple, min and supported) instead of doing it partly in 2 places.
>>> 
>>> Ok, given the way we encoded the value in xen_domctl_createdomain 
>>> structure, we have that the value takes
>>> very little space, but a small issue is that when we encode it, we are 
>>> dividing it by 128, which is fine for user params
>>> that are multiple of 128, but it’s less fine if the user passes “129”.
>>> 
>>> To overcome this issue we are checking the value when it is not already 
>>> encoded. Now, thinking about it, the check
>>> "&& (val <= SVE_VL_MAX_BITS)” is not really needed, because even if the 
>>> value is above, then in arch_sanitise_domain_config
>>> we will hit the top limit of the platform maximum VL.
>>> 
>>> 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 )
>>>    {
>>>        dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
>>>                config->flags);
>>>        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;
>>>        }
>>>    }
>>>   [...]
>>> 
>>> Now, I understand your point, we could check everything in 
>>> sve_sanitize_vl_param(), but it would leave a problem
>>> for domains created by hypercalls if I am not wrong.
>>> 
>>> What do you think?
>> I thought about that and another possibility is to store “sve_vl” as 
>> uint16_t inside struct xen_arch_domainconfig, and
>> check it inside arch_sanitise_domain_config() for it to be mod 128 and less 
>> than the max supported VL, this will
>> allow to have all the checks in one place, taking a bit more space, anyway 
>> we would take the space from the implicit
>> padding as this is the current status:

Hi Julien,

> 
> Sorry, I am having trouble to follow the discussion. If you are checking the 
> value in arch_sanitise_domain_config(), then why do you need to take more 
> space in arch_domain?

Yes I am checking the value in arch_sanitise_domain_config, the value checked 
is from arch_domain and it is the vector length divided by 128, so an encoded 
value.

Bertrand was puzzled by the fact that I also put a check in 
sve_sanitize_vl_param to see if the vector length passed by the user is mod 
128, his point is that we should check a value only in one place and not in 
two, and it is a valid point but in this case we can’t put all the check into 
arch_sanitise_domain_config because we don’t have the “full” value from 
arch_domain, and we can’t put all the checks in sve_sanitize_vl_param because 
it will leave out from the check domains created by hyper calls.

So to have only one point where the checks are done (mod 128 and <= HW 
supported VL), we might need to have a full resolution VL value in struct 
xen_arch_domainconfig (16 bit).

But if we want to save space for the future, we could leave the code as it is 
and rely on the fact that the tools (or Xen) when creating the domain 
configuration, will check that the SVE VL parameter is mod 128.
In this last case what is in struct xen_arch_domainconfig is implicitly mod 128 
and only the upper boundary of the max supported VL will be checked by Xen 
inside arch_sanitise_domain_config.

> 
> Cheers,
> 
> -- 
> Julien Grall