Re: [PATCH 07/10] xen/physinfo: add arm SVE arch capability and vector length

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 23 Feb 2023, Bertrand Marquis wrote:
> Hi Jan,
> 
> > On 13 Feb 2023, at 09:36, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > 
> > On 10.02.2023 16:54, Luca Fancellu wrote:
> >>> On 2 Feb 2023, at 12:05, Jan Beulich <jbeulich@xxxxxxxx> 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