Hi Anthony,
Thanks for your review
> On 31 Mar 2023, at 15:23, Anthony PERARD <anthony.per...@citrix.com> wrote:
>
> On Mon, Mar 27, 2023 at 11:59:42AM +0100, Luca Fancellu wrote:
>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>> index 10f37990be57..adf48fe8ac1d 100644
>> --- a/docs/man/xl.cfg.5.pod.in
>> +++ b/docs/man/xl.cfg.5.pod.in
>> @@ -2952,6 +2952,17 @@ Currently, only the "sbsa_uart" model is supported
>> for ARM.
>>
>> =back
>>
>> +=item B<sve="NUMBER">
>> +
>> +To enable SVE, user must specify a number different from zero, maximum 2048
>> and
>> +multiple of 128. That value will be the maximum number of SVE registers bits
>> +that the hypervisor will impose to this guest. If the platform has a lower
>
> Maybe start by describing what the "sve" value is before imposing
> limits. Maybe something like:
>
> Set the maximum vector length that a guest's Scalable Vector
> Extension (SVE) can use. Or disable it by specifying 0, the default.
>
> Value needs to be a multiple of 128, with a maximum of 2048 or the
> maximum supported by the platform.
>
> Would this, or something like that be a good explanation of the "sve"
> configuration option?
Yes I can change it, a need to do it anyway because I think also here, the
suggestion
From Jan can apply and we could pass a negative value that means “max VL
supported
by the platform"
>
>> +supported bits value, then the domain creation will fail.
>> +A value equal to zero is the default and it means this guest is not allowed
>> to
>> +use SVE.
>> +
>> +=back
>> +
>> =head3 x86
>>
>> =over 4
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index ddc7b2a15975..16a49031fd51 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -211,6 +211,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>> return ERROR_FAIL;
>> }
>>
>> + config->arch.sve_vl = d_config->b_info.arch_arm.sve;
>
> This truncate a 16bit value into an 8bit value, I think you should check
> that the value can actually fit.
>
> And maybe check `d_config->b_info.arch_arm.sve` value here instead of
> `xl` as commented later.
Yes I can do it, one question, can I use here xc_physinfo to retrieve the
maximum
Vector length from arch_capabilities?
I mean, is there a better way or I can go for that?
>
>> +
>> return 0;
>> }
>>
>> diff --git a/tools/libs/light/libxl_types.idl
>> b/tools/libs/light/libxl_types.idl
>> index fd31dacf7d5a..ef4a8358e54e 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -690,6 +690,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>
>> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>> ("vuart", libxl_vuart_type),
>> + ("sve", uint16),
>
> I wonder if renaming "sve" to "sve_vl" here would make sense, seeing
> that "sve_vl" is actually used in other places.
Yes I can rename it as sve_vl, I will also change the type to “integer"
>
>> ])),
>> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>> ])),
>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> index 1f6f47daf4e1..3cbc23b36952 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -12,6 +12,7 @@
>> * GNU Lesser General Public License for more details.
>> */
>>
>> +#include <arm-arch-capabilities.h>
>
> Could you add this headers after public ones?
Yes
>
>> #include <ctype.h>
>> #include <inttypes.h>
>> #include <limits.h>
>> @@ -1312,8 +1313,6 @@ void parse_config_data(const char *config_source,
>> exit(EXIT_FAILURE);
>> }
>>
>> - libxl_physinfo_dispose(&physinfo);
>> -
>> config= xlu_cfg_init(stderr, config_source);
>> if (!config) {
>> fprintf(stderr, "Failed to allocate for configuration\n");
>> @@ -2887,6 +2886,29 @@ skip_usbdev:
>> }
>> }
>>
>> + if (!xlu_cfg_get_long (config, "sve", &l, 0)) {
>> + unsigned int arm_sve_vl =
>> + arch_capabilities_arm_sve(physinfo.arch_capabilities);
>> + if (!arm_sve_vl) {
>> + fprintf(stderr, "SVE is not supported by the platform\n");
>> + exit(-ERROR_FAIL);
>
> "ERROR_FAIL" is a "libxl_error", instead exit with "EXIT_FAILURE".
Ok I will use the right type
>
>> + } else if (((l % 128) != 0) || (l > 2048)) {
>> + fprintf(stderr,
>> + "Invalid sve value: %ld. Needs to be <= 2048 and
>> multiple"
>> + " of 128\n", l);
>> + exit(-ERROR_FAIL);
>> + } else if (l > arm_sve_vl) {
>> + fprintf(stderr,
>> + "Invalid sve value: %ld. Platform supports up to %u
>> bits\n",
>> + l, arm_sve_vl);
>> + exit(-ERROR_FAIL);
>> + }
>> + /* Vector length is divided by 128 in domain configuration struct */
>
> That's wrong, beside this comment there's nothing that say that
> `b_info->arch_arm.sve` needs to have a value divided by 128.
> `b_info->arch_arm.sve` is just of type uint16_t (libxl_type.idl).
>
> BTW, "tools/xl" (xl) use just one user of "tools/libs/light" (libxl), so
> it's possible that other users would set `sve` to a value that haven't
> been checked. So I think all the check that the `sve` value is correct
> could be done in libxl instead.
Sure I will do that
>
>
>> + b_info->arch_arm.sve = l / 128U;
>> + }
>> +
>> + libxl_physinfo_dispose(&physinfo);
>> +
>> parse_vkb_list(config, d_config);
>>
>> d_config->virtios = NULL;
>
> Thanks,
>
> --
> Anthony PERARD
