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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Anthony,

Thank you for your review.

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

> 
>> +    }
>> +
>>     if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
>>         return 0;
>> 
>> diff --git a/tools/libs/light/libxl_types.idl 
>> b/tools/libs/light/libxl_types.idl
>> index fd31dacf7d5a..9e48bb772646 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -523,6 +523,27 @@ libxl_tee_type = Enumeration("tee_type", [
>>     (1, "optee")
>>     ], init_val = "LIBXL_TEE_TYPE_NONE")
>> 
>> +libxl_sve_type = Enumeration("sve_type", [
>> +    (-1, "hw"),
>> +    (0, "disabled"),
>> +    (128, "128"),
>> +    (256, "256"),
>> +    (384, "384"),
>> +    (512, "512"),
>> +    (640, "640"),
>> +    (768, "768"),
>> +    (896, "896"),
>> +    (1024, "1024"),
>> +    (1152, "1152"),
>> +    (1280, "1280"),
>> +    (1408, "1408"),
>> +    (1536, "1536"),
>> +    (1664, "1664"),
>> +    (1792, "1792"),
>> +    (1920, "1920"),
>> +    (2048, "2048")
>> +    ], init_val = "LIBXL_SVE_TYPE_DISABLED")
> 
> I'm not sure if I like that or not. Is there a reason to stop at 2048?
> It is possible that there will be more value available in the future?

Uhm... possibly there might be some extension, I thought that when it will
be the case, the only thing to do was to add another entry, I used this way
also to have for free the checks on the %128 and maximum 2048.

> 
> Also this mean that users of libxl (like libvirt) would be supposed to
> use LIBXL_SVE_TYPE_1024 for e.g., or use libxl_sve_type_from_string().
> 
> Also, it feels weird to me to mostly use numerical value of the enum
> rather than the enum itself.
> 
> Anyway, hopefully that enum will work fine.
> 
>> libxl_rdm_reserve = Struct("rdm_reserve", [
>>     ("strategy",    libxl_rdm_reserve_strategy),
>>     ("policy",      libxl_rdm_reserve_policy),
> 
> Thanks,
> 
> -- 
> Anthony PERARD