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

["Radim Krčmář" <rkrcmar@redhat.com>]

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.