On Mon, 2 Sep 2024 11:25:36 +0100 Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Mon, 2 Sept 2024 at 11:07, Philippe Mathieu-Daudé > <phi...@linaro.org> wrote: > > > > Hi Alireza, > > > > On 30/8/24 20:47, Alireza Sanaee via 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 wrappers for different types of CPUs > > > available in tcg to decribe caches. Two functions `make_ccsidr32` > > > and `make_ccsidr64` describing descriptions. The 32 bit version > > > receives extra parameters that became unknown later in 64 bit. > > > > > > For CCSIDR register, 32 bit version follows specification [1]. > > > Conversely, 64 bit version follows specification [2]. > > > > > > [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/ > > > > > > Signed-off-by: Alireza Sanaee <alireza.san...@huawei.com> > > > --- > > > target/arm/cpu-features.h | 53 ++++++++++++++++++++++++ > > > target/arm/cpu64.c | 19 ++++++--- > > > target/arm/tcg/cpu64.c | 86 > > > ++++++++++++++++++--------------------- 3 files changed, 105 > > > insertions(+), 53 deletions(-) > > > > > > diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h > > > index c59ca104fe..00a0f0d963 100644 > > > --- a/target/arm/cpu-features.h > > > +++ b/target/arm/cpu-features.h > > > @@ -1022,6 +1022,59 @@ static inline bool > > > isar_feature_any_evt(const ARMISARegisters *id) return > > > isar_feature_aa64_evt(id) || isar_feature_aa32_evt(id); } > > > > > > +static inline uint64_t make_ccsidr32(unsigned assoc, unsigned > > > linesize, > > > + unsigned cachesize, uint8_t > > > flags) +{ > > > + unsigned lg_linesize = ctz32(linesize); > > > + unsigned sets; > > > + > > > + /* > > > + * The 32-bit CCSIDR_EL1 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 > > > + */ > > > + 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)(flags) << 28) > > > + | ((sets - 1) << 13) > > > + | ((assoc - 1) << 3) > > > + | (lg_linesize - 4); > > > +} > > > + > > > +static inline 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); > > > +} > > > + > > > /* > > > * Forward to the above feature tests given an ARMCPU pointer. > > > */ > > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > > > index 262a1d6c0b..57ebc1b979 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_ccsidr32(4, 64, 32 * KiB, 7); > > > + /* 48KB L1 icache */ > > > + cpu->ccsidr[1] = make_ccsidr32(3, 64, 48 * KiB, 2); > > > + /* 2048KB L2 cache */ > > > + cpu->ccsidr[2] = make_ccsidr32(16, 64, 2 * MiB, 7); > > > > I like the uses of make_ccsidrXX() instead of the magic values. > > > > I don't like much the code duplication between make_ccsidrXX() > > definitions, I'd prefer both call a common (static?) one. > > How about we have > typedef enum { > CCSIDR_FORMAT_LEGACY, > CCSIDR_FORMAT_CCIDX, > } CCSIDRFormat; > > and a single > uint64_t make_ccsidr(CCSIDRFormat format, unsigned assoc, unsigned > linesize, unsigned cachesize, unsigned flags); > > ? Since the only difference between the two functions is the final > line that assembles the return value, that seems like maybe > a better way to avoid the code duplication than a common > sub-function. > > -- PMM I like this suggestion. I can address Philippe's concern too if I move functions around. I thought a bit how to avoid duplication then I ended up saying let's see what others might say. Alireza
