Re: [PATCH 17/21] libs/guest: introduce helper set cpu topology in cpu policy

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/03/2021 09:58, Roger Pau Monne wrote:
> This logic is pulled out from xc_cpuid_apply_policy and placed into a
> separate helper.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  tools/include/xenctrl.h         |   4 +
>  tools/libs/guest/xg_cpuid_x86.c | 181 +++++++++++++++++---------------
>  2 files changed, 102 insertions(+), 83 deletions(-)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 6f7158156fa..9f94e61523e 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2631,6 +2631,10 @@ int xc_cpu_policy_calc_compatible(xc_interface *xch,
>  int xc_cpu_policy_make_compatible(xc_interface *xch, xc_cpu_policy_t policy,
>                                    bool hvm);
>  
> +/* Setup the policy topology. */
> +int xc_cpu_policy_topology(xc_interface *xch, xc_cpu_policy_t policy,
> +                           bool hvm);

I'm not sure how I feel about this.  It's repeating the mistake we've
currently got with topology handling.

One part of it needs to be part of "compatible".  We need to run the
below logic, *in this form* as part of magic-ing a policy out of thin
air for the incoming VM with no data.

However, for any non-broken logic, the caller needs to specify the
topology which wishes to be expressed.

Do we want SMT at all?  Do we want 1, 2, 4 or other threads/core.
How many cores per socket?  Its very common these days for
non-power-of-2 numbers.  Our default case ought to be to match the host
topology.

Do we want to support 3 threads/core?  Sure - its weird to think about,
but its semantically equivalent to using non-power-of-2 numbers at other
levels, and would certainly be useful to express for testing purposes.

What about Intel's leaf 0x1f with the SMT > Core > Module > Tile > Die
topology layout?

The answers to these questions also need to fix Xen so that APIC_ID
isn't vcpu_id * 2 (which is horribly broken on non-Intel or Intel
Knight* hardware).  It also needs to change how the MADT is written for
guests, and how the IO-APIC IDs are assigned (matters for the AMD
topology algorithms).

There are further implications.  Should we prohibit creating a 4-vcpu VM
with cores/socket=128?  A regular kernel will demand an IOMMU for this
configuration as we end up with APIC IDs above 255.  OTOH, there are
also virtualisation schemes now to support 32k vcpus without an IOMMU,
which KVM and HyperV now speak.

Fixing our topology problems is a monumental can of worms.  While we
should keep it in mind, we should try not to conflate it with "make
libxl/libxc's CPUID logic more sane, and include MSRs", which is large
enough task on its own.

What I suspect we want in the short term is
xc_cpu_policy_legacy_adjust() or equivalent, which is very clear that it
is a transitional API only, which for now can be used everywhere where
xc_cpuid_apply_policy() is used.  As we pull various logical areas out
of this, we'll adjust the callers appropriately, and eventually delete
this function.

~Andrew