| Message ID | 1462826940-2422-1-git-send-email-rkrcmar@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 >
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.
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.
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 --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
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(+)