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

[Peter Maydell]

On Mon, 2 Sept 2024 at 11:28, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Alireza Sanaee via <qemu-arm@nongnu.org> writes:
>
> > 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)
> > +{
>
> isn't this returning a 32 bit value?
>
> > +    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);
>
> This is a nice improvement but using deposit() will ensure you don't
> accidentally overflow fields with the shift/or combos. So something
> like:
>
>   uint32_t ccsidr32 = 0;
>   ..
>   ccsidr32 = deposit32(ccsidr32, 28,  4, flags);
>   ccsidr32 = deposit32(ccsidr32, 13, 14, sets - 1);
>   ccsidr32 = deposit32(ccsidr32,  3, 10, assoc - 1);
>   ccsidr32 = deposit32(ccsidr32,  0,  3, lg_linesize - 1);
>
> And leave the compiler to simplify everything (not that it matters that
> much for an init function).
>
> Actually I note CCSIDR already has some field definitions so it would
> be:
>
>   ccsidr32 = FIELD_DP32(ccsidr32, CCSIDR_EL1, LINESIZE, lg_linesize -1);
>
> etc. Although I notice it two sets of defines to account for FEAT_CCIDX

Mmm. Though I feel like we (me absolutely included) are rather
getting into bikeshedding a fairly simple refactoring patch...

thanks
-- PMM