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

[Alireza Sanaee]

On Mon, 2 Sep 2024 11:25:36 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 2 Sept 2024 at 11:07, Philippe Mathieu-Daudé
> <philmd@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.sanaee@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