Re: [Qemu-devel] [PATCH] target-i386: implement CPUID[0xB] (Extended Topology Enumeration)

[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.

> 
> >> Signed-off-by: Radim Krčmář <address@hidden>
> >> ---
> >> 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.

-- 
Eduardo