| Message ID | 20230202110816.1252419-8-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SVE feature for arm guests | expand |
On 02.02.2023 12:08, Luca Fancellu wrote: > When the arm platform supports SVE, advertise the feature by a new > flag for the arch_capabilities in struct xen_sysctl_physinfo and add > a new field "arm_sve_vl_bits" where on arm there can be stored the > maximum SVE vector length in bits. > > Update the padding. > > Bump XEN_SYSCTL_INTERFACE_VERSION for the changes. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > Changes from RFC: > - new patch > --- > Here I need an opinion from arm and x86 maintainer, I see there is no arch > specific structure in struct xen_sysctl_physinfo, hw_cap is used only by x86 > and now arch_capabilities and the new arm_sve_vl_bits will be used only by arm. > So how should we proceed here? Shall we create a struct arch for each > architecture and for example move arch_capabilities inside it (renaming to > capabilities) and every arch specific field as well? Counter question: Why don't you use (part of) arch_capabilities (and not just a single bit)? It looks to be entirely unused at present. Vector length being zero would sufficiently indicate absence of the feature without a separate boolean. > (is hw_cap only for x86?) I suppose it is, but I also expect it would better go away than be moved. It doesn't hold a complete set of information, and it has been superseded. Question is (and I think I did raise this before, perhaps in different context) whether Arm wouldn't want to follow x86 in how hardware as well as hypervisor derived / used ones are exposed to the tool stack (XEN_SYSCTL_get_cpu_featureset). I guess there's nothing really precluding that data to consist of more than just boolean fields. > --- a/xen/include/public/sysctl.h > +++ b/xen/include/public/sysctl.h > @@ -18,7 +18,7 @@ > #include "domctl.h" > #include "physdev.h" > > -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015 > +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000016 Why? You ... > @@ -104,7 +110,8 @@ struct xen_sysctl_physinfo { > uint32_t cpu_khz; > uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */ > uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_{X86,ARM,...}_??? */ > - uint32_t pad; > + uint16_t arm_sve_vl_bits; > + uint16_t pad; > uint64_aligned_t total_pages; > uint64_aligned_t free_pages; > uint64_aligned_t scrub_pages; ... add no new fields, and the only producer of the data zero-fills the struct first thing. Jan
> On 2 Feb 2023, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: > > On 02.02.2023 12:08, Luca Fancellu wrote: >> When the arm platform supports SVE, advertise the feature by a new >> flag for the arch_capabilities in struct xen_sysctl_physinfo and add >> a new field "arm_sve_vl_bits" where on arm there can be stored the >> maximum SVE vector length in bits. >> >> Update the padding. >> >> Bump XEN_SYSCTL_INTERFACE_VERSION for the changes. >> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> --- >> Changes from RFC: >> - new patch >> --- Hi Jan, Thanks for your review, >> Here I need an opinion from arm and x86 maintainer, I see there is no arch >> specific structure in struct xen_sysctl_physinfo, hw_cap is used only by x86 >> and now arch_capabilities and the new arm_sve_vl_bits will be used only by arm. >> So how should we proceed here? Shall we create a struct arch for each >> architecture and for example move arch_capabilities inside it (renaming to >> capabilities) and every arch specific field as well? > > Counter question: Why don't you use (part of) arch_capabilities (and not > just a single bit)? It looks to be entirely unused at present. Vector > length being zero would sufficiently indicate absence of the feature > without a separate boolean. Yes I could have used just the value to determine if the platform is SVE capable or not, but since this field was there (even if with no user) I was unsure about renaming it and use it for this purpose. In the end I did what was more logic to me at the moment, and I reserved a flag in it for SVE. > > >> (is hw_cap only for x86?) > > I suppose it is, but I also expect it would better go away than be moved. > It doesn't hold a complete set of information, and it has been superseded. > > Question is (and I think I did raise this before, perhaps in different > context) whether Arm wouldn't want to follow x86 in how hardware as well > as hypervisor derived / used ones are exposed to the tool stack > (XEN_SYSCTL_get_cpu_featureset). I guess there's nothing really precluding > that data to consist of more than just boolean fields. Yes I guess that infrastructure could work, however I don’t have the bandwidth to put it in place at the moment, so I would like the Arm maintainers to give me a suggestion on how I can expose the vector length to XL if putting its value here needs to be avoided > >> --- a/xen/include/public/sysctl.h >> +++ b/xen/include/public/sysctl.h >> @@ -18,7 +18,7 @@ >> #include "domctl.h" >> #include "physdev.h" >> >> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015 >> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000016 > > Why? You ... > >> @@ -104,7 +110,8 @@ struct xen_sysctl_physinfo { >> uint32_t cpu_khz; >> uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */ >> uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_{X86,ARM,...}_??? */ >> - uint32_t pad; >> + uint16_t arm_sve_vl_bits; >> + uint16_t pad; >> uint64_aligned_t total_pages; >> uint64_aligned_t free_pages; >> uint64_aligned_t scrub_pages; > > ... add no new fields, and the only producer of the data zero-fills the > struct first thing. Yes that is right, I will wait to understand how I can proceed here: 1) rename arch_capabilities to arm_sve_vl_bits and put vector length there. 2) leave arch_capabilities untouched, no flag creation/setting, create uint32_t arm_sve_vl_bits field removing “pad”, Use its value to determine if SVE is present or not. 3) ?? Thank you Cheers, Luca > > Jan
On 10.02.2023 16:54, Luca Fancellu wrote: >> On 2 Feb 2023, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: >> On 02.02.2023 12:08, Luca Fancellu wrote: >>> (is hw_cap only for x86?) >> >> I suppose it is, but I also expect it would better go away than be moved. >> It doesn't hold a complete set of information, and it has been superseded. >> >> Question is (and I think I did raise this before, perhaps in different >> context) whether Arm wouldn't want to follow x86 in how hardware as well >> as hypervisor derived / used ones are exposed to the tool stack >> (XEN_SYSCTL_get_cpu_featureset). I guess there's nothing really precluding >> that data to consist of more than just boolean fields. > > Yes I guess that infrastructure could work, however I don’t have the bandwidth to > put it in place at the moment, so I would like the Arm maintainers to give me a > suggestion on how I can expose the vector length to XL if putting its value here > needs to be avoided Since you've got a reply from Andrew boiling down to the same suggestion (or should I even say request), I guess it wants seriously considering to introduce abstract base infrastructure first. As Andrew says, time not invested now will very likely mean yet more time to be invested later. >>> --- a/xen/include/public/sysctl.h >>> +++ b/xen/include/public/sysctl.h >>> @@ -18,7 +18,7 @@ >>> #include "domctl.h" >>> #include "physdev.h" >>> >>> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015 >>> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000016 >> >> Why? You ... >> >>> @@ -104,7 +110,8 @@ struct xen_sysctl_physinfo { >>> uint32_t cpu_khz; >>> uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */ >>> uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_{X86,ARM,...}_??? */ >>> - uint32_t pad; >>> + uint16_t arm_sve_vl_bits; >>> + uint16_t pad; >>> uint64_aligned_t total_pages; >>> uint64_aligned_t free_pages; >>> uint64_aligned_t scrub_pages; >> >> ... add no new fields, and the only producer of the data zero-fills the >> struct first thing. > > Yes that is right, I will wait to understand how I can proceed here: > > 1) rename arch_capabilities to arm_sve_vl_bits and put vector length there. > 2) leave arch_capabilities untouched, no flag creation/setting, create uint32_t arm_sve_vl_bits field removing “pad”, > Use its value to determine if SVE is present or not. > 3) ?? 3) Introduce suitable #define(s) to use part of arch_capabilities for your purpose, without renaming the field. (But that's of course only if Arm maintainers agree with you on _not_ going the proper feature handling route right away.) Jan
Hi Jan, > On 13 Feb 2023, at 09:36, Jan Beulich <jbeulich@suse.com> wrote: > > On 10.02.2023 16:54, Luca Fancellu wrote: >>> On 2 Feb 2023, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: >>> On 02.02.2023 12:08, Luca Fancellu wrote: >>>> (is hw_cap only for x86?) >>> >>> I suppose it is, but I also expect it would better go away than be moved. >>> It doesn't hold a complete set of information, and it has been superseded. >>> >>> Question is (and I think I did raise this before, perhaps in different >>> context) whether Arm wouldn't want to follow x86 in how hardware as well >>> as hypervisor derived / used ones are exposed to the tool stack >>> (XEN_SYSCTL_get_cpu_featureset). I guess there's nothing really precluding >>> that data to consist of more than just boolean fields. >> >> Yes I guess that infrastructure could work, however I don’t have the bandwidth to >> put it in place at the moment, so I would like the Arm maintainers to give me a >> suggestion on how I can expose the vector length to XL if putting its value here >> needs to be avoided > > Since you've got a reply from Andrew boiling down to the same suggestion > (or should I even say request), I guess it wants seriously considering > to introduce abstract base infrastructure first. As Andrew says, time not > invested now will very likely mean yet more time to be invested later. > >>>> --- a/xen/include/public/sysctl.h >>>> +++ b/xen/include/public/sysctl.h >>>> @@ -18,7 +18,7 @@ >>>> #include "domctl.h" >>>> #include "physdev.h" >>>> >>>> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015 >>>> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000016 >>> >>> Why? You ... >>> >>>> @@ -104,7 +110,8 @@ struct xen_sysctl_physinfo { >>>> uint32_t cpu_khz; >>>> uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */ >>>> uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_{X86,ARM,...}_??? */ >>>> - uint32_t pad; >>>> + uint16_t arm_sve_vl_bits; >>>> + uint16_t pad; >>>> uint64_aligned_t total_pages; >>>> uint64_aligned_t free_pages; >>>> uint64_aligned_t scrub_pages; >>> >>> ... add no new fields, and the only producer of the data zero-fills the >>> struct first thing. >> >> Yes that is right, I will wait to understand how I can proceed here: >> >> 1) rename arch_capabilities to arm_sve_vl_bits and put vector length there. >> 2) leave arch_capabilities untouched, no flag creation/setting, create uint32_t arm_sve_vl_bits field removing “pad”, >> Use its value to determine if SVE is present or not. >> 3) ?? > > 3) Introduce suitable #define(s) to use part of arch_capabilities for your > purpose, without renaming the field. (But that's of course only if Arm > maintainers agree with you on _not_ going the proper feature handling route > right away.) As Luca said, he does not have the required bandwidth to do this so I think it is ok for him to go with your solution 3. @Julien/Stefano: any thoughts here ? Bertrand > > Jan
On Thu, 23 Feb 2023, Bertrand Marquis wrote: > Hi Jan, > > > On 13 Feb 2023, at 09:36, Jan Beulich <jbeulich@suse.com> wrote: > > > > On 10.02.2023 16:54, Luca Fancellu wrote: > >>> On 2 Feb 2023, at 12:05, Jan Beulich <jbeulich@suse.com> wrote: > >>> On 02.02.2023 12:08, Luca Fancellu wrote: > >>>> (is hw_cap only for x86?) > >>> > >>> I suppose it is, but I also expect it would better go away than be moved. > >>> It doesn't hold a complete set of information, and it has been superseded. > >>> > >>> Question is (and I think I did raise this before, perhaps in different > >>> context) whether Arm wouldn't want to follow x86 in how hardware as well > >>> as hypervisor derived / used ones are exposed to the tool stack > >>> (XEN_SYSCTL_get_cpu_featureset). I guess there's nothing really precluding > >>> that data to consist of more than just boolean fields. > >> > >> Yes I guess that infrastructure could work, however I don’t have the bandwidth to > >> put it in place at the moment, so I would like the Arm maintainers to give me a > >> suggestion on how I can expose the vector length to XL if putting its value here > >> needs to be avoided > > > > Since you've got a reply from Andrew boiling down to the same suggestion > > (or should I even say request), I guess it wants seriously considering > > to introduce abstract base infrastructure first. As Andrew says, time not > > invested now will very likely mean yet more time to be invested later. > > > >>>> --- a/xen/include/public/sysctl.h > >>>> +++ b/xen/include/public/sysctl.h > >>>> @@ -18,7 +18,7 @@ > >>>> #include "domctl.h" > >>>> #include "physdev.h" > >>>> > >>>> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015 > >>>> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000016 > >>> > >>> Why? You ... > >>> > >>>> @@ -104,7 +110,8 @@ struct xen_sysctl_physinfo { > >>>> uint32_t cpu_khz; > >>>> uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */ > >>>> uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_{X86,ARM,...}_??? */ > >>>> - uint32_t pad; > >>>> + uint16_t arm_sve_vl_bits; > >>>> + uint16_t pad; > >>>> uint64_aligned_t total_pages; > >>>> uint64_aligned_t free_pages; > >>>> uint64_aligned_t scrub_pages; > >>> > >>> ... add no new fields, and the only producer of the data zero-fills the > >>> struct first thing. > >> > >> Yes that is right, I will wait to understand how I can proceed here: > >> > >> 1) rename arch_capabilities to arm_sve_vl_bits and put vector length there. > >> 2) leave arch_capabilities untouched, no flag creation/setting, create uint32_t arm_sve_vl_bits field removing “pad”, > >> Use its value to determine if SVE is present or not. > >> 3) ?? > > > > 3) Introduce suitable #define(s) to use part of arch_capabilities for your > > purpose, without renaming the field. (But that's of course only if Arm > > maintainers agree with you on _not_ going the proper feature handling route > > right away.) > > As Luca said, he does not have the required bandwidth to do this so I think it is ok for him to go with your solution 3. > > @Julien/Stefano: any thoughts here ? I am OK with that
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c index b0a78a8b10d0..d65f8be498f4 100644 --- a/xen/arch/arm/sysctl.c +++ b/xen/arch/arm/sysctl.c @@ -11,11 +11,18 @@ #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; + + if ( cpu_has_sve ) + { + pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_ARM_sve; + pi->arm_sve_vl_bits = get_sys_vl_len(); + } } long arch_do_sysctl(struct xen_sysctl *sysctl, diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 001a4de27375..5acbb41256bc 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -18,7 +18,7 @@ #include "domctl.h" #include "physdev.h" -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015 +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000016 /* * Read console content from Xen buffer ring. @@ -94,6 +94,12 @@ struct xen_sysctl_tbuf_op { /* Max XEN_SYSCTL_PHYSCAP_* constant. Used for ABI checking. */ #define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_gnttab_v2 +/* Arm platform is SVE capable */ +#define XEN_SYSCTL_PHYSCAP_ARM_sve (1u << 0) + +/* Max XEN_SYSCTL_PHYSCAP_ARM_* constant. Used for ABI checking. */ +#define XEN_SYSCTL_PHYSCAP_ARM_MAX XEN_SYSCTL_PHYSCAP_ARM_sve + struct xen_sysctl_physinfo { uint32_t threads_per_core; uint32_t cores_per_socket; @@ -104,7 +110,8 @@ struct xen_sysctl_physinfo { uint32_t cpu_khz; uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */ uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_{X86,ARM,...}_??? */ - uint32_t pad; + uint16_t arm_sve_vl_bits; + uint16_t pad; uint64_aligned_t total_pages; uint64_aligned_t free_pages; uint64_aligned_t scrub_pages;
When the arm platform supports SVE, advertise the feature by a new flag for the arch_capabilities in struct xen_sysctl_physinfo and add a new field "arm_sve_vl_bits" where on arm there can be stored the maximum SVE vector length in bits. Update the padding. Bump XEN_SYSCTL_INTERFACE_VERSION for the changes. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- Changes from RFC: - new patch --- Here I need an opinion from arm and x86 maintainer, I see there is no arch specific structure in struct xen_sysctl_physinfo, hw_cap is used only by x86 and now arch_capabilities and the new arm_sve_vl_bits will be used only by arm. So how should we proceed here? Shall we create a struct arch for each architecture and for example move arch_capabilities inside it (renaming to capabilities) and every arch specific field as well? (is hw_cap only for x86?) --- xen/arch/arm/sysctl.c | 7 +++++++ xen/include/public/sysctl.h | 11 +++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-)