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

[Alireza Sanaee via <qemu-devel@nongnu.org>]

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;
> > +    }
> > +}  
>