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
