[PATCH v3] target/arm/tcg: refine cache descriptions with a wrapper

Alireza Sanaee via posted 1 patch 2 months, 3 weeks ago
There is a newer version of this series
target/arm/cpu-features.h |  50 ++++++++++++++++++
target/arm/cpu64.c        |  19 ++++---
target/arm/tcg/cpu64.c    | 108 +++++++++++++++++++-------------------
3 files changed, 117 insertions(+), 60 deletions(-)
[PATCH v3] target/arm/tcg: refine cache descriptions with a wrapper
Posted by Alireza Sanaee via 2 months, 3 weeks ago
This patch allows for easier manipulation of the cache description
register, CCSIDR. Which is helpful for testing as well. Currently,
numbers get hard-coded and might be prone to errors.

Therefore, this patch adds a wrapper for different types of CPUs
available in tcg to decribe caches. One function `make_ccsidr` supports
two cases by carrying a parameter as FORMAT that can be LEGACY and
CCIDX which determines the specification of the register.

For CCSIDR register, 32 bit version follows specification [1].
Conversely, 64 bit version follows specification [2].

Changes from v2 [3] -> v3:

1) add only one function instead of ccsidr32 and ccsidr64
2) use deposit32 and deposit64 in ccsidr function

[1] B4.1.19, ARM Architecture Reference Manual ARMv7-A and ARMv7-R
edition, https://developer.arm.com/documentation/ddi0406
[2] D23.2.29, ARM Architecture Reference Manual for A-profile Architecture,
https://developer.arm.com/documentation/ddi0487/latest/
[3] https://lore.kernel.org/qemu-devel/20240830184713.224-1-alireza.sanaee@huawei.com/

Signed-off-by: Alireza Sanaee <alireza.sanaee@huawei.com>
---
 target/arm/cpu-features.h |  50 ++++++++++++++++++
 target/arm/cpu64.c        |  19 ++++---
 target/arm/tcg/cpu64.c    | 108 +++++++++++++++++++-------------------
 3 files changed, 117 insertions(+), 60 deletions(-)

diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h
index c59ca104fe..ef40b0dfdc 100644
--- a/target/arm/cpu-features.h
+++ b/target/arm/cpu-features.h
@@ -1022,6 +1022,56 @@ static inline bool isar_feature_any_evt(const ARMISARegisters *id)
     return isar_feature_aa64_evt(id) || isar_feature_aa32_evt(id);
 }
 
+typedef enum {
+    CCSIDR_FORMAT_LEGACY,
+    CCSIDR_FORMAT_CCIDX,
+} CCSIDRFormat;
+
+static inline uint64_t make_ccsidr(CCSIDRFormat format, unsigned assoc,
+                                   unsigned linesize, unsigned cachesize,
+                                   uint8_t flags)
+{
+    unsigned lg_linesize = ctz32(linesize);
+    unsigned sets;
+    uint32_t ccsidr32 = 0;
+    uint64_t ccsidr64 = 0;
+
+    assert(assoc != 0);
+    assert(is_power_of_2(linesize));
+    assert(lg_linesize >= 4 && lg_linesize <= 7 + 4);
+
+    /* sets * associativity * linesize == cachesize. */
+    sets = cachesize / (assoc * linesize);
+    assert(cachesize % (assoc * linesize) == 0);
+
+    if (format == CCSIDR_FORMAT_LEGACY) {
+        /*
+         * The 32-bit CCSIDR format is:
+         *   [27:13] number of sets - 1
+         *   [12:3]  associativity - 1
+         *   [2:0]   log2(linesize) - 4
+         *           so 0 == 16 bytes, 1 == 32 bytes, 2 == 64 bytes, etc
+         */
+        ccsidr32 = deposit32(ccsidr32, 28,  4, flags);
+        ccsidr32 = deposit32(ccsidr32, 13, 15, sets - 1);
+        ccsidr32 = deposit32(ccsidr32,  3, 10, assoc - 1);
+        ccsidr32 = deposit32(ccsidr32,  0,  3, lg_linesize - 4);
+        return (uint64_t)ccsidr32;
+    } else {
+        /*
+         * The 64-bit CCSIDR_EL1 format is:
+         *   [55:32] number of sets - 1
+         *   [23:3]  associativity - 1
+         *   [2:0]   log2(linesize) - 4
+         *           so 0 == 16 bytes, 1 == 32 bytes, 2 == 64 bytes, etc
+         */
+        ccsidr64 = deposit64(ccsidr64, 32, 24, sets - 1);
+        ccsidr64 = deposit64(ccsidr64,  3, 21, assoc - 1);
+        ccsidr64 = deposit64(ccsidr64,  0,  3, lg_linesize - 4);
+        return ccsidr64;
+    }
+}
+
 /*
  * Forward to the above feature tests given an ARMCPU pointer.
  */
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 262a1d6c0b..458d1cee01 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -23,6 +23,7 @@
 #include "cpu.h"
 #include "cpregs.h"
 #include "qemu/module.h"
+#include "qemu/units.h"
 #include "sysemu/kvm.h"
 #include "sysemu/hvf.h"
 #include "sysemu/qtest.h"
@@ -642,9 +643,12 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->isar.dbgdevid1 = 0x2;
     cpu->isar.reset_pmcr_el0 = 0x41013000;
     cpu->clidr = 0x0a200023;
-    cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */
-    cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */
-    cpu->ccsidr[2] = 0x70ffe07a; /* 2048KB L2 cache */
+    /* 32KB L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 32 * KiB, 7);
+    /* 48KB L1 icache */
+    cpu->ccsidr[1] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 3, 64, 48 * KiB, 2);
+    /* 2048KB L2 cache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 16, 64, 2 * MiB, 7);
     cpu->dcz_blocksize = 4; /* 64 bytes */
     cpu->gic_num_lrs = 4;
     cpu->gic_vpribits = 5;
@@ -700,9 +704,12 @@ static void aarch64_a53_initfn(Object *obj)
     cpu->isar.dbgdevid1 = 0x1;
     cpu->isar.reset_pmcr_el0 = 0x41033000;
     cpu->clidr = 0x0a200023;
-    cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
-    cpu->ccsidr[1] = 0x201fe00a; /* 32KB L1 icache */
-    cpu->ccsidr[2] = 0x707fe07a; /* 1024KB L2 cache */
+    /* 32KB L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 32 * KiB, 7);
+    /* 32KB L1 icache */
+    cpu->ccsidr[1] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 1, 64, 32 * KiB, 2);
+    /* 1024KB L2 cache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 16, 64, 1 * MiB, 7);
     cpu->dcz_blocksize = 4; /* 64 bytes */
     cpu->gic_num_lrs = 4;
     cpu->gic_vpribits = 5;
diff --git a/target/arm/tcg/cpu64.c b/target/arm/tcg/cpu64.c
index fe232eb306..904a7e90b4 100644
--- a/target/arm/tcg/cpu64.c
+++ b/target/arm/tcg/cpu64.c
@@ -29,32 +29,6 @@
 #include "cpu-features.h"
 #include "cpregs.h"
 
-static uint64_t make_ccsidr64(unsigned assoc, unsigned linesize,
-                              unsigned cachesize)
-{
-    unsigned lg_linesize = ctz32(linesize);
-    unsigned sets;
-
-    /*
-     * The 64-bit CCSIDR_EL1 format is:
-     *   [55:32] number of sets - 1
-     *   [23:3]  associativity - 1
-     *   [2:0]   log2(linesize) - 4
-     *           so 0 == 16 bytes, 1 == 32 bytes, 2 == 64 bytes, etc
-     */
-    assert(assoc != 0);
-    assert(is_power_of_2(linesize));
-    assert(lg_linesize >= 4 && lg_linesize <= 7 + 4);
-
-    /* sets * associativity * linesize == cachesize. */
-    sets = cachesize / (assoc * linesize);
-    assert(cachesize % (assoc * linesize) == 0);
-
-    return ((uint64_t)(sets - 1) << 32)
-         | ((assoc - 1) << 3)
-         | (lg_linesize - 4);
-}
-
 static void aarch64_a35_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
@@ -106,9 +80,12 @@ static void aarch64_a35_initfn(Object *obj)
     cpu->isar.reset_pmcr_el0 = 0x410a3000;
 
     /* From B2.29 Cache ID registers */
-    cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
-    cpu->ccsidr[1] = 0x201fe00a; /* 32KB L1 icache */
-    cpu->ccsidr[2] = 0x703fe03a; /* 512KB L2 cache */
+    /* 32KB L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 32 * KiB, 7);
+    /* 32KB L1 icache */
+    cpu->ccsidr[1] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 32 * KiB, 2);
+    /* 512KB L2 cache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 16, 64, 512 * KiB, 7);
 
     /* From B3.5 VGIC Type register */
     cpu->gic_num_lrs = 4;
@@ -272,9 +249,12 @@ static void aarch64_a55_initfn(Object *obj)
     cpu->revidr = 0;
 
     /* From B2.23 CCSIDR_EL1 */
-    cpu->ccsidr[0] = 0x700fe01a; /* 32KB L1 dcache */
-    cpu->ccsidr[1] = 0x200fe01a; /* 32KB L1 icache */
-    cpu->ccsidr[2] = 0x703fe07a; /* 512KB L2 cache */
+    /* 32KB L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 32 * KiB, 7);
+    /* 32KB L1 icache */
+    cpu->ccsidr[1] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 32 * KiB, 2);
+    /* 512KB L2 cache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 16, 64, 512 * KiB, 7);
 
     /* From B2.96 SCTLR_EL3 */
     cpu->reset_sctlr = 0x30c50838;
@@ -338,9 +318,12 @@ static void aarch64_a72_initfn(Object *obj)
     cpu->isar.dbgdevid1 = 0x2;
     cpu->isar.reset_pmcr_el0 = 0x41023000;
     cpu->clidr = 0x0a200023;
-    cpu->ccsidr[0] = 0x701fe00a; /* 32KB L1 dcache */
-    cpu->ccsidr[1] = 0x201fe012; /* 48KB L1 icache */
-    cpu->ccsidr[2] = 0x707fe07a; /* 1MB L2 cache */
+    /* 32KB L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 32 * KiB, 7);
+    /* 48KB L1 dcache */
+    cpu->ccsidr[1] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 3, 64, 48 * KiB, 2);
+    /* 1MB L2 cache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 16, 64, 1 * MiB, 7);
     cpu->dcz_blocksize = 4; /* 64 bytes */
     cpu->gic_num_lrs = 4;
     cpu->gic_vpribits = 5;
@@ -397,9 +380,12 @@ static void aarch64_a76_initfn(Object *obj)
     cpu->revidr = 0;
 
     /* From B2.18 CCSIDR_EL1 */
-    cpu->ccsidr[0] = 0x701fe01a; /* 64KB L1 dcache */
-    cpu->ccsidr[1] = 0x201fe01a; /* 64KB L1 icache */
-    cpu->ccsidr[2] = 0x707fe03a; /* 512KB L2 cache */
+    /* 64KB L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 64 * KiB, 7);
+    /* 64KB L1 icache */
+    cpu->ccsidr[1] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 64 * KiB, 2);
+    /* 512KB L2 cache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 8, 64, 512 * KiB, 7);
 
     /* From B2.93 SCTLR_EL3 */
     cpu->reset_sctlr = 0x30c50838;
@@ -449,9 +435,12 @@ static void aarch64_a64fx_initfn(Object *obj)
     cpu->isar.id_aa64isar1 = 0x0000000000010001;
     cpu->isar.id_aa64zfr0 = 0x0000000000000000;
     cpu->clidr = 0x0000000080000023;
-    cpu->ccsidr[0] = 0x7007e01c; /* 64KB L1 dcache */
-    cpu->ccsidr[1] = 0x2007e01c; /* 64KB L1 icache */
-    cpu->ccsidr[2] = 0x70ffe07c; /* 8MB L2 cache */
+    /* 64KB L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 256, 64 * KiB, 7);
+    /* 64KB L1 icache */
+    cpu->ccsidr[1] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 256, 64 * KiB, 2);
+    /* 8MB L2 cache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 16, 256, 8 * MiB, 7);
     cpu->dcz_blocksize = 6; /* 256 bytes */
     cpu->gic_num_lrs = 4;
     cpu->gic_vpribits = 5;
@@ -637,9 +626,12 @@ static void aarch64_neoverse_n1_initfn(Object *obj)
     cpu->revidr = 0;
 
     /* From B2.23 CCSIDR_EL1 */
-    cpu->ccsidr[0] = 0x701fe01a; /* 64KB L1 dcache */
-    cpu->ccsidr[1] = 0x201fe01a; /* 64KB L1 icache */
-    cpu->ccsidr[2] = 0x70ffe03a; /* 1MB L2 cache */
+    /* 64KB L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 64 * KiB, 7);
+    /* 64KB L1 icache */
+    cpu->ccsidr[1] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 4, 64, 64 * KiB, 2);
+    /* 1MB L2 dcache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_LEGACY, 8, 64, 1 * MiB, 7);
 
     /* From B2.98 SCTLR_EL3 */
     cpu->reset_sctlr = 0x30c50838;
@@ -721,9 +713,12 @@ static void aarch64_neoverse_v1_initfn(Object *obj)
      * L2: 8-way set associative, 64 byte line size, either 512K or 1MB.
      * L3: No L3 (this matches the CLIDR_EL1 value).
      */
-    cpu->ccsidr[0] = make_ccsidr64(4, 64, 64 * KiB); /* L1 dcache */
-    cpu->ccsidr[1] = cpu->ccsidr[0];                 /* L1 icache */
-    cpu->ccsidr[2] = make_ccsidr64(8, 64, 1 * MiB);  /* L2 cache */
+    /* 64KB L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_CCIDX, 4, 64, 64 * KiB, 0);
+    /* 64KB L1 icache */
+    cpu->ccsidr[1] = cpu->ccsidr[0];
+    /* 1MB L2 cache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_CCIDX, 8, 64, 1 * MiB, 0);
 
     /* From 3.2.115 SCTLR_EL3 */
     cpu->reset_sctlr = 0x30c50838;
@@ -959,9 +954,12 @@ static void aarch64_a710_initfn(Object *obj)
      * L1: 4-way set associative 64-byte line size, total either 32K or 64K.
      * L2: 8-way set associative 64 byte line size, total either 256K or 512K.
      */
-    cpu->ccsidr[0] = make_ccsidr64(4, 64, 64 * KiB);   /* L1 dcache */
-    cpu->ccsidr[1] = cpu->ccsidr[0];                   /* L1 icache */
-    cpu->ccsidr[2] = make_ccsidr64(8, 64, 512 * KiB);  /* L2 cache */
+    /* L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_CCIDX, 4, 64, 64 * KiB, 0);
+    /* L1 icache */
+    cpu->ccsidr[1] = cpu->ccsidr[0];
+    /* L2 cache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_CCIDX, 8, 64, 512 * KiB, 0);
 
     /* FIXME: Not documented -- copied from neoverse-v1 */
     cpu->reset_sctlr = 0x30c50838;
@@ -1057,10 +1055,12 @@ static void aarch64_neoverse_n2_initfn(Object *obj)
      * L1: 4-way set associative 64-byte line size, total 64K.
      * L2: 8-way set associative 64 byte line size, total either 512K or 1024K.
      */
-    cpu->ccsidr[0] = make_ccsidr64(4, 64, 64 * KiB);   /* L1 dcache */
-    cpu->ccsidr[1] = cpu->ccsidr[0];                   /* L1 icache */
-    cpu->ccsidr[2] = make_ccsidr64(8, 64, 512 * KiB);  /* L2 cache */
-
+    /* L1 dcache */
+    cpu->ccsidr[0] = make_ccsidr(CCSIDR_FORMAT_CCIDX, 4, 64, 64 * KiB, 0);
+    /* L1 icache */
+    cpu->ccsidr[1] = cpu->ccsidr[0];
+    /* L2 cache */
+    cpu->ccsidr[2] = make_ccsidr(CCSIDR_FORMAT_CCIDX, 8, 64, 512 * KiB, 0);
     /* FIXME: Not documented -- copied from neoverse-v1 */
     cpu->reset_sctlr = 0x30c50838;
 
-- 
2.34.1
Re: [PATCH v3] target/arm/tcg: refine cache descriptions with a wrapper
Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago
Hi Alireza,

On 2/9/24 22:32, Alireza Sanaee wrote:
> This patch allows for easier manipulation of the cache description
> register, CCSIDR. Which is helpful for testing as well. Currently,
> numbers get hard-coded and might be prone to errors.
> 
> Therefore, this patch adds a wrapper for different types of CPUs
> available in tcg to decribe caches. One function `make_ccsidr` supports
> two cases by carrying a parameter as FORMAT that can be LEGACY and
> CCIDX which determines the specification of the register.
> 
> For CCSIDR register, 32 bit version follows specification [1].
> Conversely, 64 bit version follows specification [2].

---

[cut]

> Changes from v2 [3] -> v3:
> 
> 1) add only one function instead of ccsidr32 and ccsidr64
> 2) use deposit32 and deposit64 in ccsidr function
> 
---

> [1] B4.1.19, ARM Architecture Reference Manual ARMv7-A and ARMv7-R
> edition, https://developer.arm.com/documentation/ddi0406
> [2] D23.2.29, ARM Architecture Reference Manual for A-profile Architecture,
> https://developer.arm.com/documentation/ddi0487/latest/

---

[cut]

> [3] https://lore.kernel.org/qemu-devel/20240830184713.224-1-alireza.sanaee@huawei.com/
> 

---

> Signed-off-by: Alireza Sanaee <alireza.sanaee@huawei.com>
> ---
>   target/arm/cpu-features.h |  50 ++++++++++++++++++
>   target/arm/cpu64.c        |  19 ++++---
>   target/arm/tcg/cpu64.c    | 108 +++++++++++++++++++-------------------
>   3 files changed, 117 insertions(+), 60 deletions(-)
> 
> diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h
> index c59ca104fe..ef40b0dfdc 100644
> --- a/target/arm/cpu-features.h
> +++ b/target/arm/cpu-features.h
> @@ -1022,6 +1022,56 @@ static inline bool isar_feature_any_evt(const ARMISARegisters *id)
>       return isar_feature_aa64_evt(id) || isar_feature_aa32_evt(id);
>   }
>   
> +typedef enum {
> +    CCSIDR_FORMAT_LEGACY,
> +    CCSIDR_FORMAT_CCIDX,
> +} CCSIDRFormat;
> +
> +static inline uint64_t make_ccsidr(CCSIDRFormat format, unsigned assoc,
> +                                   unsigned linesize, unsigned cachesize,
> +                                   uint8_t flags)
> +{
> +    unsigned lg_linesize = ctz32(linesize);
> +    unsigned sets;
> +    uint32_t ccsidr32 = 0;
> +    uint64_t ccsidr64 = 0;

deposit32() works with unsigned so you can use 'uint64_t ccsidr' for
both cases and return once.

> +
> +    assert(assoc != 0);
> +    assert(is_power_of_2(linesize));

As mentioned in v2, if you insist in using an inlined method, you have
to include "qemu/host-utils.h" to get is_power_of_2() declaration.

If the inlining is proven problematic later we can still un-inline
it, so as this is an useful cleanup (preferably with one 'ccsidr'
variable and the include):

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    assert(lg_linesize >= 4 && lg_linesize <= 7 + 4);
> +
> +    /* sets * associativity * linesize == cachesize. */
> +    sets = cachesize / (assoc * linesize);
> +    assert(cachesize % (assoc * linesize) == 0);
> +
> +    if (format == CCSIDR_FORMAT_LEGACY) {
> +        /*
> +         * The 32-bit CCSIDR format is:
> +         *   [27:13] number of sets - 1
> +         *   [12:3]  associativity - 1
> +         *   [2:0]   log2(linesize) - 4
> +         *           so 0 == 16 bytes, 1 == 32 bytes, 2 == 64 bytes, etc
> +         */
> +        ccsidr32 = deposit32(ccsidr32, 28,  4, flags);
> +        ccsidr32 = deposit32(ccsidr32, 13, 15, sets - 1);
> +        ccsidr32 = deposit32(ccsidr32,  3, 10, assoc - 1);
> +        ccsidr32 = deposit32(ccsidr32,  0,  3, lg_linesize - 4);
> +        return (uint64_t)ccsidr32;
> +    } else {
> +        /*
> +         * The 64-bit CCSIDR_EL1 format is:
> +         *   [55:32] number of sets - 1
> +         *   [23:3]  associativity - 1
> +         *   [2:0]   log2(linesize) - 4
> +         *           so 0 == 16 bytes, 1 == 32 bytes, 2 == 64 bytes, etc
> +         */
> +        ccsidr64 = deposit64(ccsidr64, 32, 24, sets - 1);
> +        ccsidr64 = deposit64(ccsidr64,  3, 21, assoc - 1);
> +        ccsidr64 = deposit64(ccsidr64,  0,  3, lg_linesize - 4);
> +        return ccsidr64;
> +    }
> +}


Re: [PATCH v3] target/arm/tcg: refine cache descriptions with a wrapper
Posted by Alireza Sanaee via 2 months, 3 weeks ago
On Tue, 3 Sep 2024 00:11:04 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> Hi Alireza,
> 
> On 2/9/24 22:32, Alireza Sanaee wrote:
> > This patch allows for easier manipulation of the cache description
> > register, CCSIDR. Which is helpful for testing as well. Currently,
> > numbers get hard-coded and might be prone to errors.
> > 
> > Therefore, this patch adds a wrapper for different types of CPUs
> > available in tcg to decribe caches. One function `make_ccsidr`
> > supports two cases by carrying a parameter as FORMAT that can be
> > LEGACY and CCIDX which determines the specification of the register.
> > 
> > For CCSIDR register, 32 bit version follows specification [1].
> > Conversely, 64 bit version follows specification [2].  
> 
> ---
> 
> [cut]
> 
> > Changes from v2 [3] -> v3:
> > 
> > 1) add only one function instead of ccsidr32 and ccsidr64
> > 2) use deposit32 and deposit64 in ccsidr function
> >   
> ---
> 
> > [1] B4.1.19, ARM Architecture Reference Manual ARMv7-A and ARMv7-R
> > edition, https://developer.arm.com/documentation/ddi0406
> > [2] D23.2.29, ARM Architecture Reference Manual for A-profile
> > Architecture, https://developer.arm.com/documentation/ddi0487/latest/  
> 
> ---
> 
> [cut]
> 
> > [3] https://lore.kernel.org/qemu-devel/20240830184713.224-1-alireza.sanaee@huawei.com/
> >   
> 
> ---
> 
> > Signed-off-by: Alireza Sanaee <alireza.sanaee@huawei.com>
> > ---
> >   target/arm/cpu-features.h |  50 ++++++++++++++++++
> >   target/arm/cpu64.c        |  19 ++++---
> >   target/arm/tcg/cpu64.c    | 108
> > +++++++++++++++++++------------------- 3 files changed, 117
> > insertions(+), 60 deletions(-)
> > 
> > diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h
> > index c59ca104fe..ef40b0dfdc 100644
> > --- a/target/arm/cpu-features.h
> > +++ b/target/arm/cpu-features.h
> > @@ -1022,6 +1022,56 @@ static inline bool
> > isar_feature_any_evt(const ARMISARegisters *id) return
> > isar_feature_aa64_evt(id) || isar_feature_aa32_evt(id); }
> >   
> > +typedef enum {
> > +    CCSIDR_FORMAT_LEGACY,
> > +    CCSIDR_FORMAT_CCIDX,
> > +} CCSIDRFormat;
> > +
> > +static inline uint64_t make_ccsidr(CCSIDRFormat format, unsigned
> > assoc,
> > +                                   unsigned linesize, unsigned
> > cachesize,
> > +                                   uint8_t flags)
> > +{
> > +    unsigned lg_linesize = ctz32(linesize);
> > +    unsigned sets;
> > +    uint32_t ccsidr32 = 0;
> > +    uint64_t ccsidr64 = 0;  
> 
> deposit32() works with unsigned so you can use 'uint64_t ccsidr' for
> both cases and return once.
> 
> > +
> > +    assert(assoc != 0);
> > +    assert(is_power_of_2(linesize));  
> 
> As mentioned in v2, if you insist in using an inlined method, you have
> to include "qemu/host-utils.h" to get is_power_of_2() declaration.
> 
> If the inlining is proven problematic later we can still un-inline
> it, so as this is an useful cleanup (preferably with one 'ccsidr'
> variable and the include):
> 

Hi. sure! that makes sense. applied those two changes in v4. sorry for
missing out the comment on v2.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> > +    assert(lg_linesize >= 4 && lg_linesize <= 7 + 4);
> > +
> > +    /* sets * associativity * linesie == cachesize. */
> > +    sets = cachesize / (assoc * linesize);
> > +    assert(cachesize % (assoc * linesize) == 0);
> > +
> > +    if (format == CCSIDR_FORMAT_LEGACY) {
> > +        /*
> > +         * The 32-bit CCSIDR format is:
> > +         *   [27:13] number of sets - 1
> > +         *   [12:3]  associativity - 1
> > +         *   [2:0]   log2(linesize) - 4
> > +         *           so 0 == 16 bytes, 1 == 32 bytes, 2 == 64
> > bytes, etc
> > +         */
> > +        ccsidr32 = deposit32(ccsidr32, 28,  4, flags);
> > +        ccsidr32 = deposit32(ccsidr32, 13, 15, sets - 1);
> > +        ccsidr32 = deposit32(ccsidr32,  3, 10, assoc - 1);
> > +        ccsidr32 = deposit32(ccsidr32,  0,  3, lg_linesize - 4);
> > +        return (uint64_t)ccsidr32;
> > +    } else {
> > +        /*
> > +         * The 64-bit CCSIDR_EL1 format is:
> > +         *   [55:32] number of sets - 1
> > +         *   [23:3]  associativity - 1
> > +         *   [2:0]   log2(linesize) - 4
> > +         *           so 0 == 16 bytes, 1 == 32 bytes, 2 == 64
> > bytes, etc
> > +         */
> > +        ccsidr64 = deposit64(ccsidr64, 32, 24, sets - 1);
> > +        ccsidr64 = deposit64(ccsidr64,  3, 21, assoc - 1);
> > +        ccsidr64 = deposit64(ccsidr64,  0,  3, lg_linesize - 4);
> > +        return ccsidr64;
> > +    }
> > +}  
>