> On 20 Apr 2023, at 14:08, Julien Grall <jul...@xen.org> wrote:
>
> Hi Luca,
>
> On 20/04/2023 13:43, Luca Fancellu wrote:
>>> On 20 Apr 2023, at 13:29, Julien Grall <jul...@xen.org> 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.
>
> I think this is where the disconnect is coming from.
> arch_sanitise_domain_config() doesn't use struct arch_domain because the
> domain itself is not yet allocated. Instead, it is using
> xen_arch_domainconfig.
>
> I care less about the increase of xen_arch_domainconfig because this is a one
> off structure.
I’m sorry, yes, I meant struct xen_domctl_createdomain, sorry I got confused
copying from this thread
>
>> 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.
>
> Before answering to the approach, can you explain why you ask the user to
> pass a value that is a multiple of 128 rather than the already "divided"
> value? Is it more natural for the user?
Yes I thought is was more natural for the user to think about number of bits
(for example 512) instead of an encoded value (4 in this case).
>
> Cheers,
>
> --
> Julien Grall
