[PATCH v4 0/5] Enable TOPOEXT to support hyperthreading on AMD CPU

[PATCH v4 0/5] 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 

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 Krčmář).
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 (3):
  target/i386: Generalize some of the macro definitions
  target/i386: Enable TOPOEXT feature on AMD EPYC CPU
  target/i386: Remove generic SMT thread check

Stanislav Lanci (2):
  target/i386: Populate AMD Processor Cache Information
  target/i386: Add support for CPUID_8000_001E for AMD

 target/i386/cpu.c | 150 +++++++++++++++++++++++++++++++++++++++++-------------
 target/i386/kvm.c |  29 +++++++++--
 2 files changed, 142 insertions(+), 37 deletions(-)

-- 
1.8.3.1

[PATCH v4 1/5] target/i386: Generalize some of the macro definitions

[Babu Moger]

Generalize some of the macro definitions which are generic cache
properties that are common between CPUID 4 and CPUID 0x8000001D
in preparation for adding support for 0x8000001D.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b5e431e..42dd381 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -66,22 +66,22 @@
 #define CPUID_2_L3_16MB_16WAY_64B 0x4d
 
 
-/* CPUID Leaf 4 constants: */
+/* Cache specific constants: */
 
 /* EAX: */
-#define CPUID_4_TYPE_DCACHE  1
-#define CPUID_4_TYPE_ICACHE  2
-#define CPUID_4_TYPE_UNIFIED 3
+#define TYPE_DCACHE  1
+#define TYPE_ICACHE  2
+#define TYPE_UNIFIED 3
 
-#define CPUID_4_LEVEL(l)          ((l) << 5)
+#define CACHE_LEVEL(l)          ((l) << 5)
 
-#define CPUID_4_SELF_INIT_LEVEL (1 << 8)
-#define CPUID_4_FULLY_ASSOC     (1 << 9)
+#define CACHE_SELF_INIT_LEVEL (1 << 8)
+#define CACHE_FULLY_ASSOC     (1 << 9)
 
 /* EDX: */
-#define CPUID_4_NO_INVD_SHARING (1 << 0)
-#define CPUID_4_INCLUSIVE       (1 << 1)
-#define CPUID_4_COMPLEX_IDX     (1 << 2)
+#define CACHE_NO_INVD_SHARING (1 << 0)
+#define CACHE_INCLUSIVE       (1 << 1)
+#define CACHE_COMPLEX_IDX     (1 << 2)
 
 #define ASSOC_FULL 0xFF
 
@@ -3273,29 +3273,29 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *eax = 0;
             switch (count) {
             case 0: /* L1 dcache info */
-                *eax |= CPUID_4_TYPE_DCACHE | \
-                        CPUID_4_LEVEL(1) | \
-                        CPUID_4_SELF_INIT_LEVEL;
+                *eax |= TYPE_DCACHE | \
+                        CACHE_LEVEL(1) | \
+                        CACHE_SELF_INIT_LEVEL;
                 *ebx = (L1D_LINE_SIZE - 1) | \
                        ((L1D_PARTITIONS - 1) << 12) | \
                        ((L1D_ASSOCIATIVITY - 1) << 22);
                 *ecx = L1D_SETS - 1;
-                *edx = CPUID_4_NO_INVD_SHARING;
+                *edx = CACHE_NO_INVD_SHARING;
                 break;
             case 1: /* L1 icache info */
-                *eax |= CPUID_4_TYPE_ICACHE | \
-                        CPUID_4_LEVEL(1) | \
-                        CPUID_4_SELF_INIT_LEVEL;
+                *eax |= TYPE_ICACHE | \
+                        CACHE_LEVEL(1) | \
+                        CACHE_SELF_INIT_LEVEL;
                 *ebx = (L1I_LINE_SIZE - 1) | \
                        ((L1I_PARTITIONS - 1) << 12) | \
                        ((L1I_ASSOCIATIVITY - 1) << 22);
                 *ecx = L1I_SETS - 1;
-                *edx = CPUID_4_NO_INVD_SHARING;
+                *edx = CACHE_NO_INVD_SHARING;
                 break;
             case 2: /* L2 cache info */
-                *eax |= CPUID_4_TYPE_UNIFIED | \
-                        CPUID_4_LEVEL(2) | \
-                        CPUID_4_SELF_INIT_LEVEL;
+                *eax |= TYPE_UNIFIED | \
+                        CACHE_LEVEL(2) | \
+                        CACHE_SELF_INIT_LEVEL;
                 if (cs->nr_threads > 1) {
                     *eax |= (cs->nr_threads - 1) << 14;
                 }
@@ -3303,7 +3303,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                        ((L2_PARTITIONS - 1) << 12) | \
                        ((L2_ASSOCIATIVITY - 1) << 22);
                 *ecx = L2_SETS - 1;
-                *edx = CPUID_4_NO_INVD_SHARING;
+                *edx = CACHE_NO_INVD_SHARING;
                 break;
             case 3: /* L3 cache info */
                 if (!cpu->enable_l3_cache) {
@@ -3313,16 +3313,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                     *edx = 0;
                     break;
                 }
-                *eax |= CPUID_4_TYPE_UNIFIED | \
-                        CPUID_4_LEVEL(3) | \
-                        CPUID_4_SELF_INIT_LEVEL;
+                *eax |= TYPE_UNIFIED | \
+                        CACHE_LEVEL(3) | \
+                        CACHE_SELF_INIT_LEVEL;
                 pkg_offset = apicid_pkg_offset(cs->nr_cores, cs->nr_threads);
                 *eax |= ((1 << pkg_offset) - 1) << 14;
                 *ebx = (L3_N_LINE_SIZE - 1) | \
                        ((L3_N_PARTITIONS - 1) << 12) | \
                        ((L3_N_ASSOCIATIVITY - 1) << 22);
                 *ecx = L3_N_SETS - 1;
-                *edx = CPUID_4_INCLUSIVE | CPUID_4_COMPLEX_IDX;
+                *edx = CACHE_INCLUSIVE | CACHE_COMPLEX_IDX;
                 break;
             default: /* end of info */
                 *eax = 0;
-- 
1.8.3.1

Re: [PATCH v4 1/5] target/i386: Generalize some of the macro definitions

[Eduardo Habkost]

Hi,

Sorry for not reviewing the previous versions of this series
(making it miss soft freeze).


On Mon, Mar 12, 2018 at 05:00:45PM -0400, Babu Moger wrote:
> Generalize some of the macro definitions which are generic cache
> properties that are common between CPUID 4 and CPUID 0x8000001D
> in preparation for adding support for 0x8000001D.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c | 52 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b5e431e..42dd381 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -66,22 +66,22 @@
>  #define CPUID_2_L3_16MB_16WAY_64B 0x4d
>  
>  
> -/* CPUID Leaf 4 constants: */
> +/* Cache specific constants: */

We have too many CPUID leaves that describe caches, so I would
like to explicitly document on which CPUID leaves these macros
can be used.  I suggest:

  /* Macros for CPUID[4] and CPUID[0x8000001D] */


>  
>  /* EAX: */
> -#define CPUID_4_TYPE_DCACHE  1
> -#define CPUID_4_TYPE_ICACHE  2
> -#define CPUID_4_TYPE_UNIFIED 3
> +#define TYPE_DCACHE  1
> +#define TYPE_ICACHE  2
> +#define TYPE_UNIFIED 3

TYPE_* can be confused with QOM type names, I'd use something
else.  Maybe CACHE_TYPE_D, CACHE_TYPE_I, CACHE_TYPE_UNIFIED?


>  
> -#define CPUID_4_LEVEL(l)          ((l) << 5)
> +#define CACHE_LEVEL(l)          ((l) << 5)
>  
> -#define CPUID_4_SELF_INIT_LEVEL (1 << 8)
> -#define CPUID_4_FULLY_ASSOC     (1 << 9)
> +#define CACHE_SELF_INIT_LEVEL (1 << 8)
> +#define CACHE_FULLY_ASSOC     (1 << 9)
>  
>  /* EDX: */
> -#define CPUID_4_NO_INVD_SHARING (1 << 0)
> -#define CPUID_4_INCLUSIVE       (1 << 1)
> -#define CPUID_4_COMPLEX_IDX     (1 << 2)
> +#define CACHE_NO_INVD_SHARING (1 << 0)
> +#define CACHE_INCLUSIVE       (1 << 1)
> +#define CACHE_COMPLEX_IDX     (1 << 2)
>  
>  #define ASSOC_FULL 0xFF
>  
[...]

-- 
Eduardo

[PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Babu Moger]

From: Stanislav Lanci <pixo@polepetko.eu>

Add information for cpuid 0x8000001D leaf. Populate cache topology information
for different cache types(Data Cache, Instruction Cache, L2 and L3) supported
by 0x8000001D leaf. Please refer Processor Programming Reference (PPR) for AMD
Family 17h Model for more details.

Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 target/i386/kvm.c | 29 ++++++++++++++++++++++---
 2 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 42dd381..5fdbedd 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -118,6 +118,7 @@
 #define L1I_LINE_SIZE         64
 #define L1I_ASSOCIATIVITY      8
 #define L1I_SETS              64
+#define L1I_SETS_AMD         256
 #define L1I_PARTITIONS         1
 /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 32KiB */
 #define L1I_DESCRIPTOR CPUID_2_L1I_32KB_8WAY_64B
@@ -129,7 +130,9 @@
 /* Level 2 unified cache: */
 #define L2_LINE_SIZE          64
 #define L2_ASSOCIATIVITY      16
+#define L2_ASSOCIATIVITY_AMD   8
 #define L2_SETS             4096
+#define L2_SETS_AMD         1024
 #define L2_PARTITIONS          1
 /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 4MiB */
 /*FIXME: CPUID leaf 2 descriptor is inconsistent with CPUID leaf 4 */
@@ -146,6 +149,7 @@
 #define L3_N_LINE_SIZE         64
 #define L3_N_ASSOCIATIVITY     16
 #define L3_N_SETS           16384
+#define L3_N_SETS_AMD        8192
 #define L3_N_PARTITIONS         1
 #define L3_N_DESCRIPTOR CPUID_2_L3_16MB_16WAY_64B
 #define L3_N_LINES_PER_TAG      1
@@ -3590,6 +3594,67 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *edx = 0;
         }
         break;
+    case 0x8000001D: /* AMD TOPOEXT cache info */
+        switch (count) {
+        case 0: /* L1 dcache info */
+            *eax |= TYPE_DCACHE | \
+                    CACHE_LEVEL(1) | \
+                    CACHE_SELF_INIT_LEVEL | \
+                    ((cs->nr_threads - 1) << 14);
+            *ebx = (L1D_LINE_SIZE - 1) | \
+                   ((L1D_PARTITIONS - 1) << 12) | \
+                   ((L1D_ASSOCIATIVITY - 1) << 22);
+            *ecx = L1D_SETS - 1;
+            *edx = 0;
+            break;
+        case 1: /* L1 icache info */
+            *eax |= TYPE_ICACHE | \
+                    CACHE_LEVEL(1) | \
+                    CACHE_SELF_INIT_LEVEL | \
+                    ((cs->nr_threads - 1) << 14);
+            *ebx = (L1I_LINE_SIZE - 1) | \
+                   ((L1I_PARTITIONS - 1) << 12) | \
+                   ((L1I_ASSOCIATIVITY_AMD - 1) << 22);
+            *ecx = L1I_SETS_AMD - 1;
+            *edx = 0;
+            break;
+        case 2: /* L2 cache info */
+            *eax |= TYPE_UNIFIED | \
+                    CACHE_LEVEL(2) | \
+                    CACHE_SELF_INIT_LEVEL | \
+                    ((cs->nr_threads - 1) << 14);
+            *ebx = (L2_LINE_SIZE - 1) | \
+                   ((L2_PARTITIONS - 1) << 12) | \
+                   ((L2_ASSOCIATIVITY_AMD - 1) << 22);
+            *ecx = L2_SETS_AMD - 1;
+            *edx = CACHE_INCLUSIVE;
+            break;
+        case 3: /* L3 cache info */
+            if (cpu->enable_l3_cache) {
+                *eax |= TYPE_UNIFIED | \
+                        CACHE_LEVEL(3) | \
+                        CACHE_SELF_INIT_LEVEL | \
+                        ((cs->nr_cores * cs->nr_threads - 1) << 14);
+                *ebx = (L3_N_LINE_SIZE - 1) | \
+                       ((L3_N_PARTITIONS - 1) << 12) | \
+                       ((L3_N_ASSOCIATIVITY - 1) << 22);
+                *ecx = L3_N_SETS_AMD - 1;
+                *edx = CACHE_NO_INVD_SHARING;
+            } else {
+                *eax = 0;
+                *ebx = 0;
+                *ecx = 0;
+                *edx = 0;
+            }
+            break;
+        default: /* end of info */
+            *eax = 0;
+            *ebx = 0;
+            *ecx = 0;
+            *edx = 0;
+            break;
+        }
+        break;
     case 0xC0000000:
         *eax = env->cpuid_xlevel2;
         *ebx = 0;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index ad4b159..0eb39b52 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -909,9 +909,32 @@ int kvm_arch_init_vcpu(CPUState *cs)
         }
         c = &cpuid_data.entries[cpuid_i++];
 
-        c->function = i;
-        c->flags = 0;
-        cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
+        switch (i) {
+        case 0x8000001d:
+            /* Query for all AMD cache information leaves */
+            for (j = 0; ; j++) {
+                c->function = i;
+                c->flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+                c->index = j;
+                cpu_x86_cpuid(env, i, j, &c->eax, &c->ebx, &c->ecx, &c->edx);
+
+                if (c->eax == 0) {
+                    break;
+                }
+                if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
+                    fprintf(stderr, "cpuid_data is full, no space for "
+                            "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
+                    abort();
+                }
+                c = &cpuid_data.entries[cpuid_i++];
+            }
+            break;
+        default:
+            c->function = i;
+            c->flags = 0;
+            cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
+            break;
+        }
     }
 
     /* Call Centaur's CPUID instructions they are supported. */
-- 
1.8.3.1

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Eduardo Habkost]

Hi,

Sorry for not reviewing the previous versions of this series (and
making it miss soft freeze).


On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> From: Stanislav Lanci <pixo@polepetko.eu>
> 
> Add information for cpuid 0x8000001D leaf. Populate cache topology information
> for different cache types(Data Cache, Instruction Cache, L2 and L3) supported
> by 0x8000001D leaf. Please refer Processor Programming Reference (PPR) for AMD
> Family 17h Model for more details.
> 
> Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  target/i386/kvm.c | 29 ++++++++++++++++++++++---
>  2 files changed, 91 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 42dd381..5fdbedd 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -118,6 +118,7 @@
>  #define L1I_LINE_SIZE         64
>  #define L1I_ASSOCIATIVITY      8
>  #define L1I_SETS              64
> +#define L1I_SETS_AMD         256
>  #define L1I_PARTITIONS         1
>  /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 32KiB */
>  #define L1I_DESCRIPTOR CPUID_2_L1I_32KB_8WAY_64B
> @@ -129,7 +130,9 @@
>  /* Level 2 unified cache: */
>  #define L2_LINE_SIZE          64
>  #define L2_ASSOCIATIVITY      16
> +#define L2_ASSOCIATIVITY_AMD   8
>  #define L2_SETS             4096
> +#define L2_SETS_AMD         1024
>  #define L2_PARTITIONS          1
>  /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 4MiB */
>  /*FIXME: CPUID leaf 2 descriptor is inconsistent with CPUID leaf 4 */
> @@ -146,6 +149,7 @@
>  #define L3_N_LINE_SIZE         64
>  #define L3_N_ASSOCIATIVITY     16
>  #define L3_N_SETS           16384
> +#define L3_N_SETS_AMD        8192
>  #define L3_N_PARTITIONS         1
>  #define L3_N_DESCRIPTOR CPUID_2_L3_16MB_16WAY_64B
>  #define L3_N_LINES_PER_TAG      1

I wouldn't like to add even more inconsistencies between
different CPUID leaves.

If you really wish to have different defaults on AMD and Intel,
let's either hide Intel-specific CPUID leaves when using AMD
values, or make all of them agree (and choose the defaults based
on CPU model or vendor id).

-- 
Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Eduardo Habkost]

On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> From: Stanislav Lanci <pixo@polepetko.eu>
> 
> Add information for cpuid 0x8000001D leaf. Populate cache topology information
> for different cache types(Data Cache, Instruction Cache, L2 and L3) supported
> by 0x8000001D leaf. Please refer Processor Programming Reference (PPR) for AMD
> Family 17h Model for more details.
> 
> Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> Signed-off-by: Babu Moger <babu.moger@amd.com>

The new CPUID leaves don't seem to match the existing AMD cache information
leaves.  Is this intentional?  Why?

Details below:

> ---
>  target/i386/cpu.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  target/i386/kvm.c | 29 ++++++++++++++++++++++---
>  2 files changed, 91 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 42dd381..5fdbedd 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
[...]
> @@ -3590,6 +3594,67 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              *edx = 0;
>          }
>          break;
> +    case 0x8000001D: /* AMD TOPOEXT cache info */
> +        switch (count) {

Copying macro definitions here, for reference:

> /* L1 data cache: */
> #define L1D_LINE_SIZE         64
> #define L1D_ASSOCIATIVITY      8
> #define L1D_SETS              64
> #define L1D_PARTITIONS         1
> /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 32KiB */
> #define L1D_DESCRIPTOR CPUID_2_L1D_32KB_8WAY_64B
> /*FIXME: CPUID leaf 0x80000005 is inconsistent with leaves 2 & 4 */
> #define L1D_LINES_PER_TAG      1
> #define L1D_SIZE_KB_AMD       64
> #define L1D_ASSOCIATIVITY_AMD  2

So, we already have:

CPUID[2]: 32KB 8-way cache, 64-byte lines
CPUID[4]: 8-way cache, 64-byte lines, 64 sets, 1 partition (32 KB)
CPUID[0x80000005]: 64 KB 2-way cache, 1 line per tag


> +        case 0: /* L1 dcache info */
> +            *eax |= TYPE_DCACHE | \
> +                    CACHE_LEVEL(1) | \
> +                    CACHE_SELF_INIT_LEVEL | \
> +                    ((cs->nr_threads - 1) << 14);
> +            *ebx = (L1D_LINE_SIZE - 1) | \
> +                   ((L1D_PARTITIONS - 1) << 12) | \
> +                   ((L1D_ASSOCIATIVITY - 1) << 22);
> +            *ecx = L1D_SETS - 1;
> +            *edx = 0;
> +            break;

This adds:
CPUID[0x8000001D]: 8-way cache, 64-byte lines, 64 sets, 1 partition (32 KiB)

This agrees with CPUID[2] and CPUID[4] (Intel leaves, reserved on AMD), but not
with CPUID[0x80000005].

>  
>  /* L1 instruction cache: */
>  #define L1I_LINE_SIZE         64
>  #define L1I_ASSOCIATIVITY      8
>  #define L1I_SETS              64
> +#define L1I_SETS_AMD         256
>  #define L1I_PARTITIONS         1
>  /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 32KiB */
>  #define L1I_DESCRIPTOR CPUID_2_L1I_32KB_8WAY_64B
>  /*FIXME: CPUID leaf 0x80000005 is inconsistent with leaves 2 & 4 */
>  #define L1I_LINES_PER_TAG      1
>  #define L1I_SIZE_KB_AMD       64
>  #define L1I_ASSOCIATIVITY_AMD  2

Currently we have:

CPUID[2]: 32KiB 8-way cache, 64-byte lines
CPUID[4]: 8-way cache, 64-byte lines, 64 sets, 1 partition (32 KiB)
CPUID[0x80000005]: 64 KiB 2-way cache, 1 line per tag


>  
> +        case 1: /* L1 icache info */
> +            *eax |= TYPE_ICACHE | \
> +                    CACHE_LEVEL(1) | \
> +                    CACHE_SELF_INIT_LEVEL | \
> +                    ((cs->nr_threads - 1) << 14);
> +            *ebx = (L1I_LINE_SIZE - 1) | \
> +                   ((L1I_PARTITIONS - 1) << 12) | \
> +                   ((L1I_ASSOCIATIVITY_AMD - 1) << 22);
> +            *ecx = L1I_SETS_AMD - 1;
> +            *edx = 0;
> +            break;

This adds:
CPUID[0x8000001D]: 2-way cache, 64-byte lines, 256 sets, 1 partition (32 KiB)

This doesn't match any of the existing leaves.


>  /* Level 2 unified cache: */
>  #define L2_LINE_SIZE          64
>  #define L2_ASSOCIATIVITY      16
> +#define L2_ASSOCIATIVITY_AMD   8
>  #define L2_SETS             4096
> +#define L2_SETS_AMD         1024
>  #define L2_PARTITIONS          1
>  /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 4MiB */
>  /*FIXME: CPUID leaf 2 descriptor is inconsistent with CPUID leaf 4 */
>  #define L2_DESCRIPTOR CPUID_2_L2_2MB_8WAY_64B
>  /*FIXME: CPUID leaf 0x80000006 is inconsistent with leaves 2 & 4 */
>  #define L2_LINES_PER_TAG       1
>  #define L2_SIZE_KB_AMD       512

Currently we have:
CPUID[2]: 4MiB 8-way cache, 64-byte lines
CPUID[4]: 64-byte lines, 16-way, 1 partition, 4096 sets (4 MiB)
CPUID[0x80000006]: 512 KiB, 16-way cache, 1 line per tag

>  
> +        case 2: /* L2 cache info */
> +            *eax |= TYPE_UNIFIED | \
> +                    CACHE_LEVEL(2) | \
> +                    CACHE_SELF_INIT_LEVEL | \
> +                    ((cs->nr_threads - 1) << 14);
> +            *ebx = (L2_LINE_SIZE - 1) | \
> +                   ((L2_PARTITIONS - 1) << 12) | \
> +                   ((L2_ASSOCIATIVITY_AMD - 1) << 22);
> +            *ecx = L2_SETS_AMD - 1;
> +            *edx = CACHE_INCLUSIVE;
> +            break;

This adds:
CPUID[0x8000001D]: 64-byte lines, 8-way, 1 partition, 1024 sets (512 KiB).

This doesn't match any of the existing leaves.


>  /* Level 3 unified cache: */
>  #define L3_N_LINE_SIZE         64
>  #define L3_N_ASSOCIATIVITY     16
>  #define L3_N_SETS           16384
> +#define L3_N_SETS_AMD        8192
>  #define L3_N_PARTITIONS         1
>  #define L3_N_DESCRIPTOR CPUID_2_L3_16MB_16WAY_64B
>  #define L3_N_LINES_PER_TAG      1
>  #define L3_N_SIZE_KB_AMD    16384
>  

Currently we have:
CPUID[2]: 16MiB 16-way cache, 64-byte lines
CPUID[4]: 64-byte lines, 16-way, 16384 sets, 1 partition (16 MiB)
CPUID[0x80000006]: 16 MiB cache, 16-way, 1 line per tag


> +        case 3: /* L3 cache info */
> +            if (cpu->enable_l3_cache) {
> +                *eax |= TYPE_UNIFIED | \
> +                        CACHE_LEVEL(3) | \
> +                        CACHE_SELF_INIT_LEVEL | \
> +                        ((cs->nr_cores * cs->nr_threads - 1) << 14);
> +                *ebx = (L3_N_LINE_SIZE - 1) | \
> +                       ((L3_N_PARTITIONS - 1) << 12) | \
> +                       ((L3_N_ASSOCIATIVITY - 1) << 22);
> +                *ecx = L3_N_SETS_AMD - 1;
> +                *edx = CACHE_NO_INVD_SHARING;

This adds:
CPUID[0x8000001D]: 64-byte lines, 16-way, 1 partition, 8192 sets (8 MiB)

This doesn't match any of the existing leaves.


> +            } else {
> +                *eax = 0;
> +                *ebx = 0;
> +                *ecx = 0;
> +                *edx = 0;
> +            }
> +            break;
> +        default: /* end of info */
> +            *eax = 0;
> +            *ebx = 0;
> +            *ecx = 0;
> +            *edx = 0;
> +            break;
> +        }
> +        break;
>      case 0xC0000000:
>          *eax = env->cpuid_xlevel2;
>          *ebx = 0;
[...]

-- 
Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Moger, Babu]

Hi Eduardo, Thanks for the comments. Please see the response inline.

> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Friday, March 16, 2018 1:00 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> Processor Cache Information
> 
> On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> > From: Stanislav Lanci <pixo@polepetko.eu>
> >
> > Add information for cpuid 0x8000001D leaf. Populate cache topology
> information
> > for different cache types(Data Cache, Instruction Cache, L2 and L3)
> supported
> > by 0x8000001D leaf. Please refer Processor Programming Reference (PPR)
> for AMD
> > Family 17h Model for more details.
> >
> > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> 
> The new CPUID leaves don't seem to match the existing AMD cache
> information
> leaves.  Is this intentional?  Why?

It is not intentional. These values are from older family of processors. These values have changed from Family 14  or later.
The latest one is Family 17. You can see the differences here.
 https://support.amd.com/TechDocs/41131.pdf
https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-7Fh_BKDG.pdf
https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-0Fh.pdf

Some of these are bugs in our code. For some we need to add checks for the family and correct these values.
You understand the code much better than me. Correct me if I missed something. 

Note that older family of processors don't support topology extensions.  

> 
> Details below:
> 
> > ---
> >  target/i386/cpu.c | 65
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  target/i386/kvm.c | 29 ++++++++++++++++++++++---
> >  2 files changed, 91 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 42dd381..5fdbedd 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> [...]
> > @@ -3590,6 +3594,67 @@ void cpu_x86_cpuid(CPUX86State *env,
> uint32_t index, uint32_t count,
> >              *edx = 0;
> >          }
> >          break;
> > +    case 0x8000001D: /* AMD TOPOEXT cache info */
> > +        switch (count) {
> 
> Copying macro definitions here, for reference:
> 
> > /* L1 data cache: */
> > #define L1D_LINE_SIZE         64
> > #define L1D_ASSOCIATIVITY      8
> > #define L1D_SETS              64
> > #define L1D_PARTITIONS         1
> > /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 32KiB */
> > #define L1D_DESCRIPTOR CPUID_2_L1D_32KB_8WAY_64B
> > /*FIXME: CPUID leaf 0x80000005 is inconsistent with leaves 2 & 4 */
> > #define L1D_LINES_PER_TAG      1
> > #define L1D_SIZE_KB_AMD       64
> > #define L1D_ASSOCIATIVITY_AMD  2
> 
> So, we already have:
> 
> CPUID[2]: 32KB 8-way cache, 64-byte lines
> CPUID[4]: 8-way cache, 64-byte lines, 64 sets, 1 partition (32 KB)
> CPUID[0x80000005]: 64 KB 2-way cache, 1 line per tag

64 KiB, 2-way are for the older family products. 
Newer one should be 8-way, 32KiB. Will need to add checks here.

> 
> 
> > +        case 0: /* L1 dcache info */
> > +            *eax |= TYPE_DCACHE | \
> > +                    CACHE_LEVEL(1) | \
> > +                    CACHE_SELF_INIT_LEVEL | \
> > +                    ((cs->nr_threads - 1) << 14);
> > +            *ebx = (L1D_LINE_SIZE - 1) | \
> > +                   ((L1D_PARTITIONS - 1) << 12) | \
> > +                   ((L1D_ASSOCIATIVITY - 1) << 22);
> > +            *ecx = L1D_SETS - 1;
> > +            *edx = 0;
> > +            break;
> 
> This adds:
> CPUID[0x8000001D]: 8-way cache, 64-byte lines, 64 sets, 1 partition (32 KiB)

Should match after the above new check.

> 
> This agrees with CPUID[2] and CPUID[4] (Intel leaves, reserved on AMD), but
> not
> with CPUID[0x80000005].
> 
> >
> >  /* L1 instruction cache: */
> >  #define L1I_LINE_SIZE         64
> >  #define L1I_ASSOCIATIVITY      8
> >  #define L1I_SETS              64
> > +#define L1I_SETS_AMD         256
> >  #define L1I_PARTITIONS         1
> >  /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 32KiB */
> >  #define L1I_DESCRIPTOR CPUID_2_L1I_32KB_8WAY_64B
> >  /*FIXME: CPUID leaf 0x80000005 is inconsistent with leaves 2 & 4 */
> >  #define L1I_LINES_PER_TAG      1
> >  #define L1I_SIZE_KB_AMD       64
> >  #define L1I_ASSOCIATIVITY_AMD  2
> 
> Currently we have:
> 
> CPUID[2]: 32KiB 8-way cache, 64-byte lines
> CPUID[4]: 8-way cache, 64-byte lines, 64 sets, 1 partition (32 KiB)
> CPUID[0x80000005]: 64 KiB 2-way cache, 1 line per tag

This is for the older family(64 KiB 2-way). Newer one is 64 KiB 4-way.   We need to add check here.
> 
> 
> >
> > +        case 1: /* L1 icache info */
> > +            *eax |= TYPE_ICACHE | \
> > +                    CACHE_LEVEL(1) | \
> > +                    CACHE_SELF_INIT_LEVEL | \
> > +                    ((cs->nr_threads - 1) << 14);
> > +            *ebx = (L1I_LINE_SIZE - 1) | \
> > +                   ((L1I_PARTITIONS - 1) << 12) | \
> > +                   ((L1I_ASSOCIATIVITY_AMD - 1) << 22);
> > +            *ecx = L1I_SETS_AMD - 1;
> > +            *edx = 0;
> > +            break;
> 
> This adds:
> CPUID[0x8000001D]: 2-way cache, 64-byte lines, 256 sets, 1 partition (32 KiB)
> 
> This doesn't match any of the existing leaves.

This is bug in my code. This should be 4-way.  Should match after the fix.
> 
> 
> >  /* Level 2 unified cache: */
> >  #define L2_LINE_SIZE          64
> >  #define L2_ASSOCIATIVITY      16
> > +#define L2_ASSOCIATIVITY_AMD   8
> >  #define L2_SETS             4096
> > +#define L2_SETS_AMD         1024
> >  #define L2_PARTITIONS          1
> >  /* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 4MiB */
> >  /*FIXME: CPUID leaf 2 descriptor is inconsistent with CPUID leaf 4 */
> >  #define L2_DESCRIPTOR CPUID_2_L2_2MB_8WAY_64B
> >  /*FIXME: CPUID leaf 0x80000006 is inconsistent with leaves 2 & 4 */
> >  #define L2_LINES_PER_TAG       1
> >  #define L2_SIZE_KB_AMD       512
> 
> Currently we have:
> CPUID[2]: 4MiB 8-way cache, 64-byte lines
> CPUID[4]: 64-byte lines, 16-way, 1 partition, 4096 sets (4 MiB)
> CPUID[0x80000006]: 512 KiB, 16-way cache, 1 line per tag

This should have been 8-way. This is a bug. Will fix.
This should have been (AMD_ENC_ASSOC(L2_ASSOCIATIVITY_AMD) << 12)


> 
> >
> > +        case 2: /* L2 cache info */
> > +            *eax |= TYPE_UNIFIED | \
> > +                    CACHE_LEVEL(2) | \
> > +                    CACHE_SELF_INIT_LEVEL | \
> > +                    ((cs->nr_threads - 1) << 14);
> > +            *ebx = (L2_LINE_SIZE - 1) | \
> > +                   ((L2_PARTITIONS - 1) << 12) | \
> > +                   ((L2_ASSOCIATIVITY_AMD - 1) << 22);
> > +            *ecx = L2_SETS_AMD - 1;
> > +            *edx = CACHE_INCLUSIVE;
> > +            break;
> 
> This adds:
> CPUID[0x8000001D]: 64-byte lines, 8-way, 1 partition, 1024 sets (512 KiB).
> 
> This doesn't match any of the existing leaves.

Should match after above fix.
> 
> 
> >  /* Level 3 unified cache: */
> >  #define L3_N_LINE_SIZE         64
> >  #define L3_N_ASSOCIATIVITY     16
> >  #define L3_N_SETS           16384
> > +#define L3_N_SETS_AMD        8192
> >  #define L3_N_PARTITIONS         1
> >  #define L3_N_DESCRIPTOR CPUID_2_L3_16MB_16WAY_64B
> >  #define L3_N_LINES_PER_TAG      1
> >  #define L3_N_SIZE_KB_AMD    16384
> >
> 
> Currently we have:
> CPUID[2]: 16MiB 16-way cache, 64-byte lines
> CPUID[4]: 64-byte lines, 16-way, 16384 sets, 1 partition (16 MiB)
> CPUID[0x80000006]: 16 MiB cache, 16-way, 1 line per tag
> 
> 
> > +        case 3: /* L3 cache info */
> > +            if (cpu->enable_l3_cache) {
> > +                *eax |= TYPE_UNIFIED | \
> > +                        CACHE_LEVEL(3) | \
> > +                        CACHE_SELF_INIT_LEVEL | \
> > +                        ((cs->nr_cores * cs->nr_threads - 1) << 14);
> > +                *ebx = (L3_N_LINE_SIZE - 1) | \
> > +                       ((L3_N_PARTITIONS - 1) << 12) | \
> > +                       ((L3_N_ASSOCIATIVITY - 1) << 22);
> > +                *ecx = L3_N_SETS_AMD - 1;
> > +                *edx = CACHE_NO_INVD_SHARING;
> 
> This adds:
> CPUID[0x8000001D]: 64-byte lines, 16-way, 1 partition, 8192 sets (8 MiB)

I am not very clear about this. It appears the number of sets should be 16384.
Let me talk to someone and confirm.

> 
> This doesn't match any of the existing leaves.
> 
> 
> > +            } else {
> > +                *eax = 0;
> > +                *ebx = 0;
> > +                *ecx = 0;
> > +                *edx = 0;
> > +            }
> > +            break;
> > +        default: /* end of info */
> > +            *eax = 0;
> > +            *ebx = 0;
> > +            *ecx = 0;
> > +            *edx = 0;
> > +            break;
> > +        }
> > +        break;
> >      case 0xC0000000:
> >          *eax = env->cpuid_xlevel2;
> >          *ebx = 0;
> [...]
> 
> --
> Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Eduardo Habkost]

On Tue, Mar 20, 2018 at 05:25:52PM +0000, Moger, Babu wrote:
> Hi Eduardo, Thanks for the comments. Please see the response inline.
> 
> > -----Original Message-----
> > From: Eduardo Habkost <ehabkost@redhat.com>
> > Sent: Friday, March 16, 2018 1:00 PM
> > To: Moger, Babu <Babu.Moger@amd.com>
> > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > devel@nongnu.org
> > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > Processor Cache Information
> > 
> > On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> > > From: Stanislav Lanci <pixo@polepetko.eu>
> > >
> > > Add information for cpuid 0x8000001D leaf. Populate cache topology
> > information
> > > for different cache types(Data Cache, Instruction Cache, L2 and L3)
> > supported
> > > by 0x8000001D leaf. Please refer Processor Programming Reference (PPR)
> > for AMD
> > > Family 17h Model for more details.
> > >
> > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > 
> > The new CPUID leaves don't seem to match the existing AMD cache
> > information
> > leaves.  Is this intentional?  Why?
> 
> It is not intentional. These values are from older family of processors. These values have changed from Family 14  or later.
> The latest one is Family 17. You can see the differences here.
>  https://support.amd.com/TechDocs/41131.pdf
> https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-7Fh_BKDG.pdf
> https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-0Fh.pdf
> 
> Some of these are bugs in our code. For some we need to add checks for the family and correct these values.
> You understand the code much better than me. Correct me if I missed something. 
> 
> Note that older family of processors don't support topology extensions.  

If you want to make the cache size/topology look different
depending on the CPU model/options, this would require more work,
but it would be an interesting feature.

The "i386: Helpers to encode cache information consistently"
patch I sent last week might be a useful starting point for that.

If you plan to implement that, please keep in mind that existing
CPUID cache info needs to be kept on previous machine-types (this
is implemented by adding QOM properties that can be used to
enable the old behavior, and by setting them at
MachineClass::compat_props).

-- 
Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Tuesday, March 20, 2018 12:54 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> Processor Cache Information
> 
> On Tue, Mar 20, 2018 at 05:25:52PM +0000, Moger, Babu wrote:
> > Hi Eduardo, Thanks for the comments. Please see the response inline.
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > Sent: Friday, March 16, 2018 1:00 PM
> > > To: Moger, Babu <Babu.Moger@amd.com>
> > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > devel@nongnu.org
> > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > Processor Cache Information
> > >
> > > On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> > > > From: Stanislav Lanci <pixo@polepetko.eu>
> > > >
> > > > Add information for cpuid 0x8000001D leaf. Populate cache topology
> > > information
> > > > for different cache types(Data Cache, Instruction Cache, L2 and L3)
> > > supported
> > > > by 0x8000001D leaf. Please refer Processor Programming Reference
> (PPR)
> > > for AMD
> > > > Family 17h Model for more details.
> > > >
> > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > >
> > > The new CPUID leaves don't seem to match the existing AMD cache
> > > information
> > > leaves.  Is this intentional?  Why?
> >
> > It is not intentional. These values are from older family of processors.
> These values have changed from Family 14  or later.
> > The latest one is Family 17. You can see the differences here.
> >  https://support.amd.com/TechDocs/41131.pdf
> >
> https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-
> 7Fh_BKDG.pdf
> >
> https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-
> 0Fh.pdf
> >
> > Some of these are bugs in our code. For some we need to add checks for
> the family and correct these values.
> > You understand the code much better than me. Correct me if I missed
> something.
> >
> > Note that older family of processors don't support topology extensions.
> 
> If you want to make the cache size/topology look different
> depending on the CPU model/options, this would require more work,
> but it would be an interesting feature.
> 
> The "i386: Helpers to encode cache information consistently"
> patch I sent last week might be a useful starting point for that.

Yes. Looking at your patch.
> 
> If you plan to implement that, please keep in mind that existing
> CPUID cache info needs to be kept on previous machine-types (this
> is implemented by adding QOM properties that can be used to
> enable the old behavior, and by setting them at
> MachineClass::compat_props).

Yes. Will look into it.  This code is new to me.  Let me take a look.
Thanks

> 
> --
> Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Moger, Babu]

Hi Eduardo,

> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Tuesday, March 20, 2018 12:54 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> Processor Cache Information
> 
> On Tue, Mar 20, 2018 at 05:25:52PM +0000, Moger, Babu wrote:
> > Hi Eduardo, Thanks for the comments. Please see the response inline.
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > Sent: Friday, March 16, 2018 1:00 PM
> > > To: Moger, Babu <Babu.Moger@amd.com>
> > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > devel@nongnu.org
> > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > Processor Cache Information
> > >
> > > On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> > > > From: Stanislav Lanci <pixo@polepetko.eu>
> > > >
> > > > Add information for cpuid 0x8000001D leaf. Populate cache topology
> > > information
> > > > for different cache types(Data Cache, Instruction Cache, L2 and L3)
> > > supported
> > > > by 0x8000001D leaf. Please refer Processor Programming Reference
> (PPR)
> > > for AMD
> > > > Family 17h Model for more details.
> > > >
> > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > >
> > > The new CPUID leaves don't seem to match the existing AMD cache
> > > information
> > > leaves.  Is this intentional?  Why?
> >
> > It is not intentional. These values are from older family of processors.
> These values have changed from Family 14  or later.
> > The latest one is Family 17. You can see the differences here.
> >  https://support.amd.com/TechDocs/41131.pdf
> >
> https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-
> 7Fh_BKDG.pdf
> >
> https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-
> 0Fh.pdf
> >
> > Some of these are bugs in our code. For some we need to add checks for
> the family and correct these values.
> > You understand the code much better than me. Correct me if I missed
> something.
> >
> > Note that older family of processors don't support topology extensions.
> 
> If you want to make the cache size/topology look different
> depending on the CPU model/options, this would require more work,
> but it would be an interesting feature.
> 
> The "i386: Helpers to encode cache information consistently"
> patch I sent last week might be a useful starting point for that.
> 
> If you plan to implement that, please keep in mind that existing
> CPUID cache info needs to be kept on previous machine-types (this
> is implemented by adding QOM properties that can be used to
> enable the old behavior, and by setting them at
> MachineClass::compat_props).

Wanted to get some confirmation what you meant by setting MachineClass::compat_props.
Here is the patch I created to add new property for cpu. Now, I can use enable_topoext to display
new  change properly based on family.  Is that what you meant ?  


diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ffee841..d1ee053 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -369,6 +369,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
     HW_COMPAT_2_7 \
     {\
         .driver   = TYPE_X86_CPU,\
+        .property = "topoext",\
+        .value    = "off",\
+    },\
+    {\
+        .driver   = TYPE_X86_CPU,\
         .property = "l3-cache",\
         .value    = "off",\
     },\
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 84d64de..557a2d6 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5080,6 +5080,7 @@ static Property x86_cpu_properties[] = {
                      false),
     DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU, vmware_cpuid_freq, true),
     DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
+    DEFINE_PROP_BOOL("topoext", X86CPU, enable_topoext, true),

     /*
      * From "Requirements for Implementing the Microsoft
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 2e2bab5..3d3caa1 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1333,6 +1333,11 @@ struct X86CPU {
      */
     bool enable_l3_cache;

+    /* Compatibility bits for old machine types.
+     * If true present the new cache topology information
+     */
+    bool enable_topoext;
+
     /* Compatibility bits for old machine types: */
     bool enable_cpuid_0xb;



> 
> --
> Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Eduardo Habkost]

On Wed, Mar 21, 2018 at 03:58:41PM +0000, Moger, Babu wrote:
> Hi Eduardo,
> 
> > -----Original Message-----
> > From: Eduardo Habkost <ehabkost@redhat.com>
> > Sent: Tuesday, March 20, 2018 12:54 PM
> > To: Moger, Babu <Babu.Moger@amd.com>
> > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > devel@nongnu.org
> > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > Processor Cache Information
> > 
> > On Tue, Mar 20, 2018 at 05:25:52PM +0000, Moger, Babu wrote:
> > > Hi Eduardo, Thanks for the comments. Please see the response inline.
> > >
> > > > -----Original Message-----
> > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > Sent: Friday, March 16, 2018 1:00 PM
> > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > > devel@nongnu.org
> > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > > Processor Cache Information
> > > >
> > > > On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> > > > > From: Stanislav Lanci <pixo@polepetko.eu>
> > > > >
> > > > > Add information for cpuid 0x8000001D leaf. Populate cache topology
> > > > information
> > > > > for different cache types(Data Cache, Instruction Cache, L2 and L3)
> > > > supported
> > > > > by 0x8000001D leaf. Please refer Processor Programming Reference
> > (PPR)
> > > > for AMD
> > > > > Family 17h Model for more details.
> > > > >
> > > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> > > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > >
> > > > The new CPUID leaves don't seem to match the existing AMD cache
> > > > information
> > > > leaves.  Is this intentional?  Why?
> > >
> > > It is not intentional. These values are from older family of processors.
> > These values have changed from Family 14  or later.
> > > The latest one is Family 17. You can see the differences here.
> > >  https://support.amd.com/TechDocs/41131.pdf
> > >
> > https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-
> > 7Fh_BKDG.pdf
> > >
> > https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-
> > 0Fh.pdf
> > >
> > > Some of these are bugs in our code. For some we need to add checks for
> > the family and correct these values.
> > > You understand the code much better than me. Correct me if I missed
> > something.
> > >
> > > Note that older family of processors don't support topology extensions.
> > 
> > If you want to make the cache size/topology look different
> > depending on the CPU model/options, this would require more work,
> > but it would be an interesting feature.
> > 
> > The "i386: Helpers to encode cache information consistently"
> > patch I sent last week might be a useful starting point for that.
> > 
> > If you plan to implement that, please keep in mind that existing
> > CPUID cache info needs to be kept on previous machine-types (this
> > is implemented by adding QOM properties that can be used to
> > enable the old behavior, and by setting them at
> > MachineClass::compat_props).
> 
> Wanted to get some confirmation what you meant by setting MachineClass::compat_props.
> Here is the patch I created to add new property for cpu. Now, I can use enable_topoext to display
> new  change properly based on family.  Is that what you meant ?  

If the only change you introduce in the defaults is enabling
topoext but keeping the same default cache sizes, the example
following would make sense (but see comments below about
implementation details).

But if you also want to change the default cache size, you will
also need properties that control the cache size.

(In theory, you could make the "topoext" property control the
cache size too, but I don't see why the cache size should be
coupled to topoext)

> 
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ffee841..d1ee053 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -369,6 +369,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>      HW_COMPAT_2_7 \
>      {\
>          .driver   = TYPE_X86_CPU,\
> +        .property = "topoext",\
> +        .value    = "off",\

This makes sense if you want to change a CPU model to enable
topoext by default.  But I suggest setting it for
("EPYC" "-" TYPE_X86_CPU) only, not TYPE_X86_CPU, as EPYC is the
only CPU model affected by patch 4/5.

However, the property registration can be simpler:

> +    },\
> +    {\
> +        .driver   = TYPE_X86_CPU,\
>          .property = "l3-cache",\
>          .value    = "off",\
>      },\
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 84d64de..557a2d6 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5080,6 +5080,7 @@ static Property x86_cpu_properties[] = {
>                       false),
>      DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU, vmware_cpuid_freq, true),
>      DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
> +    DEFINE_PROP_BOOL("topoext", X86CPU, enable_topoext, true),

You don't need to manually add a X86CPU field + property if you
are just controlling a CPUID feature flag.  In this case, you can
just add "topoext" to
feature_word_info[FEAT_8000_0001_ECX].feat_names[22], and
a "topoext" QOM property will be available automatically.

> 
>      /*
>       * From "Requirements for Implementing the Microsoft
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 2e2bab5..3d3caa1 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1333,6 +1333,11 @@ struct X86CPU {
>       */
>      bool enable_l3_cache;
> 
> +    /* Compatibility bits for old machine types.
> +     * If true present the new cache topology information
> +     */
> +    bool enable_topoext;
> +
>      /* Compatibility bits for old machine types: */
>      bool enable_cpuid_0xb;
> 
> 
> 
> > 
> > --
> > Eduardo

-- 
Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Kash Pande]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

On 2018-03-21 01:09 PM, Eduardo Habkost wrote:
> This makes sense if you want to change a CPU model to enable
> topoext by default.  But I suggest setting it for
> ("EPYC" "-" TYPE_X86_CPU) only, not TYPE_X86_CPU, as EPYC is the
> only CPU model affected by patch 4/5.


AIUI, all 17h CPUs have TOPOEXT and SMT support. I've been in contact
with Babu to have the patches extended to more general use case.



Kash



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Wednesday, March 21, 2018 12:10 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> Processor Cache Information
> 
> On Wed, Mar 21, 2018 at 03:58:41PM +0000, Moger, Babu wrote:
> > Hi Eduardo,
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > Sent: Tuesday, March 20, 2018 12:54 PM
> > > To: Moger, Babu <Babu.Moger@amd.com>
> > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > devel@nongnu.org
> > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > Processor Cache Information
> > >
> > > On Tue, Mar 20, 2018 at 05:25:52PM +0000, Moger, Babu wrote:
> > > > Hi Eduardo, Thanks for the comments. Please see the response inline.
> > > >
> > > > > -----Original Message-----
> > > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > > Sent: Friday, March 16, 2018 1:00 PM
> > > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org;
> kash@tripleback.net;
> > > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > > > devel@nongnu.org
> > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > > > Processor Cache Information
> > > > >
> > > > > On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> > > > > > From: Stanislav Lanci <pixo@polepetko.eu>
> > > > > >
> > > > > > Add information for cpuid 0x8000001D leaf. Populate cache topology
> > > > > information
> > > > > > for different cache types(Data Cache, Instruction Cache, L2 and L3)
> > > > > supported
> > > > > > by 0x8000001D leaf. Please refer Processor Programming Reference
> > > (PPR)
> > > > > for AMD
> > > > > > Family 17h Model for more details.
> > > > > >
> > > > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > > >
> > > > > The new CPUID leaves don't seem to match the existing AMD cache
> > > > > information
> > > > > leaves.  Is this intentional?  Why?
> > > >
> > > > It is not intentional. These values are from older family of processors.
> > > These values have changed from Family 14  or later.
> > > > The latest one is Family 17. You can see the differences here.
> > > >  https://support.amd.com/TechDocs/41131.pdf
> > > >
> > >
> https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-
> > > 7Fh_BKDG.pdf
> > > >
> > >
> https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-
> > > 0Fh.pdf
> > > >
> > > > Some of these are bugs in our code. For some we need to add checks
> for
> > > the family and correct these values.
> > > > You understand the code much better than me. Correct me if I missed
> > > something.
> > > >
> > > > Note that older family of processors don't support topology extensions.
> > >
> > > If you want to make the cache size/topology look different
> > > depending on the CPU model/options, this would require more work,
> > > but it would be an interesting feature.
> > >
> > > The "i386: Helpers to encode cache information consistently"
> > > patch I sent last week might be a useful starting point for that.
> > >
> > > If you plan to implement that, please keep in mind that existing
> > > CPUID cache info needs to be kept on previous machine-types (this
> > > is implemented by adding QOM properties that can be used to
> > > enable the old behavior, and by setting them at
> > > MachineClass::compat_props).
> >
> > Wanted to get some confirmation what you meant by setting
> MachineClass::compat_props.
> > Here is the patch I created to add new property for cpu. Now, I can use
> enable_topoext to display
> > new  change properly based on family.  Is that what you meant ?
> 
> If the only change you introduce in the defaults is enabling
> topoext but keeping the same default cache sizes, the example
> following would make sense (but see comments below about
> implementation details).
> 
> But if you also want to change the default cache size, you will
> also need properties that control the cache size.

Yes. We will need cpu family information.  Let me see if that information is
 already available or we need to add that as well.

> 
> (In theory, you could make the "topoext" property control the
> cache size too, but I don't see why the cache size should be
> coupled to topoext)
> 
> >
> >
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index ffee841..d1ee053 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -369,6 +369,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
> uint64_t *);
> >      HW_COMPAT_2_7 \
> >      {\
> >          .driver   = TYPE_X86_CPU,\
> > +        .property = "topoext",\
> > +        .value    = "off",\
> 
> This makes sense if you want to change a CPU model to enable
> topoext by default.  But I suggest setting it for
> ("EPYC" "-" TYPE_X86_CPU) only, not TYPE_X86_CPU, as EPYC is the
> only CPU model affected by patch 4/5.

As Kash already mentioned, right now we have two family's  EPYC and EPYC-IBPB supporting 
this new cache sizes/info. So, ("EPYC" "-" TYPE_X86_CPU) may not work. Will keep it as old one.

> 
> However, the property registration can be simpler:
> 
> > +    },\
> > +    {\
> > +        .driver   = TYPE_X86_CPU,\
> >          .property = "l3-cache",\
> >          .value    = "off",\
> >      },\
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 84d64de..557a2d6 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -5080,6 +5080,7 @@ static Property x86_cpu_properties[] = {
> >                       false),
> >      DEFINE_PROP_BOOL("vmware-cpuid-freq", X86CPU,
> vmware_cpuid_freq, true),
> >      DEFINE_PROP_BOOL("tcg-cpuid", X86CPU, expose_tcg, true),
> > +    DEFINE_PROP_BOOL("topoext", X86CPU, enable_topoext, true),
> 
> You don't need to manually add a X86CPU field + property if you
> are just controlling a CPUID feature flag.  In this case, you can
> just add "topoext" to
> feature_word_info[FEAT_8000_0001_ECX].feat_names[22], and
> a "topoext" QOM property will be available automatically.

Actually "topoext" feature is already there as cupid feature. I should have not used topoext
to control the cache sizes. I will come up with a new name. 
 

> 
> >
> >      /*
> >       * From "Requirements for Implementing the Microsoft
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 2e2bab5..3d3caa1 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -1333,6 +1333,11 @@ struct X86CPU {
> >       */
> >      bool enable_l3_cache;
> >
> > +    /* Compatibility bits for old machine types.
> > +     * If true present the new cache topology information
> > +     */
> > +    bool enable_topoext;
> > +
> >      /* Compatibility bits for old machine types: */
> >      bool enable_cpuid_0xb;
> >
> >
> >
> > >
> > > --
> > > Eduardo
> 
> --
> Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Eduardo Habkost]

On Wed, Mar 21, 2018 at 05:47:28PM +0000, Moger, Babu wrote:
> 
> 
> > -----Original Message-----
> > From: Eduardo Habkost <ehabkost@redhat.com>
> > Sent: Wednesday, March 21, 2018 12:10 PM
> > To: Moger, Babu <Babu.Moger@amd.com>
> > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > devel@nongnu.org
> > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > Processor Cache Information
> > 
> > On Wed, Mar 21, 2018 at 03:58:41PM +0000, Moger, Babu wrote:
> > > Hi Eduardo,
> > >
> > > > -----Original Message-----
> > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > Sent: Tuesday, March 20, 2018 12:54 PM
> > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > > devel@nongnu.org
> > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > > Processor Cache Information
> > > >
> > > > On Tue, Mar 20, 2018 at 05:25:52PM +0000, Moger, Babu wrote:
> > > > > Hi Eduardo, Thanks for the comments. Please see the response inline.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > Sent: Friday, March 16, 2018 1:00 PM
> > > > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org;
> > kash@tripleback.net;
> > > > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > > > > devel@nongnu.org
> > > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > > > > Processor Cache Information
> > > > > >
> > > > > > On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> > > > > > > From: Stanislav Lanci <pixo@polepetko.eu>
> > > > > > >
> > > > > > > Add information for cpuid 0x8000001D leaf. Populate cache topology
> > > > > > information
> > > > > > > for different cache types(Data Cache, Instruction Cache, L2 and L3)
> > > > > > supported
> > > > > > > by 0x8000001D leaf. Please refer Processor Programming Reference
> > > > (PPR)
> > > > > > for AMD
> > > > > > > Family 17h Model for more details.
> > > > > > >
> > > > > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > > > >
> > > > > > The new CPUID leaves don't seem to match the existing AMD cache
> > > > > > information
> > > > > > leaves.  Is this intentional?  Why?
> > > > >
> > > > > It is not intentional. These values are from older family of processors.
> > > > These values have changed from Family 14  or later.
> > > > > The latest one is Family 17. You can see the differences here.
> > > > >  https://support.amd.com/TechDocs/41131.pdf
> > > > >
> > > >
> > https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-
> > > > 7Fh_BKDG.pdf
> > > > >
> > > >
> > https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-
> > > > 0Fh.pdf
> > > > >
> > > > > Some of these are bugs in our code. For some we need to add checks
> > for
> > > > the family and correct these values.
> > > > > You understand the code much better than me. Correct me if I missed
> > > > something.
> > > > >
> > > > > Note that older family of processors don't support topology extensions.
> > > >
> > > > If you want to make the cache size/topology look different
> > > > depending on the CPU model/options, this would require more work,
> > > > but it would be an interesting feature.
> > > >
> > > > The "i386: Helpers to encode cache information consistently"
> > > > patch I sent last week might be a useful starting point for that.
> > > >
> > > > If you plan to implement that, please keep in mind that existing
> > > > CPUID cache info needs to be kept on previous machine-types (this
> > > > is implemented by adding QOM properties that can be used to
> > > > enable the old behavior, and by setting them at
> > > > MachineClass::compat_props).
> > >
> > > Wanted to get some confirmation what you meant by setting
> > MachineClass::compat_props.
> > > Here is the patch I created to add new property for cpu. Now, I can use
> > enable_topoext to display
> > > new  change properly based on family.  Is that what you meant ?
> > 
> > If the only change you introduce in the defaults is enabling
> > topoext but keeping the same default cache sizes, the example
> > following would make sense (but see comments below about
> > implementation details).
> > 
> > But if you also want to change the default cache size, you will
> > also need properties that control the cache size.
> 
> Yes. We will need cpu family information.  Let me see if that information is
>  already available or we need to add that as well.

CPU family doesn't seem to be enough, if you want to keep
compatibility on older machine-type versions.  e.g.:

If you want to make EPYC expose a different cache size, you'd
just need a new field on X86CPUDefinition.  This is the easy
part.

But you still need "-machine pc-2.11 -cpu EPYC" to expose the old
cache size, for guest ABI compatibility.

This means you need an additional knob that pc-2.10 can use in
compat_props, to override the new family/model-dependent cache
size in EPYC, and force the old cache sizes to be used even with
"-cpu EPYC".

-- 
Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Wednesday, March 21, 2018 1:15 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> Processor Cache Information
> 
> On Wed, Mar 21, 2018 at 05:47:28PM +0000, Moger, Babu wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > Sent: Wednesday, March 21, 2018 12:10 PM
> > > To: Moger, Babu <Babu.Moger@amd.com>
> > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > devel@nongnu.org
> > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > Processor Cache Information
> > >
> > > On Wed, Mar 21, 2018 at 03:58:41PM +0000, Moger, Babu wrote:
> > > > Hi Eduardo,
> > > >
> > > > > -----Original Message-----
> > > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > > Sent: Tuesday, March 20, 2018 12:54 PM
> > > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org;
> kash@tripleback.net;
> > > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > > > devel@nongnu.org
> > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > > > Processor Cache Information
> > > > >
> > > > > On Tue, Mar 20, 2018 at 05:25:52PM +0000, Moger, Babu wrote:
> > > > > > Hi Eduardo, Thanks for the comments. Please see the response
> inline.
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > > Sent: Friday, March 16, 2018 1:00 PM
> > > > > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > > > > Cc: pbonzini@redhat.com; rth@twiddle.net;
> rkrcmar@redhat.com;
> > > > > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org;
> > > kash@tripleback.net;
> > > > > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>;
> qemu-
> > > > > > > devel@nongnu.org
> > > > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate
> AMD
> > > > > > > Processor Cache Information
> > > > > > >
> > > > > > > On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> > > > > > > > From: Stanislav Lanci <pixo@polepetko.eu>
> > > > > > > >
> > > > > > > > Add information for cpuid 0x8000001D leaf. Populate cache
> topology
> > > > > > > information
> > > > > > > > for different cache types(Data Cache, Instruction Cache, L2 and
> L3)
> > > > > > > supported
> > > > > > > > by 0x8000001D leaf. Please refer Processor Programming
> Reference
> > > > > (PPR)
> > > > > > > for AMD
> > > > > > > > Family 17h Model for more details.
> > > > > > > >
> > > > > > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> > > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > > > > >
> > > > > > > The new CPUID leaves don't seem to match the existing AMD
> cache
> > > > > > > information
> > > > > > > leaves.  Is this intentional?  Why?
> > > > > >
> > > > > > It is not intentional. These values are from older family of
> processors.
> > > > > These values have changed from Family 14  or later.
> > > > > > The latest one is Family 17. You can see the differences here.
> > > > > >  https://support.amd.com/TechDocs/41131.pdf
> > > > > >
> > > > >
> > >
> https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-
> > > > > 7Fh_BKDG.pdf
> > > > > >
> > > > >
> > >
> https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-
> > > > > 0Fh.pdf
> > > > > >
> > > > > > Some of these are bugs in our code. For some we need to add
> checks
> > > for
> > > > > the family and correct these values.
> > > > > > You understand the code much better than me. Correct me if I
> missed
> > > > > something.
> > > > > >
> > > > > > Note that older family of processors don't support topology
> extensions.
> > > > >
> > > > > If you want to make the cache size/topology look different
> > > > > depending on the CPU model/options, this would require more work,
> > > > > but it would be an interesting feature.
> > > > >
> > > > > The "i386: Helpers to encode cache information consistently"
> > > > > patch I sent last week might be a useful starting point for that.
> > > > >
> > > > > If you plan to implement that, please keep in mind that existing
> > > > > CPUID cache info needs to be kept on previous machine-types (this
> > > > > is implemented by adding QOM properties that can be used to
> > > > > enable the old behavior, and by setting them at
> > > > > MachineClass::compat_props).
> > > >
> > > > Wanted to get some confirmation what you meant by setting
> > > MachineClass::compat_props.
> > > > Here is the patch I created to add new property for cpu. Now, I can use
> > > enable_topoext to display
> > > > new  change properly based on family.  Is that what you meant ?
> > >
> > > If the only change you introduce in the defaults is enabling
> > > topoext but keeping the same default cache sizes, the example
> > > following would make sense (but see comments below about
> > > implementation details).
> > >
> > > But if you also want to change the default cache size, you will
> > > also need properties that control the cache size.
> >
> > Yes. We will need cpu family information.  Let me see if that information is
> >  already available or we need to add that as well.
> 
> CPU family doesn't seem to be enough, if you want to keep
> compatibility on older machine-type versions.  e.g.:
> 
> If you want to make EPYC expose a different cache size, you'd
> just need a new field on X86CPUDefinition.  This is the easy
> part.

Actually, I already have this information.  I can just decode cpuid_version information.
I may not need to add new field in new field on X86CPUDefinition.

> 
> But you still need "-machine pc-2.11 -cpu EPYC" to expose the old
> cache size, for guest ABI compatibility.
> 
> This means you need an additional knob that pc-2.10 can use in
> compat_props, to override the new family/model-dependent cache
> size in EPYC, and force the old cache sizes to be used even with
> "-cpu EPYC".

Ok.  Will add a new property legacy_cache(or something). I can use this to  override
all the new information. Will plan to send first version tomorrow.
 
> 
> --
> Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Eduardo Habkost]

On Wed, Mar 21, 2018 at 08:07:54PM +0000, Moger, Babu wrote:
> 
> > -----Original Message-----
> > From: Eduardo Habkost <ehabkost@redhat.com>
> > Sent: Wednesday, March 21, 2018 1:15 PM
> > To: Moger, Babu <Babu.Moger@amd.com>
> > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > devel@nongnu.org
> > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > Processor Cache Information
> > 
> > On Wed, Mar 21, 2018 at 05:47:28PM +0000, Moger, Babu wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > Sent: Wednesday, March 21, 2018 12:10 PM
> > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > > devel@nongnu.org
> > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > > Processor Cache Information
> > > >
> > > > On Wed, Mar 21, 2018 at 03:58:41PM +0000, Moger, Babu wrote:
> > > > > Hi Eduardo,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > Sent: Tuesday, March 20, 2018 12:54 PM
> > > > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org;
> > kash@tripleback.net;
> > > > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > > > > devel@nongnu.org
> > > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > > > > Processor Cache Information
> > > > > >
> > > > > > On Tue, Mar 20, 2018 at 05:25:52PM +0000, Moger, Babu wrote:
> > > > > > > Hi Eduardo, Thanks for the comments. Please see the response
> > inline.
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > > > Sent: Friday, March 16, 2018 1:00 PM
> > > > > > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > > > > > Cc: pbonzini@redhat.com; rth@twiddle.net;
> > rkrcmar@redhat.com;
> > > > > > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > > > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org;
> > > > kash@tripleback.net;
> > > > > > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>;
> > qemu-
> > > > > > > > devel@nongnu.org
> > > > > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate
> > AMD
> > > > > > > > Processor Cache Information
> > > > > > > >
> > > > > > > > On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger wrote:
> > > > > > > > > From: Stanislav Lanci <pixo@polepetko.eu>
> > > > > > > > >
> > > > > > > > > Add information for cpuid 0x8000001D leaf. Populate cache
> > topology
> > > > > > > > information
> > > > > > > > > for different cache types(Data Cache, Instruction Cache, L2 and
> > L3)
> > > > > > > > supported
> > > > > > > > > by 0x8000001D leaf. Please refer Processor Programming
> > Reference
> > > > > > (PPR)
> > > > > > > > for AMD
> > > > > > > > > Family 17h Model for more details.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> > > > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > > > > > >
> > > > > > > > The new CPUID leaves don't seem to match the existing AMD
> > cache
> > > > > > > > information
> > > > > > > > leaves.  Is this intentional?  Why?
> > > > > > >
> > > > > > > It is not intentional. These values are from older family of
> > processors.
> > > > > > These values have changed from Family 14  or later.
> > > > > > > The latest one is Family 17. You can see the differences here.
> > > > > > >  https://support.amd.com/TechDocs/41131.pdf
> > > > > > >
> > > > > >
> > > >
> > https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-
> > > > > > 7Fh_BKDG.pdf
> > > > > > >
> > > > > >
> > > >
> > https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-
> > > > > > 0Fh.pdf
> > > > > > >
> > > > > > > Some of these are bugs in our code. For some we need to add
> > checks
> > > > for
> > > > > > the family and correct these values.
> > > > > > > You understand the code much better than me. Correct me if I
> > missed
> > > > > > something.
> > > > > > >
> > > > > > > Note that older family of processors don't support topology
> > extensions.
> > > > > >
> > > > > > If you want to make the cache size/topology look different
> > > > > > depending on the CPU model/options, this would require more work,
> > > > > > but it would be an interesting feature.
> > > > > >
> > > > > > The "i386: Helpers to encode cache information consistently"
> > > > > > patch I sent last week might be a useful starting point for that.
> > > > > >
> > > > > > If you plan to implement that, please keep in mind that existing
> > > > > > CPUID cache info needs to be kept on previous machine-types (this
> > > > > > is implemented by adding QOM properties that can be used to
> > > > > > enable the old behavior, and by setting them at
> > > > > > MachineClass::compat_props).
> > > > >
> > > > > Wanted to get some confirmation what you meant by setting
> > > > MachineClass::compat_props.
> > > > > Here is the patch I created to add new property for cpu. Now, I can use
> > > > enable_topoext to display
> > > > > new  change properly based on family.  Is that what you meant ?
> > > >
> > > > If the only change you introduce in the defaults is enabling
> > > > topoext but keeping the same default cache sizes, the example
> > > > following would make sense (but see comments below about
> > > > implementation details).
> > > >
> > > > But if you also want to change the default cache size, you will
> > > > also need properties that control the cache size.
> > >
> > > Yes. We will need cpu family information.  Let me see if that information is
> > >  already available or we need to add that as well.
> > 
> > CPU family doesn't seem to be enough, if you want to keep
> > compatibility on older machine-type versions.  e.g.:
> > 
> > If you want to make EPYC expose a different cache size, you'd
> > just need a new field on X86CPUDefinition.  This is the easy
> > part.
> 
> Actually, I already have this information.  I can just decode cpuid_version information.
> I may not need to add new field in new field on X86CPUDefinition.

I'd prefer CPUCacheInfo* fields on X86CPUDefinition instead of
hardcoding a cpuid_version check.  This way all
family/model-specific information about CPU models will
centralized in the CPU model table.

> 
> > 
> > But you still need "-machine pc-2.11 -cpu EPYC" to expose the old
> > cache size, for guest ABI compatibility.
> > 
> > This means you need an additional knob that pc-2.10 can use in
> > compat_props, to override the new family/model-dependent cache
> > size in EPYC, and force the old cache sizes to be used even with
> > "-cpu EPYC".
> 
> Ok.  Will add a new property legacy_cache(or something). I can use this to  override
> all the new information. Will plan to send first version tomorrow.
>  
> > 
> > --
> > Eduardo

-- 
Eduardo

Re: [PATCH v4 2/5] target/i386: Populate AMD Processor Cache Information

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Wednesday, March 21, 2018 3:30 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> Processor Cache Information
> 
> On Wed, Mar 21, 2018 at 08:07:54PM +0000, Moger, Babu wrote:
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > Sent: Wednesday, March 21, 2018 1:15 PM
> > > To: Moger, Babu <Babu.Moger@amd.com>
> > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > <brijesh.singh@amd.com>; kvm@vger.kernel.org; kash@tripleback.net;
> > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > devel@nongnu.org
> > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > Processor Cache Information
> > >
> > > On Wed, Mar 21, 2018 at 05:47:28PM +0000, Moger, Babu wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > > Sent: Wednesday, March 21, 2018 12:10 PM
> > > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > > Cc: pbonzini@redhat.com; rth@twiddle.net; rkrcmar@redhat.com;
> > > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org;
> kash@tripleback.net;
> > > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>; qemu-
> > > > > devel@nongnu.org
> > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate AMD
> > > > > Processor Cache Information
> > > > >
> > > > > On Wed, Mar 21, 2018 at 03:58:41PM +0000, Moger, Babu wrote:
> > > > > > Hi Eduardo,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > > Sent: Tuesday, March 20, 2018 12:54 PM
> > > > > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > > > > Cc: pbonzini@redhat.com; rth@twiddle.net;
> rkrcmar@redhat.com;
> > > > > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh, Brijesh
> > > > > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org;
> > > kash@tripleback.net;
> > > > > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>;
> qemu-
> > > > > > > devel@nongnu.org
> > > > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386: Populate
> AMD
> > > > > > > Processor Cache Information
> > > > > > >
> > > > > > > On Tue, Mar 20, 2018 at 05:25:52PM +0000, Moger, Babu wrote:
> > > > > > > > Hi Eduardo, Thanks for the comments. Please see the response
> > > inline.
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > > > > Sent: Friday, March 16, 2018 1:00 PM
> > > > > > > > > To: Moger, Babu <Babu.Moger@amd.com>
> > > > > > > > > Cc: pbonzini@redhat.com; rth@twiddle.net;
> > > rkrcmar@redhat.com;
> > > > > > > > > Lendacky, Thomas <Thomas.Lendacky@amd.com>; Singh,
> Brijesh
> > > > > > > > > <brijesh.singh@amd.com>; kvm@vger.kernel.org;
> > > > > kash@tripleback.net;
> > > > > > > > > mtosatti@redhat.com; Hook, Gary <Gary.Hook@amd.com>;
> > > qemu-
> > > > > > > > > devel@nongnu.org
> > > > > > > > > Subject: Re: [Qemu-devel] [PATCH v4 2/5] target/i386:
> Populate
> > > AMD
> > > > > > > > > Processor Cache Information
> > > > > > > > >
> > > > > > > > > On Mon, Mar 12, 2018 at 05:00:46PM -0400, Babu Moger
> wrote:
> > > > > > > > > > From: Stanislav Lanci <pixo@polepetko.eu>
> > > > > > > > > >
> > > > > > > > > > Add information for cpuid 0x8000001D leaf. Populate cache
> > > topology
> > > > > > > > > information
> > > > > > > > > > for different cache types(Data Cache, Instruction Cache, L2
> and
> > > L3)
> > > > > > > > > supported
> > > > > > > > > > by 0x8000001D leaf. Please refer Processor Programming
> > > Reference
> > > > > > > (PPR)
> > > > > > > > > for AMD
> > > > > > > > > > Family 17h Model for more details.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Stanislav Lanci <pixo@polepetko.eu>
> > > > > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > > > > > > > >
> > > > > > > > > The new CPUID leaves don't seem to match the existing AMD
> > > cache
> > > > > > > > > information
> > > > > > > > > leaves.  Is this intentional?  Why?
> > > > > > > >
> > > > > > > > It is not intentional. These values are from older family of
> > > processors.
> > > > > > > These values have changed from Family 14  or later.
> > > > > > > > The latest one is Family 17. You can see the differences here.
> > > > > > > >  https://support.amd.com/TechDocs/41131.pdf
> > > > > > > >
> > > > > > >
> > > > >
> > >
> https://support.amd.com/TechDocs/55072_AMD_Family_15h_Models_70h-
> > > > > > > 7Fh_BKDG.pdf
> > > > > > > >
> > > > > > >
> > > > >
> > >
> https://support.amd.com/TechDocs/54945_PPR_Family_17h_Models_00h-
> > > > > > > 0Fh.pdf
> > > > > > > >
> > > > > > > > Some of these are bugs in our code. For some we need to add
> > > checks
> > > > > for
> > > > > > > the family and correct these values.
> > > > > > > > You understand the code much better than me. Correct me if I
> > > missed
> > > > > > > something.
> > > > > > > >
> > > > > > > > Note that older family of processors don't support topology
> > > extensions.
> > > > > > >
> > > > > > > If you want to make the cache size/topology look different
> > > > > > > depending on the CPU model/options, this would require more
> work,
> > > > > > > but it would be an interesting feature.
> > > > > > >
> > > > > > > The "i386: Helpers to encode cache information consistently"
> > > > > > > patch I sent last week might be a useful starting point for that.
> > > > > > >
> > > > > > > If you plan to implement that, please keep in mind that existing
> > > > > > > CPUID cache info needs to be kept on previous machine-types
> (this
> > > > > > > is implemented by adding QOM properties that can be used to
> > > > > > > enable the old behavior, and by setting them at
> > > > > > > MachineClass::compat_props).
> > > > > >
> > > > > > Wanted to get some confirmation what you meant by setting
> > > > > MachineClass::compat_props.
> > > > > > Here is the patch I created to add new property for cpu. Now, I can
> use
> > > > > enable_topoext to display
> > > > > > new  change properly based on family.  Is that what you meant ?
> > > > >
> > > > > If the only change you introduce in the defaults is enabling
> > > > > topoext but keeping the same default cache sizes, the example
> > > > > following would make sense (but see comments below about
> > > > > implementation details).
> > > > >
> > > > > But if you also want to change the default cache size, you will
> > > > > also need properties that control the cache size.
> > > >
> > > > Yes. We will need cpu family information.  Let me see if that
> information is
> > > >  already available or we need to add that as well.
> > >
> > > CPU family doesn't seem to be enough, if you want to keep
> > > compatibility on older machine-type versions.  e.g.:
> > >
> > > If you want to make EPYC expose a different cache size, you'd
> > > just need a new field on X86CPUDefinition.  This is the easy
> > > part.
> >
> > Actually, I already have this information.  I can just decode cpuid_version
> information.
> > I may not need to add new field in new field on X86CPUDefinition.
> 
> I'd prefer CPUCacheInfo* fields on X86CPUDefinition instead of
> hardcoding a cpuid_version check.  This way all
> family/model-specific information about CPU models will
> centralized in the CPU model table.

Eduardo,  Posted the v5 version. Please see if it addresses your feedback here.
Thanks.

> 
> >
> > >
> > > But you still need "-machine pc-2.11 -cpu EPYC" to expose the old
> > > cache size, for guest ABI compatibility.
> > >
> > > This means you need an additional knob that pc-2.10 can use in
> > > compat_props, to override the new family/model-dependent cache
> > > size in EPYC, and force the old cache sizes to be used even with
> > > "-cpu EPYC".
> >
> > Ok.  Will add a new property legacy_cache(or something). I can use this to
> override
> > all the new information. Will plan to send first version tomorrow.
> >
> > >
> > > --
> > > Eduardo
> 
> --
> Eduardo

[PATCH v4 3/5] target/i386: Add support for CPUID_8000_001E for AMD

[Babu Moger]

From: Stanislav Lanci <pixo@polepetko.eu>

Populate threads/core_id/apic_ids/socket_id when CPUID_EXT3_TOPOEXT
feature is supported. This is required to support hyperthreading feature
on AMD CPUs. This is supported via CPUID_8000_001E extended functions.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5fdbedd..ba63d0c 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3655,6 +3655,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             break;
         }
         break;
+    case 0x8000001E:
+        assert(cpu->core_id <= 255);
+        *eax = cpu->apic_id;
+        *ebx = (cs->nr_threads - 1) << 8 | cpu->core_id;
+        *ecx = cpu->socket_id;
+        *edx = 0;
+        break;
     case 0xC0000000:
         *eax = env->cpuid_xlevel2;
         *ebx = 0;
-- 
1.8.3.1

[PATCH v4 4/5] target/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 ba63d0c..ade272e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1955,7 +1955,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 |
@@ -1999,7 +2000,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] =
@@ -4113,6 +4115,11 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
         if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {
             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);
+        }
     }
 
     /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly set */
-- 
1.8.3.1

[PATCH v4 5/5] target/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>
---
 target/i386/cpu.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ade272e..10970b7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4330,17 +4330,20 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     qemu_init_vcpu(cs);
 
-    /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this
-     * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
-     * based on inputs (sockets,cores,threads), it is still better to gives
+    /* Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
+     * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
+     * based on inputs (sockets,cores,threads), it is still better to give
      * users a warning.
      *
      * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
      * cs->nr_threads hasn't be populated yet and the checking is incorrect.
      */
-    if (!IS_INTEL_CPU(env) && cs->nr_threads > 1 && !ht_warned) {
-        error_report("AMD CPU doesn't support hyperthreading. Please configure"
-                     " -smp options properly.");
+     if (IS_AMD_CPU(env) &&
+         !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
+         cs->nr_threads > 1 && !ht_warned) {
+            error_report("This family of AMD CPU doesn't support "
+                         "hyperthreading. Please configure -smp "
+                         "options properly.");
         ht_warned = true;
     }
 
-- 
1.8.3.1

Re: [PATCH v4 0/5] Enable TOPOEXT to support hyperthreading on AMD CPU

[Kash Pande]

[-- Attachment #1: Type: text/plain, Size: 47 bytes --]

Tested-by: Kash Pande <kash@tripleback.net>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]