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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

Jan