On Mon, 2 Sept 2024 at 11:28, Alex Bennée <alex.ben...@linaro.org> wrote: > > Alireza Sanaee via <qemu-...@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.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) > > +{ > > 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
