All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v13 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU
@ 2018-06-08 22:56 ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 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 

v13:
 Patches are based off of Eduardo's git://github.com/ehabkost/qemu.git x86-next.
 Some of the patches are queued already. Submitting remaining series.
 Summary of changes.
 1.Fixed the error format if the topology cannot be supported.
 2.Fixed the compatibility issues with old cpu models and new machine types.
   Here is the discussion thread.
   Here is the discussion thread. https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg01239.html
 3.I am still testing it. But sending it to get review feedback.

v12:
 Patches are based off of Eduardo's git://github.com/ehabkost/qemu.git x86-next.
 Some of the patches are queued already. Submitting remaining series.

 Summary of changes.
 1.Added more comments explaining CPUID_Fn8000001E bit definitions.
 2.Split the patch into separate patch to check the topology. Moved the code to
   x86_cpu_realizefn. Display the error if topoext feature cannot be enabled.
 3.Few more text corrections.

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 (5):
  i386: Add support for CPUID_8000_001E for AMD
  i386: Introduce auto_topoext bit to manage topoext
  i386: Enable TOPOEXT feature on AMD EPYC CPU
  i386: Verify and enable topoext feature if supported
  i386: Remove generic SMT thread check

 include/hw/i386/pc.h |   4 ++
 target/i386/cpu.c    | 162 +++++++++++++++++++++++++++++++++++++++++++++++++--
 target/i386/cpu.h    |   5 ++
 3 files changed, 165 insertions(+), 6 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [Qemu-devel] [PATCH v13 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU
@ 2018-06-08 22:56 ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 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 

v13:
 Patches are based off of Eduardo's git://github.com/ehabkost/qemu.git x86-next.
 Some of the patches are queued already. Submitting remaining series.
 Summary of changes.
 1.Fixed the error format if the topology cannot be supported.
 2.Fixed the compatibility issues with old cpu models and new machine types.
   Here is the discussion thread.
   Here is the discussion thread. https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg01239.html
 3.I am still testing it. But sending it to get review feedback.

v12:
 Patches are based off of Eduardo's git://github.com/ehabkost/qemu.git x86-next.
 Some of the patches are queued already. Submitting remaining series.

 Summary of changes.
 1.Added more comments explaining CPUID_Fn8000001E bit definitions.
 2.Split the patch into separate patch to check the topology. Moved the code to
   x86_cpu_realizefn. Display the error if topoext feature cannot be enabled.
 3.Few more text corrections.

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 (5):
  i386: Add support for CPUID_8000_001E for AMD
  i386: Introduce auto_topoext bit to manage topoext
  i386: Enable TOPOEXT feature on AMD EPYC CPU
  i386: Verify and enable topoext feature if supported
  i386: Remove generic SMT thread check

 include/hw/i386/pc.h |   4 ++
 target/i386/cpu.c    | 162 +++++++++++++++++++++++++++++++++++++++++++++++++--
 target/i386/cpu.h    |   5 ++
 3 files changed, 165 insertions(+), 6 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
  2018-06-08 22:56 ` [Qemu-devel] " Babu Moger
@ 2018-06-08 22:56   ` Babu Moger
  -1 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 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 | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1e69e68..86fb1a4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -427,6 +427,87 @@ 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 index for this core in the topology
+     * This can be 0,1,2,3 with max 4 cores in a core complex
+     */
+    int core_id;
+    /* Node id for this core index */
+    int node_id;
+    /* Number of nodes in this config */
+    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;
+    topo->num_nodes = nodes;
+}
+
+/* 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;
+    /*
+     * CPUID_Fn8000001E_EBX
+     * 31:16 Reserved
+     * 15:8  Threads per core (The number of threads per core is
+     *       Threads per core + 1)
+     *  7:0  Core id (see bit decoding below)
+     *       SMT:
+     *           4:3 node id
+     *             2 Core complex id
+     *           1:0 Core id
+     *       Non SMT:
+     *           5:4 node id
+     *             3 Core complex id
+     *           1:0 Core 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;
+    }
+    /*
+     * CPUID_Fn8000001E_ECX
+     * 31:11 Reserved
+     * 10:8  Nodes per processor (Nodes per processor is number of nodes + 1)
+     *  7:0  Node id (see bit decoding below)
+     *         2  Socket id
+     *       1:0  Node id
+     */
+    *ecx = ((topo.num_nodes - 1) << 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
@@ -4120,6 +4201,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] 72+ messages in thread

* [Qemu-devel] [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
@ 2018-06-08 22:56   ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 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 | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1e69e68..86fb1a4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -427,6 +427,87 @@ 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 index for this core in the topology
+     * This can be 0,1,2,3 with max 4 cores in a core complex
+     */
+    int core_id;
+    /* Node id for this core index */
+    int node_id;
+    /* Number of nodes in this config */
+    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;
+    topo->num_nodes = nodes;
+}
+
+/* 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;
+    /*
+     * CPUID_Fn8000001E_EBX
+     * 31:16 Reserved
+     * 15:8  Threads per core (The number of threads per core is
+     *       Threads per core + 1)
+     *  7:0  Core id (see bit decoding below)
+     *       SMT:
+     *           4:3 node id
+     *             2 Core complex id
+     *           1:0 Core id
+     *       Non SMT:
+     *           5:4 node id
+     *             3 Core complex id
+     *           1:0 Core 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;
+    }
+    /*
+     * CPUID_Fn8000001E_ECX
+     * 31:11 Reserved
+     * 10:8  Nodes per processor (Nodes per processor is number of nodes + 1)
+     *  7:0  Node id (see bit decoding below)
+     *         2  Socket id
+     *       1:0  Node id
+     */
+    *ecx = ((topo.num_nodes - 1) << 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
@@ -4120,6 +4201,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] 72+ messages in thread

* [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
  2018-06-08 22:56 ` [Qemu-devel] " Babu Moger
@ 2018-06-08 22:56   ` Babu Moger
  -1 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 UTC (permalink / raw)
  To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
  Cc: geoff, babu.moger, kash, qemu-devel, kvm

Introduce the auto_topoext bit to to control topoext feature.

Also add new field auto_topoext(in X86CPUDefinition). This will
be used to enable topoext on newer CPU models where topoext can
be supported.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 include/hw/i386/pc.h |  4 ++++
 target/i386/cpu.c    | 12 ++++++++++++
 target/i386/cpu.h    |  5 +++++
 3 files changed, 21 insertions(+)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 04d1f8c..cc30ec3 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = TYPE_X86_CPU,\
         .property = "legacy-cache",\
         .value    = "on",\
+    },{\
+        .driver   = TYPE_X86_CPU,\
+        .property = "auto-topoext",\
+        .value    = "off",\
     },
 
 #define PC_COMPAT_2_11 \
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 86fb1a4..d3411ed 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
     FeatureWordArray features;
     const char *model_id;
     CPUCaches *cache_info;
+    /* Set it if topoext can be enabled in CPU models */
+    int auto_topoext;
 };
 
 static CPUCaches epyc_cache_info = {
@@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     /* legacy-cache defaults to 'off' if CPU model provides cache info */
     cpu->legacy_cache = !def->cache_info;
 
+    /* Set auto_topoext if both machine property and CPU model supports it */
+    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;
+
     /* Special cases not set in the X86CPUDefinition structs: */
     /* TODO: in-kernel irqchip for hvf */
     if (kvm_enabled()) {
@@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
 
     /*
+     * auto-topoext property will be used to enable topoext feature.
+     * This will be disabled on all the older CPU models. Will be
+     * enabled on newer CPU modeles which can support topology extention.
+     */
+     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
+
+    /*
      * From "Requirements for Implementing the Microsoft
      * Hypervisor Interface":
      * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 89c82be..8783d36 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1409,6 +1409,11 @@ struct X86CPU {
      */
     bool legacy_cache;
 
+    /* Compatibility bits to enable topoext feature on all newer machines
+     * Disabled on older machines. Enabled on newer CPU models
+     */
+    bool auto_topoext;
+
     /* Compatibility bits for old machine types: */
     bool enable_cpuid_0xb;
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [Qemu-devel] [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
@ 2018-06-08 22:56   ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 UTC (permalink / raw)
  To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
  Cc: qemu-devel, kvm, babu.moger, kash, geoff

Introduce the auto_topoext bit to to control topoext feature.

Also add new field auto_topoext(in X86CPUDefinition). This will
be used to enable topoext on newer CPU models where topoext can
be supported.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 include/hw/i386/pc.h |  4 ++++
 target/i386/cpu.c    | 12 ++++++++++++
 target/i386/cpu.h    |  5 +++++
 3 files changed, 21 insertions(+)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 04d1f8c..cc30ec3 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = TYPE_X86_CPU,\
         .property = "legacy-cache",\
         .value    = "on",\
+    },{\
+        .driver   = TYPE_X86_CPU,\
+        .property = "auto-topoext",\
+        .value    = "off",\
     },
 
 #define PC_COMPAT_2_11 \
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 86fb1a4..d3411ed 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
     FeatureWordArray features;
     const char *model_id;
     CPUCaches *cache_info;
+    /* Set it if topoext can be enabled in CPU models */
+    int auto_topoext;
 };
 
 static CPUCaches epyc_cache_info = {
@@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     /* legacy-cache defaults to 'off' if CPU model provides cache info */
     cpu->legacy_cache = !def->cache_info;
 
+    /* Set auto_topoext if both machine property and CPU model supports it */
+    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;
+
     /* Special cases not set in the X86CPUDefinition structs: */
     /* TODO: in-kernel irqchip for hvf */
     if (kvm_enabled()) {
@@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
 
     /*
+     * auto-topoext property will be used to enable topoext feature.
+     * This will be disabled on all the older CPU models. Will be
+     * enabled on newer CPU modeles which can support topology extention.
+     */
+     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
+
+    /*
      * From "Requirements for Implementing the Microsoft
      * Hypervisor Interface":
      * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 89c82be..8783d36 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1409,6 +1409,11 @@ struct X86CPU {
      */
     bool legacy_cache;
 
+    /* Compatibility bits to enable topoext feature on all newer machines
+     * Disabled on older machines. Enabled on newer CPU models
+     */
+    bool auto_topoext;
+
     /* Compatibility bits for old machine types: */
     bool enable_cpuid_0xb;
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-08 22:56 ` [Qemu-devel] " Babu Moger
@ 2018-06-08 22:56   ` Babu Moger
  -1 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 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.

TOPOEXT feature is disabled for legacy machines.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d3411ed..4dd9a82 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2574,6 +2574,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .xlevel = 0x8000000A,
         .model_id = "AMD EPYC Processor",
         .cache_info = &epyc_cache_info,
+        .auto_topoext = 1,
     },
     {
         .name = "EPYC-IBPB",
@@ -2621,6 +2622,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .xlevel = 0x8000000A,
         .model_id = "AMD EPYC Processor (with IBPB)",
         .cache_info = &epyc_cache_info,
+        .auto_topoext = 1,
     },
 };
 
@@ -4672,6 +4674,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] 72+ messages in thread

* [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-08 22:56   ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 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.

TOPOEXT feature is disabled for legacy machines.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d3411ed..4dd9a82 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2574,6 +2574,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .xlevel = 0x8000000A,
         .model_id = "AMD EPYC Processor",
         .cache_info = &epyc_cache_info,
+        .auto_topoext = 1,
     },
     {
         .name = "EPYC-IBPB",
@@ -2621,6 +2622,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .xlevel = 0x8000000A,
         .model_id = "AMD EPYC Processor (with IBPB)",
         .cache_info = &epyc_cache_info,
+        .auto_topoext = 1,
     },
 };
 
@@ -4672,6 +4674,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] 72+ messages in thread

* [PATCH v13 4/5] i386: Verify and enable topoext feature if supported
  2018-06-08 22:56 ` [Qemu-devel] " Babu Moger
@ 2018-06-08 22:56   ` Babu Moger
  -1 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 UTC (permalink / raw)
  To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
  Cc: geoff, babu.moger, kash, qemu-devel, kvm

If the CPU model supports topoext feature, enabled the
feature automatically if it can be supported.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4dd9a82..88bc73d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4763,6 +4763,33 @@ static int x86_cpu_filter_features(X86CPU *cpu)
 #define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
                          (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
                          (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
+/*
+ * 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 topology_supports_topoext(int nr_cores, int nr_threads,
+            Error **errp)
+{
+    if (nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) {
+        error_setg(errp, "TOPOEXT unsupported with %d cores per socket",
+                   nr_cores);
+        error_append_hint(errp, "TOPOEXT supports only up to %d cores per"
+                          " socket\n",
+                          (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
+        return false;
+    }
+
+    if (nr_threads > 2) {
+        error_setg(errp, "TOPOEXT unsupported with %d threads per core",
+                   nr_threads);
+        error_append_hint(errp, "TOPOEXT supports only up to 2 threads"
+                          " per core\n");
+        return false;
+    }
+    return true;
+}
+
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
@@ -4953,6 +4980,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     qemu_init_vcpu(cs);
 
+    if (cpu->auto_topoext &&
+        !(env->user_features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
+        if (cs->nr_cores <= (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET) &&
+            (cs->nr_threads <= 2)) {
+            env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_TOPOEXT;
+        }
+    }
+
+    if ((env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
+        !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
+        return;
+    }
+
     /* 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
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [Qemu-devel] [PATCH v13 4/5] i386: Verify and enable topoext feature if supported
@ 2018-06-08 22:56   ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 UTC (permalink / raw)
  To: mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
  Cc: qemu-devel, kvm, babu.moger, kash, geoff

If the CPU model supports topoext feature, enabled the
feature automatically if it can be supported.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4dd9a82..88bc73d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4763,6 +4763,33 @@ static int x86_cpu_filter_features(X86CPU *cpu)
 #define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
                          (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
                          (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
+/*
+ * 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 topology_supports_topoext(int nr_cores, int nr_threads,
+            Error **errp)
+{
+    if (nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) {
+        error_setg(errp, "TOPOEXT unsupported with %d cores per socket",
+                   nr_cores);
+        error_append_hint(errp, "TOPOEXT supports only up to %d cores per"
+                          " socket\n",
+                          (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
+        return false;
+    }
+
+    if (nr_threads > 2) {
+        error_setg(errp, "TOPOEXT unsupported with %d threads per core",
+                   nr_threads);
+        error_append_hint(errp, "TOPOEXT supports only up to 2 threads"
+                          " per core\n");
+        return false;
+    }
+    return true;
+}
+
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
@@ -4953,6 +4980,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     qemu_init_vcpu(cs);
 
+    if (cpu->auto_topoext &&
+        !(env->user_features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
+        if (cs->nr_cores <= (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET) &&
+            (cs->nr_threads <= 2)) {
+            env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_TOPOEXT;
+        }
+    }
+
+    if ((env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
+        !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
+        return;
+    }
+
     /* 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
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH v13 5/5] i386: Remove generic SMT thread check
  2018-06-08 22:56 ` [Qemu-devel] " Babu Moger
@ 2018-06-08 22:56   ` Babu Moger
  -1 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 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 | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 88bc73d..a55e501 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4993,17 +4993,22 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    /* 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 or try enabling topoext feature.",
+                         cs->nr_threads);
         ht_warned = true;
     }
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [Qemu-devel] [PATCH v13 5/5] i386: Remove generic SMT thread check
@ 2018-06-08 22:56   ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-08 22:56 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 | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 88bc73d..a55e501 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4993,17 +4993,22 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    /* 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 or try enabling topoext feature.",
+                         cs->nr_threads);
         ht_warned = true;
     }
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
  2018-06-08 22:56   ` [Qemu-devel] " Babu Moger
@ 2018-06-11 20:25     ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 20:25 UTC (permalink / raw)
  To: Moger, Babu, mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
  Cc: geoff, kash, qemu-devel, kvm

Hi Eduardo,
Planning  to make couple of changes after testing and review.  Let me know if I missed something.
Will wait for your feedback before the next revision.

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Babu Moger
> Sent: Friday, June 8, 2018 5:56 PM
> To: 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; Moger, Babu
> <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com
> Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> topoext
> 
> Introduce the auto_topoext bit to to control topoext feature.
> 
> Also add new field auto_topoext(in X86CPUDefinition). This will
> be used to enable topoext on newer CPU models where topoext can
> be supported.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  include/hw/i386/pc.h |  4 ++++
>  target/i386/cpu.c    | 12 ++++++++++++
>  target/i386/cpu.h    |  5 +++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 04d1f8c..cc30ec3 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> uint64_t *);
>          .driver   = TYPE_X86_CPU,\
>          .property = "legacy-cache",\
>          .value    = "on",\
> +    },{\
> +        .driver   = TYPE_X86_CPU,\
> +        .property = "auto-topoext",\
> +        .value    = "off",\
>      },

We don't need this.  We are already setting this false in x86_cpu_properties.
But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we define it. 
That will be a separate patch. Correct?

> 
>  #define PC_COMPAT_2_11 \
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 86fb1a4..d3411ed 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
>      FeatureWordArray features;
>      const char *model_id;
>      CPUCaches *cache_info;
> +    /* Set it if topoext can be enabled in CPU models */
> +    int auto_topoext;
>  };
> 
>  static CPUCaches epyc_cache_info = {
> @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu,
> X86CPUDefinition *def, Error **errp)
>      /* legacy-cache defaults to 'off' if CPU model provides cache info */
>      cpu->legacy_cache = !def->cache_info;
> 
> +    /* Set auto_topoext if both machine property and CPU model supports it
> */
> +    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;

This could be more appropriate like this.

 cpu->auto_topoext =  cpu->auto_topoext && def->auto_topoext;

> +
>      /* Special cases not set in the X86CPUDefinition structs: */
>      /* TODO: in-kernel irqchip for hvf */
>      if (kvm_enabled()) {
> @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
> 
>      /*
> +     * auto-topoext property will be used to enable topoext feature.
> +     * This will be disabled on all the older CPU models. Will be
> +     * enabled on newer CPU modeles which can support topology extention.
> +     */
> +     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
> +
> +    /*
>       * From "Requirements for Implementing the Microsoft
>       * Hypervisor Interface":
>       * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-
> windows/reference/tlfs
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 89c82be..8783d36 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1409,6 +1409,11 @@ struct X86CPU {
>       */
>      bool legacy_cache;
> 
> +    /* Compatibility bits to enable topoext feature on all newer machines
> +     * Disabled on older machines. Enabled on newer CPU models
> +     */
> +    bool auto_topoext;
> +
>      /* Compatibility bits for old machine types: */
>      bool enable_cpuid_0xb;
> 
> --
> 1.8.3.1

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
@ 2018-06-11 20:25     ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 20:25 UTC (permalink / raw)
  To: Moger, Babu, mst, marcel.apfelbaum, pbonzini, rth, ehabkost, mtosatti
  Cc: qemu-devel, kvm, kash, geoff

Hi Eduardo,
Planning  to make couple of changes after testing and review.  Let me know if I missed something.
Will wait for your feedback before the next revision.

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Babu Moger
> Sent: Friday, June 8, 2018 5:56 PM
> To: 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; Moger, Babu
> <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com
> Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> topoext
> 
> Introduce the auto_topoext bit to to control topoext feature.
> 
> Also add new field auto_topoext(in X86CPUDefinition). This will
> be used to enable topoext on newer CPU models where topoext can
> be supported.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  include/hw/i386/pc.h |  4 ++++
>  target/i386/cpu.c    | 12 ++++++++++++
>  target/i386/cpu.h    |  5 +++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 04d1f8c..cc30ec3 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> uint64_t *);
>          .driver   = TYPE_X86_CPU,\
>          .property = "legacy-cache",\
>          .value    = "on",\
> +    },{\
> +        .driver   = TYPE_X86_CPU,\
> +        .property = "auto-topoext",\
> +        .value    = "off",\
>      },

We don't need this.  We are already setting this false in x86_cpu_properties.
But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we define it. 
That will be a separate patch. Correct?

> 
>  #define PC_COMPAT_2_11 \
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 86fb1a4..d3411ed 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
>      FeatureWordArray features;
>      const char *model_id;
>      CPUCaches *cache_info;
> +    /* Set it if topoext can be enabled in CPU models */
> +    int auto_topoext;
>  };
> 
>  static CPUCaches epyc_cache_info = {
> @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu,
> X86CPUDefinition *def, Error **errp)
>      /* legacy-cache defaults to 'off' if CPU model provides cache info */
>      cpu->legacy_cache = !def->cache_info;
> 
> +    /* Set auto_topoext if both machine property and CPU model supports it
> */
> +    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;

This could be more appropriate like this.

 cpu->auto_topoext =  cpu->auto_topoext && def->auto_topoext;

> +
>      /* Special cases not set in the X86CPUDefinition structs: */
>      /* TODO: in-kernel irqchip for hvf */
>      if (kvm_enabled()) {
> @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
> 
>      /*
> +     * auto-topoext property will be used to enable topoext feature.
> +     * This will be disabled on all the older CPU models. Will be
> +     * enabled on newer CPU modeles which can support topology extention.
> +     */
> +     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
> +
> +    /*
>       * From "Requirements for Implementing the Microsoft
>       * Hypervisor Interface":
>       * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-
> windows/reference/tlfs
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 89c82be..8783d36 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1409,6 +1409,11 @@ struct X86CPU {
>       */
>      bool legacy_cache;
> 
> +    /* Compatibility bits to enable topoext feature on all newer machines
> +     * Disabled on older machines. Enabled on newer CPU models
> +     */
> +    bool auto_topoext;
> +
>      /* Compatibility bits for old machine types: */
>      bool enable_cpuid_0xb;
> 
> --
> 1.8.3.1

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
  2018-06-11 20:25     ` [Qemu-devel] " Moger, Babu
@ 2018-06-11 20:46       ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 20:46 UTC (permalink / raw)
  To: Moger, Babu; +Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth

On Mon, Jun 11, 2018 at 08:25:32PM +0000, Moger, Babu wrote:
> Hi Eduardo,
> Planning  to make couple of changes after testing and review.  Let me know if I missed something.
> Will wait for your feedback before the next revision.
> 
> > -----Original Message-----
> > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> > On Behalf Of Babu Moger
> > Sent: Friday, June 8, 2018 5:56 PM
> > To: 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; Moger, Babu
> > <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com
> > Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> > topoext
> > 
> > Introduce the auto_topoext bit to to control topoext feature.
> > 
> > Also add new field auto_topoext(in X86CPUDefinition). This will
> > be used to enable topoext on newer CPU models where topoext can
> > be supported.
> > 
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  include/hw/i386/pc.h |  4 ++++
> >  target/i386/cpu.c    | 12 ++++++++++++
> >  target/i386/cpu.h    |  5 +++++
> >  3 files changed, 21 insertions(+)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 04d1f8c..cc30ec3 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> > uint64_t *);
> >          .driver   = TYPE_X86_CPU,\
> >          .property = "legacy-cache",\
> >          .value    = "on",\
> > +    },{\
> > +        .driver   = TYPE_X86_CPU,\
> > +        .property = "auto-topoext",\
> > +        .value    = "off",\
> >      },
> 
> We don't need this.  We are already setting this false in x86_cpu_properties.
> But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we define it. 
> That will be a separate patch. Correct?

PC_COMPAT_3_0 will exist only on QEMU 3.1.  If you want to define
something as the default behavior in pc-3.0, just define it as
the default in the device code, which is exactly what you are
already doing in x86_cpu_load_def() below.

In other words, just set auto_topoext=true on the CPU model table
for EPYC, and it should be enough.

On PC_COMPAT_2_12, both would work:
  { TYPE_X86_CPU, "auto-topoext", "off" }
or
  { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.

I prefer the latter, but both would work.


> 
> > 
> >  #define PC_COMPAT_2_11 \
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 86fb1a4..d3411ed 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
> >      FeatureWordArray features;
> >      const char *model_id;
> >      CPUCaches *cache_info;
> > +    /* Set it if topoext can be enabled in CPU models */
> > +    int auto_topoext;
> >  };
> > 
> >  static CPUCaches epyc_cache_info = {
> > @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu,
> > X86CPUDefinition *def, Error **errp)
> >      /* legacy-cache defaults to 'off' if CPU model provides cache info */
> >      cpu->legacy_cache = !def->cache_info;
> > 
> > +    /* Set auto_topoext if both machine property and CPU model supports it
> > */
> > +    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;
> 
> This could be more appropriate like this.
> 
>  cpu->auto_topoext =  cpu->auto_topoext && def->auto_topoext;

cpu->auto_topoext is supposed to be already false, here.  Why not
just:
    cpu->auto_topoext = def->auto_topoext;
?

> 
> > +
> >      /* Special cases not set in the X86CPUDefinition structs: */
> >      /* TODO: in-kernel irqchip for hvf */
> >      if (kvm_enabled()) {
> > @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
> > 
> >      /*
> > +     * auto-topoext property will be used to enable topoext feature.
> > +     * This will be disabled on all the older CPU models. Will be
> > +     * enabled on newer CPU modeles which can support topology extention.
> > +     */
> > +     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
> > +
> > +    /*
> >       * From "Requirements for Implementing the Microsoft
> >       * Hypervisor Interface":
> >       * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-
> > windows/reference/tlfs
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 89c82be..8783d36 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -1409,6 +1409,11 @@ struct X86CPU {
> >       */
> >      bool legacy_cache;
> > 
> > +    /* Compatibility bits to enable topoext feature on all newer machines
> > +     * Disabled on older machines. Enabled on newer CPU models
> > +     */
> > +    bool auto_topoext;
> > +
> >      /* Compatibility bits for old machine types: */
> >      bool enable_cpuid_0xb;
> > 
> > --
> > 1.8.3.1
> 

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
@ 2018-06-11 20:46       ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 20:46 UTC (permalink / raw)
  To: Moger, Babu
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff

On Mon, Jun 11, 2018 at 08:25:32PM +0000, Moger, Babu wrote:
> Hi Eduardo,
> Planning  to make couple of changes after testing and review.  Let me know if I missed something.
> Will wait for your feedback before the next revision.
> 
> > -----Original Message-----
> > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> > On Behalf Of Babu Moger
> > Sent: Friday, June 8, 2018 5:56 PM
> > To: 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; Moger, Babu
> > <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com
> > Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> > topoext
> > 
> > Introduce the auto_topoext bit to to control topoext feature.
> > 
> > Also add new field auto_topoext(in X86CPUDefinition). This will
> > be used to enable topoext on newer CPU models where topoext can
> > be supported.
> > 
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  include/hw/i386/pc.h |  4 ++++
> >  target/i386/cpu.c    | 12 ++++++++++++
> >  target/i386/cpu.h    |  5 +++++
> >  3 files changed, 21 insertions(+)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 04d1f8c..cc30ec3 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> > uint64_t *);
> >          .driver   = TYPE_X86_CPU,\
> >          .property = "legacy-cache",\
> >          .value    = "on",\
> > +    },{\
> > +        .driver   = TYPE_X86_CPU,\
> > +        .property = "auto-topoext",\
> > +        .value    = "off",\
> >      },
> 
> We don't need this.  We are already setting this false in x86_cpu_properties.
> But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we define it. 
> That will be a separate patch. Correct?

PC_COMPAT_3_0 will exist only on QEMU 3.1.  If you want to define
something as the default behavior in pc-3.0, just define it as
the default in the device code, which is exactly what you are
already doing in x86_cpu_load_def() below.

In other words, just set auto_topoext=true on the CPU model table
for EPYC, and it should be enough.

On PC_COMPAT_2_12, both would work:
  { TYPE_X86_CPU, "auto-topoext", "off" }
or
  { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.

I prefer the latter, but both would work.


> 
> > 
> >  #define PC_COMPAT_2_11 \
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 86fb1a4..d3411ed 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
> >      FeatureWordArray features;
> >      const char *model_id;
> >      CPUCaches *cache_info;
> > +    /* Set it if topoext can be enabled in CPU models */
> > +    int auto_topoext;
> >  };
> > 
> >  static CPUCaches epyc_cache_info = {
> > @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu,
> > X86CPUDefinition *def, Error **errp)
> >      /* legacy-cache defaults to 'off' if CPU model provides cache info */
> >      cpu->legacy_cache = !def->cache_info;
> > 
> > +    /* Set auto_topoext if both machine property and CPU model supports it
> > */
> > +    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;
> 
> This could be more appropriate like this.
> 
>  cpu->auto_topoext =  cpu->auto_topoext && def->auto_topoext;

cpu->auto_topoext is supposed to be already false, here.  Why not
just:
    cpu->auto_topoext = def->auto_topoext;
?

> 
> > +
> >      /* Special cases not set in the X86CPUDefinition structs: */
> >      /* TODO: in-kernel irqchip for hvf */
> >      if (kvm_enabled()) {
> > @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
> > 
> >      /*
> > +     * auto-topoext property will be used to enable topoext feature.
> > +     * This will be disabled on all the older CPU models. Will be
> > +     * enabled on newer CPU modeles which can support topology extention.
> > +     */
> > +     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
> > +
> > +    /*
> >       * From "Requirements for Implementing the Microsoft
> >       * Hypervisor Interface":
> >       * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-
> > windows/reference/tlfs
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 89c82be..8783d36 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -1409,6 +1409,11 @@ struct X86CPU {
> >       */
> >      bool legacy_cache;
> > 
> > +    /* Compatibility bits to enable topoext feature on all newer machines
> > +     * Disabled on older machines. Enabled on newer CPU models
> > +     */
> > +    bool auto_topoext;
> > +
> >      /* Compatibility bits for old machine types: */
> >      bool enable_cpuid_0xb;
> > 
> > --
> > 1.8.3.1
> 

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-08 22:56   ` [Qemu-devel] " Babu Moger
@ 2018-06-11 20:50     ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 20:50 UTC (permalink / raw)
  To: Babu Moger; +Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth

On Fri, Jun 08, 2018 at 06:56:19PM -0400, Babu Moger wrote:
> Enable TOPOEXT feature on EPYC CPU. This is required to support
> hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> 
> TOPOEXT feature is disabled for legacy machines.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index d3411ed..4dd9a82 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2574,6 +2574,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .xlevel = 0x8000000A,
>          .model_id = "AMD EPYC Processor",
>          .cache_info = &epyc_cache_info,
> +        .auto_topoext = 1,
>      },
>      {
>          .name = "EPYC-IBPB",
> @@ -2621,6 +2622,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .xlevel = 0x8000000A,
>          .model_id = "AMD EPYC Processor (with IBPB)",
>          .cache_info = &epyc_cache_info,
> +        .auto_topoext = 1,
>      },
>  };
>  
> @@ -4672,6 +4674,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);
> +        }

I suggest moving this hunk to a separate patch.  I'm not 100%
sure yet if this will require compat_props code to disable
auto-xlevel-increase on older machine-types.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-11 20:50     ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 20:50 UTC (permalink / raw)
  To: Babu Moger
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff

On Fri, Jun 08, 2018 at 06:56:19PM -0400, Babu Moger wrote:
> Enable TOPOEXT feature on EPYC CPU. This is required to support
> hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> 
> TOPOEXT feature is disabled for legacy machines.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index d3411ed..4dd9a82 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2574,6 +2574,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .xlevel = 0x8000000A,
>          .model_id = "AMD EPYC Processor",
>          .cache_info = &epyc_cache_info,
> +        .auto_topoext = 1,
>      },
>      {
>          .name = "EPYC-IBPB",
> @@ -2621,6 +2622,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>          .xlevel = 0x8000000A,
>          .model_id = "AMD EPYC Processor (with IBPB)",
>          .cache_info = &epyc_cache_info,
> +        .auto_topoext = 1,
>      },
>  };
>  
> @@ -4672,6 +4674,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);
> +        }

I suggest moving this hunk to a separate patch.  I'm not 100%
sure yet if this will require compat_props code to disable
auto-xlevel-increase on older machine-types.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 4/5] i386: Verify and enable topoext feature if supported
  2018-06-08 22:56   ` [Qemu-devel] " Babu Moger
@ 2018-06-11 20:51     ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 20:51 UTC (permalink / raw)
  To: Babu Moger; +Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth

On Fri, Jun 08, 2018 at 06:56:20PM -0400, Babu Moger wrote:
> If the CPU model supports topoext feature, enabled the
> feature automatically if it can be supported.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 4dd9a82..88bc73d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4763,6 +4763,33 @@ static int x86_cpu_filter_features(X86CPU *cpu)
>  #define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
>                           (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
>                           (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> +/*
> + * 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 topology_supports_topoext(int nr_cores, int nr_threads,
> +            Error **errp)
> +{
> +    if (nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) {
> +        error_setg(errp, "TOPOEXT unsupported with %d cores per socket",
> +                   nr_cores);
> +        error_append_hint(errp, "TOPOEXT supports only up to %d cores per"
> +                          " socket\n",
> +                          (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> +        return false;
> +    }
> +
> +    if (nr_threads > 2) {
> +        error_setg(errp, "TOPOEXT unsupported with %d threads per core",
> +                   nr_threads);
> +        error_append_hint(errp, "TOPOEXT supports only up to 2 threads"
> +                          " per core\n");
> +        return false;
> +    }
> +    return true;
> +}
> +
>  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -4953,6 +4980,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      qemu_init_vcpu(cs);
>  
> +    if (cpu->auto_topoext &&
> +        !(env->user_features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
> +        if (cs->nr_cores <= (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET) &&
> +            (cs->nr_threads <= 2)) {

This duplicates the logic from topology_supports_topoext().  Why
not just call
  topology_supports_topoext(cs->nr_cores, cs->nr_threads, NULL)
here?

> +            env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_TOPOEXT;
> +        }
> +    }
> +
> +    if ((env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
> +        !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
> +        return;
> +    }
> +
>      /* 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
> -- 
> 1.8.3.1
> 

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 4/5] i386: Verify and enable topoext feature if supported
@ 2018-06-11 20:51     ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 20:51 UTC (permalink / raw)
  To: Babu Moger
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff

On Fri, Jun 08, 2018 at 06:56:20PM -0400, Babu Moger wrote:
> If the CPU model supports topoext feature, enabled the
> feature automatically if it can be supported.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 4dd9a82..88bc73d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4763,6 +4763,33 @@ static int x86_cpu_filter_features(X86CPU *cpu)
>  #define IS_AMD_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && \
>                           (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
>                           (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> +/*
> + * 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 topology_supports_topoext(int nr_cores, int nr_threads,
> +            Error **errp)
> +{
> +    if (nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) {
> +        error_setg(errp, "TOPOEXT unsupported with %d cores per socket",
> +                   nr_cores);
> +        error_append_hint(errp, "TOPOEXT supports only up to %d cores per"
> +                          " socket\n",
> +                          (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> +        return false;
> +    }
> +
> +    if (nr_threads > 2) {
> +        error_setg(errp, "TOPOEXT unsupported with %d threads per core",
> +                   nr_threads);
> +        error_append_hint(errp, "TOPOEXT supports only up to 2 threads"
> +                          " per core\n");
> +        return false;
> +    }
> +    return true;
> +}
> +
>  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -4953,6 +4980,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      qemu_init_vcpu(cs);
>  
> +    if (cpu->auto_topoext &&
> +        !(env->user_features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
> +        if (cs->nr_cores <= (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET) &&
> +            (cs->nr_threads <= 2)) {

This duplicates the logic from topology_supports_topoext().  Why
not just call
  topology_supports_topoext(cs->nr_cores, cs->nr_threads, NULL)
here?

> +            env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_TOPOEXT;
> +        }
> +    }
> +
> +    if ((env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
> +        !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
> +        return;
> +    }
> +
>      /* 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
> -- 
> 1.8.3.1
> 

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
  2018-06-08 22:56   ` [Qemu-devel] " Babu Moger
@ 2018-06-11 20:54     ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 20:54 UTC (permalink / raw)
  To: Babu Moger; +Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth

On Fri, Jun 08, 2018 at 06:56:17PM -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>

Queued on x86-next, thanks.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
@ 2018-06-11 20:54     ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 20:54 UTC (permalink / raw)
  To: Babu Moger
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff

On Fri, Jun 08, 2018 at 06:56:17PM -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>

Queued on x86-next, thanks.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
  2018-06-11 20:46       ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-11 20:59         ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 20:59 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 11, 2018 3: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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> topoext
> 
> On Mon, Jun 11, 2018 at 08:25:32PM +0000, Moger, Babu wrote:
> > Hi Eduardo,
> > Planning  to make couple of changes after testing and review.  Let me know
> if I missed something.
> > Will wait for your feedback before the next revision.
> >
> > > -----Original Message-----
> > > From: kvm-owner@vger.kernel.org [mailto:kvm-
> owner@vger.kernel.org]
> > > On Behalf Of Babu Moger
> > > Sent: Friday, June 8, 2018 5:56 PM
> > > To: 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; Moger, Babu
> > > <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com
> > > Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> > > topoext
> > >
> > > Introduce the auto_topoext bit to to control topoext feature.
> > >
> > > Also add new field auto_topoext(in X86CPUDefinition). This will
> > > be used to enable topoext on newer CPU models where topoext can
> > > be supported.
> > >
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > ---
> > >  include/hw/i386/pc.h |  4 ++++
> > >  target/i386/cpu.c    | 12 ++++++++++++
> > >  target/i386/cpu.h    |  5 +++++
> > >  3 files changed, 21 insertions(+)
> > >
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 04d1f8c..cc30ec3 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> > > uint64_t *);
> > >          .driver   = TYPE_X86_CPU,\
> > >          .property = "legacy-cache",\
> > >          .value    = "on",\
> > > +    },{\
> > > +        .driver   = TYPE_X86_CPU,\
> > > +        .property = "auto-topoext",\
> > > +        .value    = "off",\
> > >      },
> >
> > We don't need this.  We are already setting this false in
> x86_cpu_properties.
> > But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we
> define it.
> > That will be a separate patch. Correct?
> 
> PC_COMPAT_3_0 will exist only on QEMU 3.1.  If you want to define
> something as the default behavior in pc-3.0, just define it as
> the default in the device code, which is exactly what you are
> already doing in x86_cpu_load_def() below.
> 
> In other words, just set auto_topoext=true on the CPU model table
> for EPYC, and it should be enough.
> 
> On PC_COMPAT_2_12, both would work:
>   { TYPE_X86_CPU, "auto-topoext", "off" }
> or
>   { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.
> 
> I prefer the latter, but both would work.

Ok. Sure

> 
> 
> >
> > >
> > >  #define PC_COMPAT_2_11 \
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 86fb1a4..d3411ed 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
> > >      FeatureWordArray features;
> > >      const char *model_id;
> > >      CPUCaches *cache_info;
> > > +    /* Set it if topoext can be enabled in CPU models */
> > > +    int auto_topoext;
> > >  };
> > >
> > >  static CPUCaches epyc_cache_info = {
> > > @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu,
> > > X86CPUDefinition *def, Error **errp)
> > >      /* legacy-cache defaults to 'off' if CPU model provides cache info */
> > >      cpu->legacy_cache = !def->cache_info;
> > >
> > > +    /* Set auto_topoext if both machine property and CPU model
> supports it
> > > */
> > > +    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;
> >
> > This could be more appropriate like this.
> >
> >  cpu->auto_topoext =  cpu->auto_topoext && def->auto_topoext;
> 
> cpu->auto_topoext is supposed to be already false, here.  Why not
> just:
>     cpu->auto_topoext = def->auto_topoext;
> ?

Ok. Will do it.

> 
> >
> > > +
> > >      /* Special cases not set in the X86CPUDefinition structs: */
> > >      /* TODO: in-kernel irqchip for hvf */
> > >      if (kvm_enabled()) {
> > > @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
> > >      DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
> > >
> > >      /*
> > > +     * auto-topoext property will be used to enable topoext feature.
> > > +     * This will be disabled on all the older CPU models. Will be
> > > +     * enabled on newer CPU modeles which can support topology
> extention.
> > > +     */
> > > +     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
> > > +
> > > +    /*
> > >       * From "Requirements for Implementing the Microsoft
> > >       * Hypervisor Interface":
> > >       * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-
> > > windows/reference/tlfs
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index 89c82be..8783d36 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -1409,6 +1409,11 @@ struct X86CPU {
> > >       */
> > >      bool legacy_cache;
> > >
> > > +    /* Compatibility bits to enable topoext feature on all newer machines
> > > +     * Disabled on older machines. Enabled on newer CPU models
> > > +     */
> > > +    bool auto_topoext;
> > > +
> > >      /* Compatibility bits for old machine types: */
> > >      bool enable_cpuid_0xb;
> > >
> > > --
> > > 1.8.3.1
> >
> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
@ 2018-06-11 20:59         ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 20:59 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff



> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Monday, June 11, 2018 3: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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> topoext
> 
> On Mon, Jun 11, 2018 at 08:25:32PM +0000, Moger, Babu wrote:
> > Hi Eduardo,
> > Planning  to make couple of changes after testing and review.  Let me know
> if I missed something.
> > Will wait for your feedback before the next revision.
> >
> > > -----Original Message-----
> > > From: kvm-owner@vger.kernel.org [mailto:kvm-
> owner@vger.kernel.org]
> > > On Behalf Of Babu Moger
> > > Sent: Friday, June 8, 2018 5:56 PM
> > > To: 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; Moger, Babu
> > > <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com
> > > Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> > > topoext
> > >
> > > Introduce the auto_topoext bit to to control topoext feature.
> > >
> > > Also add new field auto_topoext(in X86CPUDefinition). This will
> > > be used to enable topoext on newer CPU models where topoext can
> > > be supported.
> > >
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > ---
> > >  include/hw/i386/pc.h |  4 ++++
> > >  target/i386/cpu.c    | 12 ++++++++++++
> > >  target/i386/cpu.h    |  5 +++++
> > >  3 files changed, 21 insertions(+)
> > >
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 04d1f8c..cc30ec3 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> > > uint64_t *);
> > >          .driver   = TYPE_X86_CPU,\
> > >          .property = "legacy-cache",\
> > >          .value    = "on",\
> > > +    },{\
> > > +        .driver   = TYPE_X86_CPU,\
> > > +        .property = "auto-topoext",\
> > > +        .value    = "off",\
> > >      },
> >
> > We don't need this.  We are already setting this false in
> x86_cpu_properties.
> > But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we
> define it.
> > That will be a separate patch. Correct?
> 
> PC_COMPAT_3_0 will exist only on QEMU 3.1.  If you want to define
> something as the default behavior in pc-3.0, just define it as
> the default in the device code, which is exactly what you are
> already doing in x86_cpu_load_def() below.
> 
> In other words, just set auto_topoext=true on the CPU model table
> for EPYC, and it should be enough.
> 
> On PC_COMPAT_2_12, both would work:
>   { TYPE_X86_CPU, "auto-topoext", "off" }
> or
>   { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.
> 
> I prefer the latter, but both would work.

Ok. Sure

> 
> 
> >
> > >
> > >  #define PC_COMPAT_2_11 \
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 86fb1a4..d3411ed 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -1283,6 +1283,8 @@ struct X86CPUDefinition {
> > >      FeatureWordArray features;
> > >      const char *model_id;
> > >      CPUCaches *cache_info;
> > > +    /* Set it if topoext can be enabled in CPU models */
> > > +    int auto_topoext;
> > >  };
> > >
> > >  static CPUCaches epyc_cache_info = {
> > > @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu,
> > > X86CPUDefinition *def, Error **errp)
> > >      /* legacy-cache defaults to 'off' if CPU model provides cache info */
> > >      cpu->legacy_cache = !def->cache_info;
> > >
> > > +    /* Set auto_topoext if both machine property and CPU model
> supports it
> > > */
> > > +    cpu->auto_topoext =  cpu->auto_topoext & def->auto_topoext;
> >
> > This could be more appropriate like this.
> >
> >  cpu->auto_topoext =  cpu->auto_topoext && def->auto_topoext;
> 
> cpu->auto_topoext is supposed to be already false, here.  Why not
> just:
>     cpu->auto_topoext = def->auto_topoext;
> ?

Ok. Will do it.

> 
> >
> > > +
> > >      /* Special cases not set in the X86CPUDefinition structs: */
> > >      /* TODO: in-kernel irqchip for hvf */
> > >      if (kvm_enabled()) {
> > > @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = {
> > >      DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true),
> > >
> > >      /*
> > > +     * auto-topoext property will be used to enable topoext feature.
> > > +     * This will be disabled on all the older CPU models. Will be
> > > +     * enabled on newer CPU modeles which can support topology
> extention.
> > > +     */
> > > +     DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false),
> > > +
> > > +    /*
> > >       * From "Requirements for Implementing the Microsoft
> > >       * Hypervisor Interface":
> > >       * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-
> > > windows/reference/tlfs
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index 89c82be..8783d36 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -1409,6 +1409,11 @@ struct X86CPU {
> > >       */
> > >      bool legacy_cache;
> > >
> > > +    /* Compatibility bits to enable topoext feature on all newer machines
> > > +     * Disabled on older machines. Enabled on newer CPU models
> > > +     */
> > > +    bool auto_topoext;
> > > +
> > >      /* Compatibility bits for old machine types: */
> > >      bool enable_cpuid_0xb;
> > >
> > > --
> > > 1.8.3.1
> >
> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-11 20:50     ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-11 21:01       ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 21:01 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 11, 2018 3:51 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Fri, Jun 08, 2018 at 06:56:19PM -0400, Babu Moger wrote:
> > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> >
> > TOPOEXT feature is disabled for legacy machines.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  target/i386/cpu.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index d3411ed..4dd9a82 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -2574,6 +2574,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
> >          .xlevel = 0x8000000A,
> >          .model_id = "AMD EPYC Processor",
> >          .cache_info = &epyc_cache_info,
> > +        .auto_topoext = 1,
> >      },
> >      {
> >          .name = "EPYC-IBPB",
> > @@ -2621,6 +2622,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
> >          .xlevel = 0x8000000A,
> >          .model_id = "AMD EPYC Processor (with IBPB)",
> >          .cache_info = &epyc_cache_info,
> > +        .auto_topoext = 1,
> >      },
> >  };
> >
> > @@ -4672,6 +4674,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);
> > +        }
> 
> I suggest moving this hunk to a separate patch.  I'm not 100%

Sure. Will move it to separate patch.

> sure yet if this will require compat_props code to disable
> auto-xlevel-increase on older machine-types.
> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-11 21:01       ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 21:01 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff



> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Monday, June 11, 2018 3:51 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Fri, Jun 08, 2018 at 06:56:19PM -0400, Babu Moger wrote:
> > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> >
> > TOPOEXT feature is disabled for legacy machines.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  target/i386/cpu.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index d3411ed..4dd9a82 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -2574,6 +2574,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
> >          .xlevel = 0x8000000A,
> >          .model_id = "AMD EPYC Processor",
> >          .cache_info = &epyc_cache_info,
> > +        .auto_topoext = 1,
> >      },
> >      {
> >          .name = "EPYC-IBPB",
> > @@ -2621,6 +2622,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
> >          .xlevel = 0x8000000A,
> >          .model_id = "AMD EPYC Processor (with IBPB)",
> >          .cache_info = &epyc_cache_info,
> > +        .auto_topoext = 1,
> >      },
> >  };
> >
> > @@ -4672,6 +4674,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);
> > +        }
> 
> I suggest moving this hunk to a separate patch.  I'm not 100%

Sure. Will move it to separate patch.

> sure yet if this will require compat_props code to disable
> auto-xlevel-increase on older machine-types.
> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 4/5] i386: Verify and enable topoext feature if supported
  2018-06-11 20:51     ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-11 21:01       ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 21:01 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 Eduardo Habkost
> Sent: Monday, June 11, 2018 3:52 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 4/5] i386: Verify and enable topoext feature if
> supported
> 
> On Fri, Jun 08, 2018 at 06:56:20PM -0400, Babu Moger wrote:
> > If the CPU model supports topoext feature, enabled the
> > feature automatically if it can be supported.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  target/i386/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 4dd9a82..88bc73d 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -4763,6 +4763,33 @@ static int x86_cpu_filter_features(X86CPU *cpu)
> >  #define IS_AMD_CPU(env) ((env)->cpuid_vendor1 ==
> CPUID_VENDOR_AMD_1 && \
> >                           (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> >                           (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> > +/*
> > + * 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 topology_supports_topoext(int nr_cores, int nr_threads,
> > +            Error **errp)
> > +{
> > +    if (nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) {
> > +        error_setg(errp, "TOPOEXT unsupported with %d cores per socket",
> > +                   nr_cores);
> > +        error_append_hint(errp, "TOPOEXT supports only up to %d cores
> per"
> > +                          " socket\n",
> > +                          (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> > +        return false;
> > +    }
> > +
> > +    if (nr_threads > 2) {
> > +        error_setg(errp, "TOPOEXT unsupported with %d threads per core",
> > +                   nr_threads);
> > +        error_append_hint(errp, "TOPOEXT supports only up to 2 threads"
> > +                          " per core\n");
> > +        return false;
> > +    }
> > +    return true;
> > +}
> > +
> >  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> >  {
> >      CPUState *cs = CPU(dev);
> > @@ -4953,6 +4980,19 @@ static void x86_cpu_realizefn(DeviceState *dev,
> Error **errp)
> >
> >      qemu_init_vcpu(cs);
> >
> > +    if (cpu->auto_topoext &&
> > +        !(env->user_features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT)) {
> > +        if (cs->nr_cores <= (MAX_CORES_IN_NODE *
> MAX_NODES_PER_SOCKET) &&
> > +            (cs->nr_threads <= 2)) {
> 
> This duplicates the logic from topology_supports_topoext().  Why
> not just call
>   topology_supports_topoext(cs->nr_cores, cs->nr_threads, NULL)
> here?

Ok. Will do it.

> 
> > +            env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_TOPOEXT;
> > +        }
> > +    }
> > +
> > +    if ((env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
> > +        !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
> > +        return;
> > +    }
> > +
> >      /* 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
> > --
> > 1.8.3.1
> >
> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 4/5] i386: Verify and enable topoext feature if supported
@ 2018-06-11 21:01       ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 21:01 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff



> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Eduardo Habkost
> Sent: Monday, June 11, 2018 3:52 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 4/5] i386: Verify and enable topoext feature if
> supported
> 
> On Fri, Jun 08, 2018 at 06:56:20PM -0400, Babu Moger wrote:
> > If the CPU model supports topoext feature, enabled the
> > feature automatically if it can be supported.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  target/i386/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 4dd9a82..88bc73d 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -4763,6 +4763,33 @@ static int x86_cpu_filter_features(X86CPU *cpu)
> >  #define IS_AMD_CPU(env) ((env)->cpuid_vendor1 ==
> CPUID_VENDOR_AMD_1 && \
> >                           (env)->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && \
> >                           (env)->cpuid_vendor3 == CPUID_VENDOR_AMD_3)
> > +/*
> > + * 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 topology_supports_topoext(int nr_cores, int nr_threads,
> > +            Error **errp)
> > +{
> > +    if (nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) {
> > +        error_setg(errp, "TOPOEXT unsupported with %d cores per socket",
> > +                   nr_cores);
> > +        error_append_hint(errp, "TOPOEXT supports only up to %d cores
> per"
> > +                          " socket\n",
> > +                          (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> > +        return false;
> > +    }
> > +
> > +    if (nr_threads > 2) {
> > +        error_setg(errp, "TOPOEXT unsupported with %d threads per core",
> > +                   nr_threads);
> > +        error_append_hint(errp, "TOPOEXT supports only up to 2 threads"
> > +                          " per core\n");
> > +        return false;
> > +    }
> > +    return true;
> > +}
> > +
> >  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> >  {
> >      CPUState *cs = CPU(dev);
> > @@ -4953,6 +4980,19 @@ static void x86_cpu_realizefn(DeviceState *dev,
> Error **errp)
> >
> >      qemu_init_vcpu(cs);
> >
> > +    if (cpu->auto_topoext &&
> > +        !(env->user_features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT)) {
> > +        if (cs->nr_cores <= (MAX_CORES_IN_NODE *
> MAX_NODES_PER_SOCKET) &&
> > +            (cs->nr_threads <= 2)) {
> 
> This duplicates the logic from topology_supports_topoext().  Why
> not just call
>   topology_supports_topoext(cs->nr_cores, cs->nr_threads, NULL)
> here?

Ok. Will do it.

> 
> > +            env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_TOPOEXT;
> > +        }
> > +    }
> > +
> > +    if ((env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
> > +        !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
> > +        return;
> > +    }
> > +
> >      /* 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
> > --
> > 1.8.3.1
> >
> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
  2018-06-11 20:54     ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-11 21:02       ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 21:02 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 11, 2018 3:55 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for
> AMD
> 
> On Fri, Jun 08, 2018 at 06:56:17PM -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>
> 
> Queued on x86-next, thanks.
> 

Thanks

> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
@ 2018-06-11 21:02       ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 21:02 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff


> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Monday, June 11, 2018 3:55 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for
> AMD
> 
> On Fri, Jun 08, 2018 at 06:56:17PM -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>
> 
> Queued on x86-next, thanks.
> 

Thanks

> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
  2018-06-11 20:46       ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-11 21:05         ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 21:05 UTC (permalink / raw)
  To: Moger, Babu; +Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini, rth

On Mon, Jun 11, 2018 at 05:46:23PM -0300, Eduardo Habkost wrote:
[...]
> On PC_COMPAT_2_12, both would work:
>   { TYPE_X86_CPU, "auto-topoext", "off" }
> or
>   { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.
> 
> I prefer the latter, but both would work.

Oh, while we're at it: please name the property "x-auto-topoext",
to indicate it's only for QEMU internal use or debugging, and not
a supported command-line option.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
@ 2018-06-11 21:05         ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 21:05 UTC (permalink / raw)
  To: Moger, Babu
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff

On Mon, Jun 11, 2018 at 05:46:23PM -0300, Eduardo Habkost wrote:
[...]
> On PC_COMPAT_2_12, both would work:
>   { TYPE_X86_CPU, "auto-topoext", "off" }
> or
>   { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.
> 
> I prefer the latter, but both would work.

Oh, while we're at it: please name the property "x-auto-topoext",
to indicate it's only for QEMU internal use or debugging, and not
a supported command-line option.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-11 20:50     ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-11 21:10       ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 21:10 UTC (permalink / raw)
  To: Babu Moger
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth

On Mon, Jun 11, 2018 at 05:50:30PM -0300, Eduardo Habkost wrote:
[...]
> > +        /* TOPOEXT feature requires 0x8000001E */
> > +        if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
> > +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> > +        }
> 
> I suggest moving this hunk to a separate patch.  I'm not 100%
> sure yet if this will require compat_props code to disable
> auto-xlevel-increase on older machine-types.

The problem here is that:
  $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext
currently results in xlevel=0x8000001A, since QEMU 1.3.

(The same applies to all machine-types between 1.3 and 2.12)

I was hoping that we could declare topoext as non-migration-safe,
but I believe libvirt will already include "topoext" when using
"host-model" if the host CPU supports TOPOEXT.  Jiri, can you
confirm that?

We can address that with a "x-topoext-auto-xlevel" property, set
to true on all CPU models by default, and disabled by
PC_COMPAT_2_12.

The code would become:

    if (cpu->topoext_auto_xlevel && env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
        x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
    }

Or, we could simply declare that "-cpu Opteron_G4,+topoext" will
never increase xlevel automatically (on any machine-type), and
change the code above to:

    if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
        x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
    }

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-11 21:10       ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-11 21:10 UTC (permalink / raw)
  To: Babu Moger
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark

On Mon, Jun 11, 2018 at 05:50:30PM -0300, Eduardo Habkost wrote:
[...]
> > +        /* TOPOEXT feature requires 0x8000001E */
> > +        if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
> > +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> > +        }
> 
> I suggest moving this hunk to a separate patch.  I'm not 100%
> sure yet if this will require compat_props code to disable
> auto-xlevel-increase on older machine-types.

The problem here is that:
  $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext
currently results in xlevel=0x8000001A, since QEMU 1.3.

(The same applies to all machine-types between 1.3 and 2.12)

I was hoping that we could declare topoext as non-migration-safe,
but I believe libvirt will already include "topoext" when using
"host-model" if the host CPU supports TOPOEXT.  Jiri, can you
confirm that?

We can address that with a "x-topoext-auto-xlevel" property, set
to true on all CPU models by default, and disabled by
PC_COMPAT_2_12.

The code would become:

    if (cpu->topoext_auto_xlevel && env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
        x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
    }

Or, we could simply declare that "-cpu Opteron_G4,+topoext" will
never increase xlevel automatically (on any machine-type), and
change the code above to:

    if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
        x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
    }

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
  2018-06-11 21:05         ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-11 21:20           ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 21:20 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 Eduardo Habkost
> Sent: Monday, June 11, 2018 4:05 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> topoext
> 
> On Mon, Jun 11, 2018 at 05:46:23PM -0300, Eduardo Habkost wrote:
> [...]
> > On PC_COMPAT_2_12, both would work:
> >   { TYPE_X86_CPU, "auto-topoext", "off" }
> > or
> >   { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.
> >
> > I prefer the latter, but both would work.
> 
> Oh, while we're at it: please name the property "x-auto-topoext",
> to indicate it's only for QEMU internal use or debugging, and not
> a supported command-line option.

Ok. Sure.

> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext
@ 2018-06-11 21:20           ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-11 21:20 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff


> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Eduardo Habkost
> Sent: Monday, June 11, 2018 4:05 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage
> topoext
> 
> On Mon, Jun 11, 2018 at 05:46:23PM -0300, Eduardo Habkost wrote:
> [...]
> > On PC_COMPAT_2_12, both would work:
> >   { TYPE_X86_CPU, "auto-topoext", "off" }
> > or
> >   { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }.
> >
> > I prefer the latter, but both would work.
> 
> Oh, while we're at it: please name the property "x-auto-topoext",
> to indicate it's only for QEMU internal use or debugging, and not
> a supported command-line option.

Ok. Sure.

> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-11 21:10       ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-12 16:29         ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-12 16:29 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth



> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Eduardo Habkost
> Sent: Monday, June 11, 2018 4:10 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Mon, Jun 11, 2018 at 05:50:30PM -0300, Eduardo Habkost wrote:
> [...]
> > > +        /* TOPOEXT feature requires 0x8000001E */
> > > +        if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)
> {
> > > +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel,
> 0x8000001E);
> > > +        }
> >
> > I suggest moving this hunk to a separate patch.  I'm not 100%
> > sure yet if this will require compat_props code to disable
> > auto-xlevel-increase on older machine-types.
> 
> The problem here is that:
>   $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext
> currently results in xlevel=0x8000001A, since QEMU 1.3.
> 
> (The same applies to all machine-types between 1.3 and 2.12)
> 
> I was hoping that we could declare topoext as non-migration-safe,
> but I believe libvirt will already include "topoext" when using
> "host-model" if the host CPU supports TOPOEXT.  Jiri, can you
> confirm that?
> 
> We can address that with a "x-topoext-auto-xlevel" property, set
> to true on all CPU models by default, and disabled by
> PC_COMPAT_2_12.
> 
> The code would become:
> 
>     if (cpu->topoext_auto_xlevel && env->features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT) {
>         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
>     }
> 
> Or, we could simply declare that "-cpu Opteron_G4,+topoext" will
> never increase xlevel automatically (on any machine-type), and
> change the code above to:
> 
>     if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT) {
>         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
>     }

I was going to do this.  But there is one problem.  We don't set the CPUID_EXT3_TOPOEXT in CPU model table. So this won't work.
One more thing I noticed that feature setting should happen much before x86_cpu_realizefn.

Couple of options. 
First option.
1. Set both feature and xlevel here(in x86_cpu_expand_features).
 if (cpu->x_auto_topoext {
        env->features[FEAT_8000_0001_ECX]  |= CPUID_EXT3_TOPOEXT;
        x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
 }
2. And remove feature setting in x86_cpu_realizefn.

Or 

Second option
1.Set the feature bit in CPU model table.
2. Set xlevel in x86_cpu_expand_features using cpu->x_auto_topoext
3. And remove feature setting in x86_cpu_realizefn.
 
I  prefer the second option. 

> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-12 16:29         ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-12 16:29 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark



> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Eduardo Habkost
> Sent: Monday, June 11, 2018 4:10 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Mon, Jun 11, 2018 at 05:50:30PM -0300, Eduardo Habkost wrote:
> [...]
> > > +        /* TOPOEXT feature requires 0x8000001E */
> > > +        if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)
> {
> > > +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel,
> 0x8000001E);
> > > +        }
> >
> > I suggest moving this hunk to a separate patch.  I'm not 100%
> > sure yet if this will require compat_props code to disable
> > auto-xlevel-increase on older machine-types.
> 
> The problem here is that:
>   $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext
> currently results in xlevel=0x8000001A, since QEMU 1.3.
> 
> (The same applies to all machine-types between 1.3 and 2.12)
> 
> I was hoping that we could declare topoext as non-migration-safe,
> but I believe libvirt will already include "topoext" when using
> "host-model" if the host CPU supports TOPOEXT.  Jiri, can you
> confirm that?
> 
> We can address that with a "x-topoext-auto-xlevel" property, set
> to true on all CPU models by default, and disabled by
> PC_COMPAT_2_12.
> 
> The code would become:
> 
>     if (cpu->topoext_auto_xlevel && env->features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT) {
>         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
>     }
> 
> Or, we could simply declare that "-cpu Opteron_G4,+topoext" will
> never increase xlevel automatically (on any machine-type), and
> change the code above to:
> 
>     if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT) {
>         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
>     }

I was going to do this.  But there is one problem.  We don't set the CPUID_EXT3_TOPOEXT in CPU model table. So this won't work.
One more thing I noticed that feature setting should happen much before x86_cpu_realizefn.

Couple of options. 
First option.
1. Set both feature and xlevel here(in x86_cpu_expand_features).
 if (cpu->x_auto_topoext {
        env->features[FEAT_8000_0001_ECX]  |= CPUID_EXT3_TOPOEXT;
        x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
 }
2. And remove feature setting in x86_cpu_realizefn.

Or 

Second option
1.Set the feature bit in CPU model table.
2. Set xlevel in x86_cpu_expand_features using cpu->x_auto_topoext
3. And remove feature setting in x86_cpu_realizefn.
 
I  prefer the second option. 

> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-12 16:29         ` [Qemu-devel] " Moger, Babu
@ 2018-06-12 17:40           ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-12 17:40 UTC (permalink / raw)
  To: Moger, Babu
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth

On Tue, Jun 12, 2018 at 04:29:25PM +0000, Moger, Babu wrote:
> > [...]
> > > > +        /* TOPOEXT feature requires 0x8000001E */
> > > > +        if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)
> > {
> > > > +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel,
> > 0x8000001E);
> > > > +        }
> > >
> > > I suggest moving this hunk to a separate patch.  I'm not 100%
> > > sure yet if this will require compat_props code to disable
> > > auto-xlevel-increase on older machine-types.
> > 
> > The problem here is that:
> >   $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext
> > currently results in xlevel=0x8000001A, since QEMU 1.3.
> > 
> > (The same applies to all machine-types between 1.3 and 2.12)
> > 
> > I was hoping that we could declare topoext as non-migration-safe,
> > but I believe libvirt will already include "topoext" when using
> > "host-model" if the host CPU supports TOPOEXT.  Jiri, can you
> > confirm that?
> > 
> > We can address that with a "x-topoext-auto-xlevel" property, set
> > to true on all CPU models by default, and disabled by
> > PC_COMPAT_2_12.
> > 
> > The code would become:
> > 
> >     if (cpu->topoext_auto_xlevel && env->features[FEAT_8000_0001_ECX] &
> > CPUID_EXT3_TOPOEXT) {
> >         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> >     }
> > 
> > Or, we could simply declare that "-cpu Opteron_G4,+topoext" will
> > never increase xlevel automatically (on any machine-type), and
> > change the code above to:
> > 
> >     if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] &
> > CPUID_EXT3_TOPOEXT) {
> >         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> >     }
> 
> I was going to do this.  But there is one problem.  We don't
> set the CPUID_EXT3_TOPOEXT in CPU model table. So this won't
> work.

Won't this work if the auto_topoext handling is done before
x86_cpu_expand_features() is called?


> One more thing I noticed that feature setting should happen
> much before x86_cpu_realizefn.

Why?

> 
> Couple of options. 
> First option.
> 1. Set both feature and xlevel here(in x86_cpu_expand_features).
>  if (cpu->x_auto_topoext {
>         env->features[FEAT_8000_0001_ECX]  |= CPUID_EXT3_TOPOEXT;
>         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
>  }
> 2. And remove feature setting in x86_cpu_realizefn.

This would make TOPOEXT be included in 'query-cpu-model-expansion
model=EPYC', which would be incorrect because TOPOEXT won't
always be enabled when using EPYC.


> 
> Or 
> 
> Second option
> 1.Set the feature bit in CPU model table.
> 2. Set xlevel in x86_cpu_expand_features using cpu->x_auto_topoext
> 3. And remove feature setting in x86_cpu_realizefn.
>  
> I  prefer the second option. 

Same here: TOPOEXT would be included in
'query-cpu-model-expansion model=EPYC', and this would be
incorrect.


I'm starting to think that enabling TOPOEXT automatically is
adding too much complexity and compatibility problems, and it's
better to leave this task to management software.

The main problem here is:

This works today with QEMU 2.12 + Linux <= 4.15:
  $ $QEMU -machine pc -cpu EPYC,enforce -smp 8,sockets=2,cores=2,threads=2"
and must keep working with QEMU 3.0 and Linux <= 4.15.

In addition to that, the results for:
  $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
must be deterministic and expose exactly the same CPUID data even
if host hardware or software changes, as long as the QEMU
command-line is the same.

Do you see a way to fulfill those two constraints while making
"-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-12 17:40           ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-12 17:40 UTC (permalink / raw)
  To: Moger, Babu
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark

On Tue, Jun 12, 2018 at 04:29:25PM +0000, Moger, Babu wrote:
> > [...]
> > > > +        /* TOPOEXT feature requires 0x8000001E */
> > > > +        if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)
> > {
> > > > +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel,
> > 0x8000001E);
> > > > +        }
> > >
> > > I suggest moving this hunk to a separate patch.  I'm not 100%
> > > sure yet if this will require compat_props code to disable
> > > auto-xlevel-increase on older machine-types.
> > 
> > The problem here is that:
> >   $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext
> > currently results in xlevel=0x8000001A, since QEMU 1.3.
> > 
> > (The same applies to all machine-types between 1.3 and 2.12)
> > 
> > I was hoping that we could declare topoext as non-migration-safe,
> > but I believe libvirt will already include "topoext" when using
> > "host-model" if the host CPU supports TOPOEXT.  Jiri, can you
> > confirm that?
> > 
> > We can address that with a "x-topoext-auto-xlevel" property, set
> > to true on all CPU models by default, and disabled by
> > PC_COMPAT_2_12.
> > 
> > The code would become:
> > 
> >     if (cpu->topoext_auto_xlevel && env->features[FEAT_8000_0001_ECX] &
> > CPUID_EXT3_TOPOEXT) {
> >         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> >     }
> > 
> > Or, we could simply declare that "-cpu Opteron_G4,+topoext" will
> > never increase xlevel automatically (on any machine-type), and
> > change the code above to:
> > 
> >     if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] &
> > CPUID_EXT3_TOPOEXT) {
> >         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> >     }
> 
> I was going to do this.  But there is one problem.  We don't
> set the CPUID_EXT3_TOPOEXT in CPU model table. So this won't
> work.

Won't this work if the auto_topoext handling is done before
x86_cpu_expand_features() is called?


> One more thing I noticed that feature setting should happen
> much before x86_cpu_realizefn.

Why?

> 
> Couple of options. 
> First option.
> 1. Set both feature and xlevel here(in x86_cpu_expand_features).
>  if (cpu->x_auto_topoext {
>         env->features[FEAT_8000_0001_ECX]  |= CPUID_EXT3_TOPOEXT;
>         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
>  }
> 2. And remove feature setting in x86_cpu_realizefn.

This would make TOPOEXT be included in 'query-cpu-model-expansion
model=EPYC', which would be incorrect because TOPOEXT won't
always be enabled when using EPYC.


> 
> Or 
> 
> Second option
> 1.Set the feature bit in CPU model table.
> 2. Set xlevel in x86_cpu_expand_features using cpu->x_auto_topoext
> 3. And remove feature setting in x86_cpu_realizefn.
>  
> I  prefer the second option. 

Same here: TOPOEXT would be included in
'query-cpu-model-expansion model=EPYC', and this would be
incorrect.


I'm starting to think that enabling TOPOEXT automatically is
adding too much complexity and compatibility problems, and it's
better to leave this task to management software.

The main problem here is:

This works today with QEMU 2.12 + Linux <= 4.15:
  $ $QEMU -machine pc -cpu EPYC,enforce -smp 8,sockets=2,cores=2,threads=2"
and must keep working with QEMU 3.0 and Linux <= 4.15.

In addition to that, the results for:
  $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
must be deterministic and expose exactly the same CPUID data even
if host hardware or software changes, as long as the QEMU
command-line is the same.

Do you see a way to fulfill those two constraints while making
"-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-12 17:40           ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-12 18:38             ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-12 18:38 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth



> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Tuesday, June 12, 2018 12:40 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Tue, Jun 12, 2018 at 04:29:25PM +0000, Moger, Babu wrote:
> > > [...]
> > > > > +        /* TOPOEXT feature requires 0x8000001E */
> > > > > +        if (env->features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT)
> > > {
> > > > > +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel,
> > > 0x8000001E);
> > > > > +        }
> > > >
> > > > I suggest moving this hunk to a separate patch.  I'm not 100%
> > > > sure yet if this will require compat_props code to disable
> > > > auto-xlevel-increase on older machine-types.
> > >
> > > The problem here is that:
> > >   $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext
> > > currently results in xlevel=0x8000001A, since QEMU 1.3.
> > >
> > > (The same applies to all machine-types between 1.3 and 2.12)
> > >
> > > I was hoping that we could declare topoext as non-migration-safe,
> > > but I believe libvirt will already include "topoext" when using
> > > "host-model" if the host CPU supports TOPOEXT.  Jiri, can you
> > > confirm that?
> > >
> > > We can address that with a "x-topoext-auto-xlevel" property, set
> > > to true on all CPU models by default, and disabled by
> > > PC_COMPAT_2_12.
> > >
> > > The code would become:
> > >
> > >     if (cpu->topoext_auto_xlevel && env-
> >features[FEAT_8000_0001_ECX] &
> > > CPUID_EXT3_TOPOEXT) {
> > >         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> > >     }
> > >
> > > Or, we could simply declare that "-cpu Opteron_G4,+topoext" will
> > > never increase xlevel automatically (on any machine-type), and
> > > change the code above to:
> > >
> > >     if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] &
> > > CPUID_EXT3_TOPOEXT) {
> > >         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> > >     }
> >
> > I was going to do this.  But there is one problem.  We don't
> > set the CPUID_EXT3_TOPOEXT in CPU model table. So this won't
> > work.
> 
> Won't this work if the auto_topoext handling is done before
> x86_cpu_expand_features() is called?

Yes. That works. We can go with this approach.

> 
> 
> > One more thing I noticed that feature setting should happen
> > much before x86_cpu_realizefn.
> 
> Why?

I was wrong here.  It should be done before x86_cpu_expand_features.

> 
> >
> > Couple of options.
> > First option.
> > 1. Set both feature and xlevel here(in x86_cpu_expand_features).
> >  if (cpu->x_auto_topoext {
> >         env->features[FEAT_8000_0001_ECX]  |= CPUID_EXT3_TOPOEXT;
> >         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> >  }
> > 2. And remove feature setting in x86_cpu_realizefn.
> 
> This would make TOPOEXT be included in 'query-cpu-model-expansion
> model=EPYC', which would be incorrect because TOPOEXT won't
> always be enabled when using EPYC.
> 
> 
> >
> > Or
> >
> > Second option
> > 1.Set the feature bit in CPU model table.
> > 2. Set xlevel in x86_cpu_expand_features using cpu->x_auto_topoext
> > 3. And remove feature setting in x86_cpu_realizefn.
> >
> > I  prefer the second option.
> 
> Same here: TOPOEXT would be included in
> 'query-cpu-model-expansion model=EPYC', and this would be
> incorrect.
> 
> 
> I'm starting to think that enabling TOPOEXT automatically is
> adding too much complexity and compatibility problems, and it's
> better to leave this task to management software.
> 
> The main problem here is:
> 
> This works today with QEMU 2.12 + Linux <= 4.15:
>   $ $QEMU -machine pc -cpu EPYC,enforce -smp
> 8,sockets=2,cores=2,threads=2"
> and must keep working with QEMU 3.0 and Linux <= 4.15.
> 
> In addition to that, the results for:
>   $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
> must be deterministic and expose exactly the same CPUID data even
> if host hardware or software changes, as long as the QEMU
> command-line is the same.
> 
> Do you see a way to fulfill those two constraints while making
> "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?
> 

Now(setting feature before x86_cpu_expand_features), enabling TOPOEXT appears to work fine.

> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-12 18:38             ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-12 18:38 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark



> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Tuesday, June 12, 2018 12:40 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Tue, Jun 12, 2018 at 04:29:25PM +0000, Moger, Babu wrote:
> > > [...]
> > > > > +        /* TOPOEXT feature requires 0x8000001E */
> > > > > +        if (env->features[FEAT_8000_0001_ECX] &
> CPUID_EXT3_TOPOEXT)
> > > {
> > > > > +            x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel,
> > > 0x8000001E);
> > > > > +        }
> > > >
> > > > I suggest moving this hunk to a separate patch.  I'm not 100%
> > > > sure yet if this will require compat_props code to disable
> > > > auto-xlevel-increase on older machine-types.
> > >
> > > The problem here is that:
> > >   $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext
> > > currently results in xlevel=0x8000001A, since QEMU 1.3.
> > >
> > > (The same applies to all machine-types between 1.3 and 2.12)
> > >
> > > I was hoping that we could declare topoext as non-migration-safe,
> > > but I believe libvirt will already include "topoext" when using
> > > "host-model" if the host CPU supports TOPOEXT.  Jiri, can you
> > > confirm that?
> > >
> > > We can address that with a "x-topoext-auto-xlevel" property, set
> > > to true on all CPU models by default, and disabled by
> > > PC_COMPAT_2_12.
> > >
> > > The code would become:
> > >
> > >     if (cpu->topoext_auto_xlevel && env-
> >features[FEAT_8000_0001_ECX] &
> > > CPUID_EXT3_TOPOEXT) {
> > >         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> > >     }
> > >
> > > Or, we could simply declare that "-cpu Opteron_G4,+topoext" will
> > > never increase xlevel automatically (on any machine-type), and
> > > change the code above to:
> > >
> > >     if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] &
> > > CPUID_EXT3_TOPOEXT) {
> > >         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> > >     }
> >
> > I was going to do this.  But there is one problem.  We don't
> > set the CPUID_EXT3_TOPOEXT in CPU model table. So this won't
> > work.
> 
> Won't this work if the auto_topoext handling is done before
> x86_cpu_expand_features() is called?

Yes. That works. We can go with this approach.

> 
> 
> > One more thing I noticed that feature setting should happen
> > much before x86_cpu_realizefn.
> 
> Why?

I was wrong here.  It should be done before x86_cpu_expand_features.

> 
> >
> > Couple of options.
> > First option.
> > 1. Set both feature and xlevel here(in x86_cpu_expand_features).
> >  if (cpu->x_auto_topoext {
> >         env->features[FEAT_8000_0001_ECX]  |= CPUID_EXT3_TOPOEXT;
> >         x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E);
> >  }
> > 2. And remove feature setting in x86_cpu_realizefn.
> 
> This would make TOPOEXT be included in 'query-cpu-model-expansion
> model=EPYC', which would be incorrect because TOPOEXT won't
> always be enabled when using EPYC.
> 
> 
> >
> > Or
> >
> > Second option
> > 1.Set the feature bit in CPU model table.
> > 2. Set xlevel in x86_cpu_expand_features using cpu->x_auto_topoext
> > 3. And remove feature setting in x86_cpu_realizefn.
> >
> > I  prefer the second option.
> 
> Same here: TOPOEXT would be included in
> 'query-cpu-model-expansion model=EPYC', and this would be
> incorrect.
> 
> 
> I'm starting to think that enabling TOPOEXT automatically is
> adding too much complexity and compatibility problems, and it's
> better to leave this task to management software.
> 
> The main problem here is:
> 
> This works today with QEMU 2.12 + Linux <= 4.15:
>   $ $QEMU -machine pc -cpu EPYC,enforce -smp
> 8,sockets=2,cores=2,threads=2"
> and must keep working with QEMU 3.0 and Linux <= 4.15.
> 
> In addition to that, the results for:
>   $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
> must be deterministic and expose exactly the same CPUID data even
> if host hardware or software changes, as long as the QEMU
> command-line is the same.
> 
> Do you see a way to fulfill those two constraints while making
> "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?
> 

Now(setting feature before x86_cpu_expand_features), enabling TOPOEXT appears to work fine.

> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-12 18:38             ` [Qemu-devel] " Moger, Babu
@ 2018-06-12 19:05               ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-12 19:05 UTC (permalink / raw)
  To: Moger, Babu
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth

On Tue, Jun 12, 2018 at 06:38:08PM +0000, Moger, Babu wrote:
[...]
> > I'm starting to think that enabling TOPOEXT automatically is
> > adding too much complexity and compatibility problems, and it's
> > better to leave this task to management software.
> > 
> > The main problem here is:
> > 
> > This works today with QEMU 2.12 + Linux <= 4.15:
> >   $ $QEMU -machine pc -cpu EPYC,enforce -smp
> > 8,sockets=2,cores=2,threads=2"
> > and must keep working with QEMU 3.0 and Linux <= 4.15.
> > 
> > In addition to that, the results for:
> >   $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
> > must be deterministic and expose exactly the same CPUID data even
> > if host hardware or software changes, as long as the QEMU
> > command-line is the same.
> > 
> > Do you see a way to fulfill those two constraints while making
> > "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?
> > 
> 
> Now(setting feature before x86_cpu_expand_features), enabling
> TOPOEXT appears to work fine.

What about the above constraints?  Are you really fulfilling
them?

This one is tricky:

] This works today with QEMU 2.12 + Linux <= 4.15:
]   $ $QEMU -machine pc -cpu EPYC,enforce -smp 8,sockets=2,cores=2,threads=2"
] and must keep working with QEMU 3.0 and Linux <= 4.15.

If we enable TOPOEXT unconditionally, the command-line won't work
with Linux <= 4.15.

If we enable TOPOEXT only if the kernel returns TOPOEXT on
GET_SUPPORTED_CPUID, we break the second constraint:

] The results for:
]   $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
] must be deterministic and expose exactly the same CPUID data even
] if host hardware or software changes, as long as the QEMU
] command-line is the same.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-12 19:05               ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-12 19:05 UTC (permalink / raw)
  To: Moger, Babu
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark

On Tue, Jun 12, 2018 at 06:38:08PM +0000, Moger, Babu wrote:
[...]
> > I'm starting to think that enabling TOPOEXT automatically is
> > adding too much complexity and compatibility problems, and it's
> > better to leave this task to management software.
> > 
> > The main problem here is:
> > 
> > This works today with QEMU 2.12 + Linux <= 4.15:
> >   $ $QEMU -machine pc -cpu EPYC,enforce -smp
> > 8,sockets=2,cores=2,threads=2"
> > and must keep working with QEMU 3.0 and Linux <= 4.15.
> > 
> > In addition to that, the results for:
> >   $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
> > must be deterministic and expose exactly the same CPUID data even
> > if host hardware or software changes, as long as the QEMU
> > command-line is the same.
> > 
> > Do you see a way to fulfill those two constraints while making
> > "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?
> > 
> 
> Now(setting feature before x86_cpu_expand_features), enabling
> TOPOEXT appears to work fine.

What about the above constraints?  Are you really fulfilling
them?

This one is tricky:

] This works today with QEMU 2.12 + Linux <= 4.15:
]   $ $QEMU -machine pc -cpu EPYC,enforce -smp 8,sockets=2,cores=2,threads=2"
] and must keep working with QEMU 3.0 and Linux <= 4.15.

If we enable TOPOEXT unconditionally, the command-line won't work
with Linux <= 4.15.

If we enable TOPOEXT only if the kernel returns TOPOEXT on
GET_SUPPORTED_CPUID, we break the second constraint:

] The results for:
]   $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
] must be deterministic and expose exactly the same CPUID data even
] if host hardware or software changes, as long as the QEMU
] command-line is the same.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-12 19:05               ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-12 19:46                 ` Babu Moger
  -1 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-12 19:46 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth



On 06/12/2018 02:05 PM, Eduardo Habkost wrote:
> On Tue, Jun 12, 2018 at 06:38:08PM +0000, Moger, Babu wrote:
> [...]
>>> I'm starting to think that enabling TOPOEXT automatically is
>>> adding too much complexity and compatibility problems, and it's
>>> better to leave this task to management software.
>>>
>>> The main problem here is:
>>>
>>> This works today with QEMU 2.12 + Linux <= 4.15:
>>>    $ $QEMU -machine pc -cpu EPYC,enforce -smp
>>> 8,sockets=2,cores=2,threads=2"
>>> and must keep working with QEMU 3.0 and Linux <= 4.15.
>>>
>>> In addition to that, the results for:
>>>    $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
>>> must be deterministic and expose exactly the same CPUID data even
>>> if host hardware or software changes, as long as the QEMU
>>> command-line is the same.
>>>
>>> Do you see a way to fulfill those two constraints while making
>>> "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?
>>>
>> Now(setting feature before x86_cpu_expand_features), enabling
>> TOPOEXT appears to work fine.
> What about the above constraints?  Are you really fulfilling
> them?
>
> This one is tricky:
>
> ] This works today with QEMU 2.12 + Linux <= 4.15:
> ]   $ $QEMU -machine pc -cpu EPYC,enforce -smp 8,sockets=2,cores=2,threads=2"
> ] and must keep working with QEMU 3.0 and Linux <= 4.15.
  This works fine on kernel <= 4.15 with some warnings(-smp 
8,sockets=2,cores=2,threads=2 -cpu EPYC).

qemu-system-x86_64: warning: host doesn't support requested feature: 
CPUID.80000001H:EDX.rdtscp [bit 27]
qemu-system-x86_64: warning: host doesn't support requested feature: 
CPUID.80000001H:ECX.topoext [bit 22]
qemu-system-x86_64: This family of AMD CPU doesn't support 
hyperthreading(2). Please configure -smp options properly or try 
enabling topoext feature.
continues..
>
> If we enable TOPOEXT unconditionally, the command-line won't work
> with Linux <= 4.15.
>
> If we enable TOPOEXT only if the kernel returns TOPOEXT on
> GET_SUPPORTED_CPUID, we break the second constraint:
>
> ] The results for:
> ]   $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
> ] must be deterministic and expose exactly the same CPUID data even
> ] if host hardware or software changes, as long as the QEMU
> ] command-line is the same.
>
This fails on kernel  <= 4.15 with following messages((-smp 
8,sockets=2,cores=2,threads=2 -cpu EPYC,enforce).
qemu-system-x86_64: warning: host doesn't support requested feature: 
CPUID.80000001H:EDX.rdtscp [bit 27]
qemu-system-x86_64: warning: host doesn't support requested feature: 
CPUID.80000001H:ECX.topoext [bit 22]
qemu-system-x86_64: Host doesn't support requested features
exits..

What do you think our options are here?

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-12 19:46                 ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2018-06-12 19:46 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark



On 06/12/2018 02:05 PM, Eduardo Habkost wrote:
> On Tue, Jun 12, 2018 at 06:38:08PM +0000, Moger, Babu wrote:
> [...]
>>> I'm starting to think that enabling TOPOEXT automatically is
>>> adding too much complexity and compatibility problems, and it's
>>> better to leave this task to management software.
>>>
>>> The main problem here is:
>>>
>>> This works today with QEMU 2.12 + Linux <= 4.15:
>>>    $ $QEMU -machine pc -cpu EPYC,enforce -smp
>>> 8,sockets=2,cores=2,threads=2"
>>> and must keep working with QEMU 3.0 and Linux <= 4.15.
>>>
>>> In addition to that, the results for:
>>>    $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
>>> must be deterministic and expose exactly the same CPUID data even
>>> if host hardware or software changes, as long as the QEMU
>>> command-line is the same.
>>>
>>> Do you see a way to fulfill those two constraints while making
>>> "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?
>>>
>> Now(setting feature before x86_cpu_expand_features), enabling
>> TOPOEXT appears to work fine.
> What about the above constraints?  Are you really fulfilling
> them?
>
> This one is tricky:
>
> ] This works today with QEMU 2.12 + Linux <= 4.15:
> ]   $ $QEMU -machine pc -cpu EPYC,enforce -smp 8,sockets=2,cores=2,threads=2"
> ] and must keep working with QEMU 3.0 and Linux <= 4.15.
  This works fine on kernel <= 4.15 with some warnings(-smp 
8,sockets=2,cores=2,threads=2 -cpu EPYC).

qemu-system-x86_64: warning: host doesn't support requested feature: 
CPUID.80000001H:EDX.rdtscp [bit 27]
qemu-system-x86_64: warning: host doesn't support requested feature: 
CPUID.80000001H:ECX.topoext [bit 22]
qemu-system-x86_64: This family of AMD CPU doesn't support 
hyperthreading(2). Please configure -smp options properly or try 
enabling topoext feature.
continues..
>
> If we enable TOPOEXT unconditionally, the command-line won't work
> with Linux <= 4.15.
>
> If we enable TOPOEXT only if the kernel returns TOPOEXT on
> GET_SUPPORTED_CPUID, we break the second constraint:
>
> ] The results for:
> ]   $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
> ] must be deterministic and expose exactly the same CPUID data even
> ] if host hardware or software changes, as long as the QEMU
> ] command-line is the same.
>
This fails on kernel  <= 4.15 with following messages((-smp 
8,sockets=2,cores=2,threads=2 -cpu EPYC,enforce).
qemu-system-x86_64: warning: host doesn't support requested feature: 
CPUID.80000001H:EDX.rdtscp [bit 27]
qemu-system-x86_64: warning: host doesn't support requested feature: 
CPUID.80000001H:ECX.topoext [bit 22]
qemu-system-x86_64: Host doesn't support requested features
exits..

What do you think our options are here?

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-12 19:46                 ` [Qemu-devel] " Babu Moger
@ 2018-06-13 16:52                   ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-13 16:52 UTC (permalink / raw)
  To: Moger, Babu, Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth



> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Babu Moger
> Sent: Tuesday, June 12, 2018 2:47 PM
> To: Eduardo Habkost <ehabkost@redhat.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> 
> 
> On 06/12/2018 02:05 PM, Eduardo Habkost wrote:
> > On Tue, Jun 12, 2018 at 06:38:08PM +0000, Moger, Babu wrote:
> > [...]
> >>> I'm starting to think that enabling TOPOEXT automatically is
> >>> adding too much complexity and compatibility problems, and it's
> >>> better to leave this task to management software.
> >>>
> >>> The main problem here is:
> >>>
> >>> This works today with QEMU 2.12 + Linux <= 4.15:
> >>>    $ $QEMU -machine pc -cpu EPYC,enforce -smp
> >>> 8,sockets=2,cores=2,threads=2"
> >>> and must keep working with QEMU 3.0 and Linux <= 4.15.
> >>>
> >>> In addition to that, the results for:
> >>>    $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
> >>> must be deterministic and expose exactly the same CPUID data even
> >>> if host hardware or software changes, as long as the QEMU
> >>> command-line is the same.
> >>>
> >>> Do you see a way to fulfill those two constraints while making
> >>> "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?
> >>>
> >> Now(setting feature before x86_cpu_expand_features), enabling
> >> TOPOEXT appears to work fine.
> > What about the above constraints?  Are you really fulfilling
> > them?
> >
> > This one is tricky:
> >
> > ] This works today with QEMU 2.12 + Linux <= 4.15:
> > ]   $ $QEMU -machine pc -cpu EPYC,enforce -smp
> 8,sockets=2,cores=2,threads=2"
> > ] and must keep working with QEMU 3.0 and Linux <= 4.15.
>   This works fine on kernel <= 4.15 with some warnings(-smp
> 8,sockets=2,cores=2,threads=2 -cpu EPYC).
> 
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.80000001H:EDX.rdtscp [bit 27]
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.80000001H:ECX.topoext [bit 22]
> qemu-system-x86_64: This family of AMD CPU doesn't support
> hyperthreading(2). Please configure -smp options properly or try
> enabling topoext feature.
> continues..
> >
> > If we enable TOPOEXT unconditionally, the command-line won't work
> > with Linux <= 4.15.
> >
> > If we enable TOPOEXT only if the kernel returns TOPOEXT on
> > GET_SUPPORTED_CPUID, we break the second constraint:
> >
> > ] The results for:
> > ]   $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
> > ] must be deterministic and expose exactly the same CPUID data even
> > ] if host hardware or software changes, as long as the QEMU
> > ] command-line is the same.
> >
> This fails on kernel  <= 4.15 with following messages((-smp
> 8,sockets=2,cores=2,threads=2 -cpu EPYC,enforce).
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.80000001H:EDX.rdtscp [bit 27]
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.80000001H:ECX.topoext [bit 22]
> qemu-system-x86_64: Host doesn't support requested features
> exits..
> 
> What do you think our options are here?

Should we drop automatic topoext completely and move forward? What are your thoughts?


^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-13 16:52                   ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-13 16:52 UTC (permalink / raw)
  To: Moger, Babu, Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark



> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Babu Moger
> Sent: Tuesday, June 12, 2018 2:47 PM
> To: Eduardo Habkost <ehabkost@redhat.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> 
> 
> On 06/12/2018 02:05 PM, Eduardo Habkost wrote:
> > On Tue, Jun 12, 2018 at 06:38:08PM +0000, Moger, Babu wrote:
> > [...]
> >>> I'm starting to think that enabling TOPOEXT automatically is
> >>> adding too much complexity and compatibility problems, and it's
> >>> better to leave this task to management software.
> >>>
> >>> The main problem here is:
> >>>
> >>> This works today with QEMU 2.12 + Linux <= 4.15:
> >>>    $ $QEMU -machine pc -cpu EPYC,enforce -smp
> >>> 8,sockets=2,cores=2,threads=2"
> >>> and must keep working with QEMU 3.0 and Linux <= 4.15.
> >>>
> >>> In addition to that, the results for:
> >>>    $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
> >>> must be deterministic and expose exactly the same CPUID data even
> >>> if host hardware or software changes, as long as the QEMU
> >>> command-line is the same.
> >>>
> >>> Do you see a way to fulfill those two constraints while making
> >>> "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?
> >>>
> >> Now(setting feature before x86_cpu_expand_features), enabling
> >> TOPOEXT appears to work fine.
> > What about the above constraints?  Are you really fulfilling
> > them?
> >
> > This one is tricky:
> >
> > ] This works today with QEMU 2.12 + Linux <= 4.15:
> > ]   $ $QEMU -machine pc -cpu EPYC,enforce -smp
> 8,sockets=2,cores=2,threads=2"
> > ] and must keep working with QEMU 3.0 and Linux <= 4.15.
>   This works fine on kernel <= 4.15 with some warnings(-smp
> 8,sockets=2,cores=2,threads=2 -cpu EPYC).
> 
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.80000001H:EDX.rdtscp [bit 27]
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.80000001H:ECX.topoext [bit 22]
> qemu-system-x86_64: This family of AMD CPU doesn't support
> hyperthreading(2). Please configure -smp options properly or try
> enabling topoext feature.
> continues..
> >
> > If we enable TOPOEXT unconditionally, the command-line won't work
> > with Linux <= 4.15.
> >
> > If we enable TOPOEXT only if the kernel returns TOPOEXT on
> > GET_SUPPORTED_CPUID, we break the second constraint:
> >
> > ] The results for:
> > ]   $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...]
> > ] must be deterministic and expose exactly the same CPUID data even
> > ] if host hardware or software changes, as long as the QEMU
> > ] command-line is the same.
> >
> This fails on kernel  <= 4.15 with following messages((-smp
> 8,sockets=2,cores=2,threads=2 -cpu EPYC,enforce).
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.80000001H:EDX.rdtscp [bit 27]
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.80000001H:ECX.topoext [bit 22]
> qemu-system-x86_64: Host doesn't support requested features
> exits..
> 
> What do you think our options are here?

Should we drop automatic topoext completely and move forward? What are your thoughts?


^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-13 16:52                   ` [Qemu-devel] " Moger, Babu
@ 2018-06-13 17:18                     ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-13 17:18 UTC (permalink / raw)
  To: Moger, Babu
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth

On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
[...]
> > What do you think our options are here?
> 
> Should we drop automatic topoext completely and move forward?
> What are your thoughts?

Let's drop automatic topoext by now, and see if we find solutions
later.  I don't want to hold the rest of the patches because of
this.

I'm thinking we could simply make kvm_arch_get_supported_cpuid()
always return TOPOEXT on AMD CPUs, because the feature flag don't
really depend on any KVM code to work (is that correct?).

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-13 17:18                     ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-13 17:18 UTC (permalink / raw)
  To: Moger, Babu
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark

On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
[...]
> > What do you think our options are here?
> 
> Should we drop automatic topoext completely and move forward?
> What are your thoughts?

Let's drop automatic topoext by now, and see if we find solutions
later.  I don't want to hold the rest of the patches because of
this.

I'm thinking we could simply make kvm_arch_get_supported_cpuid()
always return TOPOEXT on AMD CPUs, because the feature flag don't
really depend on any KVM code to work (is that correct?).

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-13 17:18                     ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-13 18:10                       ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-13 18:10 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth


> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> [...]
> > > What do you think our options are here?
> >
> > Should we drop automatic topoext completely and move forward?
> > What are your thoughts?
> 
> Let's drop automatic topoext by now, and see if we find solutions
> later.  I don't want to hold the rest of the patches because of
> this.

Ok. I will drop topoext.

> 
> I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> always return TOPOEXT on AMD CPUs, because the feature flag don't
> really depend on any KVM code to work (is that correct?).

Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM driver.

Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.

> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-13 18:10                       ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-13 18:10 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark


> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> [...]
> > > What do you think our options are here?
> >
> > Should we drop automatic topoext completely and move forward?
> > What are your thoughts?
> 
> Let's drop automatic topoext by now, and see if we find solutions
> later.  I don't want to hold the rest of the patches because of
> this.

Ok. I will drop topoext.

> 
> I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> always return TOPOEXT on AMD CPUs, because the feature flag don't
> really depend on any KVM code to work (is that correct?).

Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM driver.

Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.

> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-13 18:10                       ` [Qemu-devel] " Moger, Babu
@ 2018-06-13 18:12                         ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-13 18:12 UTC (permalink / raw)
  To: Moger, Babu, Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth



> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Moger, Babu
> Sent: Wednesday, June 13, 2018 1:11 PM
> To: Eduardo Habkost <ehabkost@redhat.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: RE: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > Denemark <jdenemar@redhat.com>
> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > CPU
> >
> > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > [...]
> > > > What do you think our options are here?
> > >
> > > Should we drop automatic topoext completely and move forward?
> > > What are your thoughts?
> >
> > Let's drop automatic topoext by now, and see if we find solutions
> > later.  I don't want to hold the rest of the patches because of
> > this.
> 
> Ok. I will drop topoext.

Sorry, I mean automatic topoext.

> 
> >
> > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > really depend on any KVM code to work (is that correct?).
> 
> Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM
> driver.
> 
> Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.
> 
> >
> > --
> > Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-13 18:12                         ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-13 18:12 UTC (permalink / raw)
  To: Moger, Babu, Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark



> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Moger, Babu
> Sent: Wednesday, June 13, 2018 1:11 PM
> To: Eduardo Habkost <ehabkost@redhat.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: RE: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > Denemark <jdenemar@redhat.com>
> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > CPU
> >
> > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > [...]
> > > > What do you think our options are here?
> > >
> > > Should we drop automatic topoext completely and move forward?
> > > What are your thoughts?
> >
> > Let's drop automatic topoext by now, and see if we find solutions
> > later.  I don't want to hold the rest of the patches because of
> > this.
> 
> Ok. I will drop topoext.

Sorry, I mean automatic topoext.

> 
> >
> > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > really depend on any KVM code to work (is that correct?).
> 
> Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM
> driver.
> 
> Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.
> 
> >
> > --
> > Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-13 18:10                       ` [Qemu-devel] " Moger, Babu
@ 2018-06-13 18:17                         ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-13 18:17 UTC (permalink / raw)
  To: Moger, Babu
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth

On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote:
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > Denemark <jdenemar@redhat.com>
> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > CPU
> > 
> > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > [...]
> > > > What do you think our options are here?
> > >
> > > Should we drop automatic topoext completely and move forward?
> > > What are your thoughts?
> > 
> > Let's drop automatic topoext by now, and see if we find solutions
> > later.  I don't want to hold the rest of the patches because of
> > this.
> 
> Ok. I will drop topoext.
> 
> > 
> > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > really depend on any KVM code to work (is that correct?).
> 
> Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM driver.
> 
> Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.

Hmm, this could actually solve all of our problems, then:

We can forget about auto-topoext: just add TOPOEXT in
kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to
the CPU models where you are interested into (EPYC only?), and
add topoext=off to pc-2.12 compat_props.

Sorry for not noticing that before.  I was incorrectly assuming
that TOPOEXT was safe to enable only if it was returned by
GET_SUPPORTED_CPUID.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-13 18:17                         ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-13 18:17 UTC (permalink / raw)
  To: Moger, Babu
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark

On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote:
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > Denemark <jdenemar@redhat.com>
> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > CPU
> > 
> > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > [...]
> > > > What do you think our options are here?
> > >
> > > Should we drop automatic topoext completely and move forward?
> > > What are your thoughts?
> > 
> > Let's drop automatic topoext by now, and see if we find solutions
> > later.  I don't want to hold the rest of the patches because of
> > this.
> 
> Ok. I will drop topoext.
> 
> > 
> > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > really depend on any KVM code to work (is that correct?).
> 
> Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM driver.
> 
> Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.

Hmm, this could actually solve all of our problems, then:

We can forget about auto-topoext: just add TOPOEXT in
kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to
the CPU models where you are interested into (EPYC only?), and
add topoext=off to pc-2.12 compat_props.

Sorry for not noticing that before.  I was incorrectly assuming
that TOPOEXT was safe to enable only if it was returned by
GET_SUPPORTED_CPUID.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-13 18:17                         ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-13 18:21                           ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-13 18:21 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth



> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 13, 2018 1:18 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote:
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > > Denemark <jdenemar@redhat.com>
> > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > > CPU
> > >
> > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > > [...]
> > > > > What do you think our options are here?
> > > >
> > > > Should we drop automatic topoext completely and move forward?
> > > > What are your thoughts?
> > >
> > > Let's drop automatic topoext by now, and see if we find solutions
> > > later.  I don't want to hold the rest of the patches because of
> > > this.
> >
> > Ok. I will drop topoext.
> >
> > >
> > > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > > really depend on any KVM code to work (is that correct?).
> >
> > Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM
> driver.
> >
> > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.
> 
> Hmm, this could actually solve all of our problems, then:
> 
> We can forget about auto-topoext: just add TOPOEXT in
> kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to
> the CPU models where you are interested into (EPYC only?), and
> add topoext=off to pc-2.12 compat_props.
> 

Ok Sure.

> Sorry for not noticing that before.  I was incorrectly assuming

No problem.

> that TOPOEXT was safe to enable only if it was returned by
> GET_SUPPORTED_CPUID.
> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-13 18:21                           ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-13 18:21 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark



> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 13, 2018 1:18 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote:
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > > Denemark <jdenemar@redhat.com>
> > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > > CPU
> > >
> > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > > [...]
> > > > > What do you think our options are here?
> > > >
> > > > Should we drop automatic topoext completely and move forward?
> > > > What are your thoughts?
> > >
> > > Let's drop automatic topoext by now, and see if we find solutions
> > > later.  I don't want to hold the rest of the patches because of
> > > this.
> >
> > Ok. I will drop topoext.
> >
> > >
> > > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > > really depend on any KVM code to work (is that correct?).
> >
> > Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM
> driver.
> >
> > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.
> 
> Hmm, this could actually solve all of our problems, then:
> 
> We can forget about auto-topoext: just add TOPOEXT in
> kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to
> the CPU models where you are interested into (EPYC only?), and
> add topoext=off to pc-2.12 compat_props.
> 

Ok Sure.

> Sorry for not noticing that before.  I was incorrectly assuming

No problem.

> that TOPOEXT was safe to enable only if it was returned by
> GET_SUPPORTED_CPUID.
> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-13 18:21                           ` [Qemu-devel] " Moger, Babu
@ 2018-06-13 18:49                             ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-13 18:49 UTC (permalink / raw)
  To: Moger, Babu
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth

On Wed, Jun 13, 2018 at 06:21:58PM +0000, Moger, Babu wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Wednesday, June 13, 2018 1:18 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; qemu-devel@nongnu.org;
> > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > Denemark <jdenemar@redhat.com>
> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > CPU
> > 
> > On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote:
> > > > -----Original Message-----
> > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > > Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > > > Denemark <jdenemar@redhat.com>
> > > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > > > CPU
> > > >
> > > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > > > [...]
> > > > > > What do you think our options are here?
> > > > >
> > > > > Should we drop automatic topoext completely and move forward?
> > > > > What are your thoughts?
> > > >
> > > > Let's drop automatic topoext by now, and see if we find solutions
> > > > later.  I don't want to hold the rest of the patches because of
> > > > this.
> > >
> > > Ok. I will drop topoext.
> > >
> > > >
> > > > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > > > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > > > really depend on any KVM code to work (is that correct?).
> > >
> > > Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM
> > driver.
> > >
> > > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.
> > 
> > Hmm, this could actually solve all of our problems, then:
> > 
> > We can forget about auto-topoext: just add TOPOEXT in
> > kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to
> > the CPU models where you are interested into (EPYC only?), and
> > add topoext=off to pc-2.12 compat_props.
> > 
> 
> Ok Sure.

Sorry, I forgot we still need to decide what to do if TOPOEXT is
enabled in the CPU model (or command-line) but the -smp options
are not compatible with it.

In other words, what should guest see on CPUID if using:

"-machine pc-q35-3.0 -cpu EPYC -smp 64,cores=64"
or:
"-machine pc-q35-3.0 -cpu Opteron_G5,+topoext -smp 64,cores=64"

I wonder what would happen if we just return zeroes on
CPUID[0x800001E] if !topology_supports_topoext(), instead of
trying to clear/set TOPOEXT depending on the -smp option?  It
would make things much simpler for QEMU and libvirt.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-13 18:49                             ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2018-06-13 18:49 UTC (permalink / raw)
  To: Moger, Babu
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark

On Wed, Jun 13, 2018 at 06:21:58PM +0000, Moger, Babu wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Wednesday, June 13, 2018 1:18 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; qemu-devel@nongnu.org;
> > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > Denemark <jdenemar@redhat.com>
> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > CPU
> > 
> > On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote:
> > > > -----Original Message-----
> > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > > Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > > > Denemark <jdenemar@redhat.com>
> > > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > > > CPU
> > > >
> > > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > > > [...]
> > > > > > What do you think our options are here?
> > > > >
> > > > > Should we drop automatic topoext completely and move forward?
> > > > > What are your thoughts?
> > > >
> > > > Let's drop automatic topoext by now, and see if we find solutions
> > > > later.  I don't want to hold the rest of the patches because of
> > > > this.
> > >
> > > Ok. I will drop topoext.
> > >
> > > >
> > > > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > > > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > > > really depend on any KVM code to work (is that correct?).
> > >
> > > Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM
> > driver.
> > >
> > > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.
> > 
> > Hmm, this could actually solve all of our problems, then:
> > 
> > We can forget about auto-topoext: just add TOPOEXT in
> > kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to
> > the CPU models where you are interested into (EPYC only?), and
> > add topoext=off to pc-2.12 compat_props.
> > 
> 
> Ok Sure.

Sorry, I forgot we still need to decide what to do if TOPOEXT is
enabled in the CPU model (or command-line) but the -smp options
are not compatible with it.

In other words, what should guest see on CPUID if using:

"-machine pc-q35-3.0 -cpu EPYC -smp 64,cores=64"
or:
"-machine pc-q35-3.0 -cpu Opteron_G5,+topoext -smp 64,cores=64"

I wonder what would happen if we just return zeroes on
CPUID[0x800001E] if !topology_supports_topoext(), instead of
trying to clear/set TOPOEXT depending on the -smp option?  It
would make things much simpler for QEMU and libvirt.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
  2018-06-13 18:49                             ` [Qemu-devel] " Eduardo Habkost
@ 2018-06-14  0:58                               ` Moger, Babu
  -1 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-14  0:58 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel, pbonzini,
	Jiri Denemark, rth



> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 13, 2018 1:49 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Wed, Jun 13, 2018 at 06:21:58PM +0000, Moger, Babu wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > Sent: Wednesday, June 13, 2018 1:18 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; qemu-devel@nongnu.org;
> > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > > Denemark <jdenemar@redhat.com>
> > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > > CPU
> > >
> > > On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote:
> > > > > -----Original Message-----
> > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > > > Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> > > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com;
> Jiri
> > > > > Denemark <jdenemar@redhat.com>
> > > > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD
> EPYC
> > > > > CPU
> > > > >
> > > > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > > > > [...]
> > > > > > > What do you think our options are here?
> > > > > >
> > > > > > Should we drop automatic topoext completely and move forward?
> > > > > > What are your thoughts?
> > > > >
> > > > > Let's drop automatic topoext by now, and see if we find solutions
> > > > > later.  I don't want to hold the rest of the patches because of
> > > > > this.
> > > >
> > > > Ok. I will drop topoext.
> > > >
> > > > >
> > > > > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > > > > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > > > > really depend on any KVM code to work (is that correct?).
> > > >
> > > > Yes, that is correct. I don't see any dependent code on TOPOEXT in
> KVM
> > > driver.
> > > >
> > > > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.
> > >
> > > Hmm, this could actually solve all of our problems, then:
> > >
> > > We can forget about auto-topoext: just add TOPOEXT in
> > > kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to
> > > the CPU models where you are interested into (EPYC only?), and
> > > add topoext=off to pc-2.12 compat_props.
> > >
> >
> > Ok Sure.
> 
> Sorry, I forgot we still need to decide what to do if TOPOEXT is
> enabled in the CPU model (or command-line) but the -smp options
> are not compatible with it.

Yes.  I have kept that check.  But, I had to implement topology_supports_topoext bit differently.
Reason for this is we need to disable this feature before the  x86_cpu_expand_features.
But problem is nr_cores and nr_threads are not populated at this time. It is populated in qemu_init_vcpus.
Please take a look at topology_supports_topoext again.
> 
> In other words, what should guest see on CPUID if using:
> 
> "-machine pc-q35-3.0 -cpu EPYC -smp 64,cores=64"
> or:
> "-machine pc-q35-3.0 -cpu Opteron_G5,+topoext -smp 64,cores=64"
> 
Tested both these cases. It works fine with some warning messages.

> I wonder what would happen if we just return zeroes on
> CPUID[0x800001E] if !topology_supports_topoext(), instead of
> trying to clear/set TOPOEXT depending on the -smp option?  It
> would make things much simpler for QEMU and libvirt.

I did not see that difference in behavior if we clear the bit versus return 0s.
Sending new patches now. Please review.
One note: I will going on vacation from June 20th for couple of weeks.
 If possible I would like to close this feature. If we cannot that is fine. Just an FYI.

> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [Qemu-devel] [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU
@ 2018-06-14  0:58                               ` Moger, Babu
  0 siblings, 0 replies; 72+ messages in thread
From: Moger, Babu @ 2018-06-14  0:58 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Jiri Denemark



> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 13, 2018 1:49 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> Denemark <jdenemar@redhat.com>
> Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Wed, Jun 13, 2018 at 06:21:58PM +0000, Moger, Babu wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > Sent: Wednesday, June 13, 2018 1:18 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; qemu-devel@nongnu.org;
> > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Jiri
> > > Denemark <jdenemar@redhat.com>
> > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC
> > > CPU
> > >
> > > On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote:
> > > > > -----Original Message-----
> > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > > > Sent: Wednesday, June 13, 2018 12:18 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; qemu-devel@nongnu.org;
> > > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com;
> Jiri
> > > > > Denemark <jdenemar@redhat.com>
> > > > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD
> EPYC
> > > > > CPU
> > > > >
> > > > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote:
> > > > > [...]
> > > > > > > What do you think our options are here?
> > > > > >
> > > > > > Should we drop automatic topoext completely and move forward?
> > > > > > What are your thoughts?
> > > > >
> > > > > Let's drop automatic topoext by now, and see if we find solutions
> > > > > later.  I don't want to hold the rest of the patches because of
> > > > > this.
> > > >
> > > > Ok. I will drop topoext.
> > > >
> > > > >
> > > > > I'm thinking we could simply make kvm_arch_get_supported_cpuid()
> > > > > always return TOPOEXT on AMD CPUs, because the feature flag don't
> > > > > really depend on any KVM code to work (is that correct?).
> > > >
> > > > Yes, that is correct. I don't see any dependent code on TOPOEXT in
> KVM
> > > driver.
> > > >
> > > > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes.
> > >
> > > Hmm, this could actually solve all of our problems, then:
> > >
> > > We can forget about auto-topoext: just add TOPOEXT in
> > > kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to
> > > the CPU models where you are interested into (EPYC only?), and
> > > add topoext=off to pc-2.12 compat_props.
> > >
> >
> > Ok Sure.
> 
> Sorry, I forgot we still need to decide what to do if TOPOEXT is
> enabled in the CPU model (or command-line) but the -smp options
> are not compatible with it.

Yes.  I have kept that check.  But, I had to implement topology_supports_topoext bit differently.
Reason for this is we need to disable this feature before the  x86_cpu_expand_features.
But problem is nr_cores and nr_threads are not populated at this time. It is populated in qemu_init_vcpus.
Please take a look at topology_supports_topoext again.
> 
> In other words, what should guest see on CPUID if using:
> 
> "-machine pc-q35-3.0 -cpu EPYC -smp 64,cores=64"
> or:
> "-machine pc-q35-3.0 -cpu Opteron_G5,+topoext -smp 64,cores=64"
> 
Tested both these cases. It works fine with some warning messages.

> I wonder what would happen if we just return zeroes on
> CPUID[0x800001E] if !topology_supports_topoext(), instead of
> trying to clear/set TOPOEXT depending on the -smp option?  It
> would make things much simpler for QEMU and libvirt.

I did not see that difference in behavior if we clear the bit versus return 0s.
Sending new patches now. Please review.
One note: I will going on vacation from June 20th for couple of weeks.
 If possible I would like to close this feature. If we cannot that is fine. Just an FYI.

> 
> --
> Eduardo

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
  2018-06-08 22:56   ` [Qemu-devel] " Babu Moger
@ 2020-06-02 17:52     ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2020-06-02 17:52 UTC (permalink / raw)
  To: Babu Moger
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Dr. David Alan Gilbert

On Fri, Jun 08, 2018 at 06:56:17PM -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>
[...]
> +    case 0x8000001E:
> +        assert(cpu->core_id <= 255);

It is possible to trigger this assert using:

$ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device intel-iommu,intremap=on,eim=on -smp 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu qemu64,xlevel=0x8000001e -device qemu64-x86_64-cpu,apic-id=257
qemu-system-x86_64: warning: Number of hotpluggable cpus requested (258) exceeds the recommended cpus supported by KVM (240)
qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid: Assertion `cpu->core_id <= 255' failed.
Aborted (core dumped)

See bug report and discussion at
https://bugzilla.redhat.com/show_bug.cgi?id=1834200

Also, it looks like encode_topo_cpuid8000001e() assumes core_id
has only 3 bits, so the existing assert() is not even sufficient.
We need to decide what to do if the user requests nr_cores > 8.

Probably omitting CPUID[0x8000001E] if the VCPU topology is
incompatible with encode_topo_cpuid8000001e() (and printing a
warning) is the safest thing to do right now.



> +        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] 72+ messages in thread

* Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
@ 2020-06-02 17:52     ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2020-06-02 17:52 UTC (permalink / raw)
  To: Babu Moger
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel,
	Dr. David Alan Gilbert, pbonzini, rth

On Fri, Jun 08, 2018 at 06:56:17PM -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>
[...]
> +    case 0x8000001E:
> +        assert(cpu->core_id <= 255);

It is possible to trigger this assert using:

$ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device intel-iommu,intremap=on,eim=on -smp 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu qemu64,xlevel=0x8000001e -device qemu64-x86_64-cpu,apic-id=257
qemu-system-x86_64: warning: Number of hotpluggable cpus requested (258) exceeds the recommended cpus supported by KVM (240)
qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid: Assertion `cpu->core_id <= 255' failed.
Aborted (core dumped)

See bug report and discussion at
https://bugzilla.redhat.com/show_bug.cgi?id=1834200

Also, it looks like encode_topo_cpuid8000001e() assumes core_id
has only 3 bits, so the existing assert() is not even sufficient.
We need to decide what to do if the user requests nr_cores > 8.

Probably omitting CPUID[0x8000001E] if the VCPU topology is
incompatible with encode_topo_cpuid8000001e() (and printing a
warning) is the safest thing to do right now.



> +        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] 72+ messages in thread

* RE: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
  2020-06-02 17:52     ` Eduardo Habkost
@ 2020-06-03 23:57       ` Babu Moger
  -1 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2020-06-03 23:57 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Dr. David Alan Gilbert

Started looking at this. Let me know if you have any ideas. Will respond
with more details later this week.

> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Tuesday, June 2, 2020 12:52 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Dr. David
> Alan Gilbert <dgilbert@redhat.com>
> Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
> 
> On Fri, Jun 08, 2018 at 06:56:17PM -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>
> [...]
> > +    case 0x8000001E:
> > +        assert(cpu->core_id <= 255);
> 
> It is possible to trigger this assert using:
> 
> $ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device
> intel-iommu,intremap=on,eim=on -smp
> 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu
> qemu64,xlevel=0x8000001e -device qemu64-x86_64-cpu,apic-id=257
> qemu-system-x86_64: warning: Number of hotpluggable cpus requested (258)
> exceeds the recommended cpus supported by KVM (240)
> qemu-system-x86_64:
> /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid:
> Assertion `cpu->core_id <= 255' failed.
> Aborted (core dumped)
> 
> See bug report and discussion at
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> redhat.com%2Fshow_bug.cgi%3Fid%3D1834200&amp;data=02%7C01%7Cbabu.
> moger%40amd.com%7C8a2724729b914bc9b53d08d8071db392%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637267171438806408&amp;sdata=ib
> iGlF%2FF%2FVtYQLf7fe988kxFsLhj4GrRiTOq4LUuOT8%3D&amp;reserved=0
> 
> Also, it looks like encode_topo_cpuid8000001e() assumes core_id
> has only 3 bits, so the existing assert() is not even sufficient.
> We need to decide what to do if the user requests nr_cores > 8.
> 
> Probably omitting CPUID[0x8000001E] if the VCPU topology is
> incompatible with encode_topo_cpuid8000001e() (and printing a
> warning) is the safest thing to do right now.
> 
> 
> 
> > +        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] 72+ messages in thread

* RE: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
@ 2020-06-03 23:57       ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2020-06-03 23:57 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel,
	Dr. David Alan Gilbert, pbonzini, rth

Started looking at this. Let me know if you have any ideas. Will respond
with more details later this week.

> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Tuesday, June 2, 2020 12:52 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Dr. David
> Alan Gilbert <dgilbert@redhat.com>
> Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
> 
> On Fri, Jun 08, 2018 at 06:56:17PM -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>
> [...]
> > +    case 0x8000001E:
> > +        assert(cpu->core_id <= 255);
> 
> It is possible to trigger this assert using:
> 
> $ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device
> intel-iommu,intremap=on,eim=on -smp
> 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu
> qemu64,xlevel=0x8000001e -device qemu64-x86_64-cpu,apic-id=257
> qemu-system-x86_64: warning: Number of hotpluggable cpus requested (258)
> exceeds the recommended cpus supported by KVM (240)
> qemu-system-x86_64:
> /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid:
> Assertion `cpu->core_id <= 255' failed.
> Aborted (core dumped)
> 
> See bug report and discussion at
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> redhat.com%2Fshow_bug.cgi%3Fid%3D1834200&amp;data=02%7C01%7Cbabu.
> moger%40amd.com%7C8a2724729b914bc9b53d08d8071db392%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637267171438806408&amp;sdata=ib
> iGlF%2FF%2FVtYQLf7fe988kxFsLhj4GrRiTOq4LUuOT8%3D&amp;reserved=0
> 
> Also, it looks like encode_topo_cpuid8000001e() assumes core_id
> has only 3 bits, so the existing assert() is not even sufficient.
> We need to decide what to do if the user requests nr_cores > 8.
> 
> Probably omitting CPUID[0x8000001E] if the VCPU topology is
> incompatible with encode_topo_cpuid8000001e() (and printing a
> warning) is the safest thing to do right now.
> 
> 
> 
> > +        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] 72+ messages in thread

* RE: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
  2020-06-02 17:52     ` Eduardo Habkost
@ 2020-06-04 14:06       ` Babu Moger
  -1 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2020-06-04 14:06 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Dr. David Alan Gilbert



> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Tuesday, June 2, 2020 12:52 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Dr. David
> Alan Gilbert <dgilbert@redhat.com>
> Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
> 
> On Fri, Jun 08, 2018 at 06:56:17PM -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>
> [...]
> > +    case 0x8000001E:
> > +        assert(cpu->core_id <= 255);
> 
> It is possible to trigger this assert using:
> 
> $ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device
> intel-iommu,intremap=on,eim=on -smp
> 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu
> qemu64,xlevel=0x8000001e -device qemu64-x86_64-cpu,apic-id=257
> qemu-system-x86_64: warning: Number of hotpluggable cpus requested (258)
> exceeds the recommended cpus supported by KVM (240)
> qemu-system-x86_64:
> /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid:
> Assertion `cpu->core_id <= 255' failed.
> Aborted (core dumped)
> 
> See bug report and discussion at
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> redhat.com%2Fshow_bug.cgi%3Fid%3D1834200&amp;data=02%7C01%7Cbabu.
> moger%40amd.com%7C8a2724729b914bc9b53d08d8071db392%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637267171438806408&amp;sdata=ib
> iGlF%2FF%2FVtYQLf7fe988kxFsLhj4GrRiTOq4LUuOT8%3D&amp;reserved=0
> 
> Also, it looks like encode_topo_cpuid8000001e() assumes core_id
> has only 3 bits, so the existing assert() is not even sufficient.
> We need to decide what to do if the user requests nr_cores > 8.
> 
> Probably omitting CPUID[0x8000001E] if the VCPU topology is
> incompatible with encode_topo_cpuid8000001e() (and printing a
> warning) is the safest thing to do right now.

Eduardo,  We need to generalize the encode_topo_cpuid8000001e decoding.
We will have to remove 3 bit limitation there. It will not scale with
latest configurations. I will take a look that.

For now, best option I think is to(like you mentioned in bug 1834200),
declaring nr_cores > 256 as never supported (or deprecated); and throw
warning.

What do you think?
> 
> 
> 
> > +        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] 72+ messages in thread

* RE: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
@ 2020-06-04 14:06       ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2020-06-04 14:06 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel,
	Dr. David Alan Gilbert, pbonzini, rth



> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Tuesday, June 2, 2020 12:52 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Dr. David
> Alan Gilbert <dgilbert@redhat.com>
> Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
> 
> On Fri, Jun 08, 2018 at 06:56:17PM -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>
> [...]
> > +    case 0x8000001E:
> > +        assert(cpu->core_id <= 255);
> 
> It is possible to trigger this assert using:
> 
> $ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device
> intel-iommu,intremap=on,eim=on -smp
> 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu
> qemu64,xlevel=0x8000001e -device qemu64-x86_64-cpu,apic-id=257
> qemu-system-x86_64: warning: Number of hotpluggable cpus requested (258)
> exceeds the recommended cpus supported by KVM (240)
> qemu-system-x86_64:
> /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid:
> Assertion `cpu->core_id <= 255' failed.
> Aborted (core dumped)
> 
> See bug report and discussion at
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> redhat.com%2Fshow_bug.cgi%3Fid%3D1834200&amp;data=02%7C01%7Cbabu.
> moger%40amd.com%7C8a2724729b914bc9b53d08d8071db392%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637267171438806408&amp;sdata=ib
> iGlF%2FF%2FVtYQLf7fe988kxFsLhj4GrRiTOq4LUuOT8%3D&amp;reserved=0
> 
> Also, it looks like encode_topo_cpuid8000001e() assumes core_id
> has only 3 bits, so the existing assert() is not even sufficient.
> We need to decide what to do if the user requests nr_cores > 8.
> 
> Probably omitting CPUID[0x8000001E] if the VCPU topology is
> incompatible with encode_topo_cpuid8000001e() (and printing a
> warning) is the safest thing to do right now.

Eduardo,  We need to generalize the encode_topo_cpuid8000001e decoding.
We will have to remove 3 bit limitation there. It will not scale with
latest configurations. I will take a look that.

For now, best option I think is to(like you mentioned in bug 1834200),
declaring nr_cores > 256 as never supported (or deprecated); and throw
warning.

What do you think?
> 
> 
> 
> > +        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] 72+ messages in thread

* Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
  2020-06-04 14:06       ` Babu Moger
@ 2020-06-04 20:54         ` Eduardo Habkost
  -1 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2020-06-04 20:54 UTC (permalink / raw)
  To: Babu Moger
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Dr. David Alan Gilbert

On Thu, Jun 04, 2020 at 09:06:27AM -0500, Babu Moger wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost <ehabkost@redhat.com>
> > Sent: Tuesday, June 2, 2020 12:52 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; qemu-devel@nongnu.org;
> > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Dr. David
> > Alan Gilbert <dgilbert@redhat.com>
> > Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
> > 
> > On Fri, Jun 08, 2018 at 06:56:17PM -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>
> > [...]
> > > +    case 0x8000001E:
> > > +        assert(cpu->core_id <= 255);
> > 
> > It is possible to trigger this assert using:
> > 
> > $ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device
> > intel-iommu,intremap=on,eim=on -smp
> > 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu
> > qemu64,xlevel=0x8000001e -device qemu64-x86_64-cpu,apic-id=257
> > qemu-system-x86_64: warning: Number of hotpluggable cpus requested (258)
> > exceeds the recommended cpus supported by KVM (240)
> > qemu-system-x86_64:
> > /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid:
> > Assertion `cpu->core_id <= 255' failed.
> > Aborted (core dumped)
> > 
> > See bug report and discussion at
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> > redhat.com%2Fshow_bug.cgi%3Fid%3D1834200&amp;data=02%7C01%7Cbabu.
> > moger%40amd.com%7C8a2724729b914bc9b53d08d8071db392%7C3dd8961fe4
> > 884e608e11a82d994e183d%7C0%7C0%7C637267171438806408&amp;sdata=ib
> > iGlF%2FF%2FVtYQLf7fe988kxFsLhj4GrRiTOq4LUuOT8%3D&amp;reserved=0
> > 
> > Also, it looks like encode_topo_cpuid8000001e() assumes core_id
> > has only 3 bits, so the existing assert() is not even sufficient.
> > We need to decide what to do if the user requests nr_cores > 8.
> > 
> > Probably omitting CPUID[0x8000001E] if the VCPU topology is
> > incompatible with encode_topo_cpuid8000001e() (and printing a
> > warning) is the safest thing to do right now.
> 
> Eduardo,  We need to generalize the encode_topo_cpuid8000001e decoding.
> We will have to remove 3 bit limitation there. It will not scale with
> latest configurations. I will take a look that.
> 
> For now, best option I think is to(like you mentioned in bug 1834200),
> declaring nr_cores > 256 as never supported (or deprecated); and throw
> warning.
> 
> What do you think?

I believe we can declare nr_cores > 256 as never supported to
address the assert failure.  Other CPUID functions also look
broken when nr_cores is too large: encode_cache_cpuid4() seems to
assume nr_cores is 128 or less.

But we still need to make nr_cores > 8 safe while
encode_topo_cpuid8000001e() is not generalized yet.

> > 
> > 
> > 
> > > +        encode_topo_cpuid8000001e(cs, cpu,
> > > +                                  eax, ebx, ecx, edx);
> > > +        break;
> > >      case 0xC0000000:
> > >          *eax = env->cpuid_xlevel2;
> > >          *ebx = 0;
> > > --
> > > 1.8.3.1
> > >
> > 
> > --
> > Eduardo
> 

-- 
Eduardo


^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
@ 2020-06-04 20:54         ` Eduardo Habkost
  0 siblings, 0 replies; 72+ messages in thread
From: Eduardo Habkost @ 2020-06-04 20:54 UTC (permalink / raw)
  To: Babu Moger
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel,
	Dr. David Alan Gilbert, pbonzini, rth

On Thu, Jun 04, 2020 at 09:06:27AM -0500, Babu Moger wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost <ehabkost@redhat.com>
> > Sent: Tuesday, June 2, 2020 12:52 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; qemu-devel@nongnu.org;
> > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Dr. David
> > Alan Gilbert <dgilbert@redhat.com>
> > Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
> > 
> > On Fri, Jun 08, 2018 at 06:56:17PM -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>
> > [...]
> > > +    case 0x8000001E:
> > > +        assert(cpu->core_id <= 255);
> > 
> > It is possible to trigger this assert using:
> > 
> > $ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device
> > intel-iommu,intremap=on,eim=on -smp
> > 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu
> > qemu64,xlevel=0x8000001e -device qemu64-x86_64-cpu,apic-id=257
> > qemu-system-x86_64: warning: Number of hotpluggable cpus requested (258)
> > exceeds the recommended cpus supported by KVM (240)
> > qemu-system-x86_64:
> > /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid:
> > Assertion `cpu->core_id <= 255' failed.
> > Aborted (core dumped)
> > 
> > See bug report and discussion at
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> > redhat.com%2Fshow_bug.cgi%3Fid%3D1834200&amp;data=02%7C01%7Cbabu.
> > moger%40amd.com%7C8a2724729b914bc9b53d08d8071db392%7C3dd8961fe4
> > 884e608e11a82d994e183d%7C0%7C0%7C637267171438806408&amp;sdata=ib
> > iGlF%2FF%2FVtYQLf7fe988kxFsLhj4GrRiTOq4LUuOT8%3D&amp;reserved=0
> > 
> > Also, it looks like encode_topo_cpuid8000001e() assumes core_id
> > has only 3 bits, so the existing assert() is not even sufficient.
> > We need to decide what to do if the user requests nr_cores > 8.
> > 
> > Probably omitting CPUID[0x8000001E] if the VCPU topology is
> > incompatible with encode_topo_cpuid8000001e() (and printing a
> > warning) is the safest thing to do right now.
> 
> Eduardo,  We need to generalize the encode_topo_cpuid8000001e decoding.
> We will have to remove 3 bit limitation there. It will not scale with
> latest configurations. I will take a look that.
> 
> For now, best option I think is to(like you mentioned in bug 1834200),
> declaring nr_cores > 256 as never supported (or deprecated); and throw
> warning.
> 
> What do you think?

I believe we can declare nr_cores > 256 as never supported to
address the assert failure.  Other CPUID functions also look
broken when nr_cores is too large: encode_cache_cpuid4() seems to
assume nr_cores is 128 or less.

But we still need to make nr_cores > 8 safe while
encode_topo_cpuid8000001e() is not generalized yet.

> > 
> > 
> > 
> > > +        encode_topo_cpuid8000001e(cs, cpu,
> > > +                                  eax, ebx, ecx, edx);
> > > +        break;
> > >      case 0xC0000000:
> > >          *eax = env->cpuid_xlevel2;
> > >          *ebx = 0;
> > > --
> > > 1.8.3.1
> > >
> > 
> > --
> > Eduardo
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 72+ messages in thread

* RE: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
  2020-06-04 20:54         ` Eduardo Habkost
@ 2020-06-05 16:21           ` Babu Moger
  -1 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2020-06-05 16:21 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: mst, marcel.apfelbaum, pbonzini, rth, mtosatti, qemu-devel, kvm,
	kash, geoff, Dr. David Alan Gilbert



> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Thursday, June 4, 2020 3:54 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Dr. David
> Alan Gilbert <dgilbert@redhat.com>
> Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
> 
> On Thu, Jun 04, 2020 at 09:06:27AM -0500, Babu Moger wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > Sent: Tuesday, June 2, 2020 12:52 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; qemu-devel@nongnu.org;
> > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Dr.
> David
> > > Alan Gilbert <dgilbert@redhat.com>
> > > Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for
> AMD
> > >
> > > On Fri, Jun 08, 2018 at 06:56:17PM -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>
> > > [...]
> > > > +    case 0x8000001E:
> > > > +        assert(cpu->core_id <= 255);
> > >
> > > It is possible to trigger this assert using:
> > >
> > > $ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device
> > > intel-iommu,intremap=on,eim=on -smp
> > > 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu
> > > qemu64,xlevel=0x8000001e -device qemu64-x86_64-cpu,apic-id=257
> > > qemu-system-x86_64: warning: Number of hotpluggable cpus requested
> (258)
> > > exceeds the recommended cpus supported by KVM (240)
> > > qemu-system-x86_64:
> > > /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid:
> > > Assertion `cpu->core_id <= 255' failed.
> > > Aborted (core dumped)
> > >
> > > See bug report and discussion at
> > >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> > >
> redhat.com%2Fshow_bug.cgi%3Fid%3D1834200&amp;data=02%7C01%7Cbabu.
> > >
> moger%40amd.com%7C8a2724729b914bc9b53d08d8071db392%7C3dd8961fe4
> > >
> 884e608e11a82d994e183d%7C0%7C0%7C637267171438806408&amp;sdata=ib
> > > iGlF%2FF%2FVtYQLf7fe988kxFsLhj4GrRiTOq4LUuOT8%3D&amp;reserved=0
> > >
> > > Also, it looks like encode_topo_cpuid8000001e() assumes core_id
> > > has only 3 bits, so the existing assert() is not even sufficient.
> > > We need to decide what to do if the user requests nr_cores > 8.
> > >
> > > Probably omitting CPUID[0x8000001E] if the VCPU topology is
> > > incompatible with encode_topo_cpuid8000001e() (and printing a
> > > warning) is the safest thing to do right now.
> >
> > Eduardo,  We need to generalize the encode_topo_cpuid8000001e decoding.
> > We will have to remove 3 bit limitation there. It will not scale with
> > latest configurations. I will take a look that.
> >
> > For now, best option I think is to(like you mentioned in bug 1834200),
> > declaring nr_cores > 256 as never supported (or deprecated); and throw
> > warning.
> >
> > What do you think?
> 
> I believe we can declare nr_cores > 256 as never supported to
> address the assert failure.  Other CPUID functions also look
> broken when nr_cores is too large: encode_cache_cpuid4() seems to
> assume nr_cores is 128 or less.

Let me know where to add this check. Or if you want to take care of that
that is fine as well.

> But we still need to make nr_cores > 8 safe while
> encode_topo_cpuid8000001e() is not generalized yet.

I am working on a patch to address this now. Will send it soon with other
patches(addressing uninitialized node_id).

> 
> > >
> > >
> > >
> > > > +        encode_topo_cpuid8000001e(cs, cpu,
> > > > +                                  eax, ebx, ecx, edx);
> > > > +        break;
> > > >      case 0xC0000000:
> > > >          *eax = env->cpuid_xlevel2;
> > > >          *ebx = 0;
> > > > --
> > > > 1.8.3.1
> > > >
> > >
> > > --
> > > Eduardo
> >
> 
> --
> Eduardo


^ permalink raw reply	[flat|nested] 72+ messages in thread

* RE: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
@ 2020-06-05 16:21           ` Babu Moger
  0 siblings, 0 replies; 72+ messages in thread
From: Babu Moger @ 2020-06-05 16:21 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: geoff, kvm, mst, kash, mtosatti, qemu-devel,
	Dr. David Alan Gilbert, pbonzini, rth



> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Thursday, June 4, 2020 3:54 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; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Dr. David
> Alan Gilbert <dgilbert@redhat.com>
> Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD
> 
> On Thu, Jun 04, 2020 at 09:06:27AM -0500, Babu Moger wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > Sent: Tuesday, June 2, 2020 12:52 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; qemu-devel@nongnu.org;
> > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Dr.
> David
> > > Alan Gilbert <dgilbert@redhat.com>
> > > Subject: Re: [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for
> AMD
> > >
> > > On Fri, Jun 08, 2018 at 06:56:17PM -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>
> > > [...]
> > > > +    case 0x8000001E:
> > > > +        assert(cpu->core_id <= 255);
> > >
> > > It is possible to trigger this assert using:
> > >
> > > $ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split -device
> > > intel-iommu,intremap=on,eim=on -smp
> > > 1,maxcpus=258,cores=258,threads=1,sockets=1 -cpu
> > > qemu64,xlevel=0x8000001e -device qemu64-x86_64-cpu,apic-id=257
> > > qemu-system-x86_64: warning: Number of hotpluggable cpus requested
> (258)
> > > exceeds the recommended cpus supported by KVM (240)
> > > qemu-system-x86_64:
> > > /home/ehabkost/rh/proj/virt/qemu/target/i386/cpu.c:5888: cpu_x86_cpuid:
> > > Assertion `cpu->core_id <= 255' failed.
> > > Aborted (core dumped)
> > >
> > > See bug report and discussion at
> > >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> > >
> redhat.com%2Fshow_bug.cgi%3Fid%3D1834200&amp;data=02%7C01%7Cbabu.
> > >
> moger%40amd.com%7C8a2724729b914bc9b53d08d8071db392%7C3dd8961fe4
> > >
> 884e608e11a82d994e183d%7C0%7C0%7C637267171438806408&amp;sdata=ib
> > > iGlF%2FF%2FVtYQLf7fe988kxFsLhj4GrRiTOq4LUuOT8%3D&amp;reserved=0
> > >
> > > Also, it looks like encode_topo_cpuid8000001e() assumes core_id
> > > has only 3 bits, so the existing assert() is not even sufficient.
> > > We need to decide what to do if the user requests nr_cores > 8.
> > >
> > > Probably omitting CPUID[0x8000001E] if the VCPU topology is
> > > incompatible with encode_topo_cpuid8000001e() (and printing a
> > > warning) is the safest thing to do right now.
> >
> > Eduardo,  We need to generalize the encode_topo_cpuid8000001e decoding.
> > We will have to remove 3 bit limitation there. It will not scale with
> > latest configurations. I will take a look that.
> >
> > For now, best option I think is to(like you mentioned in bug 1834200),
> > declaring nr_cores > 256 as never supported (or deprecated); and throw
> > warning.
> >
> > What do you think?
> 
> I believe we can declare nr_cores > 256 as never supported to
> address the assert failure.  Other CPUID functions also look
> broken when nr_cores is too large: encode_cache_cpuid4() seems to
> assume nr_cores is 128 or less.

Let me know where to add this check. Or if you want to take care of that
that is fine as well.

> But we still need to make nr_cores > 8 safe while
> encode_topo_cpuid8000001e() is not generalized yet.

I am working on a patch to address this now. Will send it soon with other
patches(addressing uninitialized node_id).

> 
> > >
> > >
> > >
> > > > +        encode_topo_cpuid8000001e(cs, cpu,
> > > > +                                  eax, ebx, ecx, edx);
> > > > +        break;
> > > >      case 0xC0000000:
> > > >          *eax = env->cpuid_xlevel2;
> > > >          *ebx = 0;
> > > > --
> > > > 1.8.3.1
> > > >
> > >
> > > --
> > > Eduardo
> >
> 
> --
> Eduardo



^ permalink raw reply	[flat|nested] 72+ messages in thread

end of thread, other threads:[~2020-06-05 16:37 UTC | newest]

Thread overview: 72+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-08 22:56 [PATCH v13 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU Babu Moger
2018-06-08 22:56 ` [Qemu-devel] " Babu Moger
2018-06-08 22:56 ` [PATCH v13 1/5] i386: Add support for CPUID_8000_001E for AMD Babu Moger
2018-06-08 22:56   ` [Qemu-devel] " Babu Moger
2018-06-11 20:54   ` Eduardo Habkost
2018-06-11 20:54     ` [Qemu-devel] " Eduardo Habkost
2018-06-11 21:02     ` Moger, Babu
2018-06-11 21:02       ` [Qemu-devel] " Moger, Babu
2020-06-02 17:52   ` Eduardo Habkost
2020-06-02 17:52     ` Eduardo Habkost
2020-06-03 23:57     ` Babu Moger
2020-06-03 23:57       ` Babu Moger
2020-06-04 14:06     ` Babu Moger
2020-06-04 14:06       ` Babu Moger
2020-06-04 20:54       ` Eduardo Habkost
2020-06-04 20:54         ` Eduardo Habkost
2020-06-05 16:21         ` Babu Moger
2020-06-05 16:21           ` Babu Moger
2018-06-08 22:56 ` [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage topoext Babu Moger
2018-06-08 22:56   ` [Qemu-devel] " Babu Moger
2018-06-11 20:25   ` Moger, Babu
2018-06-11 20:25     ` [Qemu-devel] " Moger, Babu
2018-06-11 20:46     ` Eduardo Habkost
2018-06-11 20:46       ` [Qemu-devel] " Eduardo Habkost
2018-06-11 20:59       ` Moger, Babu
2018-06-11 20:59         ` [Qemu-devel] " Moger, Babu
2018-06-11 21:05       ` Eduardo Habkost
2018-06-11 21:05         ` [Qemu-devel] " Eduardo Habkost
2018-06-11 21:20         ` Moger, Babu
2018-06-11 21:20           ` [Qemu-devel] " Moger, Babu
2018-06-08 22:56 ` [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC CPU Babu Moger
2018-06-08 22:56   ` [Qemu-devel] " Babu Moger
2018-06-11 20:50   ` Eduardo Habkost
2018-06-11 20:50     ` [Qemu-devel] " Eduardo Habkost
2018-06-11 21:01     ` Moger, Babu
2018-06-11 21:01       ` [Qemu-devel] " Moger, Babu
2018-06-11 21:10     ` Eduardo Habkost
2018-06-11 21:10       ` [Qemu-devel] " Eduardo Habkost
2018-06-12 16:29       ` Moger, Babu
2018-06-12 16:29         ` [Qemu-devel] " Moger, Babu
2018-06-12 17:40         ` Eduardo Habkost
2018-06-12 17:40           ` [Qemu-devel] " Eduardo Habkost
2018-06-12 18:38           ` Moger, Babu
2018-06-12 18:38             ` [Qemu-devel] " Moger, Babu
2018-06-12 19:05             ` Eduardo Habkost
2018-06-12 19:05               ` [Qemu-devel] " Eduardo Habkost
2018-06-12 19:46               ` Babu Moger
2018-06-12 19:46                 ` [Qemu-devel] " Babu Moger
2018-06-13 16:52                 ` Moger, Babu
2018-06-13 16:52                   ` [Qemu-devel] " Moger, Babu
2018-06-13 17:18                   ` Eduardo Habkost
2018-06-13 17:18                     ` [Qemu-devel] " Eduardo Habkost
2018-06-13 18:10                     ` Moger, Babu
2018-06-13 18:10                       ` [Qemu-devel] " Moger, Babu
2018-06-13 18:12                       ` Moger, Babu
2018-06-13 18:12                         ` [Qemu-devel] " Moger, Babu
2018-06-13 18:17                       ` Eduardo Habkost
2018-06-13 18:17                         ` [Qemu-devel] " Eduardo Habkost
2018-06-13 18:21                         ` Moger, Babu
2018-06-13 18:21                           ` [Qemu-devel] " Moger, Babu
2018-06-13 18:49                           ` Eduardo Habkost
2018-06-13 18:49                             ` [Qemu-devel] " Eduardo Habkost
2018-06-14  0:58                             ` Moger, Babu
2018-06-14  0:58                               ` [Qemu-devel] " Moger, Babu
2018-06-08 22:56 ` [PATCH v13 4/5] i386: Verify and enable topoext feature if supported Babu Moger
2018-06-08 22:56   ` [Qemu-devel] " Babu Moger
2018-06-11 20:51   ` Eduardo Habkost
2018-06-11 20:51     ` [Qemu-devel] " Eduardo Habkost
2018-06-11 21:01     ` Moger, Babu
2018-06-11 21:01       ` [Qemu-devel] " Moger, Babu
2018-06-08 22:56 ` [PATCH v13 5/5] i386: Remove generic SMT thread check Babu Moger
2018-06-08 22:56   ` [Qemu-devel] " Babu Moger

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.