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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/03/2024 12:30 pm, Roger Pau Monné wrote:
> 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>

Thanks.

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

This patch doesn't alter what PV or HVM guests see by default.  This
hunk only counteracts the prior hunk forcing visibility of the two bits
in the max policy.

Whether it is sensible for PV to see this is a different matter, and
it's actually very complicated.

On systems without CPUID Faulting, we have to contend with PV guests
seeing mostly host data, whatever Xen would prefer.

With CPUID Masking, we can hide (Intel) or hide/set (AMD) these bits in
the feature leaves, but we can never stop the host value leaking into
lppp/nc/etc.

For better or worse, when the toolstack is divining a policy for PV
guests, it also chooses the host HTT/CMP_LEGACY values.

We should reconsider all of this when we've got topology working
sensibly for HVM guests.

~Andrew