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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Apr 04, 2023 at 01:48:34PM +0000, Luca Fancellu wrote:
> > 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"

Well, it's a config file, not a C ABI, so max allowed here doesn't have to be
spelled "-1", it could also be "max", "max-allowed",
"max-size-supported", ... So fill free deviate from the restricted C
ABI. But "-1" works as long as it's the only allowed negative number.

> > 
> >> +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?

Yeah, there might be a "better" way. I think me suggestion to check the
sve value here was wrong. I still want to have it checked in libxl, but
it might be better to do that in the previous step, that is
"libxl__domain_config_setdefault". libxl__arch_domain_build_info_setdefault()
will have `physinfo` so you won't have to call xc_physinfo().


Regarding the default, libxl is capable of selecting a good set of
option, depending on the underling hardware. So is it possible that in
the future we would want to enable SVE by default? If that's even a
remote possibility, the current API wouldn't allow it because we have
"default" and "disable" been the same.

Since we want to add a max VL supported option, maybe we want to
separate the default and disable options. So we could keep the
single field `libxl_domain_build_info.arch_arm.sve` and have for example
"-1" for max-supported and "-2" for disabled, while "0" mean default.
Or alternatively, add an extra field libxl_defbool (can be
default/true/false) and keep the second one for VL. (That extra
"disabled" option would only be for libxl, for xl we can keep "sve=0"
mean disable, and the missing option just mean default.)

In any case, libxl__arch_domain_build_info_setdefault() will check
`b_info.arch_arm.sve` and set it to the value that can given to Xen. And
as of now, libxl__arch_domain_prepare_config() will just copy the value
from `b_info` to `config`. (I guess that last step _prepare_config()
could still do the division by 128.)


Thanks,

-- 
Anthony PERARD