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

[Jan Beulich <jbeulich@xxxxxxxx>]

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
> ---
> 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