| Message ID | 20230327105944.1360856-3-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SVE feature for arm guests | expand |
On 27.03.2023 12:59, Luca Fancellu wrote: > @@ -43,8 +44,16 @@ register_t compute_max_zcr(void) > } > > /* Takes a vector length in bits and returns the ZCR_ELx encoding */ > -register_t vl_to_zcr(uint16_t vl) > +register_t vl_to_zcr(unsigned int vl) > { > ASSERT(vl > 0); > return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK; > } > + > +/* Get the system sanitized value for VL in bits */ > +unsigned int get_sys_vl_len(void) > +{ > + /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */ > + return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) * > + SVE_VL_MULTIPLE_VAL; > +} Wouldn't this function better return 0 when !cpu_has_sve? > @@ -602,6 +606,31 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) > return -EINVAL; > } > > + /* Check feature flags */ > + if ( sve_vl_bits > 0 ) { Nit: Style (brace placement). > + unsigned int zcr_max_bits = get_sys_vl_len(); > + > + if ( !cpu_has_sve ) > + { > + dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); > + return -EINVAL; > + } > + else if ( !is_vl_valid(sve_vl_bits) ) If this was code I'm the maintainer for, I'd ask for the "else" to be dropped. Please consider doing so, as it makes more visible that the earlier if() cannot "fall through". (This could then be further supported by inserting blank lines, here and again right below.) As to the check - this being the only user of is_vl_valid() (at this point) I'd like to mention that half of what that function checks is now pointless, as we're dealing with a decoded value. Future further users may need the full value checked, but given that all interfaces are now using encoded values this doesn't seem very likely. Hence the respective part of the condition there may want to become an assertion instead (or be dropped). Yet another question is whether both range checks (against SVE_VL_MAX_BITS and zcr_max_bits) are actually necessary / useful. Iirc 2048 is a hard upper bound, so zcr_max_bits being higher than that value should likely lead to not using SVE at all (if it doesn't already; didn't check). > + { > + dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n", > + sve_vl_bits); > + return -EINVAL; > + } > + else if ( sve_vl_bits > zcr_max_bits ) > + { > + dprintk(XENLOG_INFO, > + "The requested SVE vector length (%u) must be lower or \n" > + "equal to the platform supported vector length (%u)\n", > + sve_vl_bits, zcr_max_bits); Again, if I was the maintainer of this code, I'd ask for the message to be shortened, such that it easily fits on one line. E.g. "requested SVE vector length (%u) > supported length (%u)\n". This would then also avoid the apparent grammar issue of "lower" not fitting with "to" (i.e. a "than" needing inserting, or "below" being used instead). Jan
Hi Jan, Thank you for your review, very appreciated, > On 28 Mar 2023, at 10:36, Jan Beulich <jbeulich@suse.com> wrote: > > On 27.03.2023 12:59, Luca Fancellu wrote: >> @@ -43,8 +44,16 @@ register_t compute_max_zcr(void) >> } >> >> /* Takes a vector length in bits and returns the ZCR_ELx encoding */ >> -register_t vl_to_zcr(uint16_t vl) >> +register_t vl_to_zcr(unsigned int vl) >> { >> ASSERT(vl > 0); >> return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK; >> } >> + >> +/* Get the system sanitized value for VL in bits */ >> +unsigned int get_sys_vl_len(void) >> +{ >> + /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */ >> + return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) * >> + SVE_VL_MULTIPLE_VAL; >> +} > > Wouldn't this function better return 0 when !cpu_has_sve? Yes I agree > >> @@ -602,6 +606,31 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) >> return -EINVAL; >> } >> >> + /* Check feature flags */ >> + if ( sve_vl_bits > 0 ) { > > Nit: Style (brace placement). Will fix > >> + unsigned int zcr_max_bits = get_sys_vl_len(); >> + >> + if ( !cpu_has_sve ) >> + { >> + dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); >> + return -EINVAL; >> + } >> + else if ( !is_vl_valid(sve_vl_bits) ) > > If this was code I'm the maintainer for, I'd ask for the "else" to be > dropped. Please consider doing so, as it makes more visible that the > earlier if() cannot "fall through". (This could then be further > supported by inserting blank lines, here and again right below.) > > As to the check - this being the only user of is_vl_valid() (at this > point) I'd like to mention that half of what that function checks is > now pointless, as we're dealing with a decoded value. Future further > users may need the full value checked, but given that all interfaces > are now using encoded values this doesn't seem very likely. Hence the > respective part of the condition there may want to become an > assertion instead (or be dropped). Yes I can do that > > Yet another question is whether both range checks (against > SVE_VL_MAX_BITS and zcr_max_bits) are actually necessary / useful. > Iirc 2048 is a hard upper bound, so zcr_max_bits being higher than > that value should likely lead to not using SVE at all (if it doesn't > already; didn't check). I think the check sve_vl_bits > zcr_max_bits is needed because from sve_vl_bits = sve_decode_vl(config->arch.sve_vl); I can get values above the maximum supported bits (zcr_max_bits), later on I will use the struct arch_domain field sve_vl to compute the size of the registers to be saved/restore Is there something I’ve missed from your comment? > >> + { >> + dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n", >> + sve_vl_bits); >> + return -EINVAL; >> + } >> + else if ( sve_vl_bits > zcr_max_bits ) >> + { >> + dprintk(XENLOG_INFO, >> + "The requested SVE vector length (%u) must be lower or \n" >> + "equal to the platform supported vector length (%u)\n", >> + sve_vl_bits, zcr_max_bits); > > Again, if I was the maintainer of this code, I'd ask for the message > to be shortened, such that it easily fits on one line. E.g. > "requested SVE vector length (%u) > supported length (%u)\n". This > would then also avoid the apparent grammar issue of "lower" not fitting > with "to" (i.e. a "than" needing inserting, or "below" being used > instead). Yes this suggestion makes sense to me. To address your comments I’m doing these modifications to the patch, I hope they caption all your feedbacks: diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c index 3c3adfb5c6bd..78f7482619da 100644 --- a/xen/arch/arm/arm64/sve.c +++ b/xen/arch/arm/arm64/sve.c @@ -53,6 +53,9 @@ register_t vl_to_zcr(unsigned int vl) /* Get the system sanitized value for VL in bits */ unsigned int get_sys_vl_len(void) { + if ( !cpu_has_sve ) + return 0; + /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */ return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) * SVE_VL_MULTIPLE_VAL; diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 7182d4567bf0..c1fb30dfc5ef 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -607,7 +607,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) } /* Check feature flags */ - if ( sve_vl_bits > 0 ) { + if ( sve_vl_bits > 0 ) + { unsigned int zcr_max_bits = get_sys_vl_len(); if ( !cpu_has_sve ) @@ -615,17 +616,18 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); return -EINVAL; } - else if ( !is_vl_valid(sve_vl_bits) ) + + if ( !is_vl_valid(sve_vl_bits) ) { dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n", sve_vl_bits); return -EINVAL; } - else if ( sve_vl_bits > zcr_max_bits ) + + if ( sve_vl_bits > zcr_max_bits ) { dprintk(XENLOG_INFO, - "The requested SVE vector length (%u) must be lower or \n" - "equal to the platform supported vector length (%u)\n", + "Requested SVE vector length (%u) > supported length (%u)\n", sve_vl_bits, zcr_max_bits); return -EINVAL; } diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h index 8037f09b5b0a..e8c01a24e6da 100644 --- a/xen/arch/arm/include/asm/arm64/sve.h +++ b/xen/arch/arm/include/asm/arm64/sve.h @@ -16,7 +16,9 @@ static inline bool is_vl_valid(unsigned int vl) { /* SVE vector length is multiple of 128 and maximum 2048 */ - return ((vl % SVE_VL_MULTIPLE_VAL) == 0) && (vl <= SVE_VL_MAX_BITS); + ASSERT((vl % SVE_VL_MULTIPLE_VAL) == 0); + + return vl <= SVE_VL_MAX_BITS; } static inline unsigned int sve_decode_vl(unsigned int sve_vl) > > Jan
On 29.03.2023 12:01, Luca Fancellu wrote: >> On 28 Mar 2023, at 10:36, Jan Beulich <jbeulich@suse.com> wrote: >> Yet another question is whether both range checks (against >> SVE_VL_MAX_BITS and zcr_max_bits) are actually necessary / useful. >> Iirc 2048 is a hard upper bound, so zcr_max_bits being higher than >> that value should likely lead to not using SVE at all (if it doesn't >> already; didn't check). > > I think the check sve_vl_bits > zcr_max_bits is needed because from > sve_vl_bits = sve_decode_vl(config->arch.sve_vl); I can get values above the > maximum supported bits (zcr_max_bits), later on I will use the struct arch_domain > field sve_vl to compute the size of the registers to be saved/restore > > Is there something I’ve missed from your comment? Hmm, I realize my earlier response may have been ambiguous: I didn't mean to question the presence of both checks individually. I merely meant to question whether in addition to the zcr_max_bits check you really also need the SVE_VL_MAX_BITS one. Jan
> On 29 Mar 2023, at 11:32, Jan Beulich <jbeulich@suse.com> wrote: > > On 29.03.2023 12:01, Luca Fancellu wrote: >>> On 28 Mar 2023, at 10:36, Jan Beulich <jbeulich@suse.com> wrote: >>> Yet another question is whether both range checks (against >>> SVE_VL_MAX_BITS and zcr_max_bits) are actually necessary / useful. >>> Iirc 2048 is a hard upper bound, so zcr_max_bits being higher than >>> that value should likely lead to not using SVE at all (if it doesn't >>> already; didn't check). >> >> I think the check sve_vl_bits > zcr_max_bits is needed because from >> sve_vl_bits = sve_decode_vl(config->arch.sve_vl); I can get values above the >> maximum supported bits (zcr_max_bits), later on I will use the struct arch_domain >> field sve_vl to compute the size of the registers to be saved/restore >> >> Is there something I’ve missed from your comment? > > Hmm, I realize my earlier response may have been ambiguous: I didn't > mean to question the presence of both checks individually. I merely > meant to question whether in addition to the zcr_max_bits check you > really also need the SVE_VL_MAX_BITS one. Oh ok now I see what you mean, this check if ( !is_vl_valid(sve_vl_bits) ) { dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n”, sve_vl_bits); return -EINVAL; } Can be removed because if ( sve_vl_bits > zcr_max_bits ) is enough, I agree and so is_vl_valid is not required anymore > > Jan
diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c index c466de61b47f..3c3adfb5c6bd 100644 --- a/xen/arch/arm/arm64/sve.c +++ b/xen/arch/arm/arm64/sve.c @@ -6,6 +6,7 @@ */ #include <xen/types.h> +#include <asm/cpufeature.h> #include <asm/arm64/sve.h> #include <asm/arm64/sysregs.h> #include <asm/processor.h> @@ -43,8 +44,16 @@ register_t compute_max_zcr(void) } /* Takes a vector length in bits and returns the ZCR_ELx encoding */ -register_t vl_to_zcr(uint16_t vl) +register_t vl_to_zcr(unsigned int vl) { ASSERT(vl > 0); return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK; } + +/* Get the system sanitized value for VL in bits */ +unsigned int get_sys_vl_len(void) +{ + /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */ + return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) * + SVE_VL_MULTIPLE_VAL; +} diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index adb6ace2e24d..7182d4567bf0 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -13,6 +13,7 @@ #include <xen/wait.h> #include <asm/alternative.h> +#include <asm/arm64/sve.h> #include <asm/cpuerrata.h> #include <asm/cpufeature.h> #include <asm/current.h> @@ -550,6 +551,8 @@ int arch_vcpu_create(struct vcpu *v) v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id); v->arch.cptr_el2 = get_default_cptr_flags(); + if ( is_sve_domain(v->domain) ) + v->arch.cptr_el2 &= ~HCPTR_CP(8); v->arch.hcr_el2 = get_default_hcr_flags(); @@ -594,6 +597,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) unsigned int max_vcpus; unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap); unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu); + unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl); if ( (config->flags & ~flags_optional) != flags_required ) { @@ -602,6 +606,31 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) return -EINVAL; } + /* Check feature flags */ + if ( sve_vl_bits > 0 ) { + unsigned int zcr_max_bits = get_sys_vl_len(); + + if ( !cpu_has_sve ) + { + dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n"); + return -EINVAL; + } + else if ( !is_vl_valid(sve_vl_bits) ) + { + dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n", + sve_vl_bits); + return -EINVAL; + } + else if ( sve_vl_bits > zcr_max_bits ) + { + dprintk(XENLOG_INFO, + "The requested SVE vector length (%u) must be lower or \n" + "equal to the platform supported vector length (%u)\n", + sve_vl_bits, zcr_max_bits); + return -EINVAL; + } + } + /* The P2M table must always be shared between the CPU and the IOMMU */ if ( config->iommu_opts & XEN_DOMCTL_IOMMU_no_sharept ) { @@ -744,6 +773,9 @@ int arch_domain_create(struct domain *d, if ( (rc = domain_vpci_init(d)) != 0 ) goto fail; + /* Copy the encoded vector length sve_vl from the domain configuration */ + d->arch.sve_vl = config->arch.sve_vl; + return 0; fail: diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h index bd56e2f24230..8037f09b5b0a 100644 --- a/xen/arch/arm/include/asm/arm64/sve.h +++ b/xen/arch/arm/include/asm/arm64/sve.h @@ -13,10 +13,23 @@ /* Vector length must be multiple of 128 */ #define SVE_VL_MULTIPLE_VAL (128U) +static inline bool is_vl_valid(unsigned int vl) +{ + /* SVE vector length is multiple of 128 and maximum 2048 */ + return ((vl % SVE_VL_MULTIPLE_VAL) == 0) && (vl <= SVE_VL_MAX_BITS); +} + +static inline unsigned int sve_decode_vl(unsigned int sve_vl) +{ + /* SVE vector length is stored as VL/128 in xen_arch_domainconfig */ + return sve_vl * SVE_VL_MULTIPLE_VAL; +} + #ifdef CONFIG_ARM64_SVE register_t compute_max_zcr(void); -register_t vl_to_zcr(uint16_t vl); +register_t vl_to_zcr(unsigned int vl); +unsigned int get_sys_vl_len(void); #else /* !CONFIG_ARM64_SVE */ @@ -25,7 +38,12 @@ static inline register_t compute_max_zcr(void) return 0; } -static inline register_t vl_to_zcr(uint16_t vl) +static inline register_t vl_to_zcr(unsigned int vl) +{ + return 0; +} + +static inline unsigned int get_sys_vl_len(void) { return 0; } diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h index e776ee704b7d..78cc2da3d4e5 100644 --- a/xen/arch/arm/include/asm/domain.h +++ b/xen/arch/arm/include/asm/domain.h @@ -31,6 +31,8 @@ enum domain_type { #define is_domain_direct_mapped(d) ((d)->cdf & CDF_directmap) +#define is_sve_domain(d) ((d)->arch.sve_vl > 0) + /* * Is the domain using the host memory layout? * @@ -67,6 +69,9 @@ struct arch_domain enum domain_type type; #endif + /* max SVE encoded vector length */ + uint8_t sve_vl; + /* Virtual MMU */ struct p2m_domain p2m; diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 1528ced5097a..38311f559581 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -300,6 +300,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t); struct xen_arch_domainconfig { /* IN/OUT */ uint8_t gic_version; + /* IN - Contains SVE vector length divided by 128 */ + uint8_t sve_vl; /* IN */ uint16_t tee_type; /* IN */ diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 51be28c3de7c..616d7a1c070d 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -21,7 +21,7 @@ #include "hvm/save.h" #include "memory.h" -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015 +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016 /* * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
Add sve_vl field to arch_domain and xen_arch_domainconfig struct, to allow the domain to have an information about the SVE feature and the number of SVE register bits that are allowed for this domain. sve_vl field is the vector length in bits divided by 128, this allows to use less space in the structures. The field is used also to allow or forbid a domain to use SVE, because a value equal to zero means the guest is not allowed to use the feature. Check that the requested vector length is lower or equal to the platform supported vector length, otherwise fail on domain creation. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- Changes from v3: - don't use fixed types when not needed, use encoded value also in arch_domain so rename sve_vl_bits in sve_vl. (Jan) - rename domainconfig_decode_vl to sve_decode_vl because it will now be used also to decode from arch_domain value - change sve_vl from uint16_t to uint8_t and move it after "type" field to optimize space. Changes from v2: - rename field in xen_arch_domainconfig from "sve_vl_bits" to "sve_vl" and use the implicit padding after gic_version to store it, now this field is the VL/128. (Jan) - Created domainconfig_decode_vl() function to decode the sve_vl field and use it as plain bits value inside arch_domain. - Changed commit message reflecting the changes Changes from v1: - no changes Changes from RFC: - restore zcr_el2 in sve_restore_state, that will be introduced later in this serie, so remove zcr_el2 related code from this patch and move everything to the later patch (Julien) - add explicit padding into struct xen_arch_domainconfig (Julien) - Don't lower down the vector length, just fail to create the domain. (Julien) --- xen/arch/arm/arm64/sve.c | 11 +++++++++- xen/arch/arm/domain.c | 32 ++++++++++++++++++++++++++++ xen/arch/arm/include/asm/arm64/sve.h | 22 +++++++++++++++++-- xen/arch/arm/include/asm/domain.h | 5 +++++ xen/include/public/arch-arm.h | 2 ++ xen/include/public/domctl.h | 2 +- 6 files changed, 70 insertions(+), 4 deletions(-)