[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPY
|
From: |
Moger, Babu |
|
Subject: |
Re: [Qemu-devel] [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPYC CPU |
|
Date: |
Tue, 5 Jun 2018 15:54:47 +0000 |
> -----Original Message-----
> From: Eduardo Habkost [mailto:address@hidden
> Sent: Monday, June 4, 2018 9:57 PM
> To: Moger, Babu <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden
> 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 <address@hidden>
> > ---
> > 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