[5/8] hw/intc/armv7m_nvic: Implement cache ID registers

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.maydell@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).
---
 target/arm/cpu.h      |  9 +++++++++
 hw/intc/armv7m_nvic.c | 13 +++++++++++++
 target/arm/machine.c  | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

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.maydell@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)

> +    case 0xd84: /* CSSELR */
> +        return cpu->env.v7m.csselr[attrs.secure];
>      /* TODO: Implement debug registers.  */
>      case 0xd90: /* MPU_TYPE */
>          /* Unified MPU; if the MPU is not present this value is zero */
> @@ -1385,6 +1393,11 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
>          qemu_log_mask(LOG_UNIMP,
>                        "NVIC: Aux fault status registers unimplemented\n");
>          break;
> +    case 0xd84: /* CSSELR */
> +        if (!arm_v7m_csselr_razwi(cpu)) {
> +            cpu->env.v7m.csselr[attrs.secure] = value & 0xf;
> +        }
> +        break;
>      case 0xd90: /* MPU_TYPE */
>          return; /* RO */
>      case 0xd94: /* MPU_CTRL */
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index a85c2430d3..968ec30b4a 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -108,6 +108,41 @@ static const VMStateDescription vmstate_m_faultmask_primask = {
>      }
>  };
>  
> +/* CSSELR is in a subsection because we didn't implement it previously.
> + * Migration from an old implementation will leave it at zero, which
> + * is OK since the only CPUs in the old implementation make the
> + * register RAZ/WI.
> + * Since there was no version of QEMU which implemented the CSSELR for
> + * just non-secure, we transfer both banks here rather than putting
> + * the secure banked version in the m-security subsection.
> + */
> +static bool csselr_vmstate_validate(void *opaque, int version_id)
> +{
> +    ARMCPU *cpu = opaque;
> +
> +    return cpu->env.v7m.csselr[M_REG_NS] < sizeof(cpu->ccsidr)
> +        && cpu->env.v7m.csselr[M_REG_S] < sizeof(cpu->ccsidr);
> +}
> +
> +static bool m_csselr_needed(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +
> +    return !arm_v7m_csselr_razwi(cpu);
> +}
> +
> +static const VMStateDescription vmstate_m_csselr = {
> +    .name = "cpu/m/csselr",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = m_csselr_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(env.v7m.csselr, ARMCPU, M_REG_NUM_BANKS),
> +        VMSTATE_VALIDATE("CSSELR is valid", csselr_vmstate_validate),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_m = {
>      .name = "cpu/m",
>      .version_id = 4,
> @@ -129,6 +164,7 @@ static const VMStateDescription vmstate_m = {
>      },
>      .subsections = (const VMStateDescription*[]) {
>          &vmstate_m_faultmask_primask,
> +        &vmstate_m_csselr,
>          NULL
>      }
>  };
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

On 5 February 2018 at 23:53, Philippe Mathieu-Daudé <f4bug@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.maydell@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