[Qemu-devel] [PATCH 0/2] i386: Introduce X86CPUCacheCPUID struct

[Qemu-devel] [PATCH 0/2] i386: Introduce X86CPUCacheCPUID struct

[Eduardo Habkost]

The new struct will let us declare the existing legacy CPU
topology info in a static constant, instead of defining it inside
x86_cpu_realizefn().  While doing it, make the CPU cache
declarations all constants.

This will help us represent the model-specific cache info as QOM
properties in the future.  Currently X86CPUDefinition::cache_info
(which is being renamed to X86CPUDefinition::cache_cpuid) is the
only CPU model field that can't be represented as a QOM property
value.

Eduardo Habkost (2):
  i386: make cache structs const-safe
  i386: Introduce X86CPUCacheCPUID struct

 target/i386/cpu.h |  15 +++---
 target/i386/cpu.c | 134 +++++++++++++++++++++++++++-------------------
 2 files changed, 84 insertions(+), 65 deletions(-)

-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 1/2] i386: make cache structs const-safe

[Eduardo Habkost]

This code will be refactored and it will be useful to make it
const-safe to catch mistakes.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.h |  8 ++++----
 target/i386/cpu.c | 36 ++++++++++++++++++------------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 0732e059ec..2f03489bf0 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1109,10 +1109,10 @@ typedef struct CPUCacheInfo {
 
 
 typedef struct CPUCaches {
-        CPUCacheInfo *l1d_cache;
-        CPUCacheInfo *l1i_cache;
-        CPUCacheInfo *l2_cache;
-        CPUCacheInfo *l3_cache;
+    const CPUCacheInfo *l1d_cache;
+    const CPUCacheInfo *l1i_cache;
+    const CPUCacheInfo *l2_cache;
+    const CPUCacheInfo *l3_cache;
 } CPUCaches;
 
 typedef struct CPUX86State {
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index fbed2eb804..a6acd71911 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -205,7 +205,7 @@ struct CPUID2CacheDescriptorInfo cpuid2_cache_descriptors[] = {
  * Return a CPUID 2 cache descriptor for a given cache.
  * If no known descriptor is found, return CACHE_DESCRIPTOR_UNAVAILABLE
  */
-static uint8_t cpuid2_cache_descriptor(CPUCacheInfo *cache)
+static uint8_t cpuid2_cache_descriptor(const CPUCacheInfo *cache)
 {
     int i;
 
@@ -249,7 +249,7 @@ static uint8_t cpuid2_cache_descriptor(CPUCacheInfo *cache)
 
 
 /* Encode cache info for CPUID[4] */
-static void encode_cache_cpuid4(CPUCacheInfo *cache,
+static void encode_cache_cpuid4(const CPUCacheInfo *cache,
                                 int num_apic_ids, int num_cores,
                                 uint32_t *eax, uint32_t *ebx,
                                 uint32_t *ecx, uint32_t *edx)
@@ -282,7 +282,7 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache,
 }
 
 /* Encode cache info for CPUID[0x80000005].ECX or CPUID[0x80000005].EDX */
-static uint32_t encode_cache_cpuid80000005(CPUCacheInfo *cache)
+static uint32_t encode_cache_cpuid80000005(const CPUCacheInfo *cache)
 {
     assert(cache->size % 1024 == 0);
     assert(cache->lines_per_tag > 0);
@@ -312,8 +312,8 @@ static uint32_t encode_cache_cpuid80000005(CPUCacheInfo *cache)
  * Encode cache info for CPUID[0x80000006].ECX and CPUID[0x80000006].EDX
  * @l3 can be NULL.
  */
-static void encode_cache_cpuid80000006(CPUCacheInfo *l2,
-                                       CPUCacheInfo *l3,
+static void encode_cache_cpuid80000006(const CPUCacheInfo *l2,
+                                       const CPUCacheInfo *l3,
                                        uint32_t *ecx, uint32_t *edx)
 {
     assert(l2->size % 1024 == 0);
@@ -394,9 +394,9 @@ static int cores_in_core_complex(int nr_cores)
 }
 
 /* Encode cache info for CPUID[8000001D] */
-static void encode_cache_cpuid8000001d(CPUCacheInfo *cache, CPUState *cs,
-                                uint32_t *eax, uint32_t *ebx,
-                                uint32_t *ecx, uint32_t *edx)
+static void encode_cache_cpuid8000001d(const CPUCacheInfo *cache, CPUState *cs,
+                                       uint32_t *eax, uint32_t *ebx,
+                                       uint32_t *ecx, uint32_t *edx)
 {
     uint32_t l3_cores;
     assert(cache->size == cache->line_size * cache->associativity *
@@ -541,7 +541,7 @@ static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
  */
 
 /* L1 data cache: */
-static CPUCacheInfo legacy_l1d_cache = {
+static const CPUCacheInfo legacy_l1d_cache = {
     .type = DATA_CACHE,
     .level = 1,
     .size = 32 * KiB,
@@ -554,7 +554,7 @@ static CPUCacheInfo legacy_l1d_cache = {
 };
 
 /*FIXME: CPUID leaf 0x80000005 is inconsistent with leaves 2 & 4 */
-static CPUCacheInfo legacy_l1d_cache_amd = {
+static const CPUCacheInfo legacy_l1d_cache_amd = {
     .type = DATA_CACHE,
     .level = 1,
     .size = 64 * KiB,
@@ -568,7 +568,7 @@ static CPUCacheInfo legacy_l1d_cache_amd = {
 };
 
 /* L1 instruction cache: */
-static CPUCacheInfo legacy_l1i_cache = {
+static const CPUCacheInfo legacy_l1i_cache = {
     .type = INSTRUCTION_CACHE,
     .level = 1,
     .size = 32 * KiB,
@@ -581,7 +581,7 @@ static CPUCacheInfo legacy_l1i_cache = {
 };
 
 /*FIXME: CPUID leaf 0x80000005 is inconsistent with leaves 2 & 4 */
-static CPUCacheInfo legacy_l1i_cache_amd = {
+static const CPUCacheInfo legacy_l1i_cache_amd = {
     .type = INSTRUCTION_CACHE,
     .level = 1,
     .size = 64 * KiB,
@@ -595,7 +595,7 @@ static CPUCacheInfo legacy_l1i_cache_amd = {
 };
 
 /* Level 2 unified cache: */
-static CPUCacheInfo legacy_l2_cache = {
+static const CPUCacheInfo legacy_l2_cache = {
     .type = UNIFIED_CACHE,
     .level = 2,
     .size = 4 * MiB,
@@ -608,7 +608,7 @@ static CPUCacheInfo legacy_l2_cache = {
 };
 
 /*FIXME: CPUID leaf 2 descriptor is inconsistent with CPUID leaf 4 */
-static CPUCacheInfo legacy_l2_cache_cpuid2 = {
+static const CPUCacheInfo legacy_l2_cache_cpuid2 = {
     .type = UNIFIED_CACHE,
     .level = 2,
     .size = 2 * MiB,
@@ -618,7 +618,7 @@ static CPUCacheInfo legacy_l2_cache_cpuid2 = {
 
 
 /*FIXME: CPUID leaf 0x80000006 is inconsistent with leaves 2 & 4 */
-static CPUCacheInfo legacy_l2_cache_amd = {
+static const CPUCacheInfo legacy_l2_cache_amd = {
     .type = UNIFIED_CACHE,
     .level = 2,
     .size = 512 * KiB,
@@ -630,7 +630,7 @@ static CPUCacheInfo legacy_l2_cache_amd = {
 };
 
 /* Level 3 unified cache: */
-static CPUCacheInfo legacy_l3_cache = {
+static const CPUCacheInfo legacy_l3_cache = {
     .type = UNIFIED_CACHE,
     .level = 3,
     .size = 16 * MiB,
@@ -1423,10 +1423,10 @@ struct X86CPUDefinition {
     int stepping;
     FeatureWordArray features;
     const char *model_id;
-    CPUCaches *cache_info;
+    const CPUCaches *cache_info;
 };
 
-static CPUCaches epyc_cache_info = {
+static const CPUCaches epyc_cache_info = {
     .l1d_cache = &(CPUCacheInfo) {
         .type = DATA_CACHE,
         .level = 1,
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 2/2] i386: Introduce X86CPUCacheCPUID struct

[Eduardo Habkost]

The new struct will be used to simplify the code that deals with
legacy cache information.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.h |   7 +---
 target/i386/cpu.c | 100 ++++++++++++++++++++++++++++------------------
 2 files changed, 63 insertions(+), 44 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 2f03489bf0..86cf04d441 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1306,11 +1306,8 @@ typedef struct CPUX86State {
     /* Features that were explicitly enabled/disabled */
     FeatureWordArray user_features;
     uint32_t cpuid_model[12];
-    /* Cache information for CPUID.  When legacy-cache=on, the cache data
-     * on each CPUID leaf will be different, because we keep compatibility
-     * with old QEMU versions.
-     */
-    CPUCaches cache_info_cpuid2, cache_info_cpuid4, cache_info_amd;
+    /* Cache information for CPUID */
+    const struct X86CPUCacheCPUID *caches;
 
     /* MTRRs */
     uint64_t mtrr_fixed[11];
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a6acd71911..e9f301f9ea 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1412,6 +1412,10 @@ static char *x86_cpu_class_get_model_name(X86CPUClass *cc)
                      strlen(class_name) - strlen(X86_CPU_TYPE_SUFFIX));
 }
 
+typedef struct X86CPUCacheCPUID {
+    CPUCaches cpuid2, cpuid4, amd;
+} X86CPUCacheCPUID;
+
 struct X86CPUDefinition {
     const char *name;
     uint32_t level;
@@ -1423,7 +1427,7 @@ struct X86CPUDefinition {
     int stepping;
     FeatureWordArray features;
     const char *model_id;
-    const CPUCaches *cache_info;
+    const X86CPUCacheCPUID *cache_cpuid;
 };
 
 static const CPUCaches epyc_cache_info = {
@@ -1476,6 +1480,39 @@ static const CPUCaches epyc_cache_info = {
     },
 };
 
+static X86CPUCacheCPUID epyc_cache_cpuid = {
+    .cpuid2 = epyc_cache_info,
+    .cpuid4 = epyc_cache_info,
+    .amd = epyc_cache_info,
+};
+
+/*
+ * Legacy cache template.  When legacy-cache=on, the cache data
+ * on each CPUID leaf will be different, because we keep compatibility
+ * with old QEMU versions.
+ */
+static X86CPUCacheCPUID legacy_cache_cpuid = {
+    .cpuid2 = {
+        .l1d_cache = &legacy_l1d_cache,
+        .l1i_cache = &legacy_l1i_cache,
+        .l2_cache = &legacy_l2_cache_cpuid2,
+        .l3_cache = &legacy_l3_cache,
+    },
+    .cpuid4 = {
+        .l1d_cache = &legacy_l1d_cache,
+        .l1i_cache = &legacy_l1i_cache,
+        .l2_cache = &legacy_l2_cache,
+        .l3_cache = &legacy_l3_cache,
+    },
+    .amd = {
+        .l1d_cache = &legacy_l1d_cache_amd,
+        .l1i_cache = &legacy_l1i_cache_amd,
+        .l2_cache = &legacy_l2_cache_amd,
+        .l3_cache = &legacy_l3_cache,
+    },
+};
+
+
 static X86CPUDefinition builtin_x86_defs[] = {
     {
         .name = "qemu64",
@@ -2886,7 +2923,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_SVM_NPT | CPUID_SVM_NRIPSAVE,
         .xlevel = 0x8000001E,
         .model_id = "AMD EPYC Processor",
-        .cache_info = &epyc_cache_info,
+        .cache_cpuid = &epyc_cache_cpuid,
     },
     {
         .name = "EPYC-IBPB",
@@ -2936,7 +2973,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_SVM_NPT | CPUID_SVM_NRIPSAVE,
         .xlevel = 0x8000001E,
         .model_id = "AMD EPYC Processor (with IBPB)",
-        .cache_info = &epyc_cache_info,
+        .cache_cpuid = &epyc_cache_cpuid,
     },
     {
         .name = "Dhyana",
@@ -2986,7 +3023,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_SVM_NPT | CPUID_SVM_NRIPSAVE,
         .xlevel = 0x8000001E,
         .model_id = "Hygon Dhyana Processor",
-        .cache_info = &epyc_cache_info,
+        .cache_cpuid = &epyc_cache_cpuid,
     },
 };
 
@@ -3951,7 +3988,7 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     }
 
     /* legacy-cache defaults to 'off' if CPU model provides cache info */
-    cpu->legacy_cache = !def->cache_info;
+    cpu->legacy_cache = !def->cache_cpuid;
 
     /* Special cases not set in the X86CPUDefinition structs: */
     /* TODO: in-kernel irqchip for hvf */
@@ -4301,11 +4338,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         if (!cpu->enable_l3_cache) {
             *ecx = 0;
         } else {
-            *ecx = cpuid2_cache_descriptor(env->cache_info_cpuid2.l3_cache);
+            *ecx = cpuid2_cache_descriptor(env->caches->cpuid2.l3_cache);
         }
-        *edx = (cpuid2_cache_descriptor(env->cache_info_cpuid2.l1d_cache) << 16) |
-               (cpuid2_cache_descriptor(env->cache_info_cpuid2.l1i_cache) <<  8) |
-               (cpuid2_cache_descriptor(env->cache_info_cpuid2.l2_cache));
+        *edx = (cpuid2_cache_descriptor(env->caches->cpuid2.l1d_cache) << 16) |
+               (cpuid2_cache_descriptor(env->caches->cpuid2.l1i_cache) <<  8) |
+               (cpuid2_cache_descriptor(env->caches->cpuid2.l2_cache));
         break;
     case 4:
         /* cache info: needed for Core compatibility */
@@ -4320,24 +4357,24 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             *eax = 0;
             switch (count) {
             case 0: /* L1 dcache info */
-                encode_cache_cpuid4(env->cache_info_cpuid4.l1d_cache,
+                encode_cache_cpuid4(env->caches->cpuid4.l1d_cache,
                                     1, cs->nr_cores,
                                     eax, ebx, ecx, edx);
                 break;
             case 1: /* L1 icache info */
-                encode_cache_cpuid4(env->cache_info_cpuid4.l1i_cache,
+                encode_cache_cpuid4(env->caches->cpuid4.l1i_cache,
                                     1, cs->nr_cores,
                                     eax, ebx, ecx, edx);
                 break;
             case 2: /* L2 cache info */
-                encode_cache_cpuid4(env->cache_info_cpuid4.l2_cache,
+                encode_cache_cpuid4(env->caches->cpuid4.l2_cache,
                                     cs->nr_threads, cs->nr_cores,
                                     eax, ebx, ecx, edx);
                 break;
             case 3: /* L3 cache info */
                 pkg_offset = apicid_pkg_offset(cs->nr_cores, cs->nr_threads);
                 if (cpu->enable_l3_cache) {
-                    encode_cache_cpuid4(env->cache_info_cpuid4.l3_cache,
+                    encode_cache_cpuid4(env->caches->cpuid4.l3_cache,
                                         (1 << pkg_offset), cs->nr_cores,
                                         eax, ebx, ecx, edx);
                     break;
@@ -4551,8 +4588,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                (L1_ITLB_2M_ASSOC <<  8) | (L1_ITLB_2M_ENTRIES);
         *ebx = (L1_DTLB_4K_ASSOC << 24) | (L1_DTLB_4K_ENTRIES << 16) | \
                (L1_ITLB_4K_ASSOC <<  8) | (L1_ITLB_4K_ENTRIES);
-        *ecx = encode_cache_cpuid80000005(env->cache_info_amd.l1d_cache);
-        *edx = encode_cache_cpuid80000005(env->cache_info_amd.l1i_cache);
+        *ecx = encode_cache_cpuid80000005(env->caches->amd.l1d_cache);
+        *edx = encode_cache_cpuid80000005(env->caches->amd.l1i_cache);
         break;
     case 0x80000006:
         /* cache info (L2 cache) */
@@ -4568,9 +4605,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                (L2_DTLB_4K_ENTRIES << 16) | \
                (AMD_ENC_ASSOC(L2_ITLB_4K_ASSOC) << 12) | \
                (L2_ITLB_4K_ENTRIES);
-        encode_cache_cpuid80000006(env->cache_info_amd.l2_cache,
+        encode_cache_cpuid80000006(env->caches->amd.l2_cache,
                                    cpu->enable_l3_cache ?
-                                   env->cache_info_amd.l3_cache : NULL,
+                                   env->caches->amd.l3_cache : NULL,
                                    ecx, edx);
         break;
     case 0x80000007:
@@ -4620,19 +4657,19 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         }
         switch (count) {
         case 0: /* L1 dcache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache, cs,
+            encode_cache_cpuid8000001d(env->caches->amd.l1d_cache, cs,
                                        eax, ebx, ecx, edx);
             break;
         case 1: /* L1 icache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache, cs,
+            encode_cache_cpuid8000001d(env->caches->amd.l1i_cache, cs,
                                        eax, ebx, ecx, edx);
             break;
         case 2: /* L2 cache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache, cs,
+            encode_cache_cpuid8000001d(env->caches->amd.l2_cache, cs,
                                        eax, ebx, ecx, edx);
             break;
         case 3: /* L3 cache info */
-            encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache, cs,
+            encode_cache_cpuid8000001d(env->caches->amd.l3_cache, cs,
                                        eax, ebx, ecx, edx);
             break;
         default: /* end of info */
@@ -5331,31 +5368,16 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     /* Cache information initialization */
     if (!cpu->legacy_cache) {
-        if (!xcc->cpu_def || !xcc->cpu_def->cache_info) {
+        if (!xcc->cpu_def || !xcc->cpu_def->cache_cpuid) {
             char *name = x86_cpu_class_get_model_name(xcc);
             error_setg(errp,
                        "CPU model '%s' doesn't support legacy-cache=off", name);
             g_free(name);
             return;
         }
-        env->cache_info_cpuid2 = env->cache_info_cpuid4 = env->cache_info_amd =
-            *xcc->cpu_def->cache_info;
+        env->caches = xcc->cpu_def->cache_cpuid;
     } else {
-        /* Build legacy cache information */
-        env->cache_info_cpuid2.l1d_cache = &legacy_l1d_cache;
-        env->cache_info_cpuid2.l1i_cache = &legacy_l1i_cache;
-        env->cache_info_cpuid2.l2_cache = &legacy_l2_cache_cpuid2;
-        env->cache_info_cpuid2.l3_cache = &legacy_l3_cache;
-
-        env->cache_info_cpuid4.l1d_cache = &legacy_l1d_cache;
-        env->cache_info_cpuid4.l1i_cache = &legacy_l1i_cache;
-        env->cache_info_cpuid4.l2_cache = &legacy_l2_cache;
-        env->cache_info_cpuid4.l3_cache = &legacy_l3_cache;
-
-        env->cache_info_amd.l1d_cache = &legacy_l1d_cache_amd;
-        env->cache_info_amd.l1i_cache = &legacy_l1i_cache_amd;
-        env->cache_info_amd.l2_cache = &legacy_l2_cache_amd;
-        env->cache_info_amd.l3_cache = &legacy_l3_cache;
+        env->caches = &legacy_cache_cpuid;
     }
 
 
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [Qemu-devel] [PATCH 0/2] i386: Introduce X86CPUCacheCPUID struct

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190620203616.22715-1-ehabkost@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      x86_64-softmmu/target/i386/svm_helper.o
  CC      x86_64-softmmu/target/i386/machine.o
  CC      x86_64-softmmu/target/i386/arch_memory_mapping.o
/tmp/qemu-test/src/target/i386/cpu.c:1484:15: error: initializer element is not a compile-time constant
    .cpuid2 = epyc_cache_info,
              ^~~~~~~~~~~~~~~
1 error generated.


The full log is available at
http://patchew.org/logs/20190620203616.22715-1-ehabkost@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com