diff mbox

target-i386: implement CPUID[0xB] (Extended Topology Enumeration)

Message ID 1462826940-2422-1-git-send-email-rkrcmar@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Radim Krčmář May 9, 2016, 8:49 p.m. UTC
I looked at a dozen Intel CPU that have this CPUID and all of them
always had Core offset as 1 (a wasted bit when hyperthreading is
disabled) and Package offset at least 4 (wasted bits at <= 4 cores).

QEMU uses more compact IDs and it doesn't make much sense to change it
now.  I keep the SMT and Core sub-leaves even if there is just one
thread/core;  it makes the code simpler and there should be no harm.

Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com>
---
 target-i386/cpu.c | 27 +++++++++++++++++++++++++++
 target-i386/cpu.h |  5 +++++
 2 files changed, 32 insertions(+)

Comments

Radim Krčmář May 10, 2016, 12:23 p.m. UTC | #1
2016-05-09 22:49+0200, Radim Kr?má?:
> I looked at a dozen Intel CPU that have this CPUID and all of them
> always had Core offset as 1 (a wasted bit when hyperthreading is
> disabled) and Package offset at least 4 (wasted bits at <= 4 cores).
> 
> QEMU uses more compact IDs and it doesn't make much sense to change it
> now.  I keep the SMT and Core sub-leaves even if there is just one
> thread/core;  it makes the code simpler and there should be no harm.
> 
> Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com>
> ---
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> @@ -2460,6 +2461,32 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              *edx = 0;
>          }
>          break;
> +    case 0xB:
> +        /* Extended Topology Enumeration Leaf */
> +        *ecx = count & 0xff;
> +        *edx = cpu->apic_id;
> +
> +        switch (*ecx) {

*ecx is wrong, should be count.  I'll wait few days for comments before
posting a v2.
Eduardo Habkost May 10, 2016, 7:53 p.m. UTC | #2
On Mon, May 09, 2016 at 10:49:00PM +0200, Radim Kr?má? wrote:
> I looked at a dozen Intel CPU that have this CPUID and all of them
> always had Core offset as 1 (a wasted bit when hyperthreading is
> disabled) and Package offset at least 4 (wasted bits at <= 4 cores).
> 
> QEMU uses more compact IDs and it doesn't make much sense to change it
> now.  I keep the SMT and Core sub-leaves even if there is just one
> thread/core;  it makes the code simpler and there should be no harm.

If in the future we really want to make the APIC ID offsets match
the CPUs you looked at, we can add extra X86CPU properties to
allow configuration of APIC ID bit offsets larger than the ones
calculated from smp_cores and smp_threads.

What about compatiblity on migration? With this patch, guests
will see CPUID data change when upgrading QEMU.

> 
> Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com>
> ---
>  target-i386/cpu.c | 27 +++++++++++++++++++++++++++
>  target-i386/cpu.h |  5 +++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d0b5b691563c..4f8c32cccc88 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -35,6 +35,7 @@
>  #include "sysemu/arch_init.h"
>  
>  #include "hw/hw.h"
> +#include "hw/i386/topology.h"
>  #if defined(CONFIG_KVM)
>  #include <linux/kvm_para.h>
>  #endif
> @@ -2460,6 +2461,32 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              *edx = 0;
>          }
>          break;
> +    case 0xB:
> +        /* Extended Topology Enumeration Leaf */
> +        *ecx = count & 0xff;
> +        *edx = cpu->apic_id;
> +
> +        switch (*ecx) {
> +        case 0:
> +            *eax = apicid_core_offset(smp_cores, smp_threads);
> +            *ebx = smp_threads;
> +            *ecx |= CPUID_TOPOLOGY_LEVEL_SMT;
> +            break;
> +        case 1:
> +            *eax = apicid_pkg_offset(smp_cores, smp_threads);
> +            *ebx = smp_cores * smp_threads;
> +            *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
> +            break;
> +        default:
> +            *eax = 0;
> +            *ebx = 0;
> +            *ecx |= CPUID_TOPOLOGY_LEVEL_INVALID;
> +        }
> +
> +        /* Preserve reserved bits.  Extremely unlikely to make a difference. */
> +        *eax &= 0x1f;
> +        *ebx &= 0xffff;

That means we don't know how to handle apicid_*_offset() >= 32,
smp_threads > 2^16, or smp_cores * smp_threads > 2^16. If that
happen one day, I would prefer to see QEMU abort than silently
truncating data in CPUID[0xB]. What about an assert()?


> +        break;
>      case 0xD: {
>          KVMState *s = cs->kvm_state;
>          uint64_t ena_mask;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 732eb6d7ec79..b460c2debc1c 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -635,6 +635,11 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_MWAIT_IBE     (1U << 1) /* Interrupts can exit capability */
>  #define CPUID_MWAIT_EMX     (1U << 0) /* enumeration supported */
>  
> +/* CPUID[0xB].ECX level types */
> +#define CPUID_TOPOLOGY_LEVEL_INVALID  (0U << 8)
> +#define CPUID_TOPOLOGY_LEVEL_SMT      (1U << 8)
> +#define CPUID_TOPOLOGY_LEVEL_CORE     (2U << 8)
> +
>  #ifndef HYPERV_SPINLOCK_NEVER_RETRY
>  #define HYPERV_SPINLOCK_NEVER_RETRY             0xFFFFFFFF
>  #endif
> -- 
> 2.8.2
>
Radim Krčmář May 11, 2016, 12:37 p.m. UTC | #3
2016-05-10 16:53-0300, Eduardo Habkost:
> On Mon, May 09, 2016 at 10:49:00PM +0200, Radim Kr?má? wrote:
>> I looked at a dozen Intel CPU that have this CPUID and all of them
>> always had Core offset as 1 (a wasted bit when hyperthreading is
>> disabled) and Package offset at least 4 (wasted bits at <= 4 cores).
>> 
>> QEMU uses more compact IDs and it doesn't make much sense to change it
>> now.  I keep the SMT and Core sub-leaves even if there is just one
>> thread/core;  it makes the code simpler and there should be no harm.
> 
> If in the future we really want to make the APIC ID offsets match
> the CPUs you looked at, we can add extra X86CPU properties to
> allow configuration of APIC ID bit offsets larger than the ones
> calculated from smp_cores and smp_threads.

Sounds good.  No hurry with that as virt has no problem with routing a
x2APIC cluster that covers two packages and I'm not sure what the
reasoning for HT is.

> What about compatiblity on migration? With this patch, guests
> will see CPUID data change when upgrading QEMU.

Ah, thanks, I always forget that QEMU doesn't migrate all configuration
and has machine types ...

I don't think that we can directly override values from cpu_x86_cpuid()
with machine type wrappers.  What about adding a compatibility flags
into X86CPUDefinition and assigning one flag to disable CPUID 0xB, or a
global flag?

>> Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com>
>> ---
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> @@ -2460,6 +2461,32 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>> +        switch (*ecx) {
>> +        case 0:
>> +            *eax = apicid_core_offset(smp_cores, smp_threads);
>> +            *ebx = smp_threads;
>> +            *ecx |= CPUID_TOPOLOGY_LEVEL_SMT;
>> +            break;
>> +        case 1:
>> +            *eax = apicid_pkg_offset(smp_cores, smp_threads);
>> +            *ebx = smp_cores * smp_threads;
>> +            *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
>> +            break;
>> +        default:
>> +            *eax = 0;
>> +            *ebx = 0;
>> +            *ecx |= CPUID_TOPOLOGY_LEVEL_INVALID;
>> +        }
>> +
>> +        /* Preserve reserved bits.  Extremely unlikely to make a difference. */
>> +        *eax &= 0x1f;
>> +        *ebx &= 0xffff;
> 
> That means we don't know how to handle apicid_*_offset() >= 32,
> smp_threads > 2^16, or smp_cores * smp_threads > 2^16. If that
> happen one day, I would prefer to see QEMU abort than silently
> truncating data in CPUID[0xB]. What about an assert()?

I skipped an assert because the spec says that *ebx cannot be taken
seriously, so killing the guest didn't seem worth it:
  Software must not use EBX[15:0] to enumerate processor topology of the
  system. This value in this field (EBX[15:0]) is only intended for
  display/diagnostic purposes. The actual number of logical processors
  available to BIOS/OS/Applications may be different from the value of
  EBX[15:0], depending on software and platform hardware configurations.

I'd warn, but I don't know if 'printf' is ok, so I skipped it too,
because the overflow really doesn't matter.
Eduardo Habkost May 11, 2016, 3:26 p.m. UTC | #4
On Wed, May 11, 2016 at 02:37:38PM +0200, Radim Kr?má? wrote:
> 2016-05-10 16:53-0300, Eduardo Habkost:
> > On Mon, May 09, 2016 at 10:49:00PM +0200, Radim Kr?má? wrote:
> >> I looked at a dozen Intel CPU that have this CPUID and all of them
> >> always had Core offset as 1 (a wasted bit when hyperthreading is
> >> disabled) and Package offset at least 4 (wasted bits at <= 4 cores).
> >> 
> >> QEMU uses more compact IDs and it doesn't make much sense to change it
> >> now.  I keep the SMT and Core sub-leaves even if there is just one
> >> thread/core;  it makes the code simpler and there should be no harm.
> > 
> > If in the future we really want to make the APIC ID offsets match
> > the CPUs you looked at, we can add extra X86CPU properties to
> > allow configuration of APIC ID bit offsets larger than the ones
> > calculated from smp_cores and smp_threads.
> 
> Sounds good.  No hurry with that as virt has no problem with routing a
> x2APIC cluster that covers two packages and I'm not sure what the
> reasoning for HT is.
> 
> > What about compatiblity on migration? With this patch, guests
> > will see CPUID data change when upgrading QEMU.
> 
> Ah, thanks, I always forget that QEMU doesn't migrate all configuration
> and has machine types ...

Even if we did migrate CPUID data (something I have been
considering), changing CPUID on non-live migration would still be
a problem. It's hard to know if CPUID changes are going to
surprise some guest software, even if it's after a VM cold
reboot.

In this case, I won't be surprised if some software uses CPU
topology information from CPUID[0xB] for license validation. And
I wouldn't want to find that out by getting a bug report from
somebody running it in production a few years from now.

> 
> I don't think that we can directly override values from cpu_x86_cpuid()
> with machine type wrappers.  What about adding a compatibility flags
> into X86CPUDefinition and assigning one flag to disable CPUID 0xB, or a
> global flag?

Adding a new boolean property to X86CPU (defaulting to true) and
setting it to false on PC_COMPAT_2_6 is the preferred way to do
it.

> 
> >> Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com>
> >> ---
> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >> @@ -2460,6 +2461,32 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >> +        switch (*ecx) {
> >> +        case 0:
> >> +            *eax = apicid_core_offset(smp_cores, smp_threads);
> >> +            *ebx = smp_threads;
> >> +            *ecx |= CPUID_TOPOLOGY_LEVEL_SMT;
> >> +            break;
> >> +        case 1:
> >> +            *eax = apicid_pkg_offset(smp_cores, smp_threads);
> >> +            *ebx = smp_cores * smp_threads;
> >> +            *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
> >> +            break;
> >> +        default:
> >> +            *eax = 0;
> >> +            *ebx = 0;
> >> +            *ecx |= CPUID_TOPOLOGY_LEVEL_INVALID;
> >> +        }
> >> +
> >> +        /* Preserve reserved bits.  Extremely unlikely to make a difference. */
> >> +        *eax &= 0x1f;
> >> +        *ebx &= 0xffff;
> > 
> > That means we don't know how to handle apicid_*_offset() >= 32,
> > smp_threads > 2^16, or smp_cores * smp_threads > 2^16. If that
> > happen one day, I would prefer to see QEMU abort than silently
> > truncating data in CPUID[0xB]. What about an assert()?
> 
> I skipped an assert because the spec says that *ebx cannot be taken
> seriously, so killing the guest didn't seem worth it:
>   Software must not use EBX[15:0] to enumerate processor topology of the
>   system. This value in this field (EBX[15:0]) is only intended for
>   display/diagnostic purposes. The actual number of logical processors
>   available to BIOS/OS/Applications may be different from the value of
>   EBX[15:0], depending on software and platform hardware configurations.
> 
> I'd warn, but I don't know if 'printf' is ok, so I skipped it too,
> because the overflow really doesn't matter.

We still have *eax, that is documented as "Software should use
this field to enumerate processor topology of the system".

But I don't mind if you prefer to not assert(). If in the future
somebody decide to support smp_threads * smp_cores >= 2^32
(that would make apicid_pkg_offset() >= 32), we can let them
decide what should be reported in CPUID[0xB] and fix this.
Radim Krčmář May 11, 2016, 3:59 p.m. UTC | #5
2016-05-11 12:26-0300, Eduardo Habkost:
> On Wed, May 11, 2016 at 02:37:38PM +0200, Radim Kr?má? wrote:
>> 2016-05-10 16:53-0300, Eduardo Habkost:
>> > On Mon, May 09, 2016 at 10:49:00PM +0200, Radim Kr?má? wrote:
>> >> I looked at a dozen Intel CPU that have this CPUID and all of them
>> >> always had Core offset as 1 (a wasted bit when hyperthreading is
>> >> disabled) and Package offset at least 4 (wasted bits at <= 4 cores).
>> >> 
>> >> QEMU uses more compact IDs and it doesn't make much sense to change it
>> >> now.  I keep the SMT and Core sub-leaves even if there is just one
>> >> thread/core;  it makes the code simpler and there should be no harm.
>> > 
>> > If in the future we really want to make the APIC ID offsets match
>> > the CPUs you looked at, we can add extra X86CPU properties to
>> > allow configuration of APIC ID bit offsets larger than the ones
>> > calculated from smp_cores and smp_threads.
>> 
>> Sounds good.  No hurry with that as virt has no problem with routing a
>> x2APIC cluster that covers two packages and I'm not sure what the
>> reasoning for HT is.
>> 
>> > What about compatiblity on migration? With this patch, guests
>> > will see CPUID data change when upgrading QEMU.
>> 
>> Ah, thanks, I always forget that QEMU doesn't migrate all configuration
>> and has machine types ...
> 
> Even if we did migrate CPUID data (something I have been
> considering), changing CPUID on non-live migration would still be
> a problem. It's hard to know if CPUID changes are going to
> surprise some guest software, even if it's after a VM cold
> reboot.
> 
> In this case, I won't be surprised if some software uses CPU
> topology information from CPUID[0xB] for license validation. And
> I wouldn't want to find that out by getting a bug report from
> somebody running it in production a few years from now.
> 
>> 
>> I don't think that we can directly override values from cpu_x86_cpuid()
>> with machine type wrappers.  What about adding a compatibility flags
>> into X86CPUDefinition and assigning one flag to disable CPUID 0xB, or a
>> global flag?
> 
> Adding a new boolean property to X86CPU (defaulting to true) and
> setting it to false on PC_COMPAT_2_6 is the preferred way to do
> it.

Will do.

>> >> Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com>
>> >> ---
>> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> >> @@ -2460,6 +2461,32 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>> >> +        switch (*ecx) {
>> >> +        case 0:
>> >> +            *eax = apicid_core_offset(smp_cores, smp_threads);
>> >> +            *ebx = smp_threads;
>> >> +            *ecx |= CPUID_TOPOLOGY_LEVEL_SMT;
>> >> +            break;
>> >> +        case 1:
>> >> +            *eax = apicid_pkg_offset(smp_cores, smp_threads);
>> >> +            *ebx = smp_cores * smp_threads;
>> >> +            *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
>> >> +            break;
>> >> +        default:
>> >> +            *eax = 0;
>> >> +            *ebx = 0;
>> >> +            *ecx |= CPUID_TOPOLOGY_LEVEL_INVALID;
>> >> +        }
>> >> +
>> >> +        /* Preserve reserved bits.  Extremely unlikely to make a difference. */
>> >> +        *eax &= 0x1f;
>> >> +        *ebx &= 0xffff;
>> > 
>> > That means we don't know how to handle apicid_*_offset() >= 32,
>> > smp_threads > 2^16, or smp_cores * smp_threads > 2^16. If that
>> > happen one day, I would prefer to see QEMU abort than silently
>> > truncating data in CPUID[0xB]. What about an assert()?
>> 
>> I skipped an assert because the spec says that *ebx cannot be taken
>> seriously, so killing the guest didn't seem worth it:
>>   Software must not use EBX[15:0] to enumerate processor topology of the
>>   system. This value in this field (EBX[15:0]) is only intended for
>>   display/diagnostic purposes. The actual number of logical processors
>>   available to BIOS/OS/Applications may be different from the value of
>>   EBX[15:0], depending on software and platform hardware configurations.
>> 
>> I'd warn, but I don't know if 'printf' is ok, so I skipped it too,
>> because the overflow really doesn't matter.
> 
> We still have *eax, that is documented as "Software should use
> this field to enumerate processor topology of the system".
> 
> But I don't mind if you prefer to not assert(). If in the future
> somebody decide to support smp_threads * smp_cores >= 2^32
> (that would make apicid_pkg_offset() >= 32), we can let them
> decide what should be reported in CPUID[0xB] and fix this.

eax can store offsets up to 31 and edx cannot store more than 32 bits,
so it should trip elsewhere.  I'll add an assert for eax, though.

Thanks.
diff mbox

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d0b5b691563c..4f8c32cccc88 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -35,6 +35,7 @@ 
 #include "sysemu/arch_init.h"
 
 #include "hw/hw.h"
+#include "hw/i386/topology.h"
 #if defined(CONFIG_KVM)
 #include <linux/kvm_para.h>
 #endif
@@ -2460,6 +2461,32 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *edx = 0;
         }
         break;
+    case 0xB:
+        /* Extended Topology Enumeration Leaf */
+        *ecx = count & 0xff;
+        *edx = cpu->apic_id;
+
+        switch (*ecx) {
+        case 0:
+            *eax = apicid_core_offset(smp_cores, smp_threads);
+            *ebx = smp_threads;
+            *ecx |= CPUID_TOPOLOGY_LEVEL_SMT;
+            break;
+        case 1:
+            *eax = apicid_pkg_offset(smp_cores, smp_threads);
+            *ebx = smp_cores * smp_threads;
+            *ecx |= CPUID_TOPOLOGY_LEVEL_CORE;
+            break;
+        default:
+            *eax = 0;
+            *ebx = 0;
+            *ecx |= CPUID_TOPOLOGY_LEVEL_INVALID;
+        }
+
+        /* Preserve reserved bits.  Extremely unlikely to make a difference. */
+        *eax &= 0x1f;
+        *ebx &= 0xffff;
+        break;
     case 0xD: {
         KVMState *s = cs->kvm_state;
         uint64_t ena_mask;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 732eb6d7ec79..b460c2debc1c 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -635,6 +635,11 @@  typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_MWAIT_IBE     (1U << 1) /* Interrupts can exit capability */
 #define CPUID_MWAIT_EMX     (1U << 0) /* enumeration supported */
 
+/* CPUID[0xB].ECX level types */
+#define CPUID_TOPOLOGY_LEVEL_INVALID  (0U << 8)
+#define CPUID_TOPOLOGY_LEVEL_SMT      (1U << 8)
+#define CPUID_TOPOLOGY_LEVEL_CORE     (2U << 8)
+
 #ifndef HYPERV_SPINLOCK_NEVER_RETRY
 #define HYPERV_SPINLOCK_NEVER_RETRY             0xFFFFFFFF
 #endif