Hi Anthony,
Thank you for your review.
> On 2 May 2023, at 18:06, Anthony PERARD <anthony.per...@citrix.com> 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
