[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Radim Krčmář
Subject: Re: [Qemu-devel] [PATCH] target-i386: implement CPUID[0xB] (Extended Topology Enumeration)
Date: Wed, 11 May 2016 14:37:38 +0200

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ář <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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]