> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@xxxxxxxxxx] > Sent: Monday, June 4, 2018 9:57 PM > To: Moger, Babu <Babu.Moger@xxxxxxx> > Cc: mst@xxxxxxxxxx; marcel.apfelbaum@xxxxxxxxx; pbonzini@xxxxxxxxxx; > rth@xxxxxxxxxxx; mtosatti@xxxxxxxxxx; geoff@xxxxxxxxxxxxxxx; > kash@xxxxxxxxxxxxxx; qemu-devel@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx > Subject: Re: [Qemu-devel] [PATCH v11 4/5] i386: Enable TOPOEXT feature on > AMD EPYC CPU > > 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@xxxxxxx> > > --- > > 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()". Ok. Sure. > > > +{ > > + 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. Ok. Will make a separate patch and include before this patch. > > > > + } > > + 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(). Ok. I will bring the function, verify_topology(or new topology_supports_topoext) in here. > > 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. Ok. Will add those checks in verify_topology(or new topology_supports_topoext). > > > > + } > > 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
