| Message ID | 20230327105944.1360856-9-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: > --- a/xen/arch/arm/arm64/sve.c > +++ b/xen/arch/arm/arm64/sve.c > @@ -124,3 +124,15 @@ int __init sve_parse_dom0_param(const char *str_begin, const char *str_end) > { > return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve); > } > + > +void sve_arch_cap_physinfo(uint32_t *arch_capabilities) > +{ > + if ( cpu_has_sve ) > + { > + /* Vector length is divided by 128 to save some space */ > + uint32_t sve_vl = MASK_INSR(sve_encode_vl(get_sys_vl_len()), > + XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK); > + > + *arch_capabilities |= sve_vl; > + } > +} I'm again wondering why a separate function is needed, when everything that's needed is ... > --- a/xen/arch/arm/sysctl.c > +++ b/xen/arch/arm/sysctl.c > @@ -11,11 +11,14 @@ > #include <xen/lib.h> > #include <xen/errno.h> > #include <xen/hypercall.h> > +#include <asm/arm64/sve.h> ... becoming available here for use ... > #include <public/sysctl.h> > > void arch_do_physinfo(struct xen_sysctl_physinfo *pi) > { > pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap; > + > + sve_arch_cap_physinfo(&pi->arch_capabilities); ... here. That would be even more so if, like suggested before, get_sys_vl_len() returned 0 when !cpu_has_sve. Jan
> On 28 Mar 2023, at 11:14, Jan Beulich <jbeulich@suse.com> wrote: > > On 27.03.2023 12:59, Luca Fancellu wrote: >> --- a/xen/arch/arm/arm64/sve.c >> +++ b/xen/arch/arm/arm64/sve.c >> @@ -124,3 +124,15 @@ int __init sve_parse_dom0_param(const char *str_begin, const char *str_end) >> { >> return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve); >> } >> + >> +void sve_arch_cap_physinfo(uint32_t *arch_capabilities) >> +{ >> + if ( cpu_has_sve ) >> + { >> + /* Vector length is divided by 128 to save some space */ >> + uint32_t sve_vl = MASK_INSR(sve_encode_vl(get_sys_vl_len()), >> + XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK); >> + >> + *arch_capabilities |= sve_vl; >> + } >> +} > > I'm again wondering why a separate function is needed, when everything > that's needed is ... > >> --- a/xen/arch/arm/sysctl.c >> +++ b/xen/arch/arm/sysctl.c >> @@ -11,11 +11,14 @@ >> #include <xen/lib.h> >> #include <xen/errno.h> >> #include <xen/hypercall.h> >> +#include <asm/arm64/sve.h> > > ... becoming available here for use ... > >> #include <public/sysctl.h> >> >> void arch_do_physinfo(struct xen_sysctl_physinfo *pi) >> { >> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap; >> + >> + sve_arch_cap_physinfo(&pi->arch_capabilities); > > ... here. That would be even more so if, like suggested before, > get_sys_vl_len() returned 0 when !cpu_has_sve. I’ve had a look on this, I can do everything In arch_do_physinfo if in xen/include/public/sysctl.h the XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK is protected by __aarch64__ or __arm__ . Do you agree on that? > > Jan
On 30.03.2023 12:41, Luca Fancellu wrote: > > >> On 28 Mar 2023, at 11:14, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 27.03.2023 12:59, Luca Fancellu wrote: >>> --- a/xen/arch/arm/arm64/sve.c >>> +++ b/xen/arch/arm/arm64/sve.c >>> @@ -124,3 +124,15 @@ int __init sve_parse_dom0_param(const char *str_begin, const char *str_end) >>> { >>> return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve); >>> } >>> + >>> +void sve_arch_cap_physinfo(uint32_t *arch_capabilities) >>> +{ >>> + if ( cpu_has_sve ) >>> + { >>> + /* Vector length is divided by 128 to save some space */ >>> + uint32_t sve_vl = MASK_INSR(sve_encode_vl(get_sys_vl_len()), >>> + XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK); >>> + >>> + *arch_capabilities |= sve_vl; >>> + } >>> +} >> >> I'm again wondering why a separate function is needed, when everything >> that's needed is ... >> >>> --- a/xen/arch/arm/sysctl.c >>> +++ b/xen/arch/arm/sysctl.c >>> @@ -11,11 +11,14 @@ >>> #include <xen/lib.h> >>> #include <xen/errno.h> >>> #include <xen/hypercall.h> >>> +#include <asm/arm64/sve.h> >> >> ... becoming available here for use ... >> >>> #include <public/sysctl.h> >>> >>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi) >>> { >>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap; >>> + >>> + sve_arch_cap_physinfo(&pi->arch_capabilities); >> >> ... here. That would be even more so if, like suggested before, >> get_sys_vl_len() returned 0 when !cpu_has_sve. > > I’ve had a look on this, I can do everything In arch_do_physinfo if in xen/include/public/sysctl.h > the XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK is protected by __aarch64__ or __arm__ . > > Do you agree on that? I don't see the connection, but guarding the #define in the public header looks not only okay, but even desirable to me. Jan
diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c index 6416403817e3..7b5223117e1b 100644 --- a/xen/arch/arm/arm64/sve.c +++ b/xen/arch/arm/arm64/sve.c @@ -124,3 +124,15 @@ int __init sve_parse_dom0_param(const char *str_begin, const char *str_end) { return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve); } + +void sve_arch_cap_physinfo(uint32_t *arch_capabilities) +{ + if ( cpu_has_sve ) + { + /* Vector length is divided by 128 to save some space */ + uint32_t sve_vl = MASK_INSR(sve_encode_vl(get_sys_vl_len()), + XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK); + + *arch_capabilities |= sve_vl; + } +} diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h index 69a1044c37d9..a6d0fc851f68 100644 --- a/xen/arch/arm/include/asm/arm64/sve.h +++ b/xen/arch/arm/include/asm/arm64/sve.h @@ -42,6 +42,7 @@ void sve_context_free(struct vcpu *v); void sve_save_state(struct vcpu *v); void sve_restore_state(struct vcpu *v); int sve_parse_dom0_param(const char *str_begin, const char *str_end); +void sve_arch_cap_physinfo(uint32_t *arch_capabilities); #else /* !CONFIG_ARM64_SVE */ @@ -70,6 +71,7 @@ static inline int sve_context_init(struct vcpu *v) static inline void sve_context_free(struct vcpu *v) {} static inline void sve_save_state(struct vcpu *v) {} static inline void sve_restore_state(struct vcpu *v) {} +static inline void sve_arch_cap_physinfo(uint32_t *arch_capabilities) {} static inline int sve_parse_dom0_param(const char *str_begin, const char *str_end) diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c index b0a78a8b10d0..64e4d3e06a6b 100644 --- a/xen/arch/arm/sysctl.c +++ b/xen/arch/arm/sysctl.c @@ -11,11 +11,14 @@ #include <xen/lib.h> #include <xen/errno.h> #include <xen/hypercall.h> +#include <asm/arm64/sve.h> #include <public/sysctl.h> void arch_do_physinfo(struct xen_sysctl_physinfo *pi) { pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap; + + sve_arch_cap_physinfo(&pi->arch_capabilities); } long arch_do_sysctl(struct xen_sysctl *sysctl, diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index e8dded9fb94a..99ea3fa0740b 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -94,6 +94,10 @@ struct xen_sysctl_tbuf_op { /* Max XEN_SYSCTL_PHYSCAP_* constant. Used for ABI checking. */ #define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_gnttab_v2 +#ifdef __aarch64__ +#define XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK (0x1FU) +#endif + struct xen_sysctl_physinfo { uint32_t threads_per_core; uint32_t cores_per_socket;
When the arm platform supports SVE, advertise the feature in the field arch_capabilities in struct xen_sysctl_physinfo by encoding the SVE vector length in it. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- Changes from v3: - domainconfig_encode_vl is now named sve_encode_vl Changes from v2: - Remove XEN_SYSCTL_PHYSCAP_ARM_SVE_SHFT, use MASK_INSR and protect with ifdef XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK (Jan) - Use the helper function sve_arch_cap_physinfo to encode the VL into physinfo arch_capabilities field. Changes from v1: - Use only arch_capabilities and some defines to encode SVE VL (Bertrand, Stefano, Jan) Changes from RFC: - new patch --- xen/arch/arm/arm64/sve.c | 12 ++++++++++++ xen/arch/arm/include/asm/arm64/sve.h | 2 ++ xen/arch/arm/sysctl.c | 3 +++ xen/include/public/sysctl.h | 4 ++++ 4 files changed, 21 insertions(+)