Re: [PATCH v6 10/12] xen/tools: add sve parameter in XL configuration

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 5 May 2023, at 17:23, Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
> 
> On Tue, May 02, 2023 at 07:54:19PM +0000, Luca Fancellu wrote:
>>> On 2 May 2023, at 18:06, Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
>>> On Mon, Apr 24, 2023 at 07:02:46AM +0100, Luca Fancellu wrote:
>>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>>> index ddc7b2a15975..1e69dac2c4fa 100644
>>>> --- a/tools/libs/light/libxl_arm.c
>>>> +++ b/tools/libs/light/libxl_arm.c
>>>> @@ -211,6 +213,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>>>        return ERROR_FAIL;
>>>>    }
>>>> 
>>>> +    /* Parameter is sanitised in libxl__arch_domain_build_info_setdefault 
>>>> */
>>>> +    if (d_config->b_info.arch_arm.sve_vl) {
>>>> +        /* Vector length is divided by 128 in struct 
>>>> xen_domctl_createdomain */
>>>> +        config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
>>>> +    }
>>>> +
>>>>    return 0;
>>>> }
>>>> 
>>>> @@ -1681,6 +1689,26 @@ int 
>>>> libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>>>>    /* ACPI is disabled by default */
>>>>    libxl_defbool_setdefault(&b_info->acpi, false);
>>>> 
>>>> +    /* Sanitise SVE parameter */
>>>> +    if (b_info->arch_arm.sve_vl) {
>>>> +        unsigned int max_sve_vl =
>>>> +            arch_capabilities_arm_sve(physinfo->arch_capabilities);
>>>> +
>>>> +        if (!max_sve_vl) {
>>>> +            LOG(ERROR, "SVE is unsupported on this machine.");
>>>> +            return ERROR_FAIL;
>>>> +        }
>>>> +
>>>> +        if (LIBXL_SVE_TYPE_HW == b_info->arch_arm.sve_vl) {
>>>> +            b_info->arch_arm.sve_vl = max_sve_vl;
>>>> +        } else if (b_info->arch_arm.sve_vl > max_sve_vl) {
>>>> +            LOG(ERROR,
>>>> +                "Invalid sve value: %d. Platform supports up to %u bits",
>>>> +                b_info->arch_arm.sve_vl, max_sve_vl);
>>>> +            return ERROR_FAIL;
>>>> +        }
>>> 
>>> You still need to check that sve_vl is one of the value from the enum,
>>> or that the value is divisible by 128.
>> 
>> I have probably missed something, I thought that using the way below to
>> specify the input I had for free that the value is 0 or divisible by 128, is 
>> it
>> not the case? Who can write to b_info->arch_arm.sve_vl different value
>> from the enum we specified in the .idl?
> 
> `xl` isn't the only user of `libxl`. There's `libvirt` as well. We also
> have libxl bindings for several languages.

Right, this point wasn’t clear to me, I will add the check there, thank you
for the explanation.

> There's nothing stopping a
> developer to write a number into `sve_vl` instead of choosing one of the
> value from the enum. I think we should probably sanitize any input that
> comes from outside of libxl, that's probably the case, I'm not sure.
> 
> So if valid values for `sve_vl` are only numbers divisible by 128, and
> some other discrete numbers, then we should check that they are, or
> check that the value is one defined by the enum.
> 
> Cheers,
> 
> -- 
> Anthony PERARD