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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 29, 2024 at 10:43:04AM +0000, 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.
> 
> Update the annotation from !A to !S which slightly better describes that it
> doesn't really exist in PV guests.  HVM guests, for which x2APIC can be
> emulated completely, already has it unconditionally set in the max policy.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> 
> 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.

Oh, so we didn't use to expose x2APIC in Xen < 4.14 for PV at all?

I think this need mentioning in the commit message, as it's not clear
whether x2APIC has always been advertised to guests.

If it's indeed only Xen 4.14 that started exposing the flag, it's IMO
less dangerous to stop exposing it.  My main concern would be OSes
having grow some dependency on it, and us no longer exposing it
causing collateral damage (which would be an OS bug anyway).

> ---
>  xen/arch/x86/cpu-policy.c                   | 19 +++++++++++++++++--
>  xen/include/public/arch-x86/cpufeatureset.h |  2 +-
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
> index 10079c26ae24..a0205672428d 100644
> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -534,6 +534,14 @@ static void __init calculate_pv_max_policy(void)
>      *p = host_cpu_policy;
>      x86_cpu_policy_to_featureset(p, fs);
>  
> +    /*
> +     * Xen at the time of writing (Feb 2024, 4.19 dev cycle) used to leak the
> +     * host x2APIC capability into PV guests, but never supported the guest
> +     * trying to turn x2APIC mode on.  Tolerate an incoming VM which saw the
> +     * x2APIC CPUID bit.
> +     */
> +    __set_bit(X86_FEATURE_X2APIC, fs);
> +
>      for ( i = 0; i < ARRAY_SIZE(fs); ++i )
>          fs[i] &= pv_max_featuremask[i];
>  
> @@ -566,6 +574,14 @@ static void __init calculate_pv_def_policy(void)
>      *p = pv_max_cpu_policy;
>      x86_cpu_policy_to_featureset(p, fs);
>  
> +    /*
> +     * PV guests have never been able to use x2APIC mode, but at the time of
> +     * writing (Feb 2024, 4.19 dev cycle), the host value used to leak into
> +     * guests.  Hide it by default so new guests don't get mislead into
> +     * thinking that they can use x2APIC.
> +     */
> +    __clear_bit(X86_FEATURE_X2APIC, fs);

IIRC if you use the 'S' tag it won't be added to the default PV policy
already, so there should be nothing to clear?  pv_def_featuremask
shouldn't contain the bit in the first place.

Thanks, Roger.