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