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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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