On Wed, 13 Nov 2024 at 17:36, Matthieu Castet <castet.matth...@free.fr> wrote:
>
> Signed-off-by: Matthieu Castet<castet.matth...@free.fr>
Hi; thanks for version 2; this looks pretty good and I just
have a couple of small nits.
Firstly, there's still no commit message body text here.
Commit messages should explain the intention of the commit
and roughly what the change is doing. If you look at
"git log" you'll see that subject-line-only commit messages
are the exception, not the norm (and are usually only for
pretty mechanical changes done as part of a whole series
that's doing the same kind of change).
> ---
> hw/intc/armv7m_nvic.c | 51 ++++++++++++++++++++++++++++++++++++++--
> target/arm/cpu.c | 5 ++--
> target/arm/ptw.c | 14 +++++++++--
> target/arm/tcg/cpu-v7m.c | 17 ++++++++++++++
> 4 files changed, 81 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 98f3cf59bc..41d2b98ee4 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -1386,7 +1386,7 @@ static uint32_t nvic_readl(NVICState *s, uint32_t
> offset, MemTxAttrs attrs)
> }
> return (cpu->env.pmsav7.drbar[region] & ~0x1f) | (region & 0xf);
> }
> - case 0xda0: /* MPU_RASR (v7M), MPU_RLAR (v8M) */
> + case 0xda0: /* MPU_RASR (v6M/v7M), MPU_RLAR (v8M) */
> case 0xda8: /* MPU_RASR_A1 (v7M), MPU_RLAR_A1 (v8M) */
> case 0xdb0: /* MPU_RASR_A2 (v7M), MPU_RLAR_A2 (v8M) */
> case 0xdb8: /* MPU_RASR_A3 (v7M), MPU_RLAR_A3 (v8M) */
> @@ -1407,6 +1407,14 @@ static uint32_t nvic_readl(NVICState *s, uint32_t
> offset, MemTxAttrs attrs)
> }
> return cpu->env.pmsav8.rlar[attrs.secure][region];
> }
> + if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
> + /*
> + * armv6-m do not support A alias
> + */
> + if (offset != 0xda0) {
> + goto bad_offset;
> + }
> + }
I think it would be neater to do the "alias registers not supported" as:
case 0xda0: /* MPU_RASR (v7M), MPU_RLAR (v8M) */
case 0xda8: /* MPU_RASR_A1 (v7M), MPU_RLAR_A1 (v8M) */
case 0xdb0: /* MPU_RASR_A2 (v7M), MPU_RLAR_A2 (v8M) */
case 0xdb8: /* MPU_RASR_A3 (v7M), MPU_RLAR_A3 (v8M) */
if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
/* No alias registers in v6M */
goto bad_offset;
}
/* fall through */
case 0xda0: /* MPU_RASR (v7M), MPU_RLAR (v8M) */
[code for MPU_RASR starts here]
You're also missing the "no RBAR aliases" handling in nvic_readl().
> if (region >= cpu->pmsav7_dregion) {
> return 0;
> @@ -1876,6 +1884,21 @@ static void nvic_writel(NVICState *s, uint32_t offset,
> uint32_t value,
> return;
> }
>
> + if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
> + if (offset != 0xd9c) {
> + /*
> + * armv6-m do not support A alias
> + */
> + goto bad_offset;
> + }
> +
> + /*
> + * armv6-m do not support region address with alignement
> + * less than 256. Force alignement.
Typos: "does not", "alignment".
> + */
> + value &= ~0xe0;
> + }
> +
> if (value & (1 << 4)) {
> /* VALID bit means use the region number specified in this
> * value and also update MPU_RNR.REGION with that value.
The rest of the patch looks good.
PS: there's no rush about producing a v3 -- we're currently in
freeze for the 9.2 release, so this won't be going into git
until we make the 9.2 release in late December.
thanks
-- PMM
