KVM: x86: Add Intel CPUID.1F cpuid emulation support
PATCH RESEND v3 1/1 KVM: x86: Add Intel CPUID.1F cpuid emulation support
Add support to expose Intel V2 Extended Topology Enumeration Leaf for some new systems with multiple software-visible die within each package.
Per Intel's SDM, when CPUID executes with EAX set to 1FH, the processor returns information about extended topology enumeration data. Software must detect the presence of CPUID leaf 1FH by verifying (a) the highest leaf index supported by CPUID is >= 1FH, and (b) CPUID.1FH:EBX[15:0] reports a non-zero value.
---
==changelog==
v3:
- Refine commit message and comment
v2: https://lkml.org/lkml/2019/4/25/1246
- Apply cpuid.1f check rule on Intel SDM page 3-222 Vol.2A
- Add comment to handle 0x1f anf 0xb in common code
- Reduce check time in a descending-break style
v1: https://lkml.org/lkml/2019/4/22/28
arch/x86/kvm/cpuid.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +10 -1
@@ -426,6 +426,11 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,switch (function) {case 0 : +/* Check if the cpuid leaf 0x1f is actually implemented */ +if (entry->eax >=0x1f && (cpuid_ebx (0x1f ) &0x0000ffff )) { + entry->eax =0x1f ; +break ; + } entry->eax =min (entry->eax, (u32)(f_intel_pt ?0x14 :0xd ));break ;case 1 :
@@ -545,7 +550,11 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, entry->edx = edx.full;break ; } -/* function 0xb has additional index. */ +/* +* Per Intel's SDM, 0x1f is a superset of 0xb, thus they can be handled +* by common code. +*/ +case 0x1f :case 0xb : {int i, level_type;
Signature · 1 line
2.21.0
2019-05-26 21:30+0800, Like Xu:
Quoted 34 lines
Add support to expose Intel V2 Extended Topology Enumeration Leaf for some new systems with multiple software-visible die within each package.
Per Intel's SDM, when CPUID executes with EAX set to 1FH, the processor returns information about extended topology enumeration data. Software must detect the presence of CPUID leaf 1FH by verifying (a) the highest leaf index supported by CPUID is >= 1FH, and (b) CPUID.1FH:EBX[15:0] reports a non-zero value.
Co-developed-by Xiaoyao Li <[email protected]>Signed-off-by Xiaoyao Li <[email protected]>Signed-off-by Like Xu <[email protected]>Reviewed-by Sean Christopherson <[email protected]>---
==changelog==
v3:
- Refine commit message and commentv2: https://lkml.org/lkml/2019/4/25/1246
- Apply cpuid.1f check rule on Intel SDM page 3-222 Vol.2A
- Add comment to handle0x1fanf0xbin common code
- Reduce check time in a descending-break stylev1: https://lkml.org/lkml/2019/4/22/28
arch/x86/kvm/cpuid.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)arch/x86/kvm/cpuid.c +0
-0
arch/x86/kvm/cpuid.c static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 431
Quoted 6 lines
arch/x86/kvm/cpuid.c +3
-0@@ -426,6 +426,11 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,switch (function) {case 0 : +/* Check if the cpuid leaf 0x1f is actually implemented */ +if (entry->eax >=0x1f && (cpuid_ebx (0x1f ) &0x0000ffff )) { + entry->eax =0x1f ;
Sorry for my late reply, but I think this check does more harm than good.
We'll need to change it if we ever add leaf above 0x1f, which also puts burden on the new submitter to check that exposing it by an unrelated feature doesn't break anything. Just like you had to see whether the leaf 0x14 is still ok when exposing it without f_intel_pt.
Also, I don't see anything that would make 0x1f worthy of protection when enabling it also exposes unimplemented leaves (0x14;0x1f). Zeroing 0x1f.ebx disables it and that is implicitly being done if the presence check above would fail.
arch/x86/kvm/cpuid.c static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 434
Quoted 3 lines
arch/x86/kvm/cpuid.c +2
-0+break ; + } entry->eax =min (entry->eax, (u32)(f_intel_pt ?0x14 :0xd ));
Similarly in the existing code. If we don't have f_intel_pt, then we should make sure that leaf 0x14 is not being filled, but we don't really have to limit the maximal index.
Adding a single clamping like
/* Limited to the highest leaf implemented in KVM. */ entry->eax = min(entry->eax, 0x1f);
seems sufficient.
(Passing the hardware value is ok in theory, but it is a cheap way to
avoid future leaves that cannot be simply zeroed for some weird reason.)
Thanks.
Quoted 10 lines · Radim Krčmář
arch/x86/kvm/cpuid.c static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 434
Quoted 3 lines
arch/x86/kvm/cpuid.c +2
-0+break ; + } entry->eax =min (entry->eax, (u32)(f_intel_pt ?0x14 :0xd ));Similarly in the existing code. If we don't have
f_intel_pt, then we should make sure that leaf0x14is not being filled, but we don't really have to limit the maximal index.Adding a single clamping like
Quoted 7 lines
/* Limited to the highest leaf implemented in KVM. */ entry->eax = min(entry->eax, 0x1f);seems sufficient.
(Passing the hardware value is ok in theory, but it is a cheap way to
avoid future leaves that cannot be simply zeroed for some weird reason.)
I don't have a strong opinion regarding the code itself, but whatever ends up getting committed should have a big beefy changelog explaining why the clamping exists, or at least extolling its virtues. I had a hell of a time understanding the intent of this one line of code because as your response shows, there is no one right answer.
Quoted 16 lines · Sean Christopherson
Quoted 15 lines · Radim Krčmář
arch/x86/kvm/cpuid.c static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 434
Quoted 3 lines
arch/x86/kvm/cpuid.c +2
-0+break ; + } entry->eax =min (entry->eax, (u32)(f_intel_pt ?0x14 :0xd ));Similarly in the existing code. If we don't have
f_intel_pt, then we should make sure that leaf0x14is not being filled, but we don't really have to limit the maximal index.Adding a single clamping like
/* Limited to the highest leaf implemented in KVM. */ entry->eax = min(entry->eax, 0x1f);seems sufficient.
Quoted 9 lines
Quoted 2 lines
(Passing the hardware value is ok in theory, but it is a cheap way to
avoid future leaves that cannot be simply zeroed for some weird reason.)I don't have a strong opinion regarding the code itself, but whatever ends up getting committed should have a big beefy changelog explaining why the clamping exists, or at least extolling its virtues. I had a hell of a time understanding the intent of this one line of code because as your response shows, there is no one right answer.
Hi Radim & Sean,
Thanks for your review and finally we find a better way.
Please help review the new version, I am not sure that the changelog could help new submitter understand why the clamping exists.