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?
> +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.
> +
> 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.
> ])),
> ("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?
> #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".
> + } 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.
> + b_info->arch_arm.sve = l / 128U;
> + }
> +
> + libxl_physinfo_dispose(&physinfo);
> +
> parse_vkb_list(config, d_config);
>
> d_config->virtios = NULL;
Thanks,
--
Anthony PERARD
