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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 2 Feb 2023, at 12:05, Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxx>
>> ---
>> 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