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

[Philippe Mathieu-Daudé]

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):

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    assert(lg_linesize >= 4 && lg_linesize <= 7 + 4);
> +
> +    /* sets * associativity * linesize == 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;
> +    }
> +}