| Message ID | 20230327105944.1360856-11-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SVE feature for arm guests | expand |
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,
Hi Anthony, Thanks for your review > On 31 Mar 2023, at 15:23, Anthony PERARD <anthony.perard@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" > >> +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
On Tue, Apr 04, 2023 at 01:48:34PM +0000, Luca Fancellu wrote: > > On 31 Mar 2023, at 15:23, Anthony PERARD <anthony.perard@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,
Hi Anthony, >> >> 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. Yes while working on the patch I’ve found that I could declare this type in Libxl: libxl_sve_type = Enumeration("sve_type", [ (0, "disabled"), (128, "128"), (256, "256"), (384, "384"), (512, "512"), (640, "640"), (768, "768"), (896, "896"), (1024, "1024"), (1152, "1152"), (1280, "1280"), (1408, "1408"), (1536, "1536"), (1664, "1664"), (1792, "1792"), (1920, "1920"), (2048, "2048"), (-1, "hw") ], init_val = "LIBXL_SVE_TYPE_DISABLED”) So that in xl I can just use libxl_sve_type_from_string > >>> >>>> +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(). Right, I’ve seen it before but I was unsure if it was the right way, now that you suggested it, I will go for that. Thank you. Cheers, Luca
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 +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/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go index 35397be2f9e2..72a3a12a6065 100644 --- a/tools/golang/xenlight/helpers.gen.go +++ b/tools/golang/xenlight/helpers.gen.go @@ -1149,6 +1149,7 @@ default: return fmt.Errorf("invalid union key '%v'", x.Type)} x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version) x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart) +x.ArchArm.Sve = uint16(xc.arch_arm.sve) if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil { return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err) } @@ -1653,6 +1654,7 @@ default: return fmt.Errorf("invalid union key '%v'", x.Type)} xc.arch_arm.gic_version = C.libxl_gic_version(x.ArchArm.GicVersion) xc.arch_arm.vuart = C.libxl_vuart_type(x.ArchArm.Vuart) +xc.arch_arm.sve = C.uint16_t(x.ArchArm.Sve) if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil { return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err) } diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go index 3d968a496744..3dc292b5f1be 100644 --- a/tools/golang/xenlight/types.gen.go +++ b/tools/golang/xenlight/types.gen.go @@ -564,6 +564,7 @@ TypeUnion DomainBuildInfoTypeUnion ArchArm struct { GicVersion GicVersion Vuart VuartType +Sve uint16 } ArchX86 struct { MsrRelaxed Defbool diff --git a/tools/include/libxl.h b/tools/include/libxl.h index cfa1a191318c..9f040316ad80 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -283,6 +283,11 @@ */ #define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1 +/* + * libxl_domain_build_info has the arch_arm.sve field. + */ +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_SVE 1 + /* * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing * 'soft reset' for domains and there is 'soft_reset' shutdown reason 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; + 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), ])), ("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> #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); + } 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 */ + b_info->arch_arm.sve = l / 128U; + } + + libxl_physinfo_dispose(&physinfo); + parse_vkb_list(config, d_config); d_config->virtios = NULL;