Re: [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

Hi,

On Wed Oct 9, 2024 at 10:40 AM BST, Jan Beulich wrote:
> On 01.10.2024 14:37, Alejandro Vallejo wrote:
> > --- a/xen/lib/x86/policy.c
> > +++ b/xen/lib/x86/policy.c
> > @@ -15,7 +15,16 @@ int x86_cpu_policies_are_compatible(const struct 
> > cpu_policy *host,
> >  #define FAIL_MSR(m) \
> >      do { e.msr = (m); goto out; } while ( 0 )
> >  
> > -    if ( guest->basic.max_leaf > host->basic.max_leaf )
> > +    /*
> > +     * Old AMD hardware doesn't expose topology information in leaf 0xb. We
> > +     * want to emulate that leaf with credible information because it must 
> > be
> > +     * present on systems in which we emulate the x2APIC.
> > +     *
> > +     * For that reason, allow the max basic guest leaf to be larger than 
> > the
> > +     * hosts' up until 0xb.
> > +     */
> > +    if ( guest->basic.max_leaf > 0xb &&
> > +         guest->basic.max_leaf > host->basic.max_leaf )
> >          FAIL_CPUID(0, NA);
> >  
> >      if ( guest->feat.max_subleaf > host->feat.max_subleaf )
>
> I'm concerned by this in multiple ways:
>
> 1) It's pretty ad hoc, and hence doesn't make clear how to deal with similar
> situations in the future.

I agree. I don't have a principled suggestion for how to deal with other cases
where we might have to bump the max leaf. It may be safe (as is here becasue
everything below it is either used or unimplemnted), but AFAIU some leaves
might be problematic to expose, even as zeroes. I suspect that's the problem
you hint at later on about AMX and AVX10?

>
> 2) Why would we permit going up to leaf 0xb when x2APIC is off in the 
> respective
> leaf?

I assume you mean when the x2APIC is not emulated? One reason is to avoid a
migration barrier, as otherwise we can't migrate VMs created in "leaf
0xb"-capable hardware to non-"leaf 0xb"-capable even though the migration is
perfectly safe.

Also, it's benign and simplifies everything. Otherwise we have to find out
during early creation not only whether the host has leaf 0xb, but also whether
we're emulating an x2APIC or not.

Furthermore, not doing this would actively prevent emulating an x2APIC on AMD
Lisbon-like silicon even though it's fine to do so. Note that we have a broken
invariant in existing code where the x2APIC is emulated and leaf 0xb is not
exposed at all; not even to show the x2APIC IDs.

>
> 3) We similarly force a higher extended leaf in order to accommodate the 
> LFENCE-
> is-dispatch-serializing bit. Yet there's no similar extra logic there in the
> function here.

That's done on the host policy though, so there's no clash.

In calculate_host_policy()...

```
      /*
       * For AMD/Hygon hardware before Zen3, we unilaterally modify LFENCE to be
       * dispatch serialising for Spectre mitigations.  Extend max_extd_leaf
       * beyond what hardware supports, to include the feature leaf containing
       * this information.
       */
      if ( cpu_has_lfence_dispatch )
          max_extd_leaf = max(max_extd_leaf, 0x80000021U);
```

One could imagine doing the same for leaf 0xb and dropping this patch, but then
we'd have to synthesise something on that leaf for hardware that doesn't have
it, which is a lot more annoying.

>
> 4) While there the guest vs host check won't matter, the situation with AMX 
> and
> AVX10 leaves imo still wants considering here right away. IOW (taken together
> with at least 3) above) I think we need to first settle on a model for
> collectively all max (sub)leaf handling. That in particular needs to properly
> spell out who's responsible for what (tool stack vs Xen).

I'm not sure I follow. What's the situation with AMX and AVX10 that you refer
to? I'd assume that making ad-hoc decisions on this is pretty much unavoidable,
but maybe the solution to the problem you mention would highlight a more
general approach.

>
> Jan

Cheers,
Alejandro