[Qemu-devel] [PATCH v12 0/4] i386: Enable TOPOEXT to support hyperthreading on AMD CPU

[Qemu-devel] [PATCH v12 0/4] i386: Enable TOPOEXT to support hyperthreading on AMD CPU

[Babu Moger]

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 

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 (4):
  i386: Add support for CPUID_8000_001E for AMD
  i386: Verify if topoext feature can be supported
  i386: Enable TOPOEXT feature on AMD EPYC CPU
  i386: Remove generic SMT thread check

 include/hw/i386/pc.h |   4 ++
 target/i386/cpu.c    | 141 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 137 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v12 1/4] i386: Add support for CPUID_8000_001E for AMD

[Babu Moger]

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

[Qemu-devel] [PATCH v12 2/4] i386: Verify if topoext feature can be supported

[Babu Moger]

topoext feature cannot be supported in certain cases
with large number of cores or threads. Add the check.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 86fb1a4..fc5c66d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -509,6 +509,20 @@ static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
 }
 
 /*
+ * Check if we can support this topology
+ * Fail if number of cores are beyond the supported config
+ * or nr_threads is more than 2
+ */
+static int topology_supports_topoext(int nr_cores, int nr_threads)
+{
+    if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) ||
+        (nr_threads > 2)) {
+        return 0;
+    }
+    return 1;
+}
+
+/*
  * Definitions of the hardcoded cache entries we expose:
  * These are legacy cache values. If there is a need to change any
  * of these values please use builtin_x86_defs
@@ -4941,6 +4955,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     qemu_init_vcpu(cs);
 
+    /* On AMD systems, check if we can support topoext feature */
+    if (IS_AMD_CPU(env) &&
+        (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
+        if (!topology_supports_topoext(cs->nr_cores, cs->nr_threads)) {
+                /* Cannot support topoext */
+                error_setg(errp, "CPU model does not support topoext feature "
+                                 "with number of cores(%d) and threads(%d). "
+                                 "Please configure -smp options properly.",
+                                 cs->nr_cores, cs->nr_threads);
+                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

[Qemu-devel] [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC CPU

[Babu Moger]

Enable TOPOEXT feature on EPYC CPU. This is required to support
hyperthreading on VM guests. Also extend xlevel to 0x8000001E.

Disable TOPOEXT feature for legacy machines.

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

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 04d1f8c..2630a91 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   = "EPYC-" TYPE_X86_CPU,\
+        .property = "topoext",\
+        .value    = "off",\
     },
 
 #define PC_COMPAT_2_11 \
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index fc5c66d..40bf5cb 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2568,7 +2568,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .features[FEAT_8000_0001_ECX] =
             CPUID_EXT3_OSVW | CPUID_EXT3_3DNOWPREFETCH |
             CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
-            CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
+            CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM |
+            CPUID_EXT3_TOPOEXT,
         .features[FEAT_7_0_EBX] =
             CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_AVX2 |
             CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_RDSEED |
@@ -2613,7 +2614,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .features[FEAT_8000_0001_ECX] =
             CPUID_EXT3_OSVW | CPUID_EXT3_3DNOWPREFETCH |
             CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
-            CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
+            CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM |
+            CPUID_EXT3_TOPOEXT,
         .features[FEAT_8000_0008_EBX] =
             CPUID_8000_0008_EBX_IBPB,
         .features[FEAT_7_0_EBX] =
@@ -4681,6 +4683,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

[Qemu-devel] [PATCH v12 4/4] i386: Remove generic SMT thread check

[Babu Moger]

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 40bf5cb..8e22b37 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4975,17 +4975,22 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
-    /* 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

Re: [Qemu-devel] [PATCH v12 1/4] i386: Add support for CPUID_8000_001E for AMD

[Eduardo Habkost]

On Wed, Jun 06, 2018 at 10:36:43AM -0400, Babu Moger wrote:
[...]
> +    /*
> +     * 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
> +     */

Where are those bit offsets documented?  AMD Family 17h PPR just
says "7:0 Core ID".

-- 
Eduardo

Re: [Qemu-devel] [PATCH v12 2/4] i386: Verify if topoext feature can be supported

[Eduardo Habkost]

On Wed, Jun 06, 2018 at 10:36:44AM -0400, Babu Moger wrote:
> topoext feature cannot be supported in certain cases
> with large number of cores or threads. Add the check.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 86fb1a4..fc5c66d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -509,6 +509,20 @@ static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
>  }
>  
>  /*
> + * Check if we can support this topology
> + * Fail if number of cores are beyond the supported config
> + * or nr_threads is more than 2
> + */
> +static int topology_supports_topoext(int nr_cores, int nr_threads)
> +{
> +    if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) ||
> +        (nr_threads > 2)) {
> +        return 0;
> +    }
> +    return 1;
> +}
> +
> +/*
>   * Definitions of the hardcoded cache entries we expose:
>   * These are legacy cache values. If there is a need to change any
>   * of these values please use builtin_x86_defs
> @@ -4941,6 +4955,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      qemu_init_vcpu(cs);
>  
> +    /* On AMD systems, check if we can support topoext feature */
> +    if (IS_AMD_CPU(env) &&
> +        (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
> +        if (!topology_supports_topoext(cs->nr_cores, cs->nr_threads)) {
> +                /* Cannot support topoext */
> +                error_setg(errp, "CPU model does not support topoext feature "
> +                                 "with number of cores(%d) and threads(%d). "
> +                                 "Please configure -smp options properly.",
> +                                 cs->nr_cores, cs->nr_threads);

See error.h documentation:

 * Error reporting system loosely patterned after Glib's GError.
 *
 * Create an error:
 *     error_setg(&err, "situation normal, all fouled up");
 *
 * Create an error and add additional explanation:
 *     error_setg(&err, "invalid quark");
 *     error_append_hint(&err, "Valid quarks are up, down, strange, "
 *                       "charm, top, bottom.\n");
 *
 * Do *not* contract this to
 *     error_setg(&err, "invalid quark\n"
 *                "Valid quarks are up, down, strange, charm, top, bottom.");
 *

I suggest something like this:

static bool 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",
                          (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");
                          (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
        return false;
    }
    return true;
}

static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
{
    /* ... */
    if (IS_AMD_CPU(env) &&
        (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
        !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
            return;
    }
    /* ... */
}

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, Jun 06, 2018 at 10:36:45AM -0400, Babu Moger wrote:
> Enable TOPOEXT feature on EPYC CPU. This is required to support
> hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> 
> Disable TOPOEXT feature for legacy machines.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Now, I just noticed we have a problem here:

"-machine pc -cpu EPYC -smp 64" works today

This patch makes it stop working, but it shouldn't.

On the other hand, I believe you expect:
* "-machine pc -cpu EPYC -smp 8" to automatically enable topoext.
* "-machine pc -cpu Opteron_G1 -smp 8" to not enable topoext.
* What about "-machine -cpu Opteron_G1 -smp 8,threads=2"?


We also have other requirements, I will try to enumerate all of
them below:

0) "-topoext" explicitly configured (any machine-type):
* Must never enable topoext.

1) "+topoext" explicitly configured (any machine-type):
* Must validate topology and refuse to start if unsupported.

2) Older machine-types:
* Must never enable topoext automatically, even if using "EPYC"
  or "threads=2"

3) "EPYC" CPU model (on new machine-types):
* Should enable topoext automatically, but only if topology is
  supported.
* Must not error out if topology is not supported.
* Should this enable topoext automatically even if threads=1?

4) Other AMD CPU models with "threads=2" (on new machine-types):
* We might want to make this enable topoext automatically, too.
  What do you think?

Is the above description accurate?  Do you agree with these
requirements?

We're trying to use the "topoext" property to cover all cases
above, but it looks like we need at least 2 bits to represent all
possible cases.


Maybe we can represent the cases above with two properties:
"topoext" and "auto-topoext".  Then each case would be
represented by:

0) "-topoext" explicitly configured (any machine-type):
* Will clear TOPOEXT on env->features and set TOPOEXT on
  env->user_features
  (already done today)

1) "+topoext" explicitly configured (any machine-type):
* Will set TOPOEXT on both env->user_features and env->features
  (already done today)

2) Older machine-types:
* Will set auto-topoext=off (can be done on compat_props)
* Will set topoext=off on EPYC CPU model (so TOPOEXT won't be set
  by default on env->features) (can be done on compat_props)

3) "EPYC" CPU model (on new machine-types):
* Will set auto-topoext=on (can be the default for all CPU
  models)
* Will set TOPOEXT on env->features) (can be done on CPU model table)

4) Other AMD CPU models with "threads=2" (on new machine-types):
* Will set auto-topoext=on (can be the default on all CPU models)
* Will keep TOPOEXT disabled on env->features (done on the CPU
  model table)


Then the rules would be:

  if {auto_topoext && TOPOEXT not in env->user_features) {
      if (supported_topology) {
          if (threads > 1)
              set TOPOEXT in env->features
      } else
          unset TOPOEXT in env->features
  }

  if (TOPOEXT in env->features && !supported_topology)
      error;
  }

I think this would fulfill all the requirements above.  Please
help me confirm that.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v12 2/4] i386: Verify if topoext feature can be supported

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 6, 2018 5:06 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 v12 2/4] i386: Verify if topoext feature can be supported
> 
> On Wed, Jun 06, 2018 at 10:36:44AM -0400, Babu Moger wrote:
> > topoext feature cannot be supported in certain cases
> > with large number of cores or threads. Add the check.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  target/i386/cpu.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 86fb1a4..fc5c66d 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -509,6 +509,20 @@ static void encode_topo_cpuid8000001e(CPUState
> *cs, X86CPU *cpu,
> >  }
> >
> >  /*
> > + * Check if we can support this topology
> > + * Fail if number of cores are beyond the supported config
> > + * or nr_threads is more than 2
> > + */
> > +static int topology_supports_topoext(int nr_cores, int nr_threads)
> > +{
> > +    if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET))
> ||
> > +        (nr_threads > 2)) {
> > +        return 0;
> > +    }
> > +    return 1;
> > +}
> > +
> > +/*
> >   * Definitions of the hardcoded cache entries we expose:
> >   * These are legacy cache values. If there is a need to change any
> >   * of these values please use builtin_x86_defs
> > @@ -4941,6 +4955,19 @@ static void x86_cpu_realizefn(DeviceState *dev,
> Error **errp)
> >
> >      qemu_init_vcpu(cs);
> >
> > +    /* On AMD systems, check if we can support topoext feature */
> > +    if (IS_AMD_CPU(env) &&
> > +        (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
> > +        if (!topology_supports_topoext(cs->nr_cores, cs->nr_threads)) {
> > +                /* Cannot support topoext */
> > +                error_setg(errp, "CPU model does not support topoext feature "
> > +                                 "with number of cores(%d) and threads(%d). "
> > +                                 "Please configure -smp options properly.",
> > +                                 cs->nr_cores, cs->nr_threads);
> 
> See error.h documentation:
> 
>  * Error reporting system loosely patterned after Glib's GError.
>  *
>  * Create an error:
>  *     error_setg(&err, "situation normal, all fouled up");
>  *
>  * Create an error and add additional explanation:
>  *     error_setg(&err, "invalid quark");
>  *     error_append_hint(&err, "Valid quarks are up, down, strange, "
>  *                       "charm, top, bottom.\n");
>  *
>  * Do *not* contract this to
>  *     error_setg(&err, "invalid quark\n"
>  *                "Valid quarks are up, down, strange, charm, top, bottom.");
>  *
> 
> I suggest something like this:

Sure. I will make these changes. Thanks

> 
> static bool 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",
>                           (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");
>                           (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
>         return false;
>     }
>     return true;
> }
> 
> static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> {
>     /* ... */
>     if (IS_AMD_CPU(env) &&
>         (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
>         !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
>             return;
>     }
>     /* ... */
> }
> 
> --
> Eduardo

Re: [Qemu-devel] [PATCH v12 2/4] i386: Verify if topoext feature can be supported

[Eduardo Habkost]

On Thu, Jun 07, 2018 at 02:24:18PM +0000, Moger, Babu wrote:
[...]
> > > +    /* On AMD systems, check if we can support topoext feature */
> > > +    if (IS_AMD_CPU(env) &&
> > > +        (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
> > > +        if (!topology_supports_topoext(cs->nr_cores, cs->nr_threads)) {
> > > +                /* Cannot support topoext */
> > > +                error_setg(errp, "CPU model does not support topoext feature "
> > > +                                 "with number of cores(%d) and threads(%d). "
> > > +                                 "Please configure -smp options properly.",
> > > +                                 cs->nr_cores, cs->nr_threads);
> > 
> > See error.h documentation:
> > 
> >  * Error reporting system loosely patterned after Glib's GError.
> >  *
> >  * Create an error:
> >  *     error_setg(&err, "situation normal, all fouled up");
> >  *
> >  * Create an error and add additional explanation:
> >  *     error_setg(&err, "invalid quark");
> >  *     error_append_hint(&err, "Valid quarks are up, down, strange, "
> >  *                       "charm, top, bottom.\n");
> >  *
> >  * Do *not* contract this to
> >  *     error_setg(&err, "invalid quark\n"
> >  *                "Valid quarks are up, down, strange, charm, top, bottom.");
> >  *
> > 
> > I suggest something like this:
> 
> Sure. I will make these changes. Thanks

Thanks.  Note that I have made a mistake below, by not including
a newline in error_append_hint().

Also, I'm not sure if it's better to mention the current value of
nr_cores in error_setg() or just in the error hint.  Markus, do
you have any suggestion?

> 
> > 
> > static bool 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",
> >                           (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");
> >                           (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> >         return false;
> >     }
> >     return true;
> > }
> > 
> > static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> > {
> >     /* ... */
> >     if (IS_AMD_CPU(env) &&
> >         (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
> >         !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
> >             return;
> >     }
> >     /* ... */
> > }
> > 
> > --
> > Eduardo

-- 
Eduardo

Re: [Qemu-devel] [PATCH v12 1/4] i386: Add support for CPUID_8000_001E for AMD

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 6, 2018 4:26 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 v12 1/4] i386: Add support for CPUID_8000_001E for
> AMD
> 
> On Wed, Jun 06, 2018 at 10:36:43AM -0400, Babu Moger wrote:
> [...]
> > +    /*
> > +     * 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
> > +     */
> 
> Where are those bit offsets documented?  AMD Family 17h PPR just
> says "7:0 Core ID".

Yes. That is right. AMD Family 17h PPR does not list all the details for core_id.
We are working with our document writer's to make those details public.
Thanks for pointing that out.
> 
> --
> Eduardo

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

[Moger, Babu]

Hi Eduardo, 
Sorry for the late response. Got pulled into something else.

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 6, 2018 5: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
> Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Wed, Jun 06, 2018 at 10:36:45AM -0400, Babu Moger wrote:
> > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> >
> > Disable TOPOEXT feature for legacy machines.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> 
> Now, I just noticed we have a problem here:
> 
> "-machine pc -cpu EPYC -smp 64" works today
> 
> This patch makes it stop working, but it shouldn't.

No. It works fine. I have tested it.

> 
> On the other hand, I believe you expect:
> * "-machine pc -cpu EPYC -smp 8" to automatically enable topoext.
Yes. Only on new machines-types
> * "-machine pc -cpu Opteron_G1 -smp 8" to not enable topoext.
Yes.
> * What about "-machine -cpu Opteron_G1 -smp 8,threads=2"?
No. This should not enable topoext.  Topoext is not supported by Opteron_G1.
This should warn about hyperthreading and continue.
> 
> 
> We also have other requirements, I will try to enumerate all of
> them below:
> 
> 0) "-topoext" explicitly configured (any machine-type):
> * Must never enable topoext.
Yes.
> 
> 1) "+topoext" explicitly configured (any machine-type):
> * Must validate topology and refuse to start if unsupported.

Yes.

> 
> 2) Older machine-types:
> * Must never enable topoext automatically, even if using "EPYC"
>   or "threads=2"
> 
Yes.

> 3) "EPYC" CPU model (on new machine-types):
> * Should enable topoext automatically, but only if topology is
>   supported.
> * Must not error out if topology is not supported.
In new machine types we will enable topoext for "EPYC" CPU model.
Right now(old machine type) we can disable for all the CPU models. 
So, we don't need two bits(topoext and auto-topoext)

I thought we should error out if topology cannot be supported. But we can warn(disable topoext) and continue that is another option.

> * Should this enable topoext automatically even if threads=1?

Yes. We should enable even with threads=1.

> 
> 4) Other AMD CPU models with "threads=2" (on new machine-types):
> * We might want to make this enable topoext automatically, too.
>   What do you think?

  No. We should not enable topoext here. We should depend on CPU model table here.

> 
> Is the above description accurate?  Do you agree with these
> requirements?

With these requirements in mind, I will send that patches. We can start our discussion.
We don't need one more bits. That is my opinion. 


> 
> We're trying to use the "topoext" property to cover all cases
> above, but it looks like we need at least 2 bits to represent all
> possible cases.
> 
> 
> Maybe we can represent the cases above with two properties:
> "topoext" and "auto-topoext".  Then each case would be
> represented by:
> 
> 0) "-topoext" explicitly configured (any machine-type):
> * Will clear TOPOEXT on env->features and set TOPOEXT on
>   env->user_features
>   (already done today)
> 
> 1) "+topoext" explicitly configured (any machine-type):
> * Will set TOPOEXT on both env->user_features and env->features
>   (already done today)
> 
> 2) Older machine-types:
> * Will set auto-topoext=off (can be done on compat_props)
> * Will set topoext=off on EPYC CPU model (so TOPOEXT won't be set
>   by default on env->features) (can be done on compat_props)
> 
> 3) "EPYC" CPU model (on new machine-types):
> * Will set auto-topoext=on (can be the default for all CPU
>   models)
> * Will set TOPOEXT on env->features) (can be done on CPU model table)
> 
> 4) Other AMD CPU models with "threads=2" (on new machine-types):
> * Will set auto-topoext=on (can be the default on all CPU models)
> * Will keep TOPOEXT disabled on env->features (done on the CPU
>   model table)
> 
> 
> Then the rules would be:
> 
>   if {auto_topoext && TOPOEXT not in env->user_features) {
>       if (supported_topology) {
>           if (threads > 1)
>               set TOPOEXT in env->features
>       } else
>           unset TOPOEXT in env->features
>   }
> 
>   if (TOPOEXT in env->features && !supported_topology)
>       error;
>   }
> 
> I think this would fulfill all the requirements above.  Please
> help me confirm that.
> 
> --
> Eduardo

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

[Eduardo Habkost]

On Fri, Jun 08, 2018 at 06:40:16PM +0000, Moger, Babu wrote:
> Hi Eduardo, 
> Sorry for the late response. Got pulled into something else.
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Wednesday, June 6, 2018 5: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
> > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC
> > CPU
> > 
> > On Wed, Jun 06, 2018 at 10:36:45AM -0400, Babu Moger wrote:
> > > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> > >
> > > Disable TOPOEXT feature for legacy machines.
> > >
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > 
> > Now, I just noticed we have a problem here:
> > 
> > "-machine pc -cpu EPYC -smp 64" works today
> > 
> > This patch makes it stop working, but it shouldn't.
> 
> No. It works fine. I have tested it.

This doesn't sound right.  The code in this series will error out
of TOPOEXT is enabled and you have more than 64 VCPUs.

But I just noticed we have a bug introduced by:

commit f548222c24342ca74689de7794f9006b43f86a54
Author: Xiao Guangrong <xiaoguangrong@tencent.com>
Date:   Thu May 3 16:06:11 2018 +0800

    migration: introduce decompress-error-check

    QEMU 3.0 enables strict check for compression & decompression to
    make the migration more robust, that depends on the source to fix
    the internal design which triggers the unexpected error conditions

    To make it work for migrating old version QEMU to 2.13 QEMU, we
    introduce this parameter to disable the error check on the
    destination which is the default behavior of the machine type
    which is older than 2.13, alternately, the strict check can be
    enabled explicitly as followings:
          -M pc-q35-2.11 -global migration.decompress-error-check=true

    Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
    Reviewed-by: Juan Quintela <quintela@redhat.com>
    Signed-off-by: Juan Quintela <quintela@redhat.com>

This commits added PC_COMPAT_2_12 to the 3.0 machine-types.
Because of this bug, TOPOEXT is being unconditionally disabled on
all machine-types, unless I apply the fix below:


diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3d81136065..b4c5b03274 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -430,7 +430,6 @@ static void pc_i440fx_3_0_machine_options(MachineClass *m)
     pc_i440fx_machine_options(m);
     m->alias = "pc";
     m->is_default = 1;
-    SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
 }
 
 DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index b60cbb9266..83d6d75efa 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -312,7 +312,6 @@ static void pc_q35_3_0_machine_options(MachineClass *m)
 {
     pc_q35_machine_options(m);
     m->alias = "q35";
-    SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
 }
 
 DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,

> 
> > 
> > On the other hand, I believe you expect:
> > * "-machine pc -cpu EPYC -smp 8" to automatically enable topoext.
> Yes. Only on new machines-types
> > * "-machine pc -cpu Opteron_G1 -smp 8" to not enable topoext.
> Yes.
> > * What about "-machine -cpu Opteron_G1 -smp 8,threads=2"?
> No. This should not enable topoext.  Topoext is not supported by Opteron_G1.
> This should warn about hyperthreading and continue.

OK, makes sense to me.

> > 
> > 
> > We also have other requirements, I will try to enumerate all of
> > them below:
> > 
> > 0) "-topoext" explicitly configured (any machine-type):
> > * Must never enable topoext.
> Yes.
> > 
> > 1) "+topoext" explicitly configured (any machine-type):
> > * Must validate topology and refuse to start if unsupported.
> 
> Yes.
> 
> > 
> > 2) Older machine-types:
> > * Must never enable topoext automatically, even if using "EPYC"
> >   or "threads=2"
> > 
> Yes.
> 
> > 3) "EPYC" CPU model (on new machine-types):
> > * Should enable topoext automatically, but only if topology is
> >   supported.
> > * Must not error out if topology is not supported.
> In new machine types we will enable topoext for "EPYC" CPU model.
> Right now(old machine type) we can disable for all the CPU models. 
> So, we don't need two bits(topoext and auto-topoext)

Right, so you agree that in this case we must _not_ error out if
topology is unsupported, correct?  Otherwise we will break this
existing use case:
  "-machine pc -cpu EPYC -smp 64".

> 
> I thought we should error out if topology cannot be supported. But we can warn(disable topoext) and continue that is another option.
> 
> > * Should this enable topoext automatically even if threads=1?
> 
> Yes. We should enable even with threads=1.
> 
> > 
> > 4) Other AMD CPU models with "threads=2" (on new machine-types):
> > * We might want to make this enable topoext automatically, too.
> >   What do you think?
> 
>   No. We should not enable topoext here. We should depend on CPU model table here.
> 
> > 
> > Is the above description accurate?  Do you agree with these
> > requirements?
> 
> With these requirements in mind, I will send that patches. We can start our discussion.
> We don't need one more bits. That is my opinion. 

Thanks for confirming the requirements above.

But it doesn't seem to be possible represent these requirements
with just one bit.  Otherwise you can't differentiate explicit
"+topoext" (1 above) from topoext being implicitly enabled by
"-cpu EPYC" (3 above).

Another problem is query-cpu-model-expansion QMP command: this
patch makes "topoext" appear on the output of
"query-cpu-model-expansion model=EPYC", meaning that management
software will assume everybody using the "EPYC" CPU model will
require +topoext.  A separate "auto-topoext" property would avoid
this issue.

(Yeah, this is tricky.  I want to eventually encode these subtle
rules in automated test cases, so these issues could be detected
by software instead of code inspection.)

-- 
Eduardo

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

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Friday, June 8, 2018 2:23 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; Juan
> Quintela <quintela@redhat.com>; xiaoguangrong@tencent.com
> Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Fri, Jun 08, 2018 at 06:40:16PM +0000, Moger, Babu wrote:
> > Hi Eduardo,
> > Sorry for the late response. Got pulled into something else.
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > Sent: Wednesday, June 6, 2018 5: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
> > > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC
> > > CPU
> > >
> > > On Wed, Jun 06, 2018 at 10:36:45AM -0400, Babu Moger wrote:
> > > > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > > > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> > > >
> > > > Disable TOPOEXT feature for legacy machines.
> > > >
> > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > >
> > > Now, I just noticed we have a problem here:
> > >
> > > "-machine pc -cpu EPYC -smp 64" works today
> > >
> > > This patch makes it stop working, but it shouldn't.
> >
> > No. It works fine. I have tested it.
> 
> This doesn't sound right.  The code in this series will error out
> of TOPOEXT is enabled and you have more than 64 VCPUs.
> 
> But I just noticed we have a bug introduced by:

Oh.. Ok..  Let me retry again with the new patch.

> 
> commit f548222c24342ca74689de7794f9006b43f86a54
> Author: Xiao Guangrong <xiaoguangrong@tencent.com>
> Date:   Thu May 3 16:06:11 2018 +0800
> 
>     migration: introduce decompress-error-check
> 
>     QEMU 3.0 enables strict check for compression & decompression to
>     make the migration more robust, that depends on the source to fix
>     the internal design which triggers the unexpected error conditions
> 
>     To make it work for migrating old version QEMU to 2.13 QEMU, we
>     introduce this parameter to disable the error check on the
>     destination which is the default behavior of the machine type
>     which is older than 2.13, alternately, the strict check can be
>     enabled explicitly as followings:
>           -M pc-q35-2.11 -global migration.decompress-error-check=true
> 
>     Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>     Reviewed-by: Juan Quintela <quintela@redhat.com>
>     Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> This commits added PC_COMPAT_2_12 to the 3.0 machine-types.
> Because of this bug, TOPOEXT is being unconditionally disabled on
> all machine-types, unless I apply the fix below:
> 
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 3d81136065..b4c5b03274 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -430,7 +430,6 @@ static void
> pc_i440fx_3_0_machine_options(MachineClass *m)
>      pc_i440fx_machine_options(m);
>      m->alias = "pc";
>      m->is_default = 1;
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
>  }
> 
>  DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index b60cbb9266..83d6d75efa 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -312,7 +312,6 @@ static void
> pc_q35_3_0_machine_options(MachineClass *m)
>  {
>      pc_q35_machine_options(m);
>      m->alias = "q35";
> -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
>  }
> 
>  DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
> 
> >
> > >
> > > On the other hand, I believe you expect:
> > > * "-machine pc -cpu EPYC -smp 8" to automatically enable topoext.
> > Yes. Only on new machines-types
> > > * "-machine pc -cpu Opteron_G1 -smp 8" to not enable topoext.
> > Yes.
> > > * What about "-machine -cpu Opteron_G1 -smp 8,threads=2"?
> > No. This should not enable topoext.  Topoext is not supported by
> Opteron_G1.
> > This should warn about hyperthreading and continue.
> 
> OK, makes sense to me.
> 
> > >
> > >
> > > We also have other requirements, I will try to enumerate all of
> > > them below:
> > >
> > > 0) "-topoext" explicitly configured (any machine-type):
> > > * Must never enable topoext.
> > Yes.
> > >
> > > 1) "+topoext" explicitly configured (any machine-type):
> > > * Must validate topology and refuse to start if unsupported.
> >
> > Yes.
> >
> > >
> > > 2) Older machine-types:
> > > * Must never enable topoext automatically, even if using "EPYC"
> > >   or "threads=2"
> > >
> > Yes.
> >
> > > 3) "EPYC" CPU model (on new machine-types):
> > > * Should enable topoext automatically, but only if topology is
> > >   supported.
> > > * Must not error out if topology is not supported.
> > In new machine types we will enable topoext for "EPYC" CPU model.
> > Right now(old machine type) we can disable for all the CPU models.
> > So, we don't need two bits(topoext and auto-topoext)
> 
> Right, so you agree that in this case we must _not_ error out if
> topology is unsupported, correct?  Otherwise we will break this
> existing use case:
>   "-machine pc -cpu EPYC -smp 64".

Ok. I will test this with new fix patch.  
> 
> >
> > I thought we should error out if topology cannot be supported. But we can
> warn(disable topoext) and continue that is another option.
> >
> > > * Should this enable topoext automatically even if threads=1?
> >
> > Yes. We should enable even with threads=1.
> >
> > >
> > > 4) Other AMD CPU models with "threads=2" (on new machine-types):
> > > * We might want to make this enable topoext automatically, too.
> > >   What do you think?
> >
> >   No. We should not enable topoext here. We should depend on CPU
> model table here.
> >
> > >
> > > Is the above description accurate?  Do you agree with these
> > > requirements?
> >
> > With these requirements in mind, I will send that patches. We can start our
> discussion.
> > We don't need one more bits. That is my opinion.
> 
> Thanks for confirming the requirements above.
> 
> But it doesn't seem to be possible represent these requirements
> with just one bit.  Otherwise you can't differentiate explicit
> "+topoext" (1 above) from topoext being implicitly enabled by
> "-cpu EPYC" (3 above).
> 
> Another problem is query-cpu-model-expansion QMP command: this
> patch makes "topoext" appear on the output of
> "query-cpu-model-expansion model=EPYC", meaning that management
> software will assume everybody using the "EPYC" CPU model will
> require +topoext.  A separate "auto-topoext" property would avoid
> this issue.

Sure.  Will work on it.  Yes, I know all these combinations make it very tricky. 
> 
> (Yeah, this is tricky.  I want to eventually encode these subtle
> rules in automated test cases, so these issues could be detected
> by software instead of code inspection.)
> 
> --
> Eduardo

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

[Eduardo Habkost]

On Fri, Jun 08, 2018 at 07:36:05PM +0000, Moger, Babu wrote:
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Friday, June 8, 2018 2:23 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; Juan
> > Quintela <quintela@redhat.com>; xiaoguangrong@tencent.com
> > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC
> > CPU
> > 
> > On Fri, Jun 08, 2018 at 06:40:16PM +0000, Moger, Babu wrote:
> > > Hi Eduardo,
> > > Sorry for the late response. Got pulled into something else.
> > >
> > > > -----Original Message-----
> > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > > Sent: Wednesday, June 6, 2018 5: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
> > > > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC
> > > > CPU
> > > >
> > > > On Wed, Jun 06, 2018 at 10:36:45AM -0400, Babu Moger wrote:
> > > > > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > > > > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> > > > >
> > > > > Disable TOPOEXT feature for legacy machines.
> > > > >
> > > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > >
> > > > Now, I just noticed we have a problem here:
> > > >
> > > > "-machine pc -cpu EPYC -smp 64" works today
> > > >
> > > > This patch makes it stop working, but it shouldn't.
> > >
> > > No. It works fine. I have tested it.
> > 
> > This doesn't sound right.  The code in this series will error out
> > of TOPOEXT is enabled and you have more than 64 VCPUs.
> > 
> > But I just noticed we have a bug introduced by:
> 
> Oh.. Ok..  Let me retry again with the new patch.
> 
> > 
> > commit f548222c24342ca74689de7794f9006b43f86a54
> > Author: Xiao Guangrong <xiaoguangrong@tencent.com>
> > Date:   Thu May 3 16:06:11 2018 +0800
> > 
> >     migration: introduce decompress-error-check
> > 
> >     QEMU 3.0 enables strict check for compression & decompression to
> >     make the migration more robust, that depends on the source to fix
> >     the internal design which triggers the unexpected error conditions
> > 
> >     To make it work for migrating old version QEMU to 2.13 QEMU, we
> >     introduce this parameter to disable the error check on the
> >     destination which is the default behavior of the machine type
> >     which is older than 2.13, alternately, the strict check can be
> >     enabled explicitly as followings:
> >           -M pc-q35-2.11 -global migration.decompress-error-check=true
> > 
> >     Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> >     Reviewed-by: Juan Quintela <quintela@redhat.com>
> >     Signed-off-by: Juan Quintela <quintela@redhat.com>
> > 
> > This commits added PC_COMPAT_2_12 to the 3.0 machine-types.
> > Because of this bug, TOPOEXT is being unconditionally disabled on
> > all machine-types, unless I apply the fix below:
> > 
> > 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 3d81136065..b4c5b03274 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -430,7 +430,6 @@ static void
> > pc_i440fx_3_0_machine_options(MachineClass *m)
> >      pc_i440fx_machine_options(m);
> >      m->alias = "pc";
> >      m->is_default = 1;
> > -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> >  }
> > 
> >  DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index b60cbb9266..83d6d75efa 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -312,7 +312,6 @@ static void
> > pc_q35_3_0_machine_options(MachineClass *m)
> >  {
> >      pc_q35_machine_options(m);
> >      m->alias = "q35";
> > -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> >  }
> > 
> >  DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
> > 
> > >
> > > >
> > > > On the other hand, I believe you expect:
> > > > * "-machine pc -cpu EPYC -smp 8" to automatically enable topoext.
> > > Yes. Only on new machines-types
> > > > * "-machine pc -cpu Opteron_G1 -smp 8" to not enable topoext.
> > > Yes.
> > > > * What about "-machine -cpu Opteron_G1 -smp 8,threads=2"?
> > > No. This should not enable topoext.  Topoext is not supported by
> > Opteron_G1.
> > > This should warn about hyperthreading and continue.
> > 
> > OK, makes sense to me.
> > 
> > > >
> > > >
> > > > We also have other requirements, I will try to enumerate all of
> > > > them below:
> > > >
> > > > 0) "-topoext" explicitly configured (any machine-type):
> > > > * Must never enable topoext.
> > > Yes.
> > > >
> > > > 1) "+topoext" explicitly configured (any machine-type):
> > > > * Must validate topology and refuse to start if unsupported.
> > >
> > > Yes.
> > >
> > > >
> > > > 2) Older machine-types:
> > > > * Must never enable topoext automatically, even if using "EPYC"
> > > >   or "threads=2"
> > > >
> > > Yes.
> > >
> > > > 3) "EPYC" CPU model (on new machine-types):
> > > > * Should enable topoext automatically, but only if topology is
> > > >   supported.
> > > > * Must not error out if topology is not supported.
> > > In new machine types we will enable topoext for "EPYC" CPU model.
> > > Right now(old machine type) we can disable for all the CPU models.
> > > So, we don't need two bits(topoext and auto-topoext)
> > 
> > Right, so you agree that in this case we must _not_ error out if
> > topology is unsupported, correct?  Otherwise we will break this
> > existing use case:
> >   "-machine pc -cpu EPYC -smp 64".
> 
> Ok. I will test this with new fix patch.  
> > 
> > >
> > > I thought we should error out if topology cannot be supported. But we can
> > warn(disable topoext) and continue that is another option.
> > >
> > > > * Should this enable topoext automatically even if threads=1?
> > >
> > > Yes. We should enable even with threads=1.
> > >
> > > >
> > > > 4) Other AMD CPU models with "threads=2" (on new machine-types):
> > > > * We might want to make this enable topoext automatically, too.
> > > >   What do you think?
> > >
> > >   No. We should not enable topoext here. We should depend on CPU
> > model table here.
> > >
> > > >
> > > > Is the above description accurate?  Do you agree with these
> > > > requirements?
> > >
> > > With these requirements in mind, I will send that patches. We can start our
> > discussion.
> > > We don't need one more bits. That is my opinion.
> > 
> > Thanks for confirming the requirements above.
> > 
> > But it doesn't seem to be possible represent these requirements
> > with just one bit.  Otherwise you can't differentiate explicit
> > "+topoext" (1 above) from topoext being implicitly enabled by
> > "-cpu EPYC" (3 above).
> > 
> > Another problem is query-cpu-model-expansion QMP command: this
> > patch makes "topoext" appear on the output of
> > "query-cpu-model-expansion model=EPYC", meaning that management
> > software will assume everybody using the "EPYC" CPU model will
> > require +topoext.  A separate "auto-topoext" property would avoid
> > this issue.
> 
> Sure.  Will work on it.  Yes, I know all these combinations make it very tricky. 

I will rewrite my proposal, to make sure we are on the same page:

0) "-topoext" explicitly configured (any machine-type):
* Will clear TOPOEXT on env->features and set TOPOEXT on
  env->user_features
  (already done today)

1) "+topoext" explicitly configured (any machine-type):
* Will set TOPOEXT on both env->user_features and env->features
  (already done today)

2) Older machine-types:
[CHANGED in v2]
* Will set auto-topoext=off (can be done on compat_props)
* No need to set topoext=off on compat_props (because no CPU model will have
  topoext=on, see #3 below)

3) "EPYC" CPU model (on new machine-types):
[CHANGED IN v2]
* Will set auto-topoext=on (using a new field on X86CPUDefinition)
* Will NOT set TOPOEXT on CPU model table (or it would break
  query-cpu-model-expansion)

4) Other AMD CPU models (on all machine-types):
[CHANGED IN v2]
* auto-topoext=off (the default)
* Will keep TOPOEXT disabled on env->features (the default)


x86_cpu_realizefn:

  if {cpu->cpudef && cpu->cpudef->auto_topoext &&
      TOPOEXT not in env->user_features &&
      supported_topology) {
      set TOPOEXT in env->features
  }

  if (TOPOEXT in env->features && !supported_topology)
      error;
  }

I think this logic will fulfill all the requirements above.

-- 
Eduardo

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

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Friday, June 8, 2018 2:50 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com;
> kash@tripleback.net; mtosatti@redhat.com; xiaoguangrong@tencent.com;
> qemu-devel@nongnu.org; Juan Quintela <quintela@redhat.com>;
> pbonzini@redhat.com; rth@twiddle.net
> Subject: Re: [Qemu-devel] [PATCH v12 3/4] i386: Enable TOPOEXT feature on
> AMD EPYC CPU
> 
> On Fri, Jun 08, 2018 at 07:36:05PM +0000, Moger, Babu wrote:
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > Sent: Friday, June 8, 2018 2:23 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;
> Juan
> > > Quintela <quintela@redhat.com>; xiaoguangrong@tencent.com
> > > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC
> > > CPU
> > >
> > > On Fri, Jun 08, 2018 at 06:40:16PM +0000, Moger, Babu wrote:
> > > > Hi Eduardo,
> > > > Sorry for the late response. Got pulled into something else.
> > > >
> > > > > -----Original Message-----
> > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > > > Sent: Wednesday, June 6, 2018 5: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
> > > > > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD
> EPYC
> > > > > CPU
> > > > >
> > > > > On Wed, Jun 06, 2018 at 10:36:45AM -0400, Babu Moger wrote:
> > > > > > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > > > > > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> > > > > >
> > > > > > Disable TOPOEXT feature for legacy machines.
> > > > > >
> > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > > >
> > > > > Now, I just noticed we have a problem here:
> > > > >
> > > > > "-machine pc -cpu EPYC -smp 64" works today
> > > > >
> > > > > This patch makes it stop working, but it shouldn't.
> > > >
> > > > No. It works fine. I have tested it.
> > >
> > > This doesn't sound right.  The code in this series will error out
> > > of TOPOEXT is enabled and you have more than 64 VCPUs.
> > >
> > > But I just noticed we have a bug introduced by:
> >
> > Oh.. Ok..  Let me retry again with the new patch.
> >
> > >
> > > commit f548222c24342ca74689de7794f9006b43f86a54
> > > Author: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > Date:   Thu May 3 16:06:11 2018 +0800
> > >
> > >     migration: introduce decompress-error-check
> > >
> > >     QEMU 3.0 enables strict check for compression & decompression to
> > >     make the migration more robust, that depends on the source to fix
> > >     the internal design which triggers the unexpected error conditions
> > >
> > >     To make it work for migrating old version QEMU to 2.13 QEMU, we
> > >     introduce this parameter to disable the error check on the
> > >     destination which is the default behavior of the machine type
> > >     which is older than 2.13, alternately, the strict check can be
> > >     enabled explicitly as followings:
> > >           -M pc-q35-2.11 -global migration.decompress-error-check=true
> > >
> > >     Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> > >     Reviewed-by: Juan Quintela <quintela@redhat.com>
> > >     Signed-off-by: Juan Quintela <quintela@redhat.com>
> > >
> > > This commits added PC_COMPAT_2_12 to the 3.0 machine-types.
> > > Because of this bug, TOPOEXT is being unconditionally disabled on
> > > all machine-types, unless I apply the fix below:
> > >
> > >
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index 3d81136065..b4c5b03274 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -430,7 +430,6 @@ static void
> > > pc_i440fx_3_0_machine_options(MachineClass *m)
> > >      pc_i440fx_machine_options(m);
> > >      m->alias = "pc";
> > >      m->is_default = 1;
> > > -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> > >  }
> > >
> > >  DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > index b60cbb9266..83d6d75efa 100644
> > > --- a/hw/i386/pc_q35.c
> > > +++ b/hw/i386/pc_q35.c
> > > @@ -312,7 +312,6 @@ static void
> > > pc_q35_3_0_machine_options(MachineClass *m)
> > >  {
> > >      pc_q35_machine_options(m);
> > >      m->alias = "q35";
> > > -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> > >  }
> > >
> > >  DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
> > >
> > > >
> > > > >
> > > > > On the other hand, I believe you expect:
> > > > > * "-machine pc -cpu EPYC -smp 8" to automatically enable topoext.
> > > > Yes. Only on new machines-types
> > > > > * "-machine pc -cpu Opteron_G1 -smp 8" to not enable topoext.
> > > > Yes.
> > > > > * What about "-machine -cpu Opteron_G1 -smp 8,threads=2"?
> > > > No. This should not enable topoext.  Topoext is not supported by
> > > Opteron_G1.
> > > > This should warn about hyperthreading and continue.
> > >
> > > OK, makes sense to me.
> > >
> > > > >
> > > > >
> > > > > We also have other requirements, I will try to enumerate all of
> > > > > them below:
> > > > >
> > > > > 0) "-topoext" explicitly configured (any machine-type):
> > > > > * Must never enable topoext.
> > > > Yes.
> > > > >
> > > > > 1) "+topoext" explicitly configured (any machine-type):
> > > > > * Must validate topology and refuse to start if unsupported.
> > > >
> > > > Yes.
> > > >
> > > > >
> > > > > 2) Older machine-types:
> > > > > * Must never enable topoext automatically, even if using "EPYC"
> > > > >   or "threads=2"
> > > > >
> > > > Yes.
> > > >
> > > > > 3) "EPYC" CPU model (on new machine-types):
> > > > > * Should enable topoext automatically, but only if topology is
> > > > >   supported.
> > > > > * Must not error out if topology is not supported.
> > > > In new machine types we will enable topoext for "EPYC" CPU model.
> > > > Right now(old machine type) we can disable for all the CPU models.
> > > > So, we don't need two bits(topoext and auto-topoext)
> > >
> > > Right, so you agree that in this case we must _not_ error out if
> > > topology is unsupported, correct?  Otherwise we will break this
> > > existing use case:
> > >   "-machine pc -cpu EPYC -smp 64".
> >
> > Ok. I will test this with new fix patch.
> > >
> > > >
> > > > I thought we should error out if topology cannot be supported. But we
> can
> > > warn(disable topoext) and continue that is another option.
> > > >
> > > > > * Should this enable topoext automatically even if threads=1?
> > > >
> > > > Yes. We should enable even with threads=1.
> > > >
> > > > >
> > > > > 4) Other AMD CPU models with "threads=2" (on new machine-types):
> > > > > * We might want to make this enable topoext automatically, too.
> > > > >   What do you think?
> > > >
> > > >   No. We should not enable topoext here. We should depend on CPU
> > > model table here.
> > > >
> > > > >
> > > > > Is the above description accurate?  Do you agree with these
> > > > > requirements?
> > > >
> > > > With these requirements in mind, I will send that patches. We can start
> our
> > > discussion.
> > > > We don't need one more bits. That is my opinion.
> > >
> > > Thanks for confirming the requirements above.
> > >
> > > But it doesn't seem to be possible represent these requirements
> > > with just one bit.  Otherwise you can't differentiate explicit
> > > "+topoext" (1 above) from topoext being implicitly enabled by
> > > "-cpu EPYC" (3 above).
> > >
> > > Another problem is query-cpu-model-expansion QMP command: this
> > > patch makes "topoext" appear on the output of
> > > "query-cpu-model-expansion model=EPYC", meaning that management
> > > software will assume everybody using the "EPYC" CPU model will
> > > require +topoext.  A separate "auto-topoext" property would avoid
> > > this issue.
> >
> > Sure.  Will work on it.  Yes, I know all these combinations make it very
> tricky.
> 
> I will rewrite my proposal, to make sure we are on the same page:

Looks good. Thanks
> 
> 0) "-topoext" explicitly configured (any machine-type):
> * Will clear TOPOEXT on env->features and set TOPOEXT on
>   env->user_features
>   (already done today)
> 
> 1) "+topoext" explicitly configured (any machine-type):
> * Will set TOPOEXT on both env->user_features and env->features
>   (already done today)
> 
> 2) Older machine-types:
> [CHANGED in v2]
> * Will set auto-topoext=off (can be done on compat_props)
> * No need to set topoext=off on compat_props (because no CPU model will
> have
>   topoext=on, see #3 below)
> 
> 3) "EPYC" CPU model (on new machine-types):
> [CHANGED IN v2]
> * Will set auto-topoext=on (using a new field on X86CPUDefinition)
> * Will NOT set TOPOEXT on CPU model table (or it would break
>   query-cpu-model-expansion)
> 
> 4) Other AMD CPU models (on all machine-types):
> [CHANGED IN v2]
> * auto-topoext=off (the default)
> * Will keep TOPOEXT disabled on env->features (the default)
> 
> 
> x86_cpu_realizefn:
> 
>   if {cpu->cpudef && cpu->cpudef->auto_topoext &&
>       TOPOEXT not in env->user_features &&
>       supported_topology) {
>       set TOPOEXT in env->features
>   }
> 
>   if (TOPOEXT in env->features && !supported_topology)
>       error;
>   }
> 
> I think this logic will fulfill all the requirements above.

Yes. I think so. Will test it and confirm. Thanks

> 
> --
> Eduardo

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

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Friday, June 8, 2018 2:50 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com;
> kash@tripleback.net; mtosatti@redhat.com; xiaoguangrong@tencent.com;
> qemu-devel@nongnu.org; Juan Quintela <quintela@redhat.com>;
> pbonzini@redhat.com; rth@twiddle.net
> Subject: Re: [Qemu-devel] [PATCH v12 3/4] i386: Enable TOPOEXT feature on
> AMD EPYC CPU
> 
> On Fri, Jun 08, 2018 at 07:36:05PM +0000, Moger, Babu wrote:
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > Sent: Friday, June 8, 2018 2:23 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;
> Juan
> > > Quintela <quintela@redhat.com>; xiaoguangrong@tencent.com
> > > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC
> > > CPU
> > >
> > > On Fri, Jun 08, 2018 at 06:40:16PM +0000, Moger, Babu wrote:
> > > > Hi Eduardo,
> > > > Sorry for the late response. Got pulled into something else.
> > > >
> > > > > -----Original Message-----
> > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > > > Sent: Wednesday, June 6, 2018 5: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
> > > > > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD
> EPYC
> > > > > CPU
> > > > >
> > > > > On Wed, Jun 06, 2018 at 10:36:45AM -0400, Babu Moger wrote:
> > > > > > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > > > > > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> > > > > >
> > > > > > Disable TOPOEXT feature for legacy machines.
> > > > > >
> > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > > >
> > > > > Now, I just noticed we have a problem here:
> > > > >
> > > > > "-machine pc -cpu EPYC -smp 64" works today
> > > > >
> > > > > This patch makes it stop working, but it shouldn't.
> > > >
> > > > No. It works fine. I have tested it.
> > >
> > > This doesn't sound right.  The code in this series will error out
> > > of TOPOEXT is enabled and you have more than 64 VCPUs.
> > >
> > > But I just noticed we have a bug introduced by:
> >
> > Oh.. Ok..  Let me retry again with the new patch.
> >
> > >
> > > commit f548222c24342ca74689de7794f9006b43f86a54
> > > Author: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > Date:   Thu May 3 16:06:11 2018 +0800
> > >
> > >     migration: introduce decompress-error-check
> > >
> > >     QEMU 3.0 enables strict check for compression & decompression to
> > >     make the migration more robust, that depends on the source to fix
> > >     the internal design which triggers the unexpected error conditions
> > >
> > >     To make it work for migrating old version QEMU to 2.13 QEMU, we
> > >     introduce this parameter to disable the error check on the
> > >     destination which is the default behavior of the machine type
> > >     which is older than 2.13, alternately, the strict check can be
> > >     enabled explicitly as followings:
> > >           -M pc-q35-2.11 -global migration.decompress-error-check=true
> > >
> > >     Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> > >     Reviewed-by: Juan Quintela <quintela@redhat.com>
> > >     Signed-off-by: Juan Quintela <quintela@redhat.com>
> > >
> > > This commits added PC_COMPAT_2_12 to the 3.0 machine-types.
> > > Because of this bug, TOPOEXT is being unconditionally disabled on
> > > all machine-types, unless I apply the fix below:
> > >
> > >
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index 3d81136065..b4c5b03274 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -430,7 +430,6 @@ static void
> > > pc_i440fx_3_0_machine_options(MachineClass *m)
> > >      pc_i440fx_machine_options(m);
> > >      m->alias = "pc";
> > >      m->is_default = 1;
> > > -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> > >  }
> > >
> > >  DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > index b60cbb9266..83d6d75efa 100644
> > > --- a/hw/i386/pc_q35.c
> > > +++ b/hw/i386/pc_q35.c
> > > @@ -312,7 +312,6 @@ static void
> > > pc_q35_3_0_machine_options(MachineClass *m)
> > >  {
> > >      pc_q35_machine_options(m);
> > >      m->alias = "q35";
> > > -    SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> > >  }
> > >
> > >  DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
> > >
> > > >
> > > > >
> > > > > On the other hand, I believe you expect:
> > > > > * "-machine pc -cpu EPYC -smp 8" to automatically enable topoext.
> > > > Yes. Only on new machines-types
> > > > > * "-machine pc -cpu Opteron_G1 -smp 8" to not enable topoext.
> > > > Yes.
> > > > > * What about "-machine -cpu Opteron_G1 -smp 8,threads=2"?
> > > > No. This should not enable topoext.  Topoext is not supported by
> > > Opteron_G1.
> > > > This should warn about hyperthreading and continue.
> > >
> > > OK, makes sense to me.
> > >
> > > > >
> > > > >
> > > > > We also have other requirements, I will try to enumerate all of
> > > > > them below:
> > > > >
> > > > > 0) "-topoext" explicitly configured (any machine-type):
> > > > > * Must never enable topoext.
> > > > Yes.
> > > > >
> > > > > 1) "+topoext" explicitly configured (any machine-type):
> > > > > * Must validate topology and refuse to start if unsupported.
> > > >
> > > > Yes.
> > > >
> > > > >
> > > > > 2) Older machine-types:
> > > > > * Must never enable topoext automatically, even if using "EPYC"
> > > > >   or "threads=2"
> > > > >
> > > > Yes.
> > > >
> > > > > 3) "EPYC" CPU model (on new machine-types):
> > > > > * Should enable topoext automatically, but only if topology is
> > > > >   supported.
> > > > > * Must not error out if topology is not supported.
> > > > In new machine types we will enable topoext for "EPYC" CPU model.
> > > > Right now(old machine type) we can disable for all the CPU models.
> > > > So, we don't need two bits(topoext and auto-topoext)
> > >
> > > Right, so you agree that in this case we must _not_ error out if
> > > topology is unsupported, correct?  Otherwise we will break this
> > > existing use case:
> > >   "-machine pc -cpu EPYC -smp 64".
> >
> > Ok. I will test this with new fix patch.
> > >
> > > >
> > > > I thought we should error out if topology cannot be supported. But we
> can
> > > warn(disable topoext) and continue that is another option.
> > > >
> > > > > * Should this enable topoext automatically even if threads=1?
> > > >
> > > > Yes. We should enable even with threads=1.
> > > >
> > > > >
> > > > > 4) Other AMD CPU models with "threads=2" (on new machine-types):
> > > > > * We might want to make this enable topoext automatically, too.
> > > > >   What do you think?
> > > >
> > > >   No. We should not enable topoext here. We should depend on CPU
> > > model table here.
> > > >
> > > > >
> > > > > Is the above description accurate?  Do you agree with these
> > > > > requirements?
> > > >
> > > > With these requirements in mind, I will send that patches. We can start
> our
> > > discussion.
> > > > We don't need one more bits. That is my opinion.
> > >
> > > Thanks for confirming the requirements above.
> > >
> > > But it doesn't seem to be possible represent these requirements
> > > with just one bit.  Otherwise you can't differentiate explicit
> > > "+topoext" (1 above) from topoext being implicitly enabled by
> > > "-cpu EPYC" (3 above).
> > >
> > > Another problem is query-cpu-model-expansion QMP command: this
> > > patch makes "topoext" appear on the output of
> > > "query-cpu-model-expansion model=EPYC", meaning that management
> > > software will assume everybody using the "EPYC" CPU model will
> > > require +topoext.  A separate "auto-topoext" property would avoid
> > > this issue.
> >
> > Sure.  Will work on it.  Yes, I know all these combinations make it very
> tricky.
> 
> I will rewrite my proposal, to make sure we are on the same page:
> 
> 0) "-topoext" explicitly configured (any machine-type):
> * Will clear TOPOEXT on env->features and set TOPOEXT on
>   env->user_features
>   (already done today)
> 
> 1) "+topoext" explicitly configured (any machine-type):
> * Will set TOPOEXT on both env->user_features and env->features
>   (already done today)
> 
> 2) Older machine-types:
> [CHANGED in v2]
> * Will set auto-topoext=off (can be done on compat_props)
> * No need to set topoext=off on compat_props (because no CPU model will
> have
>   topoext=on, see #3 below)
> 
> 3) "EPYC" CPU model (on new machine-types):
> [CHANGED IN v2]
> * Will set auto-topoext=on (using a new field on X86CPUDefinition)
> * Will NOT set TOPOEXT on CPU model table (or it would break
>   query-cpu-model-expansion)
> 
> 4) Other AMD CPU models (on all machine-types):
> [CHANGED IN v2]
> * auto-topoext=off (the default)
> * Will keep TOPOEXT disabled on env->features (the default)
> 
> 
> x86_cpu_realizefn:
> 
>   if {cpu->cpudef && cpu->cpudef->auto_topoext &&
>       TOPOEXT not in env->user_features &&
>       supported_topology) {
>       set TOPOEXT in env->features
>   }
> 
>   if (TOPOEXT in env->features && !supported_topology)
>       error;
>   }
> 
> I think this logic will fulfill all the requirements above.

Hi Eduardo ,
 I have sent the v13 version. I am still testing it. Wanted to get feedback if I missed anything. Thanks

> 
> --
> Eduardo