Re: [PATCH] KVM: x86: Add Intel CPUID.1F cpuid emulation support

From: Like Xu
Date: Thu Apr 25 2019 - 11:33:29 EST

On 2019/4/25 22:19, Sean Christopherson wrote:
On Thu, Apr 25, 2019 at 03:07:35PM +0800, Like Xu wrote:
On 2019/4/25 14:30, Xiaoyao Li wrote:
Besides, the problem of simply using cpuid_exc(0x1f) in Sean's codes is
that we cannot assmue the reserved bits 31:16 of ECX is always 0 for the
future generation.

It doesn't matter if future CPUs use 31:16 for other things since this
code only cares about whether or not CPUID.1F exists. The check
entry->eax >= 0x1f ensures that CPUID.1F will return zeros if the leaf
does *NOT* exist, ergo the check against CPUID.1F.ECX only needs to
look for a non-zero value. CPUID.1F.ECX is the logical choice for
detecting support because it is guaranteed to be non-zero if the leaf
actually exists (because bits 15:8 will be non-zero). Whether or not
bits 31:16 are non-zero is irrelevant.

Here is a case:

one future CPU may have "cpuid.0.eax > 0x1f" but not use multi-chip packaging technology thus its CPUID.1F leaf **could** not exist to expose multi-die info and it still use cpuid.0B leaf.

So the entry->eax >= 0x1f check is true and cpuid_ecx(0x1f) check is true as well due to default return value. (one of my machines for cpuid.1f.ecx would return 0x00000064 without cpuid.1f support).

When we only cares about whether or not CPUID.1F exists,
we may need (cpuid_ecx(0x1f) & 0x0000ff00) != 0 or just follow what host check_extended_topology_leaf() does.

It's true cause the statement in public spec is not "Reserved = 0" but
"Bits 31 - 16: Reserved".

In my opinion, Sean's codes is OK and much simple and clear.

If the host cpuid.0.eax is greater than 0x1f but actually it doesn't
have multi-chip packaging technology and we may want to expose
entry->eax to some value smaller than 0x1f but greater than 0x14, much
effort needs to apply on Sean's code.

My improvement is good to overwrite cpuid.0.eax in future usage
>from the perspective of kvm feature setting not just from value check.

Alright, there is something wrong in your code that you haven't realised.

When you do
entry->eax = min(entry->eax, (u32)(f_intel_pt ? 0x14 : 0xd));

it changes the entry->eax if entry->eax > 0x14. So you cannot directly use
cpuid_ecx(0x1f). At least, you need to cache the value of entry->eax, like:

u32 max_leaf = entry->eax;
entry->eax = min(entry->eax, (u32)(f_intel_pt ? 0x14 : 0xd));

//...leaf between 0x14 and 0x1f

if (max_leaf >= 0x1f && (cpuid_ecx(0x1f) & 0x0000ff00))
entry->eax = 0x1f;

The cache value make no sense on this.

Xiaoyao is pointing out that by limiting entry->eax to 0x14 (or 0xd), then
it can't be used to detect support for CPUID.1F since KVM will have lost
the required information.

The point is that its existence does not really depend on cpuid.0.eax> = 0x1f. My test machine (CLX-AP, two die in one package) has cpuid.0.eax = 0x16 but does have CPUID.1F support (a bit strange on BIOS).

However, handling in increasing order in totally wrong. Since it's to report the
max the leaf supported, we should handle in descending order, which is what Sean

There is no need to check "entry->eax >= 0x1f" before "setting entry->eax =
0x1f" if and only if cpuid_ecx(0x1f) meets requirements.

entry->eax absolutely needs to be checked, otherwise you have no idea what
CPUID leaf is actually being consumed. For example, a CPU with a maximum
basic CPUID leaf of 0xB will report information that is indistinguishable
from the actual CPUID.1F, i.e. KVM will incorrectly think the CPU supports
CPUID.1F regardless of what heuristic is used to detect a "valid" CPUID.1F
if it only looks at the result of CPUID.1F.

Based on what I mentioned, just reconsider this proposal:

case 0:
entry->eax = min(entry->eax, (u32)(f_intel_pt ? 0x14 : 0xd));
if ((cpuid_ecx(0x1f) & 0x0000ff00)) != 0)
entry->eax = 0x1f;

It increases the cpuid.0.ecx value along with the minimized feature requirement in a natural and readable way. Why don't we design a little bit future ahead of time?

An increasing manner helps to overwrite this value on demand in a flat code
flow (easy to understand and maintain) not an if-else_if-else flow.

Maybe in the future the code will need to be refactored as more cases are
added, but for now an if-else is quite readable. Worry about the future
when it happens. :-)