Re: [Xen-devel] [PATCH RFC] x86/amd: Avoid cpu_has_hypervisor evaluating true on native hardware

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/01/2020 13:59, Jan Beulich wrote:
> On 27.01.2020 21:21, Andrew Cooper wrote:
>> Currently when booting native on AMD hardware, cpuidmask_defaults._1cd gets
>> configured with the HYPERVISOR bit before native CPUID is scanned for feature
>> bits.
>>
>> This results in cpu_has_hypervisor becoming set as part of identify_cpu(), 
>> and
>> ends up appearing in the raw and host CPU policies.  Nothing has really cared
>> in the past.
>>
>> Alter amd_init_levelling() to exclude the HYPERVISOR bit from
>> cpumask_defaults, and update domain_cpu_policy_changed() to allow it to be
>> explicitly forwarded.
>>
>> This in turn highlighted that dom0 construction was asymetric with domU
>> construction, by not having any calls to domain_cpu_policy_changed().  Extend
>> arch_domain_create() to always call domain_cpu_policy_changed().
>>
>> Reported-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
>>
>> Without this fix, there is apparently a problem with Roger's "[PATCH v3 7/7]
>> x86/tlb: use Xen L0 assisted TLB flush when available" on native AMD 
>> hardware.
>> I haven't investgiated the issue with that patch specifically, because
>> cpu_has_hypervisor being wrong is obviously a bug.
>>
>> This is one of two possible approaches, and both have their downsides.  This
>> one takes an extra hit on context switches between PV vcpus and idle/hvm, as
>> they will usually differ in HYPERVISOR bit.
> Why would they differ in the HYPERVISOR bit? Maybe for idle (albeit
> off the top of my head I can't recall us special casing idle wrt
> CPUID handling), but why for PV vs HVM? The idle case, if there is
> an issue with this, could be taken care of by actually setting the
> bit there, as no-one should care about what it's set to?

d->arch.pv.cpuidmasks is only allocated for PV domains (and starts by
dup()ing the default).

When context switching levelling MSRs, any non-PV guest uses
cpumask_default.  This captures idle and HVM vcpus.

This is necessary because, at least at the time it was introduced,
{pv,hvm}_cpuid() issued native CPUID instructions to then feed data back
into guest context.  Its probably less relevant now that guest_cpuid()
doesn't issue native instructions in the general case.

Either way, HVM gained the default like idle, to cause the lazy
switching logic to switch less often.

The problem we have after this patch is that default differs from PV in
the HYPERVISOR bit, which basically guarantees that we rewrite the leaf
1 levelling on each context switch.

However, having looked at the other features bits which differ for PV,
VME and PSE36 being hidden means we're always switching leaf 1 anyway,
so this change for HYPERVISOR doesn't make the situation any worse.

>> The other approach is to order things more carefully so levelling is
>> configured after scanning for cpuid bits, but that has the downside that it 
>> is
>> very easy to regress.
>>
>> Thoughts on which is the least-bad approach to take?  Having written this
>> patch, I'm now erring on the side of doing it the other way.
> Besides the need for me to understand the aspect above, I'm afraid
> to judge I'd need to have at least a sketch of what the alternative
> would look like, in particular to figure how fragile it really is.

It would be a small bit of careful reordering in cpu/amd.c

The tipping factor is that, even if we arrange for idle context not to
have HYPERVISOR set (and therefore cpu_has_hypervisor ending up clear
when scanned), a regular CPUID instruction in PV context would see
HYPERVISOR as a property of virtualising things sensibly for guests.

As we need to cope with HYPERVISOR being visible in some contexts, its
better to consider it uniformly visible and break any kind of notional
link between cpu_has_hypervisor matching what CPUID would see as the bit.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel