On 9/19/2019 2:41 AM, Jim Mattson wrote:
On Wed, Sep 18, 2019 at 11:22 AM Jim Mattson <jmattson@xxxxxxxxxx> wrote:
[...]>>>
If I'm reading the code correctly, this is fragile and subtle. The order
of cpuid entries is controlled by userspace, which means that clearing
KVM_CPUID_FLAG_SIGNIFCANT_INDEX depends on this entry being kept after all
other entries for this function. In practice I'm guessing userspaces
naturally sort entries with the same function by ascending index, but it
seems like avoidable issue.
Though not documented, the CPUID leaf matching code has always
depended on ordering.
Also, won't matching the last entry generate the wrong values for EAX, EBX
and ECX, i.e. the valid values for the last index instead of zeroes?
This entry has CH==0. According to the SDM, "For sub-leaves that
return an invalid level-type of 0 in ECX[15:8]; EAX and EBX will
return 0."
ECX[7:0] will be wrong, but that's fixed up by the flag below.
ECX[31:16] are reserved and perhaps should be cleared here, but I'm
not sure how I would interpret it if those bits started being non-zero
for the first leaf with CH==0.
+ entry[i - 1].flags |= KVM_CPUID_FLAG_CL_IS_PASSTHROUGH;
Lastly, do we actually need to enumerate this silliness to userspace?
What if we handle this as a one-off case in CPUID emulation and avoid the
ABI breakage that way? E.g.:
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index dd5985eb61b4..aaf5cdcb88c9 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1001,6 +1001,16 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
}
out:
+ if (!best && (function == 0xb || function == 0x1f)) {
+ best = check_cpuid_limit(vcpu, function, 0);
+ if (best) {
+ *eax = 0;
+ *ebx = 0;
+ *ecx &= 0xff;
+ *edx = *best->edx;
+ }
+ }
+
Hi Sean,
your proposal above seems not work. check_cpuid_limit(vcpu, 0xb/01f, 0)
always return NULL, if there are valid 0xb/0x1f leaves in
vcpu->arch.cpuid_entries[] (maxBaiscleaf >= 0xb/0x1f)
Aside from the fact that one should never call check_cpuid_limit on
AMD systems (they don't do the "last basic leaf" nonsense), an
approach like this should work.
The above proposal doesn't correctly handle a leaf outside of ([0,
maxBasicLeaf] union [80000000H, maxExtendedLeaf]) , where maxBasicLeaf
== (0BH or 1FH) on Intel hardware...but it could be fixed up to do so,
if hard-coding this behavior in kvm_cpuid() is preferable to the API
breakage.
I vote for Sean's one-off case, how about something like this:
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 22c2720cd948..6af5febf7b12 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -976,11 +976,23 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax,
u32 *ebx,
u32 *ecx, u32 *edx, bool check_limit)
{
u32 function = *eax, index = *ecx;
- struct kvm_cpuid_entry2 *best;
+ struct kvm_cpuid_entry2 *best, tmp;
bool entry_found = true;
best = kvm_find_cpuid_entry(vcpu, function, index);
+ if (!best && (fuction == 0xb || function == 0x1f) && index > 0) {
+ best = kvm_find_cpuid_entry(vcpu, function, 0);
+ if (best) {
+ tmp.eax = 0;
+ tmp.ebx = 0;
+ tmp.ecx = index & 0xff;
+ tmp.edx = best->edx;
+ best = &tmp;
+ goto out;
+ }
+ }
+
