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