Re: [PATCH v6 10/12] xen/tools: add sve parameter in XL configuration
> On 5 May 2023, at 17:23, Anthony PERARD wrote:
>
> On Tue, May 02, 2023 at 07:54:19PM +, Luca Fancellu wrote:
>>> On 2 May 2023, at 18:06, Anthony PERARD 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
Re: [PATCH v6 10/12] xen/tools: add sve parameter in XL configuration
On Tue, May 02, 2023 at 07:54:19PM +, Luca Fancellu wrote:
> > On 2 May 2023, at 18:06, Anthony PERARD 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. 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
Re: [PATCH v6 10/12] xen/tools: add sve parameter in XL configuration
Hi Anthony,
Thank you for your review.
> On 2 May 2023, at 18:06, Anthony PERARD 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
Re: [PATCH v6 10/12] xen/tools: add sve parameter in XL configuration
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.
> +}
> +
> 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?
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
