[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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?

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

Jan

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.