On 5 February 2018 at 23:53, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> Hi Peter,
>
> On 02/05/2018 07:57 AM, Peter Maydell wrote:
>> M profile cores have a similar setup for cache ID registers
>> to A profile:
>> * Cache Level ID Register (CLIDR) is a fixed value
>> * Cache Type Register (CTR) is a fixed value
>> * Cache Size ID Registers (CCSIDR) are a bank of registers;
>> which one you see is selected by the Cache Size Selection
>> Register (CSSELR)
>>
>> The only difference is that they're in the NVIC memory mapped
>> register space rather than being coprocessor registers.
>> Implement the M profile view of them.
>>
>> Since neither Cortex-M3 nor Cortex-M4 implement caches,
>> we don't need to update their init functions and can leave
>> the ctr/clidr/ccsidr[] fields in their ARMCPU structs at zero.
>> Newer cores (like the Cortex-M33) will want to be able to
>> set these ID registers to non-zero values, though.
>>
>> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
>> ---
>> The CSSELR/CCSIDR parts are a bit under-motivated, because
>> the Cortex-M33 doesn't implement caches either and so they
>> are RAZ/WI for that as well as M3/M4, though I'd written all
>> the code before I realized that. This will be helpful if
>> we ever need a Cortex-M7 model, though (which does have
>> a couple of CSSIDR array entries).
>
> I wonder if it is easier/faster to add a check for the "Instruction not
> Data" bit and the level value is not 7 (not permitted) or simple comments.
>
>> ---
>> target/arm/cpu.h | 9 +++++++++
>> hw/intc/armv7m_nvic.c | 13 +++++++++++++
>> target/arm/machine.c | 36 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 58 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index f21f68ec4a..99c7cb996f 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -453,6 +453,7 @@ typedef struct CPUARMState {
>> uint32_t faultmask[M_REG_NUM_BANKS];
>> uint32_t aircr; /* only holds r/w state if security extn
>> implemented */
>> uint32_t secure; /* Is CPU in Secure state? (not guest visible) */
>> + uint32_t csselr[M_REG_NUM_BANKS];
>> } v7m;
>>
>> /* Information associated with an exception about to be taken:
>> @@ -2443,6 +2444,14 @@ static inline int arm_debug_target_el(CPUARMState
>> *env)
>> }
>> }
>>
>> +static inline bool arm_v7m_csselr_razwi(ARMCPU *cpu)
>> +{
>> + /* If all the CLIDR.Ctypem bits are 0 there are no caches, and
>> + * CSSELR is RAZ/WI.
>> + */
>> + return (cpu->clidr & 0x001fffff) != 0;
>> +}
>
> Suggestion to be consistent with other bitfields:
>
> /* V7M Cache Level ID (CLIDR) */
> FIELD(V7M_CLIDR, CTYPE, 0, 7 * 3)
>
> So we can use:
>
> return (cpu->clidr & R_V7M_CLIDR_CTYPE_MASK) != 0;
>
>> +
>> static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>> {
>> if (arm_is_secure(env)) {
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index eb49fd77c7..cc83c9e553 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -1025,6 +1025,14 @@ static uint32_t nvic_readl(NVICState *s, uint32_t
>> offset, MemTxAttrs attrs)
>> return cpu->id_isar4;
>> case 0xd74: /* ISAR5. */
>> return cpu->id_isar5;
>> + case 0xd78: /* CLIDR */
>> + return cpu->clidr;
>> + case 0xd7c: /* CTR */
>> + return cpu->ctr;
>> + case 0xd80: /* CSSIDR */
>> + return cpu->ccsidr[cpu->env.v7m.csselr[attrs.secure] & 0xf];
>
> /* V7M Cache Size Selection (CSSELR) */
> FIELD(V7M_CSSELR, LEVEL, 1, 3)
Yes, but the index into the csselr[] array is by both the level
field and the I/D bit: csselr[0] is L1 dcache, csselr[1] is
L1 icache, csselr[2] is L2 dcache, and so on.
I guess we could define some field constants.
thanks
-- PMM
