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

[Radim Krčmář]

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

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.