| Message ID | 20230424060248.1488859-11-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SVE feature for arm guests | expand |
On Mon, Apr 24, 2023 at 07:02:46AM +0100, Luca Fancellu wrote: > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index ddc7b2a15975..1e69dac2c4fa 100644 > --- a/tools/libs/light/libxl_arm.c > +++ b/tools/libs/light/libxl_arm.c > @@ -211,6 +213,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > return ERROR_FAIL; > } > > + /* Parameter is sanitised in libxl__arch_domain_build_info_setdefault */ > + if (d_config->b_info.arch_arm.sve_vl) { > + /* Vector length is divided by 128 in struct xen_domctl_createdomain */ > + config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U; > + } > + > return 0; > } > > @@ -1681,6 +1689,26 @@ int libxl__arch_domain_build_info_setdefault(libxl__gc *gc, > /* ACPI is disabled by default */ > libxl_defbool_setdefault(&b_info->acpi, false); > > + /* Sanitise SVE parameter */ > + if (b_info->arch_arm.sve_vl) { > + unsigned int max_sve_vl = > + arch_capabilities_arm_sve(physinfo->arch_capabilities); > + > + if (!max_sve_vl) { > + LOG(ERROR, "SVE is unsupported on this machine."); > + return ERROR_FAIL; > + } > + > + if (LIBXL_SVE_TYPE_HW == b_info->arch_arm.sve_vl) { > + b_info->arch_arm.sve_vl = max_sve_vl; > + } else if (b_info->arch_arm.sve_vl > max_sve_vl) { > + LOG(ERROR, > + "Invalid sve value: %d. Platform supports up to %u bits", > + b_info->arch_arm.sve_vl, max_sve_vl); > + return ERROR_FAIL; > + } You still need to check that sve_vl is one of the value from the enum, or that the value is divisible by 128. > + } > + > if (b_info->type != LIBXL_DOMAIN_TYPE_PV) > return 0; > > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > index fd31dacf7d5a..9e48bb772646 100644 > --- a/tools/libs/light/libxl_types.idl > +++ b/tools/libs/light/libxl_types.idl > @@ -523,6 +523,27 @@ libxl_tee_type = Enumeration("tee_type", [ > (1, "optee") > ], init_val = "LIBXL_TEE_TYPE_NONE") > > +libxl_sve_type = Enumeration("sve_type", [ > + (-1, "hw"), > + (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") > + ], init_val = "LIBXL_SVE_TYPE_DISABLED") I'm not sure if I like that or not. Is there a reason to stop at 2048? It is possible that there will be more value available in the future? Also this mean that users of libxl (like libvirt) would be supposed to use LIBXL_SVE_TYPE_1024 for e.g., or use libxl_sve_type_from_string(). Also, it feels weird to me to mostly use numerical value of the enum rather than the enum itself. Anyway, hopefully that enum will work fine. > libxl_rdm_reserve = Struct("rdm_reserve", [ > ("strategy", libxl_rdm_reserve_strategy), > ("policy", libxl_rdm_reserve_policy), Thanks,
Hi Anthony, Thank you for your review. > On 2 May 2023, at 18:06, Anthony PERARD <anthony.perard@citrix.com> wrote: > > On Mon, Apr 24, 2023 at 07:02:46AM +0100, Luca Fancellu wrote: >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c >> index ddc7b2a15975..1e69dac2c4fa 100644 >> --- a/tools/libs/light/libxl_arm.c >> +++ b/tools/libs/light/libxl_arm.c >> @@ -211,6 +213,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >> return ERROR_FAIL; >> } >> >> + /* Parameter is sanitised in libxl__arch_domain_build_info_setdefault */ >> + if (d_config->b_info.arch_arm.sve_vl) { >> + /* Vector length is divided by 128 in struct xen_domctl_createdomain */ >> + config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U; >> + } >> + >> return 0; >> } >> >> @@ -1681,6 +1689,26 @@ int libxl__arch_domain_build_info_setdefault(libxl__gc *gc, >> /* ACPI is disabled by default */ >> libxl_defbool_setdefault(&b_info->acpi, false); >> >> + /* Sanitise SVE parameter */ >> + if (b_info->arch_arm.sve_vl) { >> + unsigned int max_sve_vl = >> + arch_capabilities_arm_sve(physinfo->arch_capabilities); >> + >> + if (!max_sve_vl) { >> + LOG(ERROR, "SVE is unsupported on this machine."); >> + return ERROR_FAIL; >> + } >> + >> + if (LIBXL_SVE_TYPE_HW == b_info->arch_arm.sve_vl) { >> + b_info->arch_arm.sve_vl = max_sve_vl; >> + } else if (b_info->arch_arm.sve_vl > max_sve_vl) { >> + LOG(ERROR, >> + "Invalid sve value: %d. Platform supports up to %u bits", >> + b_info->arch_arm.sve_vl, max_sve_vl); >> + return ERROR_FAIL; >> + } > > You still need to check that sve_vl is one of the value from the enum, > or that the value is divisible by 128. I have probably missed something, I thought that using the way below to specify the input I had for free that the value is 0 or divisible by 128, is it not the case? Who can write to b_info->arch_arm.sve_vl different value from the enum we specified in the .idl? > >> + } >> + >> if (b_info->type != LIBXL_DOMAIN_TYPE_PV) >> return 0; >> >> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl >> index fd31dacf7d5a..9e48bb772646 100644 >> --- a/tools/libs/light/libxl_types.idl >> +++ b/tools/libs/light/libxl_types.idl >> @@ -523,6 +523,27 @@ libxl_tee_type = Enumeration("tee_type", [ >> (1, "optee") >> ], init_val = "LIBXL_TEE_TYPE_NONE") >> >> +libxl_sve_type = Enumeration("sve_type", [ >> + (-1, "hw"), >> + (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") >> + ], init_val = "LIBXL_SVE_TYPE_DISABLED") > > I'm not sure if I like that or not. Is there a reason to stop at 2048? > It is possible that there will be more value available in the future? Uhm... possibly there might be some extension, I thought that when it will be the case, the only thing to do was to add another entry, I used this way also to have for free the checks on the %128 and maximum 2048. > > Also this mean that users of libxl (like libvirt) would be supposed to > use LIBXL_SVE_TYPE_1024 for e.g., or use libxl_sve_type_from_string(). > > Also, it feels weird to me to mostly use numerical value of the enum > rather than the enum itself. > > Anyway, hopefully that enum will work fine. > >> libxl_rdm_reserve = Struct("rdm_reserve", [ >> ("strategy", libxl_rdm_reserve_strategy), >> ("policy", libxl_rdm_reserve_policy), > > Thanks, > > -- > Anthony PERARD
On Tue, May 02, 2023 at 07:54:19PM +0000, Luca Fancellu wrote: > > On 2 May 2023, at 18:06, Anthony PERARD <anthony.perard@citrix.com> wrote: > > On Mon, Apr 24, 2023 at 07:02:46AM +0100, Luca Fancellu wrote: > >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > >> index ddc7b2a15975..1e69dac2c4fa 100644 > >> --- a/tools/libs/light/libxl_arm.c > >> +++ b/tools/libs/light/libxl_arm.c > >> @@ -211,6 +213,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > >> return ERROR_FAIL; > >> } > >> > >> + /* Parameter is sanitised in libxl__arch_domain_build_info_setdefault */ > >> + if (d_config->b_info.arch_arm.sve_vl) { > >> + /* Vector length is divided by 128 in struct xen_domctl_createdomain */ > >> + config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U; > >> + } > >> + > >> return 0; > >> } > >> > >> @@ -1681,6 +1689,26 @@ int libxl__arch_domain_build_info_setdefault(libxl__gc *gc, > >> /* ACPI is disabled by default */ > >> libxl_defbool_setdefault(&b_info->acpi, false); > >> > >> + /* Sanitise SVE parameter */ > >> + if (b_info->arch_arm.sve_vl) { > >> + unsigned int max_sve_vl = > >> + arch_capabilities_arm_sve(physinfo->arch_capabilities); > >> + > >> + if (!max_sve_vl) { > >> + LOG(ERROR, "SVE is unsupported on this machine."); > >> + return ERROR_FAIL; > >> + } > >> + > >> + if (LIBXL_SVE_TYPE_HW == b_info->arch_arm.sve_vl) { > >> + b_info->arch_arm.sve_vl = max_sve_vl; > >> + } else if (b_info->arch_arm.sve_vl > max_sve_vl) { > >> + LOG(ERROR, > >> + "Invalid sve value: %d. Platform supports up to %u bits", > >> + b_info->arch_arm.sve_vl, max_sve_vl); > >> + return ERROR_FAIL; > >> + } > > > > You still need to check that sve_vl is one of the value from the enum, > > or that the value is divisible by 128. > > I have probably missed something, I thought that using the way below to > specify the input I had for free that the value is 0 or divisible by 128, is it > not the case? Who can write to b_info->arch_arm.sve_vl different value > from the enum we specified in the .idl? `xl` isn't the only user of `libxl`. There's `libvirt` as well. We also have libxl bindings for several languages. There's nothing stopping a developer to write a number into `sve_vl` instead of choosing one of the value from the enum. I think we should probably sanitize any input that comes from outside of libxl, that's probably the case, I'm not sure. So if valid values for `sve_vl` are only numbers divisible by 128, and some other discrete numbers, then we should check that they are, or check that the value is one defined by the enum. Cheers,
> On 5 May 2023, at 17:23, Anthony PERARD <anthony.perard@citrix.com> wrote: > > On Tue, May 02, 2023 at 07:54:19PM +0000, Luca Fancellu wrote: >>> On 2 May 2023, at 18:06, Anthony PERARD <anthony.perard@citrix.com> wrote: >>> On Mon, Apr 24, 2023 at 07:02:46AM +0100, Luca Fancellu wrote: >>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c >>>> index ddc7b2a15975..1e69dac2c4fa 100644 >>>> --- a/tools/libs/light/libxl_arm.c >>>> +++ b/tools/libs/light/libxl_arm.c >>>> @@ -211,6 +213,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >>>> return ERROR_FAIL; >>>> } >>>> >>>> + /* Parameter is sanitised in libxl__arch_domain_build_info_setdefault */ >>>> + if (d_config->b_info.arch_arm.sve_vl) { >>>> + /* Vector length is divided by 128 in struct xen_domctl_createdomain */ >>>> + config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U; >>>> + } >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -1681,6 +1689,26 @@ int libxl__arch_domain_build_info_setdefault(libxl__gc *gc, >>>> /* ACPI is disabled by default */ >>>> libxl_defbool_setdefault(&b_info->acpi, false); >>>> >>>> + /* Sanitise SVE parameter */ >>>> + if (b_info->arch_arm.sve_vl) { >>>> + unsigned int max_sve_vl = >>>> + arch_capabilities_arm_sve(physinfo->arch_capabilities); >>>> + >>>> + if (!max_sve_vl) { >>>> + LOG(ERROR, "SVE is unsupported on this machine."); >>>> + return ERROR_FAIL; >>>> + } >>>> + >>>> + if (LIBXL_SVE_TYPE_HW == b_info->arch_arm.sve_vl) { >>>> + b_info->arch_arm.sve_vl = max_sve_vl; >>>> + } else if (b_info->arch_arm.sve_vl > max_sve_vl) { >>>> + LOG(ERROR, >>>> + "Invalid sve value: %d. Platform supports up to %u bits", >>>> + b_info->arch_arm.sve_vl, max_sve_vl); >>>> + return ERROR_FAIL; >>>> + } >>> >>> You still need to check that sve_vl is one of the value from the enum, >>> or that the value is divisible by 128. >> >> I have probably missed something, I thought that using the way below to >> specify the input I had for free that the value is 0 or divisible by 128, is it >> not the case? Who can write to b_info->arch_arm.sve_vl different value >> from the enum we specified in the .idl? > > `xl` isn't the only user of `libxl`. There's `libvirt` as well. We also > have libxl bindings for several languages. Right, this point wasn’t clear to me, I will add the check there, thank you for the explanation. > There's nothing stopping a > developer to write a number into `sve_vl` instead of choosing one of the > value from the enum. I think we should probably sanitize any input that > comes from outside of libxl, that's probably the case, I'm not sure. > > So if valid values for `sve_vl` are only numbers divisible by 128, and > some other discrete numbers, then we should check that they are, or > check that the value is one defined by the enum. > > Cheers, > > -- > Anthony PERARD
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index 10f37990be57..2ea996caa256 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -2952,6 +2952,22 @@ Currently, only the "sbsa_uart" model is supported for ARM. =back +=item B<sve="vl"> + +The `sve` parameter enables Arm Scalable Vector Extension (SVE) usage for the +guest and sets the maximum SVE vector length, the option is applicable only to +AArch64 guests. +A value equal to "disabled" disables the feature, this is the default value. +Allowed values are "disabled", "128", "256", "384", "512", "640", "768", "896", +"1024", "1152", "1280", "1408", "1536", "1664", "1792", "1920", "2048", "hw". +Specifying "hw" means that the maximum vector length supported by the platform +will be used. +Please be aware that if a specific vector length is passed and its value is +above the maximum vector length supported by the platform, an error will be +raised. + +=back + =head3 x86 =over 4 diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go index 35397be2f9e2..cd1a16e32eac 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.SveVl = SveType(xc.arch_arm.sve_vl) 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_vl = C.libxl_sve_type(x.ArchArm.SveVl) 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..b131a7eedc9d 100644 --- a/tools/golang/xenlight/types.gen.go +++ b/tools/golang/xenlight/types.gen.go @@ -490,6 +490,28 @@ TeeTypeNone TeeType = 0 TeeTypeOptee TeeType = 1 ) +type SveType int +const( +SveTypeHw SveType = -1 +SveTypeDisabled SveType = 0 +SveType128 SveType = 128 +SveType256 SveType = 256 +SveType384 SveType = 384 +SveType512 SveType = 512 +SveType640 SveType = 640 +SveType768 SveType = 768 +SveType896 SveType = 896 +SveType1024 SveType = 1024 +SveType1152 SveType = 1152 +SveType1280 SveType = 1280 +SveType1408 SveType = 1408 +SveType1536 SveType = 1536 +SveType1664 SveType = 1664 +SveType1792 SveType = 1792 +SveType1920 SveType = 1920 +SveType2048 SveType = 2048 +) + type RdmReserve struct { Strategy RdmReserveStrategy Policy RdmReservePolicy @@ -564,6 +586,7 @@ TypeUnion DomainBuildInfoTypeUnion ArchArm struct { GicVersion GicVersion Vuart VuartType +SveVl SveType } ArchX86 struct { MsrRelaxed Defbool diff --git a/tools/include/libxl.h b/tools/include/libxl.h index 4fa09ff7635a..cac641a7eba2 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_vl field. + */ +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_SVE_VL 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..1e69dac2c4fa 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -3,6 +3,8 @@ #include "libxl_libfdt_compat.h" #include "libxl_arm.h" +#include <xen-tools/arm-arch-capabilities.h> + #include <stdbool.h> #include <libfdt.h> #include <assert.h> @@ -211,6 +213,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, return ERROR_FAIL; } + /* Parameter is sanitised in libxl__arch_domain_build_info_setdefault */ + if (d_config->b_info.arch_arm.sve_vl) { + /* Vector length is divided by 128 in struct xen_domctl_createdomain */ + config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U; + } + return 0; } @@ -1681,6 +1689,26 @@ int libxl__arch_domain_build_info_setdefault(libxl__gc *gc, /* ACPI is disabled by default */ libxl_defbool_setdefault(&b_info->acpi, false); + /* Sanitise SVE parameter */ + if (b_info->arch_arm.sve_vl) { + unsigned int max_sve_vl = + arch_capabilities_arm_sve(physinfo->arch_capabilities); + + if (!max_sve_vl) { + LOG(ERROR, "SVE is unsupported on this machine."); + return ERROR_FAIL; + } + + if (LIBXL_SVE_TYPE_HW == b_info->arch_arm.sve_vl) { + b_info->arch_arm.sve_vl = max_sve_vl; + } else if (b_info->arch_arm.sve_vl > max_sve_vl) { + LOG(ERROR, + "Invalid sve value: %d. Platform supports up to %u bits", + b_info->arch_arm.sve_vl, max_sve_vl); + return ERROR_FAIL; + } + } + if (b_info->type != LIBXL_DOMAIN_TYPE_PV) return 0; diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index fd31dacf7d5a..9e48bb772646 100644 --- a/tools/libs/light/libxl_types.idl +++ b/tools/libs/light/libxl_types.idl @@ -523,6 +523,27 @@ libxl_tee_type = Enumeration("tee_type", [ (1, "optee") ], init_val = "LIBXL_TEE_TYPE_NONE") +libxl_sve_type = Enumeration("sve_type", [ + (-1, "hw"), + (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") + ], init_val = "LIBXL_SVE_TYPE_DISABLED") + libxl_rdm_reserve = Struct("rdm_reserve", [ ("strategy", libxl_rdm_reserve_strategy), ("policy", libxl_rdm_reserve_policy), @@ -690,6 +711,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("arch_arm", Struct(None, [("gic_version", libxl_gic_version), ("vuart", libxl_vuart_type), + ("sve_vl", libxl_sve_type), ])), ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), ])), diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 1f6f47daf4e1..f036e56fc239 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -2887,6 +2887,14 @@ skip_usbdev: } } + if (!xlu_cfg_get_string (config, "sve", &buf, 1)) { + e = libxl_sve_type_from_string(buf, &b_info->arch_arm.sve_vl); + if (e) { + fprintf(stderr, "Unknown sve \"%s\" specified\n", buf); + exit(EXIT_FAILURE); + } + } + parse_vkb_list(config, d_config); d_config->virtios = NULL;
Add sve parameter in XL configuration to allow guests to use SVE feature. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- Changes from v5: - Update documentation - re-generated golang files Changes from v4: - Rename sve field to sve_vl (Anthony), changed type to libxl_sve_type - Sanity check of sve field in libxl instead of xl, update docs (Anthony) - drop Ack-by from George because of the changes in the Golang bits Changes from v3: - no changes Changes from v2: - domain configuration field name has changed to sve_vl, also its value now is VL/128. - Add Ack-by George for the Golang bits Changes from v1: - updated to use arch_capabilities field for vector length Changes from RFC: - changed libxl_types.idl sve field to uint16 - now toolstack uses info from physinfo to check against the sve XL value - Changed documentation --- docs/man/xl.cfg.5.pod.in | 16 ++++++++++++++++ tools/golang/xenlight/helpers.gen.go | 2 ++ tools/golang/xenlight/types.gen.go | 23 +++++++++++++++++++++++ tools/include/libxl.h | 5 +++++ tools/libs/light/libxl_arm.c | 28 ++++++++++++++++++++++++++++ tools/libs/light/libxl_types.idl | 22 ++++++++++++++++++++++ tools/xl/xl_parse.c | 8 ++++++++ 7 files changed, 104 insertions(+)