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

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

What about compatiblity on migration? With this patch, guests
will see CPUID data change when upgrading QEMU.

> 
> Signed-off-by: Radim Krčmář <address@hidden>
> ---
>  target-i386/cpu.c | 27 +++++++++++++++++++++++++++
>  target-i386/cpu.h |  5 +++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d0b5b691563c..4f8c32cccc88 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -35,6 +35,7 @@
>  #include "sysemu/arch_init.h"
>  
>  #include "hw/hw.h"
> +#include "hw/i386/topology.h"
>  #if defined(CONFIG_KVM)
>  #include <linux/kvm_para.h>
>  #endif
> @@ -2460,6 +2461,32 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>              *edx = 0;
>          }
>          break;
> +    case 0xB:
> +        /* Extended Topology Enumeration Leaf */
> +        *ecx = count & 0xff;
> +        *edx = cpu->apic_id;
> +
> +        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()?


> +        break;
>      case 0xD: {
>          KVMState *s = cs->kvm_state;
>          uint64_t ena_mask;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 732eb6d7ec79..b460c2debc1c 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -635,6 +635,11 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_MWAIT_IBE     (1U << 1) /* Interrupts can exit capability */
>  #define CPUID_MWAIT_EMX     (1U << 0) /* enumeration supported */
>  
> +/* CPUID[0xB].ECX level types */
> +#define CPUID_TOPOLOGY_LEVEL_INVALID  (0U << 8)
> +#define CPUID_TOPOLOGY_LEVEL_SMT      (1U << 8)
> +#define CPUID_TOPOLOGY_LEVEL_CORE     (2U << 8)
> +
>  #ifndef HYPERV_SPINLOCK_NEVER_RETRY
>  #define HYPERV_SPINLOCK_NEVER_RETRY             0xFFFFFFFF
>  #endif
> -- 
> 2.8.2
> 

-- 
Eduardo