Re: [PATCH] x86/cpu-policy: Fix visibility of HTT/CMP_LEGACY in max policies

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

On Fri, Mar 01, 2024 at 11:28:29AM +0000, Andrew Cooper wrote:
> The block in recalculate_cpuid_policy() predates the proper split between
> default and max policies, and was a "slightly max for a toolstack which knows
> about it" capability.  It didn't get transformed properly in Xen 4.14.
> 
> Because Xen will accept a VM with HTT/CMP_LEGACY seen, they should be visible
> in the max polices.  Keep the default policy matching host settings.
> 
> This manifested as an incorrectly-rejected migration across XenServer's Xen
> 4.13 -> 4.17 upgrade, as Xapi is slowly growing the logic to check a VM
> against the target max policy.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I have one question below.

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> ---
>  xen/arch/x86/cpu-policy.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
> index c9b32bc17849..4f558e502e01 100644
> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -464,6 +464,16 @@ static void __init 
> guest_common_max_feature_adjustments(uint32_t *fs)
>               raw_cpu_policy.feat.clwb )
>              __set_bit(X86_FEATURE_CLWB, fs);
>      }
> +
> +    /*
> +     * Topology information inside the guest is entirely at the toolstack's
> +     * disgression, and bears no relationship to the host we're running on.
> +     *
> +     * HTT identifies p->basic.lppp as valid
> +     * CMP_LEGACY identifies p->extd.nc as valid
> +     */
> +    __set_bit(X86_FEATURE_HTT, fs);
> +    __set_bit(X86_FEATURE_CMP_LEGACY, fs);
>  }
>  
>  static void __init guest_common_default_feature_adjustments(uint32_t *fs)
> @@ -514,6 +524,18 @@ static void __init 
> guest_common_default_feature_adjustments(uint32_t *fs)
>              __clear_bit(X86_FEATURE_CLWB, fs);
>      }
>  
> +    /*
> +     * Topology information is at the toolstack's discretion so these are
> +     * unconditionally set in max, but pick a default which matches the host.
> +     */
> +    __clear_bit(X86_FEATURE_HTT, fs);
> +    if ( cpu_has_htt )
> +        __set_bit(X86_FEATURE_HTT, fs);
> +
> +    __clear_bit(X86_FEATURE_CMP_LEGACY, fs);
> +    if ( cpu_has_cmp_legacy )
> +        __set_bit(X86_FEATURE_CMP_LEGACY, fs);

Not that I oppose to it, but does it make sense to expose this options
to PV guests by default?  Those guests don't really have an APIC ID,
and I think we don't expect any of the topology information to be
usable by them in the first place.

Thanks, Roger.