* [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU
@ 2018-05-24 15:43 ` Babu Moger
0 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: geoff, babu.moger, kash, qemu-devel, kvm
This series enables the TOPOEXT feature for AMD CPUs. This is required to
support hyperthreading on kvm guests.
This addresses the issues reported in these bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1481253
https://bugs.launchpad.net/qemu/+bug/1703506
v11:
Patches are based off of Eduardo's git://github.com/ehabkost/qemu.git x86-next.
Summary of changes.
1.Added more comments explaining different constants and variables.
2.Removed NUM_SHARING_CACHE macro and made the code simpler.
3.Changed the function name num_sharing_l3_cache to cores_in_core_complex.
This function is actually finding the number of cores in a core complex.
Purpose here is to re-use the code in couple more places.
4.Added new function nodes_in_socket to find number of nodes in the config.
Purpose here is to re-use the code.
5.Used DIV_ROUND_UP wherever applicable.
6.Renamed few constants and functions to generic names.
7.Few more text corrections.
v10:
Based the patches on Eduardo's git://github.com/ehabkost/qemu.git x86-next
Some of the earlier patches are already queued. So, submitting the rest of
the series here. This series adds complete redesign of the cpu topology.
Based on user given parameter, we try to build topology very close to the
hardware. Maintains symmetry as much as possible. Added new function
epyc_build_topology to build the topology based on user given nr_cores,
nr_threads.
Summary of changes.
1. Build the topology dinamically based on nr_cores and nr_threads
2. Added new epyc_build_topology to build the new topology.
3. Added new function num_sharing_l3_cache to calculate the L3 sharing
4. Added a check to verify the topology. Disabled the TOPOEXT if the
topology cannot be built.
v9:
Based the patches on Eduardo's git://github.com/ehabkost/qemu.git x86-next
tree. Following 3 patches from v8 are already queued.
i386: Add cache information in X86CPUDefinition
i386: Initialize cache information for EPYC family processors
i386: Helpers to encode cache information consistently
So, submitting the rest of the series here.
Changes:
1. Included Eduardo's clean up patch
2. Added 2.13 machine types
3. Disabled topoext for 2.12 and below versions.
4. Added the assert to core_id as discussed.
v8:
Addressed feedback from Eduardo. Thanks Eduardo for being patient with me.
Tested on AMD EPYC server and also did some basic testing on intel box.
Summary of changes.
1. Reverted back l2 cache associativity. Kept it same as legacy.
2. Changed cache_info structure in X86CPUDefinition and CPUX86State to pointers.
3. Added legacy_cache property in PC_COMPAT_2_12 and initialized legacy_cache
based on static cache_info availability.
4. Squashed patch 4 and 5 and applied it before patch 3.
5. Added legacy cache check for cpuid[2] and cpuid[4] for consistancy.
6. Simplified NUM_SHARING_CACHE definition for readability,
7. Removed assert for core_id as it appeared redundant.
8. Simplified encode_cache_cpuid8000001d little bit.
9. Few more minor changes
v7:
Rebased on top of latest tree after 2.12 release and done few basic tests. There are
no changes except for few minor hunks. Hopefully this gets pulled into 2.13 release.
Please review, let me know of any feedback.
v6:
1.Fixed problem with patch#4(Add new property to control cache info). The parameter
legacy_cache should be "on" by default on machine type "pc-q35-2.10". This was
found by Alexandr Iarygin.
2.Fixed the l3 cache size for EPYC based machines(patch#3). Also, fixed the number of
logical processors sharing the cache(patch#6). Only L3 cache is shared by multiple
cores but not L1 or L2. This was a bug while decoding. This was found by Geoffrey McRae
and he verified the fix.
v5:
In this series I tried to address the feedback from Eduardo Habkost.
The discussion thread is here.
https://patchwork.kernel.org/patch/10299745/
The previous thread is here.
http://patchwork.ozlabs.org/cover/884885/
Reason for these changes.
The cache properties for AMD family of processors have changed from
previous releases. We don't want to display the new information on the
old family of processors as this might cause compatibility issues.
Changes:
1.Based the patches on top of Eduardo's(patch#1) patch.
Changed few things.
Moved the Cache definitions to cpu.h file.
Changed the CPUID_4 names to generic names.
2.Added a new propery "legacy-cache" in cpu object(patch#2). This can be
used to display the old property even if the host supports the new cache
properties.
3.Added cache information in X86CPUDefinition and CPUX86State
4.Patch 6-7 changed quite a bit from previous version does to new approach.
5.Addressed few issues with CPUID_8000_001d and CPUID_8000_001E.
v4:
1.Removed the checks under cpuid 0x8000001D leaf(patch #2). These check are
not necessary. Found this during internal review.
2.Added CPUID_EXT3_TOPOEXT feature for all the 17 family(patch #4). This was
found by Kash Pande during his testing.
3.Removed th hardcoded cpuid xlevel and dynamically extended if CPUID_EXT3_TOPOEXT
is supported(Suggested by Brijesh Singh).
v3:
1.Removed the patch #1. Radim mentioned that original typo problem is in
linux kernel header. qemu is just copying those files.
2.In previous version, I used the cpuid 4 definitions for AMDs cpuid leaf
0x8000001D. CPUID 4 is very intel specific and we dont want to expose those
details under AMD. I have renamed some of these definitions as generic.
These changes are in patch#1. Radim, let me know if this is what you intended.
3.Added assert to for core_id(Suggested by Radim Krcmár).
4.Changed the if condition under "L3 cache info"(Suggested by Gary Hook).
5.Addressed few more text correction and code cleanup(Suggested by Thomas Lendacky).
v2:
Fixed few more minor issues per Gary Hook's comments. Thank you Gary.
Removed the patch#1. We need to handle the instruction cache associativity
seperately. It varies based on the cpu family. I will comeback to that later.
Added two more typo corrections in patch#1 and patch#5.
v1:
Stanislav Lanci posted few patches earlier.
https://patchwork.kernel.org/patch/10040903/
Rebased his patches with few changes.
1.Spit the patches into two, separating cpuid functions
0x8000001D and 0x8000001E (Patch 2 and 3).
2.Removed the generic non-intel check and made a separate patch
with some changes(Patch 5).
3.Fixed L3_N_SETS_AMD(from 4096 to 8192) based on CPUID_Fn8000001D_ECX_x03.
Added 2 more patches.
Patch 1. Fixes cache associativity.
Patch 4. Adds TOPOEXT feature on AMD EPYC CPU.
Babu Moger (4):
i386: Populate AMD Processor Cache Information for cpuid 0x8000001D
i386: Add support for CPUID_8000_001E for AMD
i386: Enable TOPOEXT feature on AMD EPYC CPU
i386: Remove generic SMT thread check
Eduardo Habkost (1):
i386: Clean up cache CPUID code
include/hw/i386/pc.h | 4 +
target/i386/cpu.c | 347 +++++++++++++++++++++++++++++++++++++++++----------
target/i386/cpu.h | 14 ++-
target/i386/kvm.c | 29 ++++-
4 files changed, 319 insertions(+), 75 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU
@ 2018-05-24 15:43 ` Babu Moger
0 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: qemu-devel, kvm, babu.moger, kash, geoff
This series enables the TOPOEXT feature for AMD CPUs. This is required to
support hyperthreading on kvm guests.
This addresses the issues reported in these bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1481253
https://bugs.launchpad.net/qemu/+bug/1703506
v11:
Patches are based off of Eduardo's git://github.com/ehabkost/qemu.git x86-next.
Summary of changes.
1.Added more comments explaining different constants and variables.
2.Removed NUM_SHARING_CACHE macro and made the code simpler.
3.Changed the function name num_sharing_l3_cache to cores_in_core_complex.
This function is actually finding the number of cores in a core complex.
Purpose here is to re-use the code in couple more places.
4.Added new function nodes_in_socket to find number of nodes in the config.
Purpose here is to re-use the code.
5.Used DIV_ROUND_UP wherever applicable.
6.Renamed few constants and functions to generic names.
7.Few more text corrections.
v10:
Based the patches on Eduardo's git://github.com/ehabkost/qemu.git x86-next
Some of the earlier patches are already queued. So, submitting the rest of
the series here. This series adds complete redesign of the cpu topology.
Based on user given parameter, we try to build topology very close to the
hardware. Maintains symmetry as much as possible. Added new function
epyc_build_topology to build the topology based on user given nr_cores,
nr_threads.
Summary of changes.
1. Build the topology dinamically based on nr_cores and nr_threads
2. Added new epyc_build_topology to build the new topology.
3. Added new function num_sharing_l3_cache to calculate the L3 sharing
4. Added a check to verify the topology. Disabled the TOPOEXT if the
topology cannot be built.
v9:
Based the patches on Eduardo's git://github.com/ehabkost/qemu.git x86-next
tree. Following 3 patches from v8 are already queued.
i386: Add cache information in X86CPUDefinition
i386: Initialize cache information for EPYC family processors
i386: Helpers to encode cache information consistently
So, submitting the rest of the series here.
Changes:
1. Included Eduardo's clean up patch
2. Added 2.13 machine types
3. Disabled topoext for 2.12 and below versions.
4. Added the assert to core_id as discussed.
v8:
Addressed feedback from Eduardo. Thanks Eduardo for being patient with me.
Tested on AMD EPYC server and also did some basic testing on intel box.
Summary of changes.
1. Reverted back l2 cache associativity. Kept it same as legacy.
2. Changed cache_info structure in X86CPUDefinition and CPUX86State to pointers.
3. Added legacy_cache property in PC_COMPAT_2_12 and initialized legacy_cache
based on static cache_info availability.
4. Squashed patch 4 and 5 and applied it before patch 3.
5. Added legacy cache check for cpuid[2] and cpuid[4] for consistancy.
6. Simplified NUM_SHARING_CACHE definition for readability,
7. Removed assert for core_id as it appeared redundant.
8. Simplified encode_cache_cpuid8000001d little bit.
9. Few more minor changes
v7:
Rebased on top of latest tree after 2.12 release and done few basic tests. There are
no changes except for few minor hunks. Hopefully this gets pulled into 2.13 release.
Please review, let me know of any feedback.
v6:
1.Fixed problem with patch#4(Add new property to control cache info). The parameter
legacy_cache should be "on" by default on machine type "pc-q35-2.10". This was
found by Alexandr Iarygin.
2.Fixed the l3 cache size for EPYC based machines(patch#3). Also, fixed the number of
logical processors sharing the cache(patch#6). Only L3 cache is shared by multiple
cores but not L1 or L2. This was a bug while decoding. This was found by Geoffrey McRae
and he verified the fix.
v5:
In this series I tried to address the feedback from Eduardo Habkost.
The discussion thread is here.
https://patchwork.kernel.org/patch/10299745/
The previous thread is here.
http://patchwork.ozlabs.org/cover/884885/
Reason for these changes.
The cache properties for AMD family of processors have changed from
previous releases. We don't want to display the new information on the
old family of processors as this might cause compatibility issues.
Changes:
1.Based the patches on top of Eduardo's(patch#1) patch.
Changed few things.
Moved the Cache definitions to cpu.h file.
Changed the CPUID_4 names to generic names.
2.Added a new propery "legacy-cache" in cpu object(patch#2). This can be
used to display the old property even if the host supports the new cache
properties.
3.Added cache information in X86CPUDefinition and CPUX86State
4.Patch 6-7 changed quite a bit from previous version does to new approach.
5.Addressed few issues with CPUID_8000_001d and CPUID_8000_001E.
v4:
1.Removed the checks under cpuid 0x8000001D leaf(patch #2). These check are
not necessary. Found this during internal review.
2.Added CPUID_EXT3_TOPOEXT feature for all the 17 family(patch #4). This was
found by Kash Pande during his testing.
3.Removed th hardcoded cpuid xlevel and dynamically extended if CPUID_EXT3_TOPOEXT
is supported(Suggested by Brijesh Singh).
v3:
1.Removed the patch #1. Radim mentioned that original typo problem is in
linux kernel header. qemu is just copying those files.
2.In previous version, I used the cpuid 4 definitions for AMDs cpuid leaf
0x8000001D. CPUID 4 is very intel specific and we dont want to expose those
details under AMD. I have renamed some of these definitions as generic.
These changes are in patch#1. Radim, let me know if this is what you intended.
3.Added assert to for core_id(Suggested by Radim Krcmár).
4.Changed the if condition under "L3 cache info"(Suggested by Gary Hook).
5.Addressed few more text correction and code cleanup(Suggested by Thomas Lendacky).
v2:
Fixed few more minor issues per Gary Hook's comments. Thank you Gary.
Removed the patch#1. We need to handle the instruction cache associativity
seperately. It varies based on the cpu family. I will comeback to that later.
Added two more typo corrections in patch#1 and patch#5.
v1:
Stanislav Lanci posted few patches earlier.
https://patchwork.kernel.org/patch/10040903/
Rebased his patches with few changes.
1.Spit the patches into two, separating cpuid functions
0x8000001D and 0x8000001E (Patch 2 and 3).
2.Removed the generic non-intel check and made a separate patch
with some changes(Patch 5).
3.Fixed L3_N_SETS_AMD(from 4096 to 8192) based on CPUID_Fn8000001D_ECX_x03.
Added 2 more patches.
Patch 1. Fixes cache associativity.
Patch 4. Adds TOPOEXT feature on AMD EPYC CPU.
Babu Moger (4):
i386: Populate AMD Processor Cache Information for cpuid 0x8000001D
i386: Add support for CPUID_8000_001E for AMD
i386: Enable TOPOEXT feature on AMD EPYC CPU
i386: Remove generic SMT thread check
Eduardo Habkost (1):
i386: Clean up cache CPUID code
include/hw/i386/pc.h | 4 +
target/i386/cpu.c | 347 +++++++++++++++++++++++++++++++++++++++++----------
target/i386/cpu.h | 14 ++-
target/i386/kvm.c | 29 ++++-
4 files changed, 319 insertions(+), 75 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v11 1/5] i386: Clean up cache CPUID code
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
@ 2018-05-24 15:43 ` Babu Moger
-1 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: geoff, babu.moger, kash, qemu-devel, kvm
From: Eduardo Habkost <ehabkost@redhat.com>
Always initialize CPUCaches structs with cache information, even
if legacy_cache=true. Use different CPUCaches struct for
CPUID[2], CPUID[4], and the AMD CPUID leaves.
This will simplify a lot the logic inside cpu_x86_cpuid().
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
target/i386/cpu.c | 117 +++++++++++++++++++++++++++---------------------------
target/i386/cpu.h | 14 ++++---
2 files changed, 67 insertions(+), 64 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d95310f..5c9bdc9 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1114,7 +1114,7 @@ struct X86CPUDefinition {
};
static CPUCaches epyc_cache_info = {
- .l1d_cache = {
+ .l1d_cache = &(CPUCacheInfo) {
.type = DCACHE,
.level = 1,
.size = 32 * KiB,
@@ -1126,7 +1126,7 @@ static CPUCaches epyc_cache_info = {
.self_init = 1,
.no_invd_sharing = true,
},
- .l1i_cache = {
+ .l1i_cache = &(CPUCacheInfo) {
.type = ICACHE,
.level = 1,
.size = 64 * KiB,
@@ -1138,7 +1138,7 @@ static CPUCaches epyc_cache_info = {
.self_init = 1,
.no_invd_sharing = true,
},
- .l2_cache = {
+ .l2_cache = &(CPUCacheInfo) {
.type = UNIFIED_CACHE,
.level = 2,
.size = 512 * KiB,
@@ -1148,7 +1148,7 @@ static CPUCaches epyc_cache_info = {
.sets = 1024,
.lines_per_tag = 1,
},
- .l3_cache = {
+ .l3_cache = &(CPUCacheInfo) {
.type = UNIFIED_CACHE,
.level = 3,
.size = 8 * MiB,
@@ -3342,9 +3342,8 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
env->features[w] = def->features[w];
}
- /* Store Cache information from the X86CPUDefinition if available */
- env->cache_info = def->cache_info;
- cpu->legacy_cache = def->cache_info ? 0 : 1;
+ /* legacy-cache defaults to 'off' if CPU model provides cache info */
+ cpu->legacy_cache = !def->cache_info;
/* Special cases not set in the X86CPUDefinition structs: */
/* TODO: in-kernel irqchip for hvf */
@@ -3695,21 +3694,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
if (!cpu->enable_l3_cache) {
*ecx = 0;
} else {
- if (env->cache_info && !cpu->legacy_cache) {
- *ecx = cpuid2_cache_descriptor(&env->cache_info->l3_cache);
- } else {
- *ecx = cpuid2_cache_descriptor(&legacy_l3_cache);
- }
- }
- if (env->cache_info && !cpu->legacy_cache) {
- *edx = (cpuid2_cache_descriptor(&env->cache_info->l1d_cache) << 16) |
- (cpuid2_cache_descriptor(&env->cache_info->l1i_cache) << 8) |
- (cpuid2_cache_descriptor(&env->cache_info->l2_cache));
- } else {
- *edx = (cpuid2_cache_descriptor(&legacy_l1d_cache) << 16) |
- (cpuid2_cache_descriptor(&legacy_l1i_cache) << 8) |
- (cpuid2_cache_descriptor(&legacy_l2_cache_cpuid2));
+ *ecx = cpuid2_cache_descriptor(env->cache_info_cpuid2.l3_cache);
}
+ *edx = (cpuid2_cache_descriptor(env->cache_info_cpuid2.l1d_cache) << 16) |
+ (cpuid2_cache_descriptor(env->cache_info_cpuid2.l1i_cache) << 8) |
+ (cpuid2_cache_descriptor(env->cache_info_cpuid2.l2_cache));
break;
case 4:
/* cache info: needed for Core compatibility */
@@ -3722,35 +3711,27 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
}
} else {
*eax = 0;
- CPUCacheInfo *l1d, *l1i, *l2, *l3;
- if (env->cache_info && !cpu->legacy_cache) {
- l1d = &env->cache_info->l1d_cache;
- l1i = &env->cache_info->l1i_cache;
- l2 = &env->cache_info->l2_cache;
- l3 = &env->cache_info->l3_cache;
- } else {
- l1d = &legacy_l1d_cache;
- l1i = &legacy_l1i_cache;
- l2 = &legacy_l2_cache;
- l3 = &legacy_l3_cache;
- }
switch (count) {
case 0: /* L1 dcache info */
- encode_cache_cpuid4(l1d, 1, cs->nr_cores,
+ encode_cache_cpuid4(env->cache_info_cpuid4.l1d_cache,
+ 1, cs->nr_cores,
eax, ebx, ecx, edx);
break;
case 1: /* L1 icache info */
- encode_cache_cpuid4(l1i, 1, cs->nr_cores,
+ encode_cache_cpuid4(env->cache_info_cpuid4.l1i_cache,
+ 1, cs->nr_cores,
eax, ebx, ecx, edx);
break;
case 2: /* L2 cache info */
- encode_cache_cpuid4(l2, cs->nr_threads, cs->nr_cores,
+ encode_cache_cpuid4(env->cache_info_cpuid4.l2_cache,
+ cs->nr_threads, cs->nr_cores,
eax, ebx, ecx, edx);
break;
case 3: /* L3 cache info */
pkg_offset = apicid_pkg_offset(cs->nr_cores, cs->nr_threads);
if (cpu->enable_l3_cache) {
- encode_cache_cpuid4(l3, (1 << pkg_offset), cs->nr_cores,
+ encode_cache_cpuid4(env->cache_info_cpuid4.l3_cache,
+ (1 << pkg_offset), cs->nr_cores,
eax, ebx, ecx, edx);
break;
}
@@ -3963,13 +3944,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
(L1_ITLB_2M_ASSOC << 8) | (L1_ITLB_2M_ENTRIES);
*ebx = (L1_DTLB_4K_ASSOC << 24) | (L1_DTLB_4K_ENTRIES << 16) | \
(L1_ITLB_4K_ASSOC << 8) | (L1_ITLB_4K_ENTRIES);
- if (env->cache_info && !cpu->legacy_cache) {
- *ecx = encode_cache_cpuid80000005(&env->cache_info->l1d_cache);
- *edx = encode_cache_cpuid80000005(&env->cache_info->l1i_cache);
- } else {
- *ecx = encode_cache_cpuid80000005(&legacy_l1d_cache_amd);
- *edx = encode_cache_cpuid80000005(&legacy_l1i_cache_amd);
- }
+ *ecx = encode_cache_cpuid80000005(env->cache_info_amd.l1d_cache);
+ *edx = encode_cache_cpuid80000005(env->cache_info_amd.l1i_cache);
break;
case 0x80000006:
/* cache info (L2 cache) */
@@ -3985,17 +3961,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
(L2_DTLB_4K_ENTRIES << 16) | \
(AMD_ENC_ASSOC(L2_ITLB_4K_ASSOC) << 12) | \
(L2_ITLB_4K_ENTRIES);
- if (env->cache_info && !cpu->legacy_cache) {
- encode_cache_cpuid80000006(&env->cache_info->l2_cache,
- cpu->enable_l3_cache ?
- &env->cache_info->l3_cache : NULL,
- ecx, edx);
- } else {
- encode_cache_cpuid80000006(&legacy_l2_cache_amd,
- cpu->enable_l3_cache ?
- &legacy_l3_cache : NULL,
- ecx, edx);
- }
+ encode_cache_cpuid80000006(env->cache_info_amd.l2_cache,
+ cpu->enable_l3_cache ?
+ env->cache_info_amd.l3_cache : NULL,
+ ecx, edx);
break;
case 0x80000007:
*eax = 0;
@@ -4692,6 +4661,37 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
cpu->phys_bits = 32;
}
}
+
+ /* Cache information initialization */
+ if (!cpu->legacy_cache) {
+ if (!xcc->cpu_def || !xcc->cpu_def->cache_info) {
+ char *name = x86_cpu_class_get_model_name(xcc);
+ error_setg(errp,
+ "CPU model '%s' doesn't support legacy-cache=off", name);
+ g_free(name);
+ return;
+ }
+ env->cache_info_cpuid2 = env->cache_info_cpuid4 = env->cache_info_amd =
+ *xcc->cpu_def->cache_info;
+ } else {
+ /* Build legacy cache information */
+ env->cache_info_cpuid2.l1d_cache = &legacy_l1d_cache;
+ env->cache_info_cpuid2.l1i_cache = &legacy_l1i_cache;
+ env->cache_info_cpuid2.l2_cache = &legacy_l2_cache_cpuid2;
+ env->cache_info_cpuid2.l3_cache = &legacy_l3_cache;
+
+ env->cache_info_cpuid4.l1d_cache = &legacy_l1d_cache;
+ env->cache_info_cpuid4.l1i_cache = &legacy_l1i_cache;
+ env->cache_info_cpuid4.l2_cache = &legacy_l2_cache;
+ env->cache_info_cpuid4.l3_cache = &legacy_l3_cache;
+
+ env->cache_info_amd.l1d_cache = &legacy_l1d_cache_amd;
+ env->cache_info_amd.l1i_cache = &legacy_l1i_cache_amd;
+ env->cache_info_amd.l2_cache = &legacy_l2_cache_amd;
+ env->cache_info_amd.l3_cache = &legacy_l3_cache;
+ }
+
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
@@ -5175,11 +5175,10 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU, vmware_cpuid_freq, true),
DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
/*
- * lecacy_cache defaults to CPU model being chosen. This is set in
- * x86_cpu_load_def based on cache_info which is initialized in
- * builtin_x86_defs
+ * lecacy_cache defaults to true unless the CPU model provides its
+ * own cache information (see x86_cpu_load_def()).
*/
- DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, false),
+ DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
/*
* From "Requirements for Implementing the Microsoft
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 8ac13f6..5ddb0e6 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1100,10 +1100,10 @@ typedef struct CPUCacheInfo {
typedef struct CPUCaches {
- CPUCacheInfo l1d_cache;
- CPUCacheInfo l1i_cache;
- CPUCacheInfo l2_cache;
- CPUCacheInfo l3_cache;
+ CPUCacheInfo *l1d_cache;
+ CPUCacheInfo *l1i_cache;
+ CPUCacheInfo *l2_cache;
+ CPUCacheInfo *l3_cache;
} CPUCaches;
typedef struct CPUX86State {
@@ -1295,7 +1295,11 @@ typedef struct CPUX86State {
/* Features that were explicitly enabled/disabled */
FeatureWordArray user_features;
uint32_t cpuid_model[12];
- CPUCaches *cache_info;
+ /* Cache information for CPUID. When legacy-cache=on, the cache data
+ * on each CPUID leaf will be different, because we keep compatibility
+ * with old QEMU versions.
+ */
+ CPUCaches cache_info_cpuid2, cache_info_cpuid4, cache_info_amd;
/* MTRRs */
uint64_t mtrr_fixed[11];
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 1/5] i386: Clean up cache CPUID code
@ 2018-05-24 15:43 ` Babu Moger
0 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: qemu-devel, kvm, babu.moger, kash, geoff
From: Eduardo Habkost <ehabkost@redhat.com>
Always initialize CPUCaches structs with cache information, even
if legacy_cache=true. Use different CPUCaches struct for
CPUID[2], CPUID[4], and the AMD CPUID leaves.
This will simplify a lot the logic inside cpu_x86_cpuid().
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
target/i386/cpu.c | 117 +++++++++++++++++++++++++++---------------------------
target/i386/cpu.h | 14 ++++---
2 files changed, 67 insertions(+), 64 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d95310f..5c9bdc9 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1114,7 +1114,7 @@ struct X86CPUDefinition {
};
static CPUCaches epyc_cache_info = {
- .l1d_cache = {
+ .l1d_cache = &(CPUCacheInfo) {
.type = DCACHE,
.level = 1,
.size = 32 * KiB,
@@ -1126,7 +1126,7 @@ static CPUCaches epyc_cache_info = {
.self_init = 1,
.no_invd_sharing = true,
},
- .l1i_cache = {
+ .l1i_cache = &(CPUCacheInfo) {
.type = ICACHE,
.level = 1,
.size = 64 * KiB,
@@ -1138,7 +1138,7 @@ static CPUCaches epyc_cache_info = {
.self_init = 1,
.no_invd_sharing = true,
},
- .l2_cache = {
+ .l2_cache = &(CPUCacheInfo) {
.type = UNIFIED_CACHE,
.level = 2,
.size = 512 * KiB,
@@ -1148,7 +1148,7 @@ static CPUCaches epyc_cache_info = {
.sets = 1024,
.lines_per_tag = 1,
},
- .l3_cache = {
+ .l3_cache = &(CPUCacheInfo) {
.type = UNIFIED_CACHE,
.level = 3,
.size = 8 * MiB,
@@ -3342,9 +3342,8 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
env->features[w] = def->features[w];
}
- /* Store Cache information from the X86CPUDefinition if available */
- env->cache_info = def->cache_info;
- cpu->legacy_cache = def->cache_info ? 0 : 1;
+ /* legacy-cache defaults to 'off' if CPU model provides cache info */
+ cpu->legacy_cache = !def->cache_info;
/* Special cases not set in the X86CPUDefinition structs: */
/* TODO: in-kernel irqchip for hvf */
@@ -3695,21 +3694,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
if (!cpu->enable_l3_cache) {
*ecx = 0;
} else {
- if (env->cache_info && !cpu->legacy_cache) {
- *ecx = cpuid2_cache_descriptor(&env->cache_info->l3_cache);
- } else {
- *ecx = cpuid2_cache_descriptor(&legacy_l3_cache);
- }
- }
- if (env->cache_info && !cpu->legacy_cache) {
- *edx = (cpuid2_cache_descriptor(&env->cache_info->l1d_cache) << 16) |
- (cpuid2_cache_descriptor(&env->cache_info->l1i_cache) << 8) |
- (cpuid2_cache_descriptor(&env->cache_info->l2_cache));
- } else {
- *edx = (cpuid2_cache_descriptor(&legacy_l1d_cache) << 16) |
- (cpuid2_cache_descriptor(&legacy_l1i_cache) << 8) |
- (cpuid2_cache_descriptor(&legacy_l2_cache_cpuid2));
+ *ecx = cpuid2_cache_descriptor(env->cache_info_cpuid2.l3_cache);
}
+ *edx = (cpuid2_cache_descriptor(env->cache_info_cpuid2.l1d_cache) << 16) |
+ (cpuid2_cache_descriptor(env->cache_info_cpuid2.l1i_cache) << 8) |
+ (cpuid2_cache_descriptor(env->cache_info_cpuid2.l2_cache));
break;
case 4:
/* cache info: needed for Core compatibility */
@@ -3722,35 +3711,27 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
}
} else {
*eax = 0;
- CPUCacheInfo *l1d, *l1i, *l2, *l3;
- if (env->cache_info && !cpu->legacy_cache) {
- l1d = &env->cache_info->l1d_cache;
- l1i = &env->cache_info->l1i_cache;
- l2 = &env->cache_info->l2_cache;
- l3 = &env->cache_info->l3_cache;
- } else {
- l1d = &legacy_l1d_cache;
- l1i = &legacy_l1i_cache;
- l2 = &legacy_l2_cache;
- l3 = &legacy_l3_cache;
- }
switch (count) {
case 0: /* L1 dcache info */
- encode_cache_cpuid4(l1d, 1, cs->nr_cores,
+ encode_cache_cpuid4(env->cache_info_cpuid4.l1d_cache,
+ 1, cs->nr_cores,
eax, ebx, ecx, edx);
break;
case 1: /* L1 icache info */
- encode_cache_cpuid4(l1i, 1, cs->nr_cores,
+ encode_cache_cpuid4(env->cache_info_cpuid4.l1i_cache,
+ 1, cs->nr_cores,
eax, ebx, ecx, edx);
break;
case 2: /* L2 cache info */
- encode_cache_cpuid4(l2, cs->nr_threads, cs->nr_cores,
+ encode_cache_cpuid4(env->cache_info_cpuid4.l2_cache,
+ cs->nr_threads, cs->nr_cores,
eax, ebx, ecx, edx);
break;
case 3: /* L3 cache info */
pkg_offset = apicid_pkg_offset(cs->nr_cores, cs->nr_threads);
if (cpu->enable_l3_cache) {
- encode_cache_cpuid4(l3, (1 << pkg_offset), cs->nr_cores,
+ encode_cache_cpuid4(env->cache_info_cpuid4.l3_cache,
+ (1 << pkg_offset), cs->nr_cores,
eax, ebx, ecx, edx);
break;
}
@@ -3963,13 +3944,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
(L1_ITLB_2M_ASSOC << 8) | (L1_ITLB_2M_ENTRIES);
*ebx = (L1_DTLB_4K_ASSOC << 24) | (L1_DTLB_4K_ENTRIES << 16) | \
(L1_ITLB_4K_ASSOC << 8) | (L1_ITLB_4K_ENTRIES);
- if (env->cache_info && !cpu->legacy_cache) {
- *ecx = encode_cache_cpuid80000005(&env->cache_info->l1d_cache);
- *edx = encode_cache_cpuid80000005(&env->cache_info->l1i_cache);
- } else {
- *ecx = encode_cache_cpuid80000005(&legacy_l1d_cache_amd);
- *edx = encode_cache_cpuid80000005(&legacy_l1i_cache_amd);
- }
+ *ecx = encode_cache_cpuid80000005(env->cache_info_amd.l1d_cache);
+ *edx = encode_cache_cpuid80000005(env->cache_info_amd.l1i_cache);
break;
case 0x80000006:
/* cache info (L2 cache) */
@@ -3985,17 +3961,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
(L2_DTLB_4K_ENTRIES << 16) | \
(AMD_ENC_ASSOC(L2_ITLB_4K_ASSOC) << 12) | \
(L2_ITLB_4K_ENTRIES);
- if (env->cache_info && !cpu->legacy_cache) {
- encode_cache_cpuid80000006(&env->cache_info->l2_cache,
- cpu->enable_l3_cache ?
- &env->cache_info->l3_cache : NULL,
- ecx, edx);
- } else {
- encode_cache_cpuid80000006(&legacy_l2_cache_amd,
- cpu->enable_l3_cache ?
- &legacy_l3_cache : NULL,
- ecx, edx);
- }
+ encode_cache_cpuid80000006(env->cache_info_amd.l2_cache,
+ cpu->enable_l3_cache ?
+ env->cache_info_amd.l3_cache : NULL,
+ ecx, edx);
break;
case 0x80000007:
*eax = 0;
@@ -4692,6 +4661,37 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
cpu->phys_bits = 32;
}
}
+
+ /* Cache information initialization */
+ if (!cpu->legacy_cache) {
+ if (!xcc->cpu_def || !xcc->cpu_def->cache_info) {
+ char *name = x86_cpu_class_get_model_name(xcc);
+ error_setg(errp,
+ "CPU model '%s' doesn't support legacy-cache=off", name);
+ g_free(name);
+ return;
+ }
+ env->cache_info_cpuid2 = env->cache_info_cpuid4 = env->cache_info_amd =
+ *xcc->cpu_def->cache_info;
+ } else {
+ /* Build legacy cache information */
+ env->cache_info_cpuid2.l1d_cache = &legacy_l1d_cache;
+ env->cache_info_cpuid2.l1i_cache = &legacy_l1i_cache;
+ env->cache_info_cpuid2.l2_cache = &legacy_l2_cache_cpuid2;
+ env->cache_info_cpuid2.l3_cache = &legacy_l3_cache;
+
+ env->cache_info_cpuid4.l1d_cache = &legacy_l1d_cache;
+ env->cache_info_cpuid4.l1i_cache = &legacy_l1i_cache;
+ env->cache_info_cpuid4.l2_cache = &legacy_l2_cache;
+ env->cache_info_cpuid4.l3_cache = &legacy_l3_cache;
+
+ env->cache_info_amd.l1d_cache = &legacy_l1d_cache_amd;
+ env->cache_info_amd.l1i_cache = &legacy_l1i_cache_amd;
+ env->cache_info_amd.l2_cache = &legacy_l2_cache_amd;
+ env->cache_info_amd.l3_cache = &legacy_l3_cache;
+ }
+
+
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
@@ -5175,11 +5175,10 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU, vmware_cpuid_freq, true),
DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
/*
- * lecacy_cache defaults to CPU model being chosen. This is set in
- * x86_cpu_load_def based on cache_info which is initialized in
- * builtin_x86_defs
+ * lecacy_cache defaults to true unless the CPU model provides its
+ * own cache information (see x86_cpu_load_def()).
*/
- DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, false),
+ DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
/*
* From "Requirements for Implementing the Microsoft
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 8ac13f6..5ddb0e6 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1100,10 +1100,10 @@ typedef struct CPUCacheInfo {
typedef struct CPUCaches {
- CPUCacheInfo l1d_cache;
- CPUCacheInfo l1i_cache;
- CPUCacheInfo l2_cache;
- CPUCacheInfo l3_cache;
+ CPUCacheInfo *l1d_cache;
+ CPUCacheInfo *l1i_cache;
+ CPUCacheInfo *l2_cache;
+ CPUCacheInfo *l3_cache;
} CPUCaches;
typedef struct CPUX86State {
@@ -1295,7 +1295,11 @@ typedef struct CPUX86State {
/* Features that were explicitly enabled/disabled */
FeatureWordArray user_features;
uint32_t cpuid_model[12];
- CPUCaches *cache_info;
+ /* Cache information for CPUID. When legacy-cache=on, the cache data
+ * on each CPUID leaf will be different, because we keep compatibility
+ * with old QEMU versions.
+ */
+ CPUCaches cache_info_cpuid2, cache_info_cpuid4, cache_info_amd;
/* MTRRs */
uint64_t mtrr_fixed[11];
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v11 2/5] i386: Populate AMD Processor Cache Information for cpuid 0x8000001D
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
@ 2018-05-24 15:43 ` Babu Moger
-1 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: geoff, babu.moger, kash, qemu-devel, kvm
Add information for cpuid 0x8000001D leaf. Populate cache topology information
for different cache types (Data Cache, Instruction Cache, L2 and L3) supported
by 0x8000001D leaf. Please refer to the Processor Programming Reference (PPR)
for AMD Family 17h Model for more details.
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
target/i386/cpu.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
target/i386/kvm.c | 29 ++++++++++++--
2 files changed, 143 insertions(+), 3 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5c9bdc9..0d423e5 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -337,6 +337,99 @@ static void encode_cache_cpuid80000006(CPUCacheInfo *l2,
}
/*
+ * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
+ * Please refer to the AMD64 Architecture Programmer’s Manual Volume 3.
+ * Define the constants to build the cpu topology. Right now, TOPOEXT
+ * feature is enabled only on EPYC. So, these constants are based on
+ * EPYC supported configurations. We may need to handle the cases if
+ * these values change in future.
+ */
+/* Maximum core complexes in a node */
+#define MAX_CCX 2
+/* Maximum cores in a core complex */
+#define MAX_CORES_IN_CCX 4
+/* Maximum cores in a node */
+#define MAX_CORES_IN_NODE 8
+/* Maximum nodes in a socket */
+#define MAX_NODES_PER_SOCKET 4
+
+/*
+ * Figure out the number of nodes required to build this config.
+ * Max cores in a node is 8
+ */
+static int nodes_in_socket(int nr_cores)
+{
+ int nodes;
+
+ nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
+
+ /* Hardware does not support config with 3 nodes, return 4 in that case */
+ return (nodes == 3) ? 4 : nodes;
+}
+
+/*
+ * Decide the number of cores in a core complex with the given nr_cores using
+ * following set constants MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE and
+ * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
+ * L3 cache is shared across all cores in a core complex. So, this will also
+ * tell us how many cores are sharing the L3 cache.
+ */
+static int cores_in_core_complex(int nr_cores)
+{
+ int nodes;
+
+ /* Check if we can fit all the cores in one core complex */
+ if (nr_cores <= MAX_CORES_IN_CCX) {
+ return nr_cores;
+ }
+ /* Get the number of nodes required to build this config */
+ nodes = nodes_in_socket(nr_cores);
+
+ /*
+ * Divide the cores accros all the core complexes
+ * Return rounded up value
+ */
+ return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
+}
+
+/* Encode cache info for CPUID[8000001D] */
+static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
+ uint32_t *eax, uint32_t *ebx,
+ uint32_t *ecx, uint32_t *edx)
+{
+ uint32_t l3_cores;
+ assert(cache->size == cache->line_size * cache->associativity *
+ cache->partitions * cache->sets);
+
+ *eax = CACHE_TYPE(cache->type) | CACHE_LEVEL(cache->level) |
+ (cache->self_init ? CACHE_SELF_INIT_LEVEL : 0);
+
+ /* L3 is shared among multiple cores */
+ if (cache->level == 3) {
+ l3_cores = cores_in_core_complex(cs->nr_cores);
+ *eax |= ((l3_cores * cs->nr_threads) - 1) << 14;
+ } else {
+ *eax |= ((cs->nr_threads - 1) << 14);
+ }
+
+ assert(cache->line_size > 0);
+ assert(cache->partitions > 0);
+ assert(cache->associativity > 0);
+ /* We don't implement fully-associative caches */
+ assert(cache->associativity < cache->sets);
+ *ebx = (cache->line_size - 1) |
+ ((cache->partitions - 1) << 12) |
+ ((cache->associativity - 1) << 22);
+
+ assert(cache->sets > 0);
+ *ecx = cache->sets - 1;
+
+ *edx = (cache->no_invd_sharing ? CACHE_NO_INVD_SHARING : 0) |
+ (cache->inclusive ? CACHE_INCLUSIVE : 0) |
+ (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
+}
+
+/*
* 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
@@ -4005,6 +4098,30 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
*edx = 0;
}
break;
+ case 0x8000001D:
+ *eax = 0;
+ switch (count) {
+ case 0: /* L1 dcache info */
+ encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
+ eax, ebx, ecx, edx);
+ break;
+ case 1: /* L1 icache info */
+ encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
+ eax, ebx, ecx, edx);
+ break;
+ case 2: /* L2 cache info */
+ encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
+ eax, ebx, ecx, edx);
+ break;
+ case 3: /* L3 cache info */
+ encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
+ eax, ebx, ecx, edx);
+ break;
+ default: /* end of info */
+ *eax = *ebx = *ecx = *edx = 0;
+ break;
+ }
+ break;
case 0xC0000000:
*eax = env->cpuid_xlevel2;
*ebx = 0;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 0c656a9..871d1ef 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -980,9 +980,32 @@ int kvm_arch_init_vcpu(CPUState *cs)
}
c = &cpuid_data.entries[cpuid_i++];
- c->function = i;
- c->flags = 0;
- cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
+ switch (i) {
+ case 0x8000001d:
+ /* Query for all AMD cache information leaves */
+ for (j = 0; ; j++) {
+ c->function = i;
+ c->flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+ c->index = j;
+ cpu_x86_cpuid(env, i, j, &c->eax, &c->ebx, &c->ecx, &c->edx);
+
+ if (c->eax == 0) {
+ break;
+ }
+ if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
+ fprintf(stderr, "cpuid_data is full, no space for "
+ "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
+ abort();
+ }
+ c = &cpuid_data.entries[cpuid_i++];
+ }
+ break;
+ default:
+ c->function = i;
+ c->flags = 0;
+ cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
+ break;
+ }
}
/* Call Centaur's CPUID instructions they are supported. */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 2/5] i386: Populate AMD Processor Cache Information for cpuid 0x8000001D
@ 2018-05-24 15:43 ` Babu Moger
0 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: qemu-devel, kvm, babu.moger, kash, geoff
Add information for cpuid 0x8000001D leaf. Populate cache topology information
for different cache types (Data Cache, Instruction Cache, L2 and L3) supported
by 0x8000001D leaf. Please refer to the Processor Programming Reference (PPR)
for AMD Family 17h Model for more details.
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
target/i386/cpu.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
target/i386/kvm.c | 29 ++++++++++++--
2 files changed, 143 insertions(+), 3 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5c9bdc9..0d423e5 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -337,6 +337,99 @@ static void encode_cache_cpuid80000006(CPUCacheInfo *l2,
}
/*
+ * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
+ * Please refer to the AMD64 Architecture Programmer’s Manual Volume 3.
+ * Define the constants to build the cpu topology. Right now, TOPOEXT
+ * feature is enabled only on EPYC. So, these constants are based on
+ * EPYC supported configurations. We may need to handle the cases if
+ * these values change in future.
+ */
+/* Maximum core complexes in a node */
+#define MAX_CCX 2
+/* Maximum cores in a core complex */
+#define MAX_CORES_IN_CCX 4
+/* Maximum cores in a node */
+#define MAX_CORES_IN_NODE 8
+/* Maximum nodes in a socket */
+#define MAX_NODES_PER_SOCKET 4
+
+/*
+ * Figure out the number of nodes required to build this config.
+ * Max cores in a node is 8
+ */
+static int nodes_in_socket(int nr_cores)
+{
+ int nodes;
+
+ nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
+
+ /* Hardware does not support config with 3 nodes, return 4 in that case */
+ return (nodes == 3) ? 4 : nodes;
+}
+
+/*
+ * Decide the number of cores in a core complex with the given nr_cores using
+ * following set constants MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE and
+ * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
+ * L3 cache is shared across all cores in a core complex. So, this will also
+ * tell us how many cores are sharing the L3 cache.
+ */
+static int cores_in_core_complex(int nr_cores)
+{
+ int nodes;
+
+ /* Check if we can fit all the cores in one core complex */
+ if (nr_cores <= MAX_CORES_IN_CCX) {
+ return nr_cores;
+ }
+ /* Get the number of nodes required to build this config */
+ nodes = nodes_in_socket(nr_cores);
+
+ /*
+ * Divide the cores accros all the core complexes
+ * Return rounded up value
+ */
+ return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
+}
+
+/* Encode cache info for CPUID[8000001D] */
+static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
+ uint32_t *eax, uint32_t *ebx,
+ uint32_t *ecx, uint32_t *edx)
+{
+ uint32_t l3_cores;
+ assert(cache->size == cache->line_size * cache->associativity *
+ cache->partitions * cache->sets);
+
+ *eax = CACHE_TYPE(cache->type) | CACHE_LEVEL(cache->level) |
+ (cache->self_init ? CACHE_SELF_INIT_LEVEL : 0);
+
+ /* L3 is shared among multiple cores */
+ if (cache->level == 3) {
+ l3_cores = cores_in_core_complex(cs->nr_cores);
+ *eax |= ((l3_cores * cs->nr_threads) - 1) << 14;
+ } else {
+ *eax |= ((cs->nr_threads - 1) << 14);
+ }
+
+ assert(cache->line_size > 0);
+ assert(cache->partitions > 0);
+ assert(cache->associativity > 0);
+ /* We don't implement fully-associative caches */
+ assert(cache->associativity < cache->sets);
+ *ebx = (cache->line_size - 1) |
+ ((cache->partitions - 1) << 12) |
+ ((cache->associativity - 1) << 22);
+
+ assert(cache->sets > 0);
+ *ecx = cache->sets - 1;
+
+ *edx = (cache->no_invd_sharing ? CACHE_NO_INVD_SHARING : 0) |
+ (cache->inclusive ? CACHE_INCLUSIVE : 0) |
+ (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
+}
+
+/*
* 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
@@ -4005,6 +4098,30 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
*edx = 0;
}
break;
+ case 0x8000001D:
+ *eax = 0;
+ switch (count) {
+ case 0: /* L1 dcache info */
+ encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
+ eax, ebx, ecx, edx);
+ break;
+ case 1: /* L1 icache info */
+ encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
+ eax, ebx, ecx, edx);
+ break;
+ case 2: /* L2 cache info */
+ encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
+ eax, ebx, ecx, edx);
+ break;
+ case 3: /* L3 cache info */
+ encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
+ eax, ebx, ecx, edx);
+ break;
+ default: /* end of info */
+ *eax = *ebx = *ecx = *edx = 0;
+ break;
+ }
+ break;
case 0xC0000000:
*eax = env->cpuid_xlevel2;
*ebx = 0;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 0c656a9..871d1ef 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -980,9 +980,32 @@ int kvm_arch_init_vcpu(CPUState *cs)
}
c = &cpuid_data.entries[cpuid_i++];
- c->function = i;
- c->flags = 0;
- cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
+ switch (i) {
+ case 0x8000001d:
+ /* Query for all AMD cache information leaves */
+ for (j = 0; ; j++) {
+ c->function = i;
+ c->flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+ c->index = j;
+ cpu_x86_cpuid(env, i, j, &c->eax, &c->ebx, &c->ecx, &c->edx);
+
+ if (c->eax == 0) {
+ break;
+ }
+ if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
+ fprintf(stderr, "cpuid_data is full, no space for "
+ "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
+ abort();
+ }
+ c = &cpuid_data.entries[cpuid_i++];
+ }
+ break;
+ default:
+ c->function = i;
+ c->flags = 0;
+ cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
+ break;
+ }
}
/* Call Centaur's CPUID instructions they are supported. */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v11 3/5] i386: Add support for CPUID_8000_001E for AMD
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
@ 2018-05-24 15:43 ` Babu Moger
-1 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: geoff, babu.moger, kash, qemu-devel, kvm
Add support for cpuid leaf CPUID_8000_001E. Build the config that closely
match the underlying hardware. Please refer to the Processor Programming
Reference (PPR) for AMD Family 17h Model for more details.
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
target/i386/cpu.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0d423e5..9f8bad9 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -429,6 +429,62 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
(cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
}
+/* Data structure to hold the configuration info for a given core index */
+struct core_topology {
+ /* core complex id of the current core index */
+ int ccx_id;
+ /* Adjusted core id for this core index in the topology */
+ int core_id;
+ /* Node id for this core index */
+ int node_id;
+ /* Number of nodes in this config, 0 based */
+ int num_nodes;
+};
+
+/*
+ * Build the configuration closely match the EPYC hardware. Using the EPYC
+ * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE)
+ * right now. This could change in future.
+ * nr_cores : Total number of cores in the config
+ * core_id : Core index of the current CPU
+ * topo : Data structure to hold all the config info for this core index
+ */
+static void build_core_topology(int nr_cores, int core_id,
+ struct core_topology *topo)
+{
+ int nodes, cores_in_ccx;
+
+ /* First get the number of nodes required */
+ nodes = nodes_in_socket(nr_cores);
+
+ cores_in_ccx = cores_in_core_complex(nr_cores);
+
+ topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
+ topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
+ topo->core_id = core_id % cores_in_ccx;
+ /* num_nodes is 0 based, return n - 1 */
+ topo->num_nodes = nodes - 1;
+}
+
+/* Encode cache info for CPUID[8000001E] */
+static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
+ uint32_t *eax, uint32_t *ebx,
+ uint32_t *ecx, uint32_t *edx)
+{
+ struct core_topology topo = {0};
+
+ build_core_topology(cs->nr_cores, cpu->core_id, &topo);
+ *eax = cpu->apic_id;
+ if (cs->nr_threads - 1) {
+ *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
+ (topo.ccx_id << 2) | topo.core_id;
+ } else {
+ *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
+ }
+ *ecx = (topo.num_nodes << 8) | (cpu->socket_id << 2) | topo.node_id;
+ *edx = 0;
+}
+
/*
* Definitions of the hardcoded cache entries we expose:
* These are legacy cache values. If there is a need to change any
@@ -4122,6 +4178,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
break;
}
break;
+ case 0x8000001E:
+ assert(cpu->core_id <= 255);
+ encode_topo_cpuid8000001e(cs, cpu,
+ eax, ebx, ecx, edx);
+ break;
case 0xC0000000:
*eax = env->cpuid_xlevel2;
*ebx = 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 3/5] i386: Add support for CPUID_8000_001E for AMD
@ 2018-05-24 15:43 ` Babu Moger
0 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: qemu-devel, kvm, babu.moger, kash, geoff
Add support for cpuid leaf CPUID_8000_001E. Build the config that closely
match the underlying hardware. Please refer to the Processor Programming
Reference (PPR) for AMD Family 17h Model for more details.
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
target/i386/cpu.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0d423e5..9f8bad9 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -429,6 +429,62 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
(cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
}
+/* Data structure to hold the configuration info for a given core index */
+struct core_topology {
+ /* core complex id of the current core index */
+ int ccx_id;
+ /* Adjusted core id for this core index in the topology */
+ int core_id;
+ /* Node id for this core index */
+ int node_id;
+ /* Number of nodes in this config, 0 based */
+ int num_nodes;
+};
+
+/*
+ * Build the configuration closely match the EPYC hardware. Using the EPYC
+ * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE)
+ * right now. This could change in future.
+ * nr_cores : Total number of cores in the config
+ * core_id : Core index of the current CPU
+ * topo : Data structure to hold all the config info for this core index
+ */
+static void build_core_topology(int nr_cores, int core_id,
+ struct core_topology *topo)
+{
+ int nodes, cores_in_ccx;
+
+ /* First get the number of nodes required */
+ nodes = nodes_in_socket(nr_cores);
+
+ cores_in_ccx = cores_in_core_complex(nr_cores);
+
+ topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
+ topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
+ topo->core_id = core_id % cores_in_ccx;
+ /* num_nodes is 0 based, return n - 1 */
+ topo->num_nodes = nodes - 1;
+}
+
+/* Encode cache info for CPUID[8000001E] */
+static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
+ uint32_t *eax, uint32_t *ebx,
+ uint32_t *ecx, uint32_t *edx)
+{
+ struct core_topology topo = {0};
+
+ build_core_topology(cs->nr_cores, cpu->core_id, &topo);
+ *eax = cpu->apic_id;
+ if (cs->nr_threads - 1) {
+ *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
+ (topo.ccx_id << 2) | topo.core_id;
+ } else {
+ *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
+ }
+ *ecx = (topo.num_nodes << 8) | (cpu->socket_id << 2) | topo.node_id;
+ *edx = 0;
+}
+
/*
* Definitions of the hardcoded cache entries we expose:
* These are legacy cache values. If there is a need to change any
@@ -4122,6 +4178,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
break;
}
break;
+ case 0x8000001E:
+ assert(cpu->core_id <= 255);
+ encode_topo_cpuid8000001e(cs, cpu,
+ eax, ebx, ecx, edx);
+ break;
case 0xC0000000:
*eax = env->cpuid_xlevel2;
*ebx = 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
@ 2018-05-24 15:43 ` Babu Moger
-1 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: geoff, babu.moger, kash, qemu-devel, kvm
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)
+{
+ if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) ||
+ (nr_threads > 2)) {
+ return 0;
+ }
+ 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;
+ }
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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-05-24 15:43 ` Babu Moger
0 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: qemu-devel, kvm, babu.moger, kash, geoff
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)
+{
+ if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) ||
+ (nr_threads > 2)) {
+ return 0;
+ }
+ 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;
+ }
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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v11 5/5] i386: Remove generic SMT thread check
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
@ 2018-05-24 15:43 ` Babu Moger
-1 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: geoff, babu.moger, kash, qemu-devel, kvm
Remove generic non-intel check while validating hyperthreading support.
Certain AMD CPUs can support hyperthreading now.
CPU family with TOPOEXT feature can support hyperthreading now.
Signed-off-by: Babu Moger <babu.moger@amd.com>
Tested-by: Geoffrey McRae <geoff@hostfission.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
target/i386/cpu.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0b62356..e09eedb 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4951,17 +4951,20 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
qemu_init_vcpu(cs);
- /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this
- * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
- * based on inputs (sockets,cores,threads), it is still better to gives
+ /* Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
+ * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
+ * based on inputs (sockets,cores,threads), it is still better to give
* users a warning.
*
* NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
* cs->nr_threads hasn't be populated yet and the checking is incorrect.
*/
- if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) {
- error_report("AMD CPU doesn't support hyperthreading. Please configure"
- " -smp options properly.");
+ if (IS_AMD_CPU(env) &&
+ !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
+ cs->nr_threads > 1 && !ht_warned) {
+ error_report("This family of AMD CPU doesn't support "
+ "hyperthreading(%d). Please configure -smp "
+ "options properly.", cs->nr_threads);
ht_warned = true;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v11 5/5] i386: Remove generic SMT thread check
@ 2018-05-24 15:43 ` Babu Moger
0 siblings, 0 replies; 34+ messages in thread
From: Babu Moger @ 2018-05-24 15:43 UTC (permalink / raw)
To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: qemu-devel, kvm, babu.moger, kash, geoff
Remove generic non-intel check while validating hyperthreading support.
Certain AMD CPUs can support hyperthreading now.
CPU family with TOPOEXT feature can support hyperthreading now.
Signed-off-by: Babu Moger <babu.moger@amd.com>
Tested-by: Geoffrey McRae <geoff@hostfission.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
target/i386/cpu.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0b62356..e09eedb 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4951,17 +4951,20 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
qemu_init_vcpu(cs);
- /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this
- * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
- * based on inputs (sockets,cores,threads), it is still better to gives
+ /* Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
+ * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
+ * based on inputs (sockets,cores,threads), it is still better to give
* users a warning.
*
* NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
* cs->nr_threads hasn't be populated yet and the checking is incorrect.
*/
- if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) {
- error_report("AMD CPU doesn't support hyperthreading. Please configure"
- " -smp options properly.");
+ if (IS_AMD_CPU(env) &&
+ !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
+ cs->nr_threads > 1 && !ht_warned) {
+ error_report("This family of AMD CPU doesn't support "
+ "hyperthreading(%d). Please configure -smp "
+ "options properly.", cs->nr_threads);
ht_warned = true;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
@ 2018-05-24 15:51 ` Kash Pande
-1 siblings, 0 replies; 34+ messages in thread
From: Kash Pande @ 2018-05-24 15:51 UTC (permalink / raw)
To: Babu Moger, mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: geoff, qemu-devel, kvm
[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]
Tested-by: Kash Pande <kash@tripleback.net>
Hopefully we can get this merged because it's taken a ridiculously long
time with many back-and-forths for small issues that could have been put
off til later.
Kash
On 2018-05-24 11:43 AM, Babu Moger wrote:
> This series enables the TOPOEXT feature for AMD CPUs. This is required to
> support hyperthreading on kvm guests.
>
> This addresses the issues reported in these bugs:
> https://bugzilla.redhat.com/show_bug.cgi?id=1481253
> https://bugs.launchpad.net/qemu/+bug/1703506
>
> v11:
> Patches are based off of Eduardo's git://github.com/ehabkost/qemu.git x86-next.
> Summary of changes.
> 1.Added more comments explaining different constants and variables.
> 2.Removed NUM_SHARING_CACHE macro and made the code simpler.
> 3.Changed the function name num_sharing_l3_cache to cores_in_core_complex.
> This function is actually finding the number of cores in a core complex.
> Purpose here is to re-use the code in couple more places.
> 4.Added new function nodes_in_socket to find number of nodes in the config.
> Purpose here is to re-use the code.
> 5.Used DIV_ROUND_UP wherever applicable.
> 6.Renamed few constants and functions to generic names.
> 7.Few more text corrections.
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU
@ 2018-05-24 15:51 ` Kash Pande
0 siblings, 0 replies; 34+ messages in thread
From: Kash Pande @ 2018-05-24 15:51 UTC (permalink / raw)
To: Babu Moger, mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: qemu-devel, kvm, geoff
[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]
Tested-by: Kash Pande <kash@tripleback.net>
Hopefully we can get this merged because it's taken a ridiculously long
time with many back-and-forths for small issues that could have been put
off til later.
Kash
On 2018-05-24 11:43 AM, Babu Moger wrote:
> This series enables the TOPOEXT feature for AMD CPUs. This is required to
> support hyperthreading on kvm guests.
>
> This addresses the issues reported in these bugs:
> https://bugzilla.redhat.com/show_bug.cgi?id=1481253
> https://bugs.launchpad.net/qemu/+bug/1703506
>
> v11:
> Patches are based off of Eduardo's git://github.com/ehabkost/qemu.git x86-next.
> Summary of changes.
> 1.Added more comments explaining different constants and variables.
> 2.Removed NUM_SHARING_CACHE macro and made the code simpler.
> 3.Changed the function name num_sharing_l3_cache to cores_in_core_complex.
> This function is actually finding the number of cores in a core complex.
> Purpose here is to re-use the code in couple more places.
> 4.Added new function nodes_in_socket to find number of nodes in the config.
> Purpose here is to re-use the code.
> 5.Used DIV_ROUND_UP wherever applicable.
> 6.Renamed few constants and functions to generic names.
> 7.Few more text corrections.
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU
2018-05-24 15:51 ` [Qemu-devel] " Kash Pande
@ 2018-05-24 15:59 ` Moger, Babu
-1 siblings, 0 replies; 34+ messages in thread
From: Moger, Babu @ 2018-05-24 15:59 UTC (permalink / raw)
To: Kash Pande, mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: geoff, qemu-devel, kvm
> -----Original Message-----
> From: Kash Pande [mailto:kash@tripleback.net]
> Sent: Thursday, May 24, 2018 10:51 AM
> To: Moger, Babu <Babu.Moger@amd.com>; mst@redhat.com;
> marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net;
> ehabkost@redhat.com; mtosatti@redhat.com
> Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; geoff@hostfission.com
> Subject: Re: [PATCH v11 0/5] i386: Enable TOPOEXT to support
> hyperthreading on AMD CPU
>
> Tested-by: Kash Pande <kash@tripleback.net>
Thanks
>
>
> Hopefully we can get this merged because it's taken a ridiculously long
> time with many back-and-forths for small issues that could have been put
> off til later.
We are almost there. Thanks for your patience. ):
>
> Kash
>
> On 2018-05-24 11:43 AM, Babu Moger wrote:
> > This series enables the TOPOEXT feature for AMD CPUs. This is required to
> > support hyperthreading on kvm guests.
> >
> > This addresses the issues reported in these bugs:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1481253
> > https://bugs.launchpad.net/qemu/+bug/1703506
> >
> > v11:
> > Patches are based off of Eduardo's git://github.com/ehabkost/qemu.git
> x86-next.
> > Summary of changes.
> > 1.Added more comments explaining different constants and variables.
> > 2.Removed NUM_SHARING_CACHE macro and made the code simpler.
> > 3.Changed the function name num_sharing_l3_cache to
> cores_in_core_complex.
> > This function is actually finding the number of cores in a core complex.
> > Purpose here is to re-use the code in couple more places.
> > 4.Added new function nodes_in_socket to find number of nodes in the
> config.
> > Purpose here is to re-use the code.
> > 5.Used DIV_ROUND_UP wherever applicable.
> > 6.Renamed few constants and functions to generic names.
> > 7.Few more text corrections.
> >
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU
@ 2018-05-24 15:59 ` Moger, Babu
0 siblings, 0 replies; 34+ messages in thread
From: Moger, Babu @ 2018-05-24 15:59 UTC (permalink / raw)
To: Kash Pande, mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
Cc: qemu-devel, kvm, geoff
> -----Original Message-----
> From: Kash Pande [mailto:kash@tripleback.net]
> Sent: Thursday, May 24, 2018 10:51 AM
> To: Moger, Babu <Babu.Moger@amd.com>; mst@redhat.com;
> marcel.apfelbaum@gmail.com; pbonzini@redhat.com; rth@twiddle.net;
> ehabkost@redhat.com; mtosatti@redhat.com
> Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; geoff@hostfission.com
> Subject: Re: [PATCH v11 0/5] i386: Enable TOPOEXT to support
> hyperthreading on AMD CPU
>
> Tested-by: Kash Pande <kash@tripleback.net>
Thanks
>
>
> Hopefully we can get this merged because it's taken a ridiculously long
> time with many back-and-forths for small issues that could have been put
> off til later.
We are almost there. Thanks for your patience. ):
>
> Kash
>
> On 2018-05-24 11:43 AM, Babu Moger wrote:
> > This series enables the TOPOEXT feature for AMD CPUs. This is required to
> > support hyperthreading on kvm guests.
> >
> > This addresses the issues reported in these bugs:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1481253
> > https://bugs.launchpad.net/qemu/+bug/1703506
> >
> > v11:
> > Patches are based off of Eduardo's git://github.com/ehabkost/qemu.git
> x86-next.
> > Summary of changes.
> > 1.Added more comments explaining different constants and variables.
> > 2.Removed NUM_SHARING_CACHE macro and made the code simpler.
> > 3.Changed the function name num_sharing_l3_cache to
> cores_in_core_complex.
> > This function is actually finding the number of cores in a core complex.
> > Purpose here is to re-use the code in couple more places.
> > 4.Added new function nodes_in_socket to find number of nodes in the
> config.
> > Purpose here is to re-use the code.
> > 5.Used DIV_ROUND_UP wherever applicable.
> > 6.Renamed few constants and functions to generic names.
> > 7.Few more text corrections.
> >
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU
2018-05-24 15:51 ` [Qemu-devel] " Kash Pande
@ 2018-05-24 19:24 ` Eduardo Habkost
-1 siblings, 0 replies; 34+ messages in thread
From: Eduardo Habkost @ 2018-05-24 19:24 UTC (permalink / raw)
To: Kash Pande
Cc: geoff, kvm, mst, mtosatti, qemu-devel, Babu Moger, pbonzini, rth
On Thu, May 24, 2018 at 11:51:29AM -0400, Kash Pande wrote:
> Tested-by: Kash Pande <kash@tripleback.net>
>
>
> Hopefully we can get this merged because it's taken a ridiculously long
> time with many back-and-forths for small issues that could have been put
> off til later.
I understand this is frustrating, and this is partly my fault
because I didn't detect the issues on the first versions of the
series. Sorry for that.
I believe the patches that weren't included yet either had bugs
that had to be solved, or depended on other patches that were not
included yet. If there are patches from other iterations of this
series where you consider the code already ready for inclusion,
please let me know. Reviewed-by lines are welcome.
--
Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU
@ 2018-05-24 19:24 ` Eduardo Habkost
0 siblings, 0 replies; 34+ messages in thread
From: Eduardo Habkost @ 2018-05-24 19:24 UTC (permalink / raw)
To: Kash Pande
Cc: Babu Moger, mst, marcel.apfelbaum, pbonzini, rth, mtosatti,
qemu-devel, kvm, geoff
On Thu, May 24, 2018 at 11:51:29AM -0400, Kash Pande wrote:
> Tested-by: Kash Pande <kash@tripleback.net>
>
>
> Hopefully we can get this merged because it's taken a ridiculously long
> time with many back-and-forths for small issues that could have been put
> off til later.
I understand this is frustrating, and this is partly my fault
because I didn't detect the issues on the first versions of the
series. Sorry for that.
I believe the patches that weren't included yet either had bugs
that had to be solved, or depended on other patches that were not
included yet. If there are patches from other iterations of this
series where you consider the code already ready for inclusion,
please let me know. Reviewed-by lines are welcome.
--
Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v11 1/5] i386: Clean up cache CPUID code
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
@ 2018-05-24 19:28 ` Eduardo Habkost
-1 siblings, 0 replies; 34+ messages in thread
From: Eduardo Habkost @ 2018-05-24 19:28 UTC (permalink / raw)
To: Babu Moger; +Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth
On Thu, May 24, 2018 at 11:43:30AM -0400, Babu Moger wrote:
> From: Eduardo Habkost <ehabkost@redhat.com>
>
> Always initialize CPUCaches structs with cache information, even
> if legacy_cache=true. Use different CPUCaches struct for
> CPUID[2], CPUID[4], and the AMD CPUID leaves.
>
> This will simplify a lot the logic inside cpu_x86_cpuid().
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
Queued, thanks.
--
Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 1/5] i386: Clean up cache CPUID code
@ 2018-05-24 19:28 ` Eduardo Habkost
0 siblings, 0 replies; 34+ messages in thread
From: Eduardo Habkost @ 2018-05-24 19:28 UTC (permalink / raw)
To: Babu Moger
Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
kash, geoff
On Thu, May 24, 2018 at 11:43:30AM -0400, Babu Moger wrote:
> From: Eduardo Habkost <ehabkost@redhat.com>
>
> Always initialize CPUCaches structs with cache information, even
> if legacy_cache=true. Use different CPUCaches struct for
> CPUID[2], CPUID[4], and the AMD CPUID leaves.
>
> This will simplify a lot the logic inside cpu_x86_cpuid().
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
Queued, thanks.
--
Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v11 2/5] i386: Populate AMD Processor Cache Information for cpuid 0x8000001D
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
@ 2018-06-05 1:59 ` Eduardo Habkost
-1 siblings, 0 replies; 34+ messages in thread
From: Eduardo Habkost @ 2018-06-05 1:59 UTC (permalink / raw)
To: Babu Moger; +Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth
On Thu, May 24, 2018 at 11:43:31AM -0400, Babu Moger wrote:
> Add information for cpuid 0x8000001D leaf. Populate cache topology information
> for different cache types (Data Cache, Instruction Cache, L2 and L3) supported
> by 0x8000001D leaf. Please refer to the Processor Programming Reference (PPR)
> for AMD Family 17h Model for more details.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> target/i386/cpu.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> target/i386/kvm.c | 29 ++++++++++++--
> 2 files changed, 143 insertions(+), 3 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5c9bdc9..0d423e5 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -337,6 +337,99 @@ static void encode_cache_cpuid80000006(CPUCacheInfo *l2,
> }
>
> /*
> + * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
> + * Please refer to the AMD64 Architecture Programmer’s Manual Volume 3.
> + * Define the constants to build the cpu topology. Right now, TOPOEXT
> + * feature is enabled only on EPYC. So, these constants are based on
> + * EPYC supported configurations. We may need to handle the cases if
> + * these values change in future.
> + */
> +/* Maximum core complexes in a node */
> +#define MAX_CCX 2
> +/* Maximum cores in a core complex */
> +#define MAX_CORES_IN_CCX 4
> +/* Maximum cores in a node */
> +#define MAX_CORES_IN_NODE 8
> +/* Maximum nodes in a socket */
> +#define MAX_NODES_PER_SOCKET 4
> +
> +/*
> + * Figure out the number of nodes required to build this config.
> + * Max cores in a node is 4
> + */
> +static int nodes_in_socket(int nr_cores)
> +{
> + int nodes;
> +
> + nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
> +
> + /* Hardware does not support config with 3 nodes, return 4 in that case */
> + return (nodes == 3) ? 4 : nodes;
> +}
> +
> +/*
> + * Decide the number of cores in a core complex with the given nr_cores using
> + * following set constants MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE and
> + * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
> + * L3 cache is shared across all cores in a core complex. So, this will also
> + * tell us how many cores are sharing the L3 cache.
> + */
> +static int cores_in_core_complex(int nr_cores)
> +{
> + int nodes;
> +
> + /* Check if we can fit all the cores in one core complex */
> + if (nr_cores <= MAX_CORES_IN_CCX) {
> + return nr_cores;
> + }
> + /* Get the number of nodes required to build this config */
> + nodes = nodes_in_socket(nr_cores);
> +
> + /*
> + * Divide the cores accros all the core complexes
> + * Return rounded up value
> + */
> + return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
> +}
It's nice that we try to figure out a good default even on weird
topologies:
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
But I still worry about the complexity of compat code in the
future if we decide to change the results of this function a
little bit. Maybe we should allow TOPOEXT to be enabled only on
cases where we really know how to fit the cores in a round number
of nodes/core-complexes?
Let's do this in a follow-up patch?
--
Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 2/5] i386: Populate AMD Processor Cache Information for cpuid 0x8000001D
@ 2018-06-05 1:59 ` Eduardo Habkost
0 siblings, 0 replies; 34+ messages in thread
From: Eduardo Habkost @ 2018-06-05 1:59 UTC (permalink / raw)
To: Babu Moger
Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, geoff, kash,
qemu-devel, kvm
On Thu, May 24, 2018 at 11:43:31AM -0400, Babu Moger wrote:
> Add information for cpuid 0x8000001D leaf. Populate cache topology information
> for different cache types (Data Cache, Instruction Cache, L2 and L3) supported
> by 0x8000001D leaf. Please refer to the Processor Programming Reference (PPR)
> for AMD Family 17h Model for more details.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> target/i386/cpu.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> target/i386/kvm.c | 29 ++++++++++++--
> 2 files changed, 143 insertions(+), 3 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5c9bdc9..0d423e5 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -337,6 +337,99 @@ static void encode_cache_cpuid80000006(CPUCacheInfo *l2,
> }
>
> /*
> + * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
> + * Please refer to the AMD64 Architecture Programmer’s Manual Volume 3.
> + * Define the constants to build the cpu topology. Right now, TOPOEXT
> + * feature is enabled only on EPYC. So, these constants are based on
> + * EPYC supported configurations. We may need to handle the cases if
> + * these values change in future.
> + */
> +/* Maximum core complexes in a node */
> +#define MAX_CCX 2
> +/* Maximum cores in a core complex */
> +#define MAX_CORES_IN_CCX 4
> +/* Maximum cores in a node */
> +#define MAX_CORES_IN_NODE 8
> +/* Maximum nodes in a socket */
> +#define MAX_NODES_PER_SOCKET 4
> +
> +/*
> + * Figure out the number of nodes required to build this config.
> + * Max cores in a node is 4
> + */
> +static int nodes_in_socket(int nr_cores)
> +{
> + int nodes;
> +
> + nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
> +
> + /* Hardware does not support config with 3 nodes, return 4 in that case */
> + return (nodes == 3) ? 4 : nodes;
> +}
> +
> +/*
> + * Decide the number of cores in a core complex with the given nr_cores using
> + * following set constants MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE and
> + * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
> + * L3 cache is shared across all cores in a core complex. So, this will also
> + * tell us how many cores are sharing the L3 cache.
> + */
> +static int cores_in_core_complex(int nr_cores)
> +{
> + int nodes;
> +
> + /* Check if we can fit all the cores in one core complex */
> + if (nr_cores <= MAX_CORES_IN_CCX) {
> + return nr_cores;
> + }
> + /* Get the number of nodes required to build this config */
> + nodes = nodes_in_socket(nr_cores);
> +
> + /*
> + * Divide the cores accros all the core complexes
> + * Return rounded up value
> + */
> + return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
> +}
It's nice that we try to figure out a good default even on weird
topologies:
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
But I still worry about the complexity of compat code in the
future if we decide to change the results of this function a
little bit. Maybe we should allow TOPOEXT to be enabled only on
cases where we really know how to fit the cores in a round number
of nodes/core-complexes?
Let's do this in a follow-up patch?
--
Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v11 3/5] i386: Add support for CPUID_8000_001E for AMD
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
@ 2018-06-05 2:46 ` Eduardo Habkost
-1 siblings, 0 replies; 34+ messages in thread
From: Eduardo Habkost @ 2018-06-05 2:46 UTC (permalink / raw)
To: Babu Moger; +Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth
On Thu, May 24, 2018 at 11:43:32AM -0400, Babu Moger wrote:
> Add support for cpuid leaf CPUID_8000_001E. Build the config that closely
> match the underlying hardware. Please refer to the Processor Programming
> Reference (PPR) for AMD Family 17h Model for more details.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> target/i386/cpu.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 0d423e5..9f8bad9 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -429,6 +429,62 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
> (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
> }
>
> +/* Data structure to hold the configuration info for a given core index */
> +struct core_topology {
> + /* core complex id of the current core index */
> + int ccx_id;
> + /* Adjusted core id for this core index in the topology */
What's an "adjusted core id"?
> + int core_id;
> + /* Node id for this core index */
> + int node_id;
> + /* Number of nodes in this config, 0 based */
> + int num_nodes;
I suggest making num_nodes carry the actual number of nodes, and
using (num_nodes - 1) when building CPUID data.
> +};
> +
> +/*
> + * Build the configuration closely match the EPYC hardware. Using the EPYC
> + * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE)
> + * right now. This could change in future.
> + * nr_cores : Total number of cores in the config
> + * core_id : Core index of the current CPU
> + * topo : Data structure to hold all the config info for this core index
> + */
> +static void build_core_topology(int nr_cores, int core_id,
> + struct core_topology *topo)
> +{
> + int nodes, cores_in_ccx;
> +
> + /* First get the number of nodes required */
> + nodes = nodes_in_socket(nr_cores);
> +
> + cores_in_ccx = cores_in_core_complex(nr_cores);
> +
> + topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
> + topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
> + topo->core_id = core_id % cores_in_ccx;
> + /* num_nodes is 0 based, return n - 1 */
> + topo->num_nodes = nodes - 1;
> +}
Is this guaranteed to work with any nr_cores value, or only with
a few specific values that match real hardware?
> +
> +/* Encode cache info for CPUID[8000001E] */
> +static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
> + uint32_t *eax, uint32_t *ebx,
> + uint32_t *ecx, uint32_t *edx)
> +{
> + struct core_topology topo = {0};
> +
> + build_core_topology(cs->nr_cores, cpu->core_id, &topo);
> + *eax = cpu->apic_id;
> + if (cs->nr_threads - 1) {
> + *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> + (topo.ccx_id << 2) | topo.core_id;
> + } else {
> + *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> + }
This part confuses me a lot. Where are those bit offsets in EBX
defined?
Is this guaranteed to work with any cs->nr_cores value?
> + *ecx = (topo.num_nodes << 8) | (cpu->socket_id << 2) | topo.node_id;
Like on EBX, I'm confused by the bit offsets. Where are they
defined?
Probably it's safer to not allow TOPOEXT to be enabled if the CPU
socket/core/thread topology configuration can't generate a sane
node/core-complex topology.
> + *edx = 0;
> +}
> +
> /*
> * Definitions of the hardcoded cache entries we expose:
> * These are legacy cache values. If there is a need to change any
> @@ -4122,6 +4178,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> break;
> }
> break;
> + case 0x8000001E:
> + assert(cpu->core_id <= 255);
> + encode_topo_cpuid8000001e(cs, cpu,
> + eax, ebx, ecx, edx);
> + break;
> case 0xC0000000:
> *eax = env->cpuid_xlevel2;
> *ebx = 0;
> --
> 1.8.3.1
>
>
--
Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 3/5] i386: Add support for CPUID_8000_001E for AMD
@ 2018-06-05 2:46 ` Eduardo Habkost
0 siblings, 0 replies; 34+ messages in thread
From: Eduardo Habkost @ 2018-06-05 2:46 UTC (permalink / raw)
To: Babu Moger
Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, geoff, kash,
qemu-devel, kvm
On Thu, May 24, 2018 at 11:43:32AM -0400, Babu Moger wrote:
> Add support for cpuid leaf CPUID_8000_001E. Build the config that closely
> match the underlying hardware. Please refer to the Processor Programming
> Reference (PPR) for AMD Family 17h Model for more details.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> target/i386/cpu.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 0d423e5..9f8bad9 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -429,6 +429,62 @@ static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
> (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
> }
>
> +/* Data structure to hold the configuration info for a given core index */
> +struct core_topology {
> + /* core complex id of the current core index */
> + int ccx_id;
> + /* Adjusted core id for this core index in the topology */
What's an "adjusted core id"?
> + int core_id;
> + /* Node id for this core index */
> + int node_id;
> + /* Number of nodes in this config, 0 based */
> + int num_nodes;
I suggest making num_nodes carry the actual number of nodes, and
using (num_nodes - 1) when building CPUID data.
> +};
> +
> +/*
> + * Build the configuration closely match the EPYC hardware. Using the EPYC
> + * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX, MAX_CORES_IN_NODE)
> + * right now. This could change in future.
> + * nr_cores : Total number of cores in the config
> + * core_id : Core index of the current CPU
> + * topo : Data structure to hold all the config info for this core index
> + */
> +static void build_core_topology(int nr_cores, int core_id,
> + struct core_topology *topo)
> +{
> + int nodes, cores_in_ccx;
> +
> + /* First get the number of nodes required */
> + nodes = nodes_in_socket(nr_cores);
> +
> + cores_in_ccx = cores_in_core_complex(nr_cores);
> +
> + topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
> + topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
> + topo->core_id = core_id % cores_in_ccx;
> + /* num_nodes is 0 based, return n - 1 */
> + topo->num_nodes = nodes - 1;
> +}
Is this guaranteed to work with any nr_cores value, or only with
a few specific values that match real hardware?
> +
> +/* Encode cache info for CPUID[8000001E] */
> +static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
> + uint32_t *eax, uint32_t *ebx,
> + uint32_t *ecx, uint32_t *edx)
> +{
> + struct core_topology topo = {0};
> +
> + build_core_topology(cs->nr_cores, cpu->core_id, &topo);
> + *eax = cpu->apic_id;
> + if (cs->nr_threads - 1) {
> + *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> + (topo.ccx_id << 2) | topo.core_id;
> + } else {
> + *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> + }
This part confuses me a lot. Where are those bit offsets in EBX
defined?
Is this guaranteed to work with any cs->nr_cores value?
> + *ecx = (topo.num_nodes << 8) | (cpu->socket_id << 2) | topo.node_id;
Like on EBX, I'm confused by the bit offsets. Where are they
defined?
Probably it's safer to not allow TOPOEXT to be enabled if the CPU
socket/core/thread topology configuration can't generate a sane
node/core-complex topology.
> + *edx = 0;
> +}
> +
> /*
> * Definitions of the hardcoded cache entries we expose:
> * These are legacy cache values. If there is a need to change any
> @@ -4122,6 +4178,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> break;
> }
> break;
> + case 0x8000001E:
> + assert(cpu->core_id <= 255);
> + encode_topo_cpuid8000001e(cs, cpu,
> + eax, ebx, ecx, edx);
> + break;
> case 0xC0000000:
> *eax = env->cpuid_xlevel2;
> *ebx = 0;
> --
> 1.8.3.1
>
>
--
Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
@ 2018-06-05 2:57 ` Eduardo Habkost
-1 siblings, 0 replies; 34+ messages in thread
From: Eduardo Habkost @ 2018-06-05 2:57 UTC (permalink / raw)
To: Babu Moger; +Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-05 2:57 ` Eduardo Habkost
0 siblings, 0 replies; 34+ messages in thread
From: Eduardo Habkost @ 2018-06-05 2:57 UTC (permalink / raw)
To: Babu Moger
Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, geoff, kash,
qemu-devel, kvm
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v11 2/5] i386: Populate AMD Processor Cache Information for cpuid 0x8000001D
2018-06-05 1:59 ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-05 15:16 ` Moger, Babu
-1 siblings, 0 replies; 34+ messages in thread
From: Moger, Babu @ 2018-06-05 15:16 UTC (permalink / raw)
To: Eduardo Habkost
Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth
> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Monday, June 4, 2018 8:59 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com;
> kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org
> Subject: Re: [Qemu-devel] [PATCH v11 2/5] i386: Populate AMD Processor
> Cache Information for cpuid 0x8000001D
>
> On Thu, May 24, 2018 at 11:43:31AM -0400, Babu Moger wrote:
> > Add information for cpuid 0x8000001D leaf. Populate cache topology
> information
> > for different cache types (Data Cache, Instruction Cache, L2 and L3)
> supported
> > by 0x8000001D leaf. Please refer to the Processor Programming Reference
> (PPR)
> > for AMD Family 17h Model for more details.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> > target/i386/cpu.c | 117
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > target/i386/kvm.c | 29 ++++++++++++--
> > 2 files changed, 143 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 5c9bdc9..0d423e5 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -337,6 +337,99 @@ static void
> encode_cache_cpuid80000006(CPUCacheInfo *l2,
> > }
> >
> > /*
> > + * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
> > + * Please refer to the AMD64 Architecture Programmer’s Manual Volume
> 3.
> > + * Define the constants to build the cpu topology. Right now, TOPOEXT
> > + * feature is enabled only on EPYC. So, these constants are based on
> > + * EPYC supported configurations. We may need to handle the cases if
> > + * these values change in future.
> > + */
> > +/* Maximum core complexes in a node */
> > +#define MAX_CCX 2
> > +/* Maximum cores in a core complex */
> > +#define MAX_CORES_IN_CCX 4
> > +/* Maximum cores in a node */
> > +#define MAX_CORES_IN_NODE 8
> > +/* Maximum nodes in a socket */
> > +#define MAX_NODES_PER_SOCKET 4
> > +
> > +/*
> > + * Figure out the number of nodes required to build this config.
> > + * Max cores in a node is 4
> > + */
> > +static int nodes_in_socket(int nr_cores)
> > +{
> > + int nodes;
> > +
> > + nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
> > +
> > + /* Hardware does not support config with 3 nodes, return 4 in that case
> */
> > + return (nodes == 3) ? 4 : nodes;
> > +}
> > +
> > +/*
> > + * Decide the number of cores in a core complex with the given nr_cores
> using
> > + * following set constants MAX_CCX, MAX_CORES_IN_CCX,
> MAX_CORES_IN_NODE and
> > + * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
> > + * L3 cache is shared across all cores in a core complex. So, this will also
> > + * tell us how many cores are sharing the L3 cache.
> > + */
> > +static int cores_in_core_complex(int nr_cores)
> > +{
> > + int nodes;
> > +
> > + /* Check if we can fit all the cores in one core complex */
> > + if (nr_cores <= MAX_CORES_IN_CCX) {
> > + return nr_cores;
> > + }
> > + /* Get the number of nodes required to build this config */
> > + nodes = nodes_in_socket(nr_cores);
> > +
> > + /*
> > + * Divide the cores accros all the core complexes
> > + * Return rounded up value
> > + */
> > + return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
> > +}
>
> It's nice that we try to figure out a good default even on weird
> topologies:
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Thanks
>
> But I still worry about the complexity of compat code in the
> future if we decide to change the results of this function a
> little bit. Maybe we should allow TOPOEXT to be enabled only on
> cases where we really know how to fit the cores in a round number
> of nodes/core-complexes?
>
> Let's do this in a follow-up patch?
I thought about this earlier. But dropped the idea because it will be too restrictive.
Most of the odd number of cores cannot be supported by real hardware.
If you strongly believe we need to add this check then I will add it.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 2/5] i386: Populate AMD Processor Cache Information for cpuid 0x8000001D
@ 2018-06-05 15:16 ` Moger, Babu
0 siblings, 0 replies; 34+ messages in thread
From: Moger, Babu @ 2018-06-05 15:16 UTC (permalink / raw)
To: Eduardo Habkost
Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, geoff, kash,
qemu-devel, kvm
> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Monday, June 4, 2018 8:59 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com;
> kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org
> Subject: Re: [Qemu-devel] [PATCH v11 2/5] i386: Populate AMD Processor
> Cache Information for cpuid 0x8000001D
>
> On Thu, May 24, 2018 at 11:43:31AM -0400, Babu Moger wrote:
> > Add information for cpuid 0x8000001D leaf. Populate cache topology
> information
> > for different cache types (Data Cache, Instruction Cache, L2 and L3)
> supported
> > by 0x8000001D leaf. Please refer to the Processor Programming Reference
> (PPR)
> > for AMD Family 17h Model for more details.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> > target/i386/cpu.c | 117
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > target/i386/kvm.c | 29 ++++++++++++--
> > 2 files changed, 143 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 5c9bdc9..0d423e5 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -337,6 +337,99 @@ static void
> encode_cache_cpuid80000006(CPUCacheInfo *l2,
> > }
> >
> > /*
> > + * Definitions used for building CPUID Leaf 0x8000001D and 0x8000001E
> > + * Please refer to the AMD64 Architecture Programmer’s Manual Volume
> 3.
> > + * Define the constants to build the cpu topology. Right now, TOPOEXT
> > + * feature is enabled only on EPYC. So, these constants are based on
> > + * EPYC supported configurations. We may need to handle the cases if
> > + * these values change in future.
> > + */
> > +/* Maximum core complexes in a node */
> > +#define MAX_CCX 2
> > +/* Maximum cores in a core complex */
> > +#define MAX_CORES_IN_CCX 4
> > +/* Maximum cores in a node */
> > +#define MAX_CORES_IN_NODE 8
> > +/* Maximum nodes in a socket */
> > +#define MAX_NODES_PER_SOCKET 4
> > +
> > +/*
> > + * Figure out the number of nodes required to build this config.
> > + * Max cores in a node is 4
> > + */
> > +static int nodes_in_socket(int nr_cores)
> > +{
> > + int nodes;
> > +
> > + nodes = DIV_ROUND_UP(nr_cores, MAX_CORES_IN_NODE);
> > +
> > + /* Hardware does not support config with 3 nodes, return 4 in that case
> */
> > + return (nodes == 3) ? 4 : nodes;
> > +}
> > +
> > +/*
> > + * Decide the number of cores in a core complex with the given nr_cores
> using
> > + * following set constants MAX_CCX, MAX_CORES_IN_CCX,
> MAX_CORES_IN_NODE and
> > + * MAX_NODES_PER_SOCKET. Maintain symmetry as much as possible
> > + * L3 cache is shared across all cores in a core complex. So, this will also
> > + * tell us how many cores are sharing the L3 cache.
> > + */
> > +static int cores_in_core_complex(int nr_cores)
> > +{
> > + int nodes;
> > +
> > + /* Check if we can fit all the cores in one core complex */
> > + if (nr_cores <= MAX_CORES_IN_CCX) {
> > + return nr_cores;
> > + }
> > + /* Get the number of nodes required to build this config */
> > + nodes = nodes_in_socket(nr_cores);
> > +
> > + /*
> > + * Divide the cores accros all the core complexes
> > + * Return rounded up value
> > + */
> > + return DIV_ROUND_UP(nr_cores, nodes * MAX_CCX);
> > +}
>
> It's nice that we try to figure out a good default even on weird
> topologies:
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Thanks
>
> But I still worry about the complexity of compat code in the
> future if we decide to change the results of this function a
> little bit. Maybe we should allow TOPOEXT to be enabled only on
> cases where we really know how to fit the cores in a round number
> of nodes/core-complexes?
>
> Let's do this in a follow-up patch?
I thought about this earlier. But dropped the idea because it will be too restrictive.
Most of the odd number of cores cannot be supported by real hardware.
If you strongly believe we need to add this check then I will add it.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v11 3/5] i386: Add support for CPUID_8000_001E for AMD
2018-06-05 2:46 ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-05 15:49 ` Moger, Babu
-1 siblings, 0 replies; 34+ messages in thread
From: Moger, Babu @ 2018-06-05 15:49 UTC (permalink / raw)
To: Eduardo Habkost
Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth
> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Monday, June 4, 2018 9:46 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com;
> kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org
> Subject: Re: [Qemu-devel] [PATCH v11 3/5] i386: Add support for
> CPUID_8000_001E for AMD
>
> On Thu, May 24, 2018 at 11:43:32AM -0400, Babu Moger wrote:
> > Add support for cpuid leaf CPUID_8000_001E. Build the config that closely
> > match the underlying hardware. Please refer to the Processor
> Programming
> > Reference (PPR) for AMD Family 17h Model for more details.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> > target/i386/cpu.c | 61
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 61 insertions(+)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 0d423e5..9f8bad9 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -429,6 +429,62 @@ static void
> encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
> > (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
> > }
> >
> > +/* Data structure to hold the configuration info for a given core index */
> > +struct core_topology {
> > + /* core complex id of the current core index */
> > + int ccx_id;
> > + /* Adjusted core id for this core index in the topology */
>
> What's an "adjusted core id"?
When you build the topology, "adjusted core id" is the core index inside the core complex.
There can be max 4 cores in a core complex, so this id can be 0, 1, 2, 3. I will change the name to ccx_core_index;
>
> > + int core_id;
> > + /* Node id for this core index */
> > + int node_id;
> > + /* Number of nodes in this config, 0 based */
> > + int num_nodes;
>
> I suggest making num_nodes carry the actual number of nodes, and
> using (num_nodes - 1) when building CPUID data.
Sure. Will do
>
> > +};
> > +
> > +/*
> > + * Build the configuration closely match the EPYC hardware. Using the
> EPYC
> > + * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX,
> MAX_CORES_IN_NODE)
> > + * right now. This could change in future.
> > + * nr_cores : Total number of cores in the config
> > + * core_id : Core index of the current CPU
> > + * topo : Data structure to hold all the config info for this core index
> > + */
> > +static void build_core_topology(int nr_cores, int core_id,
> > + struct core_topology *topo)
> > +{
> > + int nodes, cores_in_ccx;
> > +
> > + /* First get the number of nodes required */
> > + nodes = nodes_in_socket(nr_cores);
> > +
> > + cores_in_ccx = cores_in_core_complex(nr_cores);
> > +
> > + topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
> > + topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
> > + topo->core_id = core_id % cores_in_ccx;
> > + /* num_nodes is 0 based, return n - 1 */
> > + topo->num_nodes = nodes - 1;
> > +}
>
> Is this guaranteed to work with any nr_cores value, or only with
> a few specific values that match real hardware?
I have tested all the supported core values(1 to 32). All of them work fine.
>
>
>
> > +
> > +/* Encode cache info for CPUID[8000001E] */
> > +static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
> > + uint32_t *eax, uint32_t *ebx,
> > + uint32_t *ecx, uint32_t *edx)
> > +{
> > + struct core_topology topo = {0};
> > +
> > + build_core_topology(cs->nr_cores, cpu->core_id, &topo);
> > + *eax = cpu->apic_id;
> > + if (cs->nr_threads - 1) {
> > + *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> > + (topo.ccx_id << 2) | topo.core_id;
> > + } else {
> > + *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> > + }
>
> This part confuses me a lot. Where are those bit offsets in EBX
> defined?
Bit 7-0 is core id. This decodes to where in the topology the core is located.
(2 bits for node, 2 bits for core complex and 3 bit index).
I will add those details in the comments.
>
> Is this guaranteed to work with any cs->nr_cores value?
>
>
> > + *ecx = (topo.num_nodes << 8) | (cpu->socket_id << 2) | topo.node_id;
>
> Like on EBX, I'm confused by the bit offsets. Where are they
> defined?
>
Bit 7-0 is Node id. This decodes to 1 bit for socket and 2 bits for node_id.
I will add those details in comments.
> Probably it's safer to not allow TOPOEXT to be enabled if the CPU
> socket/core/thread topology configuration can't generate a sane
> node/core-complex topology.
Like I said before It may become too restrictive. I have tested all the numbers (1 to 32).
They all appear to work ok.
>
>
> > + *edx = 0;
> > +}
> > +
> > /*
> > * Definitions of the hardcoded cache entries we expose:
> > * These are legacy cache values. If there is a need to change any
> > @@ -4122,6 +4178,11 @@ void cpu_x86_cpuid(CPUX86State *env,
> uint32_t index, uint32_t count,
> > break;
> > }
> > break;
> > + case 0x8000001E:
> > + assert(cpu->core_id <= 255);
> > + encode_topo_cpuid8000001e(cs, cpu,
> > + eax, ebx, ecx, edx);
> > + break;
> > case 0xC0000000:
> > *eax = env->cpuid_xlevel2;
> > *ebx = 0;
> > --
> > 1.8.3.1
> >
> >
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 3/5] i386: Add support for CPUID_8000_001E for AMD
@ 2018-06-05 15:49 ` Moger, Babu
0 siblings, 0 replies; 34+ messages in thread
From: Moger, Babu @ 2018-06-05 15:49 UTC (permalink / raw)
To: Eduardo Habkost
Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, geoff, kash,
qemu-devel, kvm
> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Monday, June 4, 2018 9:46 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com;
> kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org
> Subject: Re: [Qemu-devel] [PATCH v11 3/5] i386: Add support for
> CPUID_8000_001E for AMD
>
> On Thu, May 24, 2018 at 11:43:32AM -0400, Babu Moger wrote:
> > Add support for cpuid leaf CPUID_8000_001E. Build the config that closely
> > match the underlying hardware. Please refer to the Processor
> Programming
> > Reference (PPR) for AMD Family 17h Model for more details.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> > target/i386/cpu.c | 61
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 61 insertions(+)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 0d423e5..9f8bad9 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -429,6 +429,62 @@ static void
> encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
> > (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
> > }
> >
> > +/* Data structure to hold the configuration info for a given core index */
> > +struct core_topology {
> > + /* core complex id of the current core index */
> > + int ccx_id;
> > + /* Adjusted core id for this core index in the topology */
>
> What's an "adjusted core id"?
When you build the topology, "adjusted core id" is the core index inside the core complex.
There can be max 4 cores in a core complex, so this id can be 0, 1, 2, 3. I will change the name to ccx_core_index;
>
> > + int core_id;
> > + /* Node id for this core index */
> > + int node_id;
> > + /* Number of nodes in this config, 0 based */
> > + int num_nodes;
>
> I suggest making num_nodes carry the actual number of nodes, and
> using (num_nodes - 1) when building CPUID data.
Sure. Will do
>
> > +};
> > +
> > +/*
> > + * Build the configuration closely match the EPYC hardware. Using the
> EPYC
> > + * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX,
> MAX_CORES_IN_NODE)
> > + * right now. This could change in future.
> > + * nr_cores : Total number of cores in the config
> > + * core_id : Core index of the current CPU
> > + * topo : Data structure to hold all the config info for this core index
> > + */
> > +static void build_core_topology(int nr_cores, int core_id,
> > + struct core_topology *topo)
> > +{
> > + int nodes, cores_in_ccx;
> > +
> > + /* First get the number of nodes required */
> > + nodes = nodes_in_socket(nr_cores);
> > +
> > + cores_in_ccx = cores_in_core_complex(nr_cores);
> > +
> > + topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
> > + topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
> > + topo->core_id = core_id % cores_in_ccx;
> > + /* num_nodes is 0 based, return n - 1 */
> > + topo->num_nodes = nodes - 1;
> > +}
>
> Is this guaranteed to work with any nr_cores value, or only with
> a few specific values that match real hardware?
I have tested all the supported core values(1 to 32). All of them work fine.
>
>
>
> > +
> > +/* Encode cache info for CPUID[8000001E] */
> > +static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
> > + uint32_t *eax, uint32_t *ebx,
> > + uint32_t *ecx, uint32_t *edx)
> > +{
> > + struct core_topology topo = {0};
> > +
> > + build_core_topology(cs->nr_cores, cpu->core_id, &topo);
> > + *eax = cpu->apic_id;
> > + if (cs->nr_threads - 1) {
> > + *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> > + (topo.ccx_id << 2) | topo.core_id;
> > + } else {
> > + *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> > + }
>
> This part confuses me a lot. Where are those bit offsets in EBX
> defined?
Bit 7-0 is core id. This decodes to where in the topology the core is located.
(2 bits for node, 2 bits for core complex and 3 bit index).
I will add those details in the comments.
>
> Is this guaranteed to work with any cs->nr_cores value?
>
>
> > + *ecx = (topo.num_nodes << 8) | (cpu->socket_id << 2) | topo.node_id;
>
> Like on EBX, I'm confused by the bit offsets. Where are they
> defined?
>
Bit 7-0 is Node id. This decodes to 1 bit for socket and 2 bits for node_id.
I will add those details in comments.
> Probably it's safer to not allow TOPOEXT to be enabled if the CPU
> socket/core/thread topology configuration can't generate a sane
> node/core-complex topology.
Like I said before It may become too restrictive. I have tested all the numbers (1 to 32).
They all appear to work ok.
>
>
> > + *edx = 0;
> > +}
> > +
> > /*
> > * Definitions of the hardcoded cache entries we expose:
> > * These are legacy cache values. If there is a need to change any
> > @@ -4122,6 +4178,11 @@ void cpu_x86_cpuid(CPUX86State *env,
> uint32_t index, uint32_t count,
> > break;
> > }
> > break;
> > + case 0x8000001E:
> > + assert(cpu->core_id <= 255);
> > + encode_topo_cpuid8000001e(cs, cpu,
> > + eax, ebx, ecx, edx);
> > + break;
> > case 0xC0000000:
> > *eax = env->cpuid_xlevel2;
> > *ebx = 0;
> > --
> > 1.8.3.1
> >
> >
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
2018-06-05 2:57 ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-05 15:54 ` Moger, Babu
-1 siblings, 0 replies; 34+ messages in thread
From: Moger, Babu @ 2018-06-05 15:54 UTC (permalink / raw)
To: Eduardo Habkost
Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth
> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Monday, June 4, 2018 9:57 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com;
> kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org
> 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@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()".
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-05 15:54 ` Moger, Babu
0 siblings, 0 replies; 34+ messages in thread
From: Moger, Babu @ 2018-06-05 15:54 UTC (permalink / raw)
To: Eduardo Habkost
Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, geoff, kash,
qemu-devel, kvm
> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Monday, June 4, 2018 9:57 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com;
> kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org
> 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@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()".
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v11 3/5] i386: Add support for CPUID_8000_001E for AMD
2018-06-05 15:49 ` [Qemu-devel] " Moger, Babu
@ 2018-06-05 17:14 ` Moger, Babu
-1 siblings, 0 replies; 34+ messages in thread
From: Moger, Babu @ 2018-06-05 17:14 UTC (permalink / raw)
To: Eduardo Habkost
Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth
> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Moger, Babu
> Sent: Tuesday, June 5, 2018 10:49 AM
> To: Eduardo Habkost <ehabkost@redhat.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com;
> kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org
> Subject: RE: [Qemu-devel] [PATCH v11 3/5] i386: Add support for
> CPUID_8000_001E for AMD
>
>
>
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Monday, June 4, 2018 9:46 PM
> > To: Moger, Babu <Babu.Moger@amd.com>
> > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com;
> pbonzini@redhat.com;
> > rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com;
> > kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org
> > Subject: Re: [Qemu-devel] [PATCH v11 3/5] i386: Add support for
> > CPUID_8000_001E for AMD
> >
> > On Thu, May 24, 2018 at 11:43:32AM -0400, Babu Moger wrote:
> > > Add support for cpuid leaf CPUID_8000_001E. Build the config that closely
> > > match the underlying hardware. Please refer to the Processor
> > Programming
> > > Reference (PPR) for AMD Family 17h Model for more details.
> > >
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > ---
> > > target/i386/cpu.c | 61
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 61 insertions(+)
> > >
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 0d423e5..9f8bad9 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -429,6 +429,62 @@ static void
> > encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
> > > (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
> > > }
> > >
> > > +/* Data structure to hold the configuration info for a given core index */
> > > +struct core_topology {
> > > + /* core complex id of the current core index */
> > > + int ccx_id;
> > > + /* Adjusted core id for this core index in the topology */
> >
> > What's an "adjusted core id"?
>
> When you build the topology, "adjusted core id" is the core index inside the
> core complex.
> There can be max 4 cores in a core complex, so this id can be 0, 1, 2, 3. I will
> change the name to ccx_core_index;
> >
> > > + int core_id;
> > > + /* Node id for this core index */
> > > + int node_id;
> > > + /* Number of nodes in this config, 0 based */
> > > + int num_nodes;
> >
> > I suggest making num_nodes carry the actual number of nodes, and
> > using (num_nodes - 1) when building CPUID data.
>
> Sure. Will do
> >
> > > +};
> > > +
> > > +/*
> > > + * Build the configuration closely match the EPYC hardware. Using the
> > EPYC
> > > + * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX,
> > MAX_CORES_IN_NODE)
> > > + * right now. This could change in future.
> > > + * nr_cores : Total number of cores in the config
> > > + * core_id : Core index of the current CPU
> > > + * topo : Data structure to hold all the config info for this core index
> > > + */
> > > +static void build_core_topology(int nr_cores, int core_id,
> > > + struct core_topology *topo)
> > > +{
> > > + int nodes, cores_in_ccx;
> > > +
> > > + /* First get the number of nodes required */
> > > + nodes = nodes_in_socket(nr_cores);
> > > +
> > > + cores_in_ccx = cores_in_core_complex(nr_cores);
> > > +
> > > + topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
> > > + topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
> > > + topo->core_id = core_id % cores_in_ccx;
> > > + /* num_nodes is 0 based, return n - 1 */
> > > + topo->num_nodes = nodes - 1;
> > > +}
> >
> > Is this guaranteed to work with any nr_cores value, or only with
> > a few specific values that match real hardware?
>
> I have tested all the supported core values(1 to 32). All of them work fine.
>
> >
> >
> >
> > > +
> > > +/* Encode cache info for CPUID[8000001E] */
> > > +static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
> > > + uint32_t *eax, uint32_t *ebx,
> > > + uint32_t *ecx, uint32_t *edx)
> > > +{
> > > + struct core_topology topo = {0};
> > > +
> > > + build_core_topology(cs->nr_cores, cpu->core_id, &topo);
> > > + *eax = cpu->apic_id;
> > > + if (cs->nr_threads - 1) {
> > > + *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> > > + (topo.ccx_id << 2) | topo.core_id;
> > > + } else {
> > > + *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> > > + }
> >
> > This part confuses me a lot. Where are those bit offsets in EBX
> > defined?
>
> Bit 7-0 is core id. This decodes to where in the topology the core is located.
> (2 bits for node, 2 bits for core complex and 3 bit index).
Sorry.. This should be 2 bits for node, 1 bit for core complex and 2 bits for index.
Will add comments..
> I will add those details in the comments.
>
> >
> > Is this guaranteed to work with any cs->nr_cores value?
> >
> >
> > > + *ecx = (topo.num_nodes << 8) | (cpu->socket_id << 2) |
> topo.node_id;
> >
> > Like on EBX, I'm confused by the bit offsets. Where are they
> > defined?
> >
>
> Bit 7-0 is Node id. This decodes to 1 bit for socket and 2 bits for node_id.
> I will add those details in comments.
>
> > Probably it's safer to not allow TOPOEXT to be enabled if the CPU
> > socket/core/thread topology configuration can't generate a sane
> > node/core-complex topology.
>
> Like I said before It may become too restrictive. I have tested all the
> numbers (1 to 32).
> They all appear to work ok.
>
> >
> >
> > > + *edx = 0;
> > > +}
> > > +
> > > /*
> > > * Definitions of the hardcoded cache entries we expose:
> > > * These are legacy cache values. If there is a need to change any
> > > @@ -4122,6 +4178,11 @@ void cpu_x86_cpuid(CPUX86State *env,
> > uint32_t index, uint32_t count,
> > > break;
> > > }
> > > break;
> > > + case 0x8000001E:
> > > + assert(cpu->core_id <= 255);
> > > + encode_topo_cpuid8000001e(cs, cpu,
> > > + eax, ebx, ecx, edx);
> > > + break;
> > > case 0xC0000000:
> > > *eax = env->cpuid_xlevel2;
> > > *ebx = 0;
> > > --
> > > 1.8.3.1
> > >
> > >
> >
> > --
> > Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v11 3/5] i386: Add support for CPUID_8000_001E for AMD
@ 2018-06-05 17:14 ` Moger, Babu
0 siblings, 0 replies; 34+ messages in thread
From: Moger, Babu @ 2018-06-05 17:14 UTC (permalink / raw)
To: Eduardo Habkost
Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, geoff, kash,
qemu-devel, kvm
> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Moger, Babu
> Sent: Tuesday, June 5, 2018 10:49 AM
> To: Eduardo Habkost <ehabkost@redhat.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com;
> kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org
> Subject: RE: [Qemu-devel] [PATCH v11 3/5] i386: Add support for
> CPUID_8000_001E for AMD
>
>
>
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Monday, June 4, 2018 9:46 PM
> > To: Moger, Babu <Babu.Moger@amd.com>
> > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com;
> pbonzini@redhat.com;
> > rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com;
> > kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org
> > Subject: Re: [Qemu-devel] [PATCH v11 3/5] i386: Add support for
> > CPUID_8000_001E for AMD
> >
> > On Thu, May 24, 2018 at 11:43:32AM -0400, Babu Moger wrote:
> > > Add support for cpuid leaf CPUID_8000_001E. Build the config that closely
> > > match the underlying hardware. Please refer to the Processor
> > Programming
> > > Reference (PPR) for AMD Family 17h Model for more details.
> > >
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > ---
> > > target/i386/cpu.c | 61
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 61 insertions(+)
> > >
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 0d423e5..9f8bad9 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -429,6 +429,62 @@ static void
> > encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
> > > (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
> > > }
> > >
> > > +/* Data structure to hold the configuration info for a given core index */
> > > +struct core_topology {
> > > + /* core complex id of the current core index */
> > > + int ccx_id;
> > > + /* Adjusted core id for this core index in the topology */
> >
> > What's an "adjusted core id"?
>
> When you build the topology, "adjusted core id" is the core index inside the
> core complex.
> There can be max 4 cores in a core complex, so this id can be 0, 1, 2, 3. I will
> change the name to ccx_core_index;
> >
> > > + int core_id;
> > > + /* Node id for this core index */
> > > + int node_id;
> > > + /* Number of nodes in this config, 0 based */
> > > + int num_nodes;
> >
> > I suggest making num_nodes carry the actual number of nodes, and
> > using (num_nodes - 1) when building CPUID data.
>
> Sure. Will do
> >
> > > +};
> > > +
> > > +/*
> > > + * Build the configuration closely match the EPYC hardware. Using the
> > EPYC
> > > + * hardware configuration values (MAX_CCX, MAX_CORES_IN_CCX,
> > MAX_CORES_IN_NODE)
> > > + * right now. This could change in future.
> > > + * nr_cores : Total number of cores in the config
> > > + * core_id : Core index of the current CPU
> > > + * topo : Data structure to hold all the config info for this core index
> > > + */
> > > +static void build_core_topology(int nr_cores, int core_id,
> > > + struct core_topology *topo)
> > > +{
> > > + int nodes, cores_in_ccx;
> > > +
> > > + /* First get the number of nodes required */
> > > + nodes = nodes_in_socket(nr_cores);
> > > +
> > > + cores_in_ccx = cores_in_core_complex(nr_cores);
> > > +
> > > + topo->node_id = core_id / (cores_in_ccx * MAX_CCX);
> > > + topo->ccx_id = (core_id % (cores_in_ccx * MAX_CCX)) / cores_in_ccx;
> > > + topo->core_id = core_id % cores_in_ccx;
> > > + /* num_nodes is 0 based, return n - 1 */
> > > + topo->num_nodes = nodes - 1;
> > > +}
> >
> > Is this guaranteed to work with any nr_cores value, or only with
> > a few specific values that match real hardware?
>
> I have tested all the supported core values(1 to 32). All of them work fine.
>
> >
> >
> >
> > > +
> > > +/* Encode cache info for CPUID[8000001E] */
> > > +static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
> > > + uint32_t *eax, uint32_t *ebx,
> > > + uint32_t *ecx, uint32_t *edx)
> > > +{
> > > + struct core_topology topo = {0};
> > > +
> > > + build_core_topology(cs->nr_cores, cpu->core_id, &topo);
> > > + *eax = cpu->apic_id;
> > > + if (cs->nr_threads - 1) {
> > > + *ebx = ((cs->nr_threads - 1) << 8) | (topo.node_id << 3) |
> > > + (topo.ccx_id << 2) | topo.core_id;
> > > + } else {
> > > + *ebx = (topo.node_id << 4) | (topo.ccx_id << 3) | topo.core_id;
> > > + }
> >
> > This part confuses me a lot. Where are those bit offsets in EBX
> > defined?
>
> Bit 7-0 is core id. This decodes to where in the topology the core is located.
> (2 bits for node, 2 bits for core complex and 3 bit index).
Sorry.. This should be 2 bits for node, 1 bit for core complex and 2 bits for index.
Will add comments..
> I will add those details in the comments.
>
> >
> > Is this guaranteed to work with any cs->nr_cores value?
> >
> >
> > > + *ecx = (topo.num_nodes << 8) | (cpu->socket_id << 2) |
> topo.node_id;
> >
> > Like on EBX, I'm confused by the bit offsets. Where are they
> > defined?
> >
>
> Bit 7-0 is Node id. This decodes to 1 bit for socket and 2 bits for node_id.
> I will add those details in comments.
>
> > Probably it's safer to not allow TOPOEXT to be enabled if the CPU
> > socket/core/thread topology configuration can't generate a sane
> > node/core-complex topology.
>
> Like I said before It may become too restrictive. I have tested all the
> numbers (1 to 32).
> They all appear to work ok.
>
> >
> >
> > > + *edx = 0;
> > > +}
> > > +
> > > /*
> > > * Definitions of the hardcoded cache entries we expose:
> > > * These are legacy cache values. If there is a need to change any
> > > @@ -4122,6 +4178,11 @@ void cpu_x86_cpuid(CPUX86State *env,
> > uint32_t index, uint32_t count,
> > > break;
> > > }
> > > break;
> > > + case 0x8000001E:
> > > + assert(cpu->core_id <= 255);
> > > + encode_topo_cpuid8000001e(cs, cpu,
> > > + eax, ebx, ecx, edx);
> > > + break;
> > > case 0xC0000000:
> > > *eax = env->cpuid_xlevel2;
> > > *ebx = 0;
> > > --
> > > 1.8.3.1
> > >
> > >
> >
> > --
> > Eduardo
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2018-06-05 17:14 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-24 15:43 [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU Babu Moger
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
2018-05-24 15:43 ` [PATCH v11 1/5] i386: Clean up cache CPUID code Babu Moger
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
2018-05-24 19:28 ` Eduardo Habkost
2018-05-24 19:28 ` [Qemu-devel] " Eduardo Habkost
2018-05-24 15:43 ` [PATCH v11 2/5] i386: Populate AMD Processor Cache Information for cpuid 0x8000001D Babu Moger
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
2018-06-05 1:59 ` Eduardo Habkost
2018-06-05 1:59 ` [Qemu-devel] " Eduardo Habkost
2018-06-05 15:16 ` Moger, Babu
2018-06-05 15:16 ` [Qemu-devel] " Moger, Babu
2018-05-24 15:43 ` [PATCH v11 3/5] i386: Add support for CPUID_8000_001E for AMD Babu Moger
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
2018-06-05 2:46 ` Eduardo Habkost
2018-06-05 2:46 ` [Qemu-devel] " Eduardo Habkost
2018-06-05 15:49 ` Moger, Babu
2018-06-05 15:49 ` [Qemu-devel] " Moger, Babu
2018-06-05 17:14 ` Moger, Babu
2018-06-05 17:14 ` [Qemu-devel] " Moger, Babu
2018-05-24 15:43 ` [PATCH v11 4/5] i386: Enable TOPOEXT feature on AMD EPYC CPU Babu Moger
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
2018-06-05 2:57 ` Eduardo Habkost
2018-06-05 2:57 ` [Qemu-devel] " Eduardo Habkost
2018-06-05 15:54 ` Moger, Babu
2018-06-05 15:54 ` [Qemu-devel] " Moger, Babu
2018-05-24 15:43 ` [PATCH v11 5/5] i386: Remove generic SMT thread check Babu Moger
2018-05-24 15:43 ` [Qemu-devel] " Babu Moger
2018-05-24 15:51 ` [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU Kash Pande
2018-05-24 15:51 ` [Qemu-devel] " Kash Pande
2018-05-24 15:59 ` Moger, Babu
2018-05-24 15:59 ` [Qemu-devel] " Moger, Babu
2018-05-24 19:24 ` Eduardo Habkost
2018-05-24 19:24 ` [Qemu-devel] " Eduardo Habkost
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.