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 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(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 80a642a0143d..f9b41f0103b3 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -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;
--
2.21.0
2019-05-26 21:30+0800, Like Xu:
> 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 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(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 80a642a0143d..f9b41f0103b3 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -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.
> + 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.
On Mon, Jun 03, 2019 at 06:56:17PM +0200, Radim Krčmář wrote:
> > + 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.)
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.
On 2019/6/4 3:18, Sean Christopherson wrote:
> On Mon, Jun 03, 2019 at 06:56:17PM +0200, Radim Krčmář wrote:
>>> + 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.)
>
> 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.