Re: [Qemu-devel] [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPYC CPU

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, May 24, 2018 at 11:43:33AM -0400, Babu Moger wrote:
> Enable TOPOEXT feature on EPYC CPU. This is required to support
> hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> 
> Disable TOPOEXT feature for legacy machines and also disable
> TOPOEXT feature if the config cannot be supported.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  include/hw/i386/pc.h |  4 ++++
>  target/i386/cpu.c    | 37 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index a0c269f..9c8db3d 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -302,6 +302,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>          .driver   = TYPE_X86_CPU,\
>          .property = "legacy-cache",\
>          .value    = "on",\
> +    },{\
> +        .driver   = "EPYC-" TYPE_X86_CPU,\
> +        .property = "topoext",\
> +        .value    = "off",\
>      },
>  
>  #define PC_COMPAT_2_11 \
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 9f8bad9..0b62356 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -486,6 +486,20 @@ static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
>  }
>  
>  /*
> + * Check if we can support this topology
> + * Fail if number of cores are beyond the supported config
> + * or nr_threads is more than 2
> + */
> +static int verify_topology(int nr_cores, int nr_threads)

I suggest using a more descriptive name, like
"topology_supports_topoext()".

> +{
> +    if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) ||
> +        (nr_threads > 2)) {
> +        return 0;

Oh, this is what I was looking for, on the previous patches.  I
suggest moving this to a separate patch, separate from the patch
that changes the EPYC CPU model.


> +    }
> +    return 1;
> +}
> +
> +/*
>   * Definitions of the hardcoded cache entries we expose:
>   * These are legacy cache values. If there is a need to change any
>   * of these values please use builtin_x86_defs
> @@ -2531,7 +2545,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .features[FEAT_8000_0001_ECX] =
>              CPUID_EXT3_OSVW | CPUID_EXT3_3DNOWPREFETCH |
>              CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
> -            CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
> +            CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM |
> +            CPUID_EXT3_TOPOEXT,
>          .features[FEAT_7_0_EBX] =
>              CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_AVX2 |
>              CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_RDSEED |
> @@ -2576,7 +2591,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .features[FEAT_8000_0001_ECX] =
>              CPUID_EXT3_OSVW | CPUID_EXT3_3DNOWPREFETCH |
>              CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
> -            CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
> +            CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM |
> +            CPUID_EXT3_TOPOEXT,
>          .features[FEAT_8000_0008_EBX] =
>              CPUID_8000_0008_EBX_IBPB,
>          .features[FEAT_7_0_EBX] =
> @@ -4156,6 +4172,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          break;
>      case 0x8000001D:
>          *eax = 0;
> +        /* Check if we can support this topology */
> +        if (!verify_topology(cs->nr_cores, cs->nr_threads)) {
> +            /* Disable topology extention */
> +            env->features[FEAT_8000_0001_ECX] &= !CPUID_EXT3_TOPOEXT;
> +            break;

Please don't change CPUX86State inside cpu_x86_cpuid().  This
belongs to x86_cpu_realizefn().

Also, we want this:
  "-cpu EPYC -smp cores=64"
to not error out, because it's a supported configuration today.

But this:
  "-cpu EPYC,+topoext -smp cores=64"
should error out because the user explicitly requested an
unsupported configuration.

You can implement this by checking if TOPOEXT is set on
env->user_features.


> +        }
>          switch (count) {
>          case 0: /* L1 dcache info */
>              encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
> @@ -4180,6 +4202,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          break;
>      case 0x8000001E:
>          assert(cpu->core_id <= 255);
> +        /* Check if we can support this topology */
> +        if (!verify_topology(cs->nr_cores, cs->nr_threads)) {
> +            /* Disable topology extention */
> +            env->features[FEAT_8000_0001_ECX] &= !CPUID_EXT3_TOPOEXT;
> +            break;
> +        }
>          encode_topo_cpuid8000001e(cs, cpu,
>                                    eax, ebx, ecx, edx);
>          break;
> @@ -4644,6 +4672,11 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>              x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000000A);
>          }
>  
> +        /* TOPOEXT feature requires 0x8000001E */
> +        if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
> +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> +        }
> +
>          /* SEV requires CPUID[0x8000001F] */
>          if (sev_enabled()) {
>              x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001F);
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo