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

[Qemu-devel] [PATCH v14 0/6] 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 

v14:
 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. Always set TOPOEXT feature in kvm_arch_get_supported_cpuid
 2. Implemented topology_supports_topoext bit differently. Reason for this is, if 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.
 3. Removed auto-topoext feature completely. The can cause lots of compatibility issues.

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 (6):
  i386: Set TOPOEXT unconditionally for comapatibility
  i386: Enable TOPOEXT feature on AMD EPYC CPU
  i386: Disable TOPOEXT feature on pc-2.12
  cpus: Add new function topology_supports_topoext
  i386: Disable TOPOEXT feature if it cannot be supported
  i386: Remove generic SMT thread check

 accel/tcg/user-exec-stub.c |  5 +++++
 cpus.c                     | 13 +++++++++++++
 include/hw/i386/pc.h       |  4 ++++
 include/qom/cpu.h          |  9 +++++++++
 target/i386/cpu.c          | 45 ++++++++++++++++++++++++++++++++++++---------
 target/i386/kvm.c          |  6 ++++++
 6 files changed, 73 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v14 1/6] i386: Set TOPOEXT unconditionally for comapatibility

[Babu Moger]

Enabling TOPOEXT feature might cause compatibility issues if
older kernels does not set this feature. Lets set this feature
unconditionally.

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

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 445e0e0..6f2cca7 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -372,6 +372,12 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
         if (host_tsx_blacklisted()) {
             ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE);
         }
+    } else if (function == 0x80000001 && reg == R_ECX) {
+        /* Enabling topoext feature might cause compatibility issues if
+         * older kernel does not set this feature. Lets set this feature
+         * unconditionally.
+         */
+        ret |= CPUID_EXT3_TOPOEXT;
     } else if (function == 0x80000001 && reg == R_EDX) {
         /* On Intel, kvm returns cpuid according to the Intel spec,
          * so add missing bits according to the AMD spec:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v14 1/6] i386: Set TOPOEXT unconditionally for comapatibility

[Eduardo Habkost]

On Wed, Jun 13, 2018 at 09:18:22PM -0400, Babu Moger wrote:
> Enabling TOPOEXT feature might cause compatibility issues if
> older kernels does not set this feature. Lets set this feature
> unconditionally.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/kvm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 445e0e0..6f2cca7 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -372,6 +372,12 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
>          if (host_tsx_blacklisted()) {
>              ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE);
>          }
> +    } else if (function == 0x80000001 && reg == R_ECX) {
> +        /* Enabling topoext feature might cause compatibility issues if
> +         * older kernel does not set this feature. Lets set this feature
> +         * unconditionally.
> +         */

Thanks.  I will apply and rewrite the comment as:

        /*
         * It's safe to enable TOPOEXT even if it's not returned
         * by GET_SUPPORTED_CPUID.  Unconditionally enabling
         * TOPOEXT here let us keep CPU models runnable on
         * older kernels even when TOPOEXT is enabled.
         */

> +        ret |= CPUID_EXT3_TOPOEXT;
>      } else if (function == 0x80000001 && reg == R_EDX) {
>          /* On Intel, kvm returns cpuid according to the Intel spec,
>           * so add missing bits according to the AMD spec:
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v14 1/6] i386: Set TOPOEXT unconditionally for comapatibility

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 13, 2018 9:22 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 v14 1/6] i386: Set TOPOEXT unconditionally for
> comapatibility
> 
> On Wed, Jun 13, 2018 at 09:18:22PM -0400, Babu Moger wrote:
> > Enabling TOPOEXT feature might cause compatibility issues if
> > older kernels does not set this feature. Lets set this feature
> > unconditionally.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  target/i386/kvm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > index 445e0e0..6f2cca7 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> > @@ -372,6 +372,12 @@ uint32_t
> kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
> >          if (host_tsx_blacklisted()) {
> >              ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE);
> >          }
> > +    } else if (function == 0x80000001 && reg == R_ECX) {
> > +        /* Enabling topoext feature might cause compatibility issues if
> > +         * older kernel does not set this feature. Lets set this feature
> > +         * unconditionally.
> > +         */
> 
> Thanks.  I will apply and rewrite the comment as:
Sure. Thanks
> 
>         /*
>          * It's safe to enable TOPOEXT even if it's not returned
>          * by GET_SUPPORTED_CPUID.  Unconditionally enabling
>          * TOPOEXT here let us keep CPU models runnable on
>          * older kernels even when TOPOEXT is enabled.
>          */
> 
> > +        ret |= CPUID_EXT3_TOPOEXT;
> >      } else if (function == 0x80000001 && reg == R_EDX) {
> >          /* On Intel, kvm returns cpuid according to the Intel spec,
> >           * so add missing bits according to the AMD spec:
> > --
> > 1.8.3.1
> >
> 
> --
> Eduardo

[Qemu-devel] [PATCH v14 2/6] 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.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 86fb1a4..2eb26da 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2554,7 +2554,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 |
@@ -2599,7 +2600,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] =
@@ -4667,6 +4669,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

Re: [Qemu-devel] [PATCH v14 2/6] i386: Enable TOPOEXT feature on AMD EPYC CPU

[Eduardo Habkost]

On Wed, Jun 13, 2018 at 09:18:23PM -0400, Babu Moger wrote:
> Enable TOPOEXT feature on EPYC CPU. This is required to support
> hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 86fb1a4..2eb26da 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2554,7 +2554,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 |
> @@ -2599,7 +2600,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] =

This part is OK, but it requires patch 3/6 to be included in the
same patch.

> @@ -4667,6 +4669,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);
> +        }

This part needs to be done more carefully to avoid breaking
compatibility.  "-machine pc-q35-2.12 -cpu Opteron_G5,+topoext"
currently results in xlevel=0x8000001A, and this must not change.

I suggest just changing setting .xlevel=0x8000001E on EPYC at
builtin_x86_defs[1], and worry about automatically increasing
xlevel later.

(If you change EPYC.xlevel in builtin_x86_defs, don't forget to
set EPYC.xlevel=0x8000000A on PC_COMPAT_2_12)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v14 2/6] i386: Enable TOPOEXT feature on AMD EPYC CPU

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Thursday, June 14, 2018 1: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 v14 2/6] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Wed, Jun 13, 2018 at 09:18:23PM -0400, Babu Moger wrote:
> > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  target/i386/cpu.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 86fb1a4..2eb26da 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -2554,7 +2554,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 |
> > @@ -2599,7 +2600,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] =
> 
> This part is OK, but it requires patch 3/6 to be included in the
> same patch.

Ok. Sure.
> 
> > @@ -4667,6 +4669,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);
> > +        }
> 
> This part needs to be done more carefully to avoid breaking
> compatibility.  "-machine pc-q35-2.12 -cpu Opteron_G5,+topoext"
> currently results in xlevel=0x8000001A, and this must not change.
Ok.
> 
> I suggest just changing setting .xlevel=0x8000001E on EPYC at
> builtin_x86_defs[1], and worry about automatically increasing
> xlevel later.
Ok.
> 
> (If you change EPYC.xlevel in builtin_x86_defs, don't forget to
> set EPYC.xlevel=0x8000000A on PC_COMPAT_2_12)
Sure.
> 
> --
> Eduardo

Re: [Qemu-devel] [PATCH v14 2/6] i386: Enable TOPOEXT feature on AMD EPYC CPU

[Moger, Babu]

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Moger, Babu
> Sent: Thursday, June 14, 2018 3:41 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
> Subject: RE: [PATCH v14 2/6] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Thursday, June 14, 2018 1: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 v14 2/6] i386: Enable TOPOEXT feature on AMD EPYC
> > CPU
> >
> > On Wed, Jun 13, 2018 at 09:18:23PM -0400, Babu Moger wrote:
> > > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> > >
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > ---
> > >  target/i386/cpu.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 86fb1a4..2eb26da 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -2554,7 +2554,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 |
> > > @@ -2599,7 +2600,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] =
> >
> > This part is OK, but it requires patch 3/6 to be included in the
> > same patch.
> 
> Ok. Sure.
> >
> > > @@ -4667,6 +4669,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);
> > > +        }
> >
> > This part needs to be done more carefully to avoid breaking
> > compatibility.  "-machine pc-q35-2.12 -cpu Opteron_G5,+topoext"
> > currently results in xlevel=0x8000001A, and this must not change.

Not sure if this could be a problem.  "+topoext" sets the feature bits. But xlevel is still 0x8000001A which does not look right.
I need to verify this case.
 
> Ok.
> >
> > I suggest just changing setting .xlevel=0x8000001E on EPYC at
> > builtin_x86_defs[1], and worry about automatically increasing
> > xlevel later.
> Ok.
> >
> > (If you change EPYC.xlevel in builtin_x86_defs, don't forget to
> > set EPYC.xlevel=0x8000000A on PC_COMPAT_2_12)
> Sure.
> >
> > --
> > Eduardo

[Qemu-devel] [PATCH v14 3/6] i386: Disable TOPOEXT feature on pc-2.12

[Babu Moger]

Disable TOPOEXT feature for older machines.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 include/hw/i386/pc.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 04d1f8c..ecccf6b 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 = "topoext",\
+        .value    = "off",\
     },
 
 #define PC_COMPAT_2_11 \
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v14 3/6] i386: Disable TOPOEXT feature on pc-2.12

[Eduardo Habkost]

On Wed, Jun 13, 2018 at 09:18:24PM -0400, Babu Moger wrote:
> Disable TOPOEXT feature for older machines.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  include/hw/i386/pc.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 04d1f8c..ecccf6b 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 = "topoext",\
> +        .value    = "off",\
>      },

This is OK, if combined with the first hunks of patch 2/6.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v14 3/6] i386: Disable TOPOEXT feature on pc-2.12

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Thursday, June 14, 2018 1:41 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 v14 3/6] i386: Disable TOPOEXT feature on pc-2.12
> 
> On Wed, Jun 13, 2018 at 09:18:24PM -0400, Babu Moger wrote:
> > Disable TOPOEXT feature for older machines.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  include/hw/i386/pc.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 04d1f8c..ecccf6b 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 = "topoext",\
> > +        .value    = "off",\
> >      },
> 
> This is OK, if combined with the first hunks of patch 2/6.
Ok. Sure.
> 
> --
> Eduardo

[Qemu-devel] [PATCH v14 4/6] cpus: Add new function topology_supports_topoext

[Babu Moger]

Add new function topology_supports_topoext to verify
if we can support topoext feature. Will be used to enable/disable
topoext feature.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 accel/tcg/user-exec-stub.c |  5 +++++
 cpus.c                     | 13 +++++++++++++
 include/qom/cpu.h          |  9 +++++++++
 3 files changed, 27 insertions(+)

diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
index dbcf1ad..715818a 100644
--- a/accel/tcg/user-exec-stub.c
+++ b/accel/tcg/user-exec-stub.c
@@ -11,6 +11,11 @@ void qemu_init_vcpu(CPUState *cpu)
 {
 }
 
+int topology_supports_topoext(int max_cores, int max_threads)
+{
+    return true;
+}
+
 /* User mode emulation does not support record/replay yet.  */
 
 bool replay_exception(void)
diff --git a/cpus.c b/cpus.c
index d1f1629..17f6f4c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1979,6 +1979,19 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
                        QEMU_THREAD_JOINABLE);
 }
 
+/*
+ * Check if we can support topoext feature with this topology
+ * Fail if number of cores are beyond the supported cores
+ * or number of threads are more than supported threads
+ */
+int topology_supports_topoext(int max_cores, int max_threads)
+{
+    if ((smp_cores > max_cores) || (smp_threads > max_threads)) {
+        return false;
+    }
+    return true;
+}
+
 void qemu_init_vcpu(CPUState *cpu)
 {
     cpu->nr_cores = smp_cores;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 9d3afc6..4ac4d49 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -1004,6 +1004,15 @@ void end_exclusive(void);
  */
 void qemu_init_vcpu(CPUState *cpu);
 
+/**
+ * topology_supports_topoext:
+ * @max_cores: Max cores topoext feature can support
+ * @max_threads: Max threads topoext feature can support
+ *
+ * Return true if topology can be supported else return false
+ */
+int topology_supports_topoext(int max_cores, int max_threads);
+
 #define SSTEP_ENABLE  0x1  /* Enable simulated HW single stepping */
 #define SSTEP_NOIRQ   0x2  /* Do not use IRQ while single stepping */
 #define SSTEP_NOTIMER 0x4  /* Do not Timers while single stepping */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v14 4/6] cpus: Add new function topology_supports_topoext

[Eduardo Habkost]

On Wed, Jun 13, 2018 at 09:18:25PM -0400, Babu Moger wrote:
> Add new function topology_supports_topoext to verify
> if we can support topoext feature. Will be used to enable/disable
> topoext feature.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  accel/tcg/user-exec-stub.c |  5 +++++
>  cpus.c                     | 13 +++++++++++++
>  include/qom/cpu.h          |  9 +++++++++
>  3 files changed, 27 insertions(+)
> 

x86-specific code doesn't belong to arch-independent code.  Why
not keep it in target/i386/cpu.c like before?

-- 
Eduardo

[Qemu-devel] [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be supported

[Babu Moger]

Disable the TOPOEXT feature if it cannot be supported.
We cannot support this feature with more than 2 nr_threads
or more than 32 cores in a socket.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 2eb26da..637d8eb 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4765,7 +4765,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
-    static bool ht_warned;
+    static bool ht_warned, topo_warned;
 
     if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
         char *name = x86_cpu_class_get_model_name(xcc);
@@ -4779,6 +4779,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    /* Disable TOPOEXT if topology cannot be supported */
+    if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
+        if (!topology_supports_topoext(MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET,
+                                      2)) {
+            env->features[FEAT_8000_0001_ECX] &= !CPUID_EXT3_TOPOEXT;
+            if (!topo_warned) {
+                error_report("TOPOEXT feature cannot be supported with more"
+                             " than %d cores or more than 2 threads per socket."
+                             " Disabling the feature.",
+                             (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
+                topo_warned = true;
+            }
+        }
+    }
+
     x86_cpu_expand_features(cpu, &local_err);
     if (local_err) {
         goto out;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be supported

[Eduardo Habkost]

On Wed, Jun 13, 2018 at 09:18:26PM -0400, Babu Moger wrote:
> Disable the TOPOEXT feature if it cannot be supported.
> We cannot support this feature with more than 2 nr_threads
> or more than 32 cores in a socket.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 2eb26da..637d8eb 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4765,7 +4765,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
>      CPUX86State *env = &cpu->env;
>      Error *local_err = NULL;
> -    static bool ht_warned;
> +    static bool ht_warned, topo_warned;
>  
>      if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
>          char *name = x86_cpu_class_get_model_name(xcc);
> @@ -4779,6 +4779,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    /* Disable TOPOEXT if topology cannot be supported */
> +    if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
> +        if (!topology_supports_topoext(MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET,
> +                                      2)) {

I understand you stopped using cpu->nr_cores/cpu->nr_threads
because it was not filled yet.

But why exactly do you need to do this before calling
x86_cpu_expand_features()?

If you really need nr_cores and nr_threads to be available
earlier, we could simply move their initialization to
cpu_exec_initfn() instead of the solution you implemented in
patch 4/6.

> +            env->features[FEAT_8000_0001_ECX] &= !CPUID_EXT3_TOPOEXT;

!CPUID_EXT3_TOPOEXT is 0, this will clear all bits in
env->features[FEAT_8000_0001_ECX].  Did you mean
~CPUID_EXT3_TOPOEXT?


> +            if (!topo_warned) {
> +                error_report("TOPOEXT feature cannot be supported with more"
> +                             " than %d cores or more than 2 threads per socket."
> +                             " Disabling the feature.",
> +                             (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> +                topo_warned = true;

This will print a warning for "-cpu EPYC -smp 64,cores=64".
We shouldn't.

I'm starting to believe we shouldn't add TOPOEXT to EPYC unless
we're ready to make the TOPOEXT CPUID leaves work for all valid
-smp configurations.  If the feature will work only on a few
specific cases, the feature should be enabled explicitly using
"-cpu ...,+topoext".

Is it really impossible to make CPUID return reasonable topology
data for larger nr_cores and nr_threads values?  It would make
everything much simpler.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be supported

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Thursday, June 14, 2018 2:13 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 v14 5/6] i386: Disable TOPOEXT feature if it cannot be
> supported
> 
> On Wed, Jun 13, 2018 at 09:18:26PM -0400, Babu Moger wrote:
> > Disable the TOPOEXT feature if it cannot be supported.
> > We cannot support this feature with more than 2 nr_threads
> > or more than 32 cores in a socket.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  target/i386/cpu.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 2eb26da..637d8eb 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -4765,7 +4765,7 @@ static void x86_cpu_realizefn(DeviceState *dev,
> Error **errp)
> >      X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> >      CPUX86State *env = &cpu->env;
> >      Error *local_err = NULL;
> > -    static bool ht_warned;
> > +    static bool ht_warned, topo_warned;
> >
> >      if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
> >          char *name = x86_cpu_class_get_model_name(xcc);
> > @@ -4779,6 +4779,21 @@ static void x86_cpu_realizefn(DeviceState *dev,
> Error **errp)
> >          return;
> >      }
> >
> > +    /* Disable TOPOEXT if topology cannot be supported */
> > +    if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
> > +        if (!topology_supports_topoext(MAX_CORES_IN_NODE *
> MAX_NODES_PER_SOCKET,
> > +                                      2)) {
> 
> I understand you stopped using cpu->nr_cores/cpu->nr_threads
> because it was not filled yet.
> 
> But why exactly do you need to do this before calling
> x86_cpu_expand_features()?

We extend the xlevel in x86_cpu_expand_features based on the TOPOEXT feature.
So, I thought it would be right to do that way.  
> 
> If you really need nr_cores and nr_threads to be available
> earlier, we could simply move their initialization to
> cpu_exec_initfn() instead of the solution you implemented in
> patch 4/6.
> 
> > +            env->features[FEAT_8000_0001_ECX] &= !CPUID_EXT3_TOPOEXT;
> 
> !CPUID_EXT3_TOPOEXT is 0, this will clear all bits in
> env->features[FEAT_8000_0001_ECX].  Did you mean
> ~CPUID_EXT3_TOPOEXT?

Yes. That is correct.  Sorry.. I missed it.
> 
> 
> > +            if (!topo_warned) {
> > +                error_report("TOPOEXT feature cannot be supported with more"
> > +                             " than %d cores or more than 2 threads per socket."
> > +                             " Disabling the feature.",
> > +                             (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> > +                topo_warned = true;
> 
> This will print a warning for "-cpu EPYC -smp 64,cores=64".
> We shouldn't.
> 
> I'm starting to believe we shouldn't add TOPOEXT to EPYC unless
> we're ready to make the TOPOEXT CPUID leaves work for all valid
> -smp configurations.  If the feature will work only on a few
> specific cases, the feature should be enabled explicitly using
> "-cpu ...,+topoext".
> 
> Is it really impossible to make CPUID return reasonable topology
> data for larger nr_cores and nr_threads values?  It would make
> everything much simpler.

I am starting to think about this.  We tried to limit the configuration based on the actual hardware configuration.
If we leave that decision to the user then we might allow the config whatever the user wants. 
I need to make some changes in for topology(0x80000001e) information to make this work.

> 
> --
> Eduardo

Re: [Qemu-devel] [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be supported

[Moger, Babu]

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On Behalf Of Moger, Babu
> Sent: Thursday, June 14, 2018 5:19 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
> Subject: RE: [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be
> supported
> 
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > Sent: Thursday, June 14, 2018 2:13 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 v14 5/6] i386: Disable TOPOEXT feature if it cannot be
> > supported
> >
> > On Wed, Jun 13, 2018 at 09:18:26PM -0400, Babu Moger wrote:
> > > Disable the TOPOEXT feature if it cannot be supported.
> > > We cannot support this feature with more than 2 nr_threads
> > > or more than 32 cores in a socket.
> > >
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > ---
> > >  target/i386/cpu.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 2eb26da..637d8eb 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -4765,7 +4765,7 @@ static void x86_cpu_realizefn(DeviceState *dev,
> > Error **errp)
> > >      X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> > >      CPUX86State *env = &cpu->env;
> > >      Error *local_err = NULL;
> > > -    static bool ht_warned;
> > > +    static bool ht_warned, topo_warned;
> > >
> > >      if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
> > >          char *name = x86_cpu_class_get_model_name(xcc);
> > > @@ -4779,6 +4779,21 @@ static void x86_cpu_realizefn(DeviceState
> *dev,
> > Error **errp)
> > >          return;
> > >      }
> > >
> > > +    /* Disable TOPOEXT if topology cannot be supported */
> > > +    if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
> > > +        if (!topology_supports_topoext(MAX_CORES_IN_NODE *
> > MAX_NODES_PER_SOCKET,
> > > +                                      2)) {
> >
> > I understand you stopped using cpu->nr_cores/cpu->nr_threads
> > because it was not filled yet.
> >
> > But why exactly do you need to do this before calling
> > x86_cpu_expand_features()?
> 
> We extend the xlevel in x86_cpu_expand_features based on the TOPOEXT
> feature.
> So, I thought it would be right to do that way.
> >
> > If you really need nr_cores and nr_threads to be available
> > earlier, we could simply move their initialization to
> > cpu_exec_initfn() instead of the solution you implemented in
> > patch 4/6.
> >
> > > +            env->features[FEAT_8000_0001_ECX] &=
> !CPUID_EXT3_TOPOEXT;
> >
> > !CPUID_EXT3_TOPOEXT is 0, this will clear all bits in
> > env->features[FEAT_8000_0001_ECX].  Did you mean
> > ~CPUID_EXT3_TOPOEXT?
> 
> Yes. That is correct.  Sorry.. I missed it.
> >
> >
> > > +            if (!topo_warned) {
> > > +                error_report("TOPOEXT feature cannot be supported with
> more"
> > > +                             " than %d cores or more than 2 threads per socket."
> > > +                             " Disabling the feature.",
> > > +                             (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> > > +                topo_warned = true;
> >
> > This will print a warning for "-cpu EPYC -smp 64,cores=64".
> > We shouldn't.

If we support all the values, we may not need this. 
> >
> > I'm starting to believe we shouldn't add TOPOEXT to EPYC unless
> > we're ready to make the TOPOEXT CPUID leaves work for all valid
> > -smp configurations.  If the feature will work only on a few
> > specific cases, the feature should be enabled explicitly using
> > "-cpu ...,+topoext".
> >
> > Is it really impossible to make CPUID return reasonable topology
> > data for larger nr_cores and nr_threads values?  It would make
> > everything much simpler.
> 
> I am starting to think about this.  We tried to limit the configuration based on
> the actual hardware configuration.
> If we leave that decision to the user then we might allow the config
> whatever the user wants.
> I need to make some changes in for topology(0x80000001e) information to
> make this work.
> 

One  more thought, we can allow all the configurations. If user creates supported configuration, it will work perfectly fine.
If user creates unsupported configuration(like more threads, more cores etc), we still create the topology, but it will not be ideal topology.
Reason for this is, I don't want to mess up the good configuration to support invalid config. That way we don't have to change anything in topology code now.
 
> >
> > --
> > Eduardo

Re: [Qemu-devel] [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be supported

[Moger, Babu]

> -----Original Message-----
> From: Moger, Babu
> Sent: Thursday, June 14, 2018 6:09 PM
> To: Moger, Babu <Babu.Moger@amd.com>; 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
> Subject: RE: [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be
> supported
> 
> 
> > -----Original Message-----
> > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> > On Behalf Of Moger, Babu
> > Sent: Thursday, June 14, 2018 5:19 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
> > Subject: RE: [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be
> > supported
> >
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> > > Sent: Thursday, June 14, 2018 2:13 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 v14 5/6] i386: Disable TOPOEXT feature if it cannot be
> > > supported
> > >
> > > On Wed, Jun 13, 2018 at 09:18:26PM -0400, Babu Moger wrote:
> > > > Disable the TOPOEXT feature if it cannot be supported.
> > > > We cannot support this feature with more than 2 nr_threads
> > > > or more than 32 cores in a socket.
> > > >
> > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > > ---
> > > >  target/i386/cpu.c | 17 ++++++++++++++++-
> > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > index 2eb26da..637d8eb 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -4765,7 +4765,7 @@ static void x86_cpu_realizefn(DeviceState
> *dev,
> > > Error **errp)
> > > >      X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> > > >      CPUX86State *env = &cpu->env;
> > > >      Error *local_err = NULL;
> > > > -    static bool ht_warned;
> > > > +    static bool ht_warned, topo_warned;
> > > >
> > > >      if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
> > > >          char *name = x86_cpu_class_get_model_name(xcc);
> > > > @@ -4779,6 +4779,21 @@ static void x86_cpu_realizefn(DeviceState
> > *dev,
> > > Error **errp)
> > > >          return;
> > > >      }
> > > >
> > > > +    /* Disable TOPOEXT if topology cannot be supported */
> > > > +    if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
> > > > +        if (!topology_supports_topoext(MAX_CORES_IN_NODE *
> > > MAX_NODES_PER_SOCKET,
> > > > +                                      2)) {
> > >
> > > I understand you stopped using cpu->nr_cores/cpu->nr_threads
> > > because it was not filled yet.
> > >
> > > But why exactly do you need to do this before calling
> > > x86_cpu_expand_features()?
> >
> > We extend the xlevel in x86_cpu_expand_features based on the TOPOEXT
> > feature.
> > So, I thought it would be right to do that way.
> > >
> > > If you really need nr_cores and nr_threads to be available
> > > earlier, we could simply move their initialization to
> > > cpu_exec_initfn() instead of the solution you implemented in
> > > patch 4/6.
> > >
> > > > +            env->features[FEAT_8000_0001_ECX] &=
> > !CPUID_EXT3_TOPOEXT;
> > >
> > > !CPUID_EXT3_TOPOEXT is 0, this will clear all bits in
> > > env->features[FEAT_8000_0001_ECX].  Did you mean
> > > ~CPUID_EXT3_TOPOEXT?
> >
> > Yes. That is correct.  Sorry.. I missed it.
> > >
> > >
> > > > +            if (!topo_warned) {
> > > > +                error_report("TOPOEXT feature cannot be supported with
> > more"
> > > > +                             " than %d cores or more than 2 threads per socket."
> > > > +                             " Disabling the feature.",
> > > > +                             (MAX_CORES_IN_NODE *
> MAX_NODES_PER_SOCKET));
> > > > +                topo_warned = true;
> > >
> > > This will print a warning for "-cpu EPYC -smp 64,cores=64".
> > > We shouldn't.
> 
> If we support all the values, we may not need this.
> > >
> > > I'm starting to believe we shouldn't add TOPOEXT to EPYC unless
> > > we're ready to make the TOPOEXT CPUID leaves work for all valid
> > > -smp configurations.  If the feature will work only on a few
> > > specific cases, the feature should be enabled explicitly using
> > > "-cpu ...,+topoext".
> > >
> > > Is it really impossible to make CPUID return reasonable topology
> > > data for larger nr_cores and nr_threads values?  It would make
> > > everything much simpler.
> >
> > I am starting to think about this.  We tried to limit the configuration based
> on
> > the actual hardware configuration.
> > If we leave that decision to the user then we might allow the config
> > whatever the user wants.
> > I need to make some changes in for topology(0x80000001e) information to
> > make this work.
> >
> 
> One  more thought, we can allow all the configurations. If user creates
> supported configuration, it will work perfectly fine.
> If user creates unsupported configuration(like more threads, more cores
> etc), we still create the topology, but it will not be ideal topology.
> Reason for this is, I don't want to mess up the good configuration to support
> invalid config. That way we don't have to change anything in topology code
> now.
> 

I am working on changes to accommodate all the nr_cores and threads. 
Need to adjust the node_id in CPUID 8000001E to accommodate more nodes.
Will send updates later today.

> > >
> > > --
> > > Eduardo

[Qemu-devel] [PATCH v14 6/6] 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 637d8eb..6783305 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4963,17 +4963,22 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     qemu_init_vcpu(cs);
 
-    /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this
-     * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
-     * based on inputs (sockets,cores,threads), it is still better to gives
+    /*
+     * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
+     * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
+     * based on inputs (sockets,cores,threads), it is still better to give
      * users a warning.
      *
      * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
      * cs->nr_threads hasn't be populated yet and the checking is incorrect.
      */
-    if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) {
-        error_report("AMD CPU doesn't support hyperthreading. Please configure"
-                     " -smp options properly.");
+     if (IS_AMD_CPU(env) &&
+         !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
+         cs->nr_threads > 1 && !ht_warned) {
+            error_report("This family of AMD CPU doesn't support "
+                         "hyperthreading(%d). Please configure -smp "
+                         "options properly or try enabling topoext feature.",
+                         cs->nr_threads);
         ht_warned = true;
     }
 
-- 
1.8.3.1