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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Anthony,

Thanks for your review

> On 31 Mar 2023, at 15:23, Anthony PERARD <anthony.perard@xxxxxxxxxx> 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