Re: [PATCH] x86/cpu-policy: Fix x2APIC visibility for PV guests

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/02/2024 1:29 pm, Jan Beulich wrote:
> On 29.02.2024 14:23, Andrew Cooper wrote:
>> On 29/02/2024 12:47 pm, Jan Beulich wrote:
>>> On 29.02.2024 11:43, Andrew Cooper wrote:
>>>> Right now, the host x2APIC setting filters into the PV max and default
>>>> policies, yet PV guests cannot set MSR_APIC_BASE.EXTD or access any of the
>>>> x2APIC MSR range.  Therefore they absolutely shouldn't see the x2APIC bit.
>>>>
>>>> Linux has workarounds for the collateral damage caused by this leakage; it
>>>> unconditionally filters out the x2APIC CPUID bit, and EXTD when reading
>>>> MSR_APIC_BASE.
>>>>
>>>> Hide the x2APIC bit in the PV default policy, but for compatibility, 
>>>> tolerate
>>>> incoming VMs which already saw the bit.  This is logic from before the
>>>> default/max split in Xen 4.14 which wasn't correctly adjusted at the time.
>>> What about guest_cpuid()'s handling of leaf 0xb then? The %edx value
>>> will change once a guest is rebooted, aiui. The comment in
>>> recalculate_cpuid_policy() that you update refers to that.
>> That comment is going in the next patch irrespective.
>>
>> But yes - this will change leaf 0xb from being
>> host-conditionally-visible to always hidden.
> Imo this wants saying explicitly,

Yeah - already started that for v2.

>  including why that's okay to do,
> especially since ...
>
>> PV guests don't have any coherent idea of topology.  Linux (with the
>> topo fixes) now explicitly ignores everything it can see and just fakes
>> up a flat non-SMT topology in a single package.
> ... you validly use "now" here. Plus Linux isn't the only PV guest we
> need to care about.

As I said on the other thread, NetBSD works in the spirit in which PV
guests were intended and completely ignores x2APIC in XENPV builds.

>
> What's wrong (more wrong than the present putting of vCPU ID * 2 there)
> with retaining the population of that leaf (by dropping the x2apic
> dependency there)?

Without an MADT it's meaningless.   For PV dom0, it's actively wrong
because there is an MADT and it's in the wrong space.

libxc has never written anything coherent in here, because it's never
had any coherent idea about topology.  Alejandro is working on that, and
I believe one of the prep series has been posted.  There's a lot more to go.

Even today, we end up overlaying the host's APIC_ID space layout over
the blind vCPU_ID * 2, which makes the result still nonsense.

Various versions of Xen have tried playing with this, without
understanding properly what they're doing, and XenServer still has a
revert of a Xen 3.4 patch in the patch, as it broken migration of guests
at the time...


There is going to be a future (hopefully soon) where HVM guests get to
see something which conforms to the architectural specs, and is sane.

But doing the same for PV guests is more complicated, because of the
conflicting requirements between PV guests not really having an APIC,
but APIC being the x86 architectural expression of topology.


>>>> This wants backporting as far as people can tollerate, but it's really not
>>>> obvious which commit in 4.14 should be referenced in a Fixes: tag.
>>> Why 4.14? In 4.7.0 I see ...
>>>
>>>> @@ -830,11 +846,10 @@ void recalculate_cpuid_policy(struct domain *d)
>>>>      }
>>>>  
>>>>      /*
>>>> -     * Allow the toolstack to set HTT, X2APIC and CMP_LEGACY.  These bits
>>>> +     * Allow the toolstack to set HTT and CMP_LEGACY.  These bits
>>>>       * affect how to interpret topology information in other cpuid leaves.
>>>>       */
>>>>      __set_bit(X86_FEATURE_HTT, max_fs);
>>>> -    __set_bit(X86_FEATURE_X2APIC, max_fs);
>>>>      __set_bit(X86_FEATURE_CMP_LEGACY, max_fs);
>>>>  
>>>>      /*
>>> ... these adjustments, just still in calculate_pv_featureset(). I
>>> haven't gone further backwards to check if/when this exposure has
>>> really appeared. I wouldn't be surprised if it's been like that
>>> for all the time since we gained x2APIC support in the hypervisor.
>> 4.14 was when we got the proper default vs max split.  Before then, this
>> block of logic was an opencoded "max(ish) for tookstacks which know
>> about it" kind of thing.
> Except it was also affecting what guests get to see, afaict.

No - this hunk explicitly doesn't.

What this hunk says is "don't override the toolstack's choice based on
what the host can see".

Because even today, Xen is still blindly zeroing toolstack settings it
doesn't like, because my series fixing this is still sat on the mailing
list from years and years ago over and argument over whether a function
lives in libx86 or elsewhere...

~Andrew