Re: [PATCH 08/10] tools: add SVE capability and SVE vector length for physinfo

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 02/02/2023 11:08 am, Luca Fancellu wrote:
> Recent changes added in struct xen_sysctl_physinfo a new flag for
> arch_capabilities to understand when the platform is SVE capable,
> another field having the maximum SVE vector length in bits was
> added as well, so update the tools to handle these new fields.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> Changes from RFC:
>  - new patch
> ---
> This patch is mostly dependent on the decisions we make in the previous, 
> anyway
> I touched some part of the toolstack where my knowledge is limited (ocaml) so
> I might need a feedback for something I may have done wrong.

There's a lot of ifdefary here which is going to make the code
impossible to maintain, especially in python and xl where you're playing
games with the position of the comma.

But taking a step back.  First, split this series into two (even if it's
first half / second half of a single series that you email out).  How to
report the presence of a capability is different from how to configure
the capability for a guest, and should be done separately.

But even still.  SVE is a single bit feature.  physinfo is the wrong
place to report it, and I dont see any need for it to be in the domain
construction hypercall.  Both are critically space-limited structures.

ARM needs to gain something similar to x86's cpu policy architecture,
both in terms of sysctls reporting system capabilities, and domctls to
get/set domain configuration.

Looking at your cover letter, SVE (in terms of what it means for Xen)
looks quite similar to x86's AMX and Arch-LBR insofar as it's a large
amount of new register state that we don't want to allocate for guests
not making use of it.

On x86, what we're planning to do (It's not done yet, because I keep on
getting preempted with security work) is to require the toolstack to
make a set_cpu_policy hypercall between createdomain and maxcpus to
configure the size of XSAVE state, after which vcpu_create() in the
hypervisor has the info it needs to construct backing state correctly.


I know I'm telling you to do a lot of work, but bodging SVE in like this
isn't ok.  Someone *is* going to have to do proper feature configuration
on ARM, and I guarantee you that it will be less effort to do it
properly now, than to retrofit it in the future.  x86 has been a
nightmare to untangle 15 years of bad interface design in a way that
doesn't regress behaviour.

~Andrew