* [PATCH 0/1] Add cortex-m0+ support @ 2024-10-22 20:34 Matthieu Castet 2024-10-22 20:34 ` [PATCH 1/1] target/arm: " Matthieu Castet 0 siblings, 1 reply; 7+ messages in thread From: Matthieu Castet @ 2024-10-22 20:34 UTC (permalink / raw) To: qemu-devel; +Cc: Matthieu Castet Hello, I have a patch that should allow to support cortex-m0+. I used it in a special arm virtual machine that is still in progress (and not submitted). I think this can be usefull. Matthieu Castet (1): target/arm: Add cortex-m0+ support hw/intc/armv7m_nvic.c | 38 +++++++++++++++++++++++++++++++++----- target/arm/cpu.c | 4 ++-- target/arm/ptw.c | 23 +++++++++++++++++++---- target/arm/tcg/cpu-v7m.c | 21 ++++++++++++++++++++- 4 files changed, 74 insertions(+), 12 deletions(-) -- 2.39.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] target/arm: Add cortex-m0+ support 2024-10-22 20:34 [PATCH 0/1] Add cortex-m0+ support Matthieu Castet @ 2024-10-22 20:34 ` Matthieu Castet 2024-10-28 15:27 ` Peter Maydell 0 siblings, 1 reply; 7+ messages in thread From: Matthieu Castet @ 2024-10-22 20:34 UTC (permalink / raw) To: qemu-devel; +Cc: Matthieu Castet, Peter Maydell, open list:ARM cores Signed-off-by: Matthieu Castet<castet.matthieu@free.fr> --- hw/intc/armv7m_nvic.c | 38 +++++++++++++++++++++++++++++++++----- target/arm/cpu.c | 4 ++-- target/arm/ptw.c | 23 +++++++++++++++++++---- target/arm/tcg/cpu-v7m.c | 21 ++++++++++++++++++++- 4 files changed, 74 insertions(+), 12 deletions(-) diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c index 98f3cf59bc..ed084e9db3 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) */ @@ -1876,6 +1876,14 @@ 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) + goto bad_offset; + + /* do not support size less than 256 */ + 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. @@ -1900,12 +1908,13 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, tlb_flush(CPU(cpu)); break; } - 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) */ + case 0xda0: /* MPU_RASR (v6M/v7M), MPU_RLAR (v8M) */ + case 0xda8: /* MPU_RASR_A1 (v6M/v7M), MPU_RLAR_A1 (v8M) */ + case 0xdb0: /* MPU_RASR_A2 (v6M/v7M), MPU_RLAR_A2 (v8M) */ + case 0xdb8: /* MPU_RASR_A3 (v6M/v7M), MPU_RLAR_A3 (v8M) */ { int region = cpu->env.pmsav7.rnr[attrs.secure]; + int rsize; if (arm_feature(&cpu->env, ARM_FEATURE_V8)) { /* PMSAv8M handling of the aliases is different from v7M: @@ -1926,6 +1935,25 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, return; } + rsize = extract32(value, 1, 5); + if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) { + if (offset != 0xda0) + goto bad_offset; + /* for armv6-m rsize >= 7 (min 256) */ + if (rsize < 7) { + qemu_log_mask(LOG_GUEST_ERROR, + "MPU region size too small %d\n", rsize); + return; + } + } + + /* for armv7-m rsize >= 4 (min 32) */ + if (rsize < 4) { + qemu_log_mask(LOG_GUEST_ERROR, + "MPU region size too small %d\n", rsize); + return; + } + if (region >= cpu->pmsav7_dregion) { return; } diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 1320fd8c8f..875e3aab69 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -508,7 +508,7 @@ static void arm_cpu_reset_hold(Object *obj, ResetType type) sizeof(*env->pmsav8.rlar[M_REG_S]) * cpu->pmsav7_dregion); } - } else if (arm_feature(env, ARM_FEATURE_V7)) { + } else if (arm_feature(env, ARM_FEATURE_M)) { memset(env->pmsav7.drbar, 0, sizeof(*env->pmsav7.drbar) * cpu->pmsav7_dregion); memset(env->pmsav7.drsr, 0, @@ -2454,7 +2454,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) } if (arm_feature(env, ARM_FEATURE_PMSA) && - arm_feature(env, ARM_FEATURE_V7)) { + arm_feature(env, ARM_FEATURE_M)) { uint32_t nr = cpu->pmsav7_dregion; if (nr > 0xff) { diff --git a/target/arm/ptw.c b/target/arm/ptw.c index dd40268397..fa771907e3 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -2383,6 +2383,13 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx, return regime_sctlr(env, mmu_idx) & SCTLR_BR; } +/* armv6m PMSAv6 is mostly compatible with PMSAv7, + * main difference : + * - min region size is 256 instead of 32 + * - TEX can be only 0 (Tex not used by qemu) + * - no alias register + * - HardFault instead of MemManage + */ static bool get_phys_addr_pmsav7(CPUARMState *env, S1Translate *ptw, uint32_t address, @@ -2423,11 +2430,19 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, continue; } - if (!rsize) { + /* Issue warning for invalid values + * for armv7-m rsize >= 4 (min 32) + * for armv6-m rsize >= 7 (min 256) + */ + if (!rsize || + (arm_feature(env, ARM_FEATURE_M) && ( + rsize < 7 || + (rsize < 4 && !arm_feature(env, ARM_FEATURE_V7))))) { qemu_log_mask(LOG_GUEST_ERROR, - "DRSR[%d]: Rsize field cannot be 0\n", n); + "DRSR[%d]: Rsize field cannot be %d\n", n, rsize); continue; } + rsize++; rmask = (1ull << rsize) - 1; @@ -3515,8 +3530,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, /* PMSAv8 */ ret = get_phys_addr_pmsav8(env, ptw, address, access_type, result, fi); - } else if (arm_feature(env, ARM_FEATURE_V7)) { - /* PMSAv7 */ + } else if (arm_feature(env, ARM_FEATURE_V7) || arm_feature(env, ARM_FEATURE_M)) { + /* PMSAv7 or PMSAv6 */ ret = get_phys_addr_pmsav7(env, ptw, address, access_type, result, fi); } else { diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c index 58e54578d6..01bc5d4375 100644 --- a/target/arm/tcg/cpu-v7m.c +++ b/target/arm/tcg/cpu-v7m.c @@ -76,6 +76,20 @@ static void cortex_m0_initfn(Object *obj) cpu->isar.id_isar6 = 0x00000000; } +static void cortex_m0p_initfn(Object *obj) +{ + ARMCPU *cpu = ARM_CPU(obj); + + /* cortex-m0p is a cortex-m0 with + * vtor and mpu extension + */ + cortex_m0_initfn(obj); + + cpu->midr = 0x410cc601; + cpu->pmsav7_dregion = 8; +} + + static void cortex_m3_initfn(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); @@ -111,6 +125,7 @@ static void cortex_m4_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); cpu->midr = 0x410fc240; /* r0p0 */ cpu->pmsav7_dregion = 8; + /* VFPv4-SP */ cpu->isar.mvfr0 = 0x10110021; cpu->isar.mvfr1 = 0x11000011; cpu->isar.mvfr2 = 0x00000000; @@ -141,6 +156,7 @@ static void cortex_m7_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); cpu->midr = 0x411fc272; /* r1p2 */ cpu->pmsav7_dregion = 8; + /* VFPv5 DP */ cpu->isar.mvfr0 = 0x10110221; cpu->isar.mvfr1 = 0x12000011; cpu->isar.mvfr2 = 0x00000040; @@ -173,6 +189,7 @@ static void cortex_m33_initfn(Object *obj) cpu->midr = 0x410fd213; /* r0p3 */ cpu->pmsav7_dregion = 16; cpu->sau_sregion = 8; + /* VFPv5 DP */ cpu->isar.mvfr0 = 0x10110021; cpu->isar.mvfr1 = 0x11000011; cpu->isar.mvfr2 = 0x00000040; @@ -209,7 +226,7 @@ static void cortex_m55_initfn(Object *obj) cpu->revidr = 0; cpu->pmsav7_dregion = 16; cpu->sau_sregion = 8; - /* These are the MVFR* values for the FPU + full MVE configuration */ + /* These are the MVFR* values for the FPv5-D16-M + full MVE configuration */ cpu->isar.mvfr0 = 0x10110221; cpu->isar.mvfr1 = 0x12100211; cpu->isar.mvfr2 = 0x00000040; @@ -267,6 +284,8 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data) static const ARMCPUInfo arm_v7m_cpus[] = { { .name = "cortex-m0", .initfn = cortex_m0_initfn, .class_init = arm_v7m_class_init }, + { .name = "cortex-m0p", .initfn = cortex_m0p_initfn, + .class_init = arm_v7m_class_init }, { .name = "cortex-m3", .initfn = cortex_m3_initfn, .class_init = arm_v7m_class_init }, { .name = "cortex-m4", .initfn = cortex_m4_initfn, -- 2.39.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] target/arm: Add cortex-m0+ support 2024-10-22 20:34 ` [PATCH 1/1] target/arm: " Matthieu Castet @ 2024-10-28 15:27 ` Peter Maydell 2024-10-29 20:47 ` castet.matthieu 0 siblings, 1 reply; 7+ messages in thread From: Peter Maydell @ 2024-10-28 15:27 UTC (permalink / raw) To: Matthieu Castet; +Cc: qemu-devel, open list:ARM cores On Tue, 22 Oct 2024 at 21:34, Matthieu Castet <castet.matthieu@free.fr> wrote: > > Signed-off-by: Matthieu Castet<castet.matthieu@free.fr> Hi; thanks for this patch. I have some initial code review comments, but the change looks broadly correct to me and I don't think there's anything obvious missing. The commit message body here is empty. Commits should almost always have some description in the body of what they are doing and why. (You don't need a separate cover-letter email for single-patch patches, by the way -- only for multi-patch series.) > --- > hw/intc/armv7m_nvic.c | 38 +++++++++++++++++++++++++++++++++----- > target/arm/cpu.c | 4 ++-- > target/arm/ptw.c | 23 +++++++++++++++++++---- > target/arm/tcg/cpu-v7m.c | 21 ++++++++++++++++++++- > 4 files changed, 74 insertions(+), 12 deletions(-) > > diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c > index 98f3cf59bc..ed084e9db3 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) */ The v6M spec says that it doesn't have the _A* aliases for RBAR and RASR so we should have a check to make those goto bad_offset in both read and write functions. > @@ -1876,6 +1876,14 @@ 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) > + goto bad_offset; Our coding style says all if() statements need braces, even one-line ones. The indentation here also seems to be off -- our indent is four-space. > + > + /* do not support size less than 256 */ > + value &= ~0xe0; This line doesn't look like it does what the comment suggests. A region size of 256 bytes would be a SIZE field of 7, where the size field is bits [5:1]. This &= ~0xe0 will (unless I've got confused) zero bits [3:1], which would turn a SIZE of 7 into an invalid SIZE of 0. More generally: in QEMU if we're enforcing limits on the values a register field can hold we typically do it on the guest-write path. Then the read path can just return the value in the register, which we already know to be in-range. (The other approach we often take which is quite frequently what real hardware does is "allow writes of any value, read back the value written, do some vaguely plausible thing at execution time when we use the value".) > + } > + > if (value & (1 << 4)) { > /* VALID bit means use the region number specified in this > * value and also update MPU_RNR.REGION with that value. > @@ -1900,12 +1908,13 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, > tlb_flush(CPU(cpu)); > break; > } > - 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) */ > + case 0xda0: /* MPU_RASR (v6M/v7M), MPU_RLAR (v8M) */ > + case 0xda8: /* MPU_RASR_A1 (v6M/v7M), MPU_RLAR_A1 (v8M) */ > + case 0xdb0: /* MPU_RASR_A2 (v6M/v7M), MPU_RLAR_A2 (v8M) */ > + case 0xdb8: /* MPU_RASR_A3 (v6M/v7M), MPU_RLAR_A3 (v8M) */ v6M doesn't have the RASR_A* aliases, so those lines shouldn't change. > { > int region = cpu->env.pmsav7.rnr[attrs.secure]; > + int rsize; > > if (arm_feature(&cpu->env, ARM_FEATURE_V8)) { > /* PMSAv8M handling of the aliases is different from v7M: > @@ -1926,6 +1935,25 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, > return; > } > > + rsize = extract32(value, 1, 5); > + if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) { > + if (offset != 0xda0) > + goto bad_offset; > + /* for armv6-m rsize >= 7 (min 256) */ > + if (rsize < 7) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "MPU region size too small %d\n", rsize); > + return; > + } > + } > + > + /* for armv7-m rsize >= 4 (min 32) */ > + if (rsize < 4) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "MPU region size too small %d\n", rsize); > + return; > + } You don't need to handle and log too-large region sizes both in the register-set codepath and then again later when we do the address translation. If you're ruling them out here then they'll never happen later. Duplicating the qemu_log_mask() line is awkward -- maybe better to have rsize_min = arm_feature(&s->cpu->env, ARM_FEATURE_V7) ? 4 : 7; if (rsize < rsize_min) { qemu_log_mask(...) } > + > if (region >= cpu->pmsav7_dregion) { > return; > } > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 1320fd8c8f..875e3aab69 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -508,7 +508,7 @@ static void arm_cpu_reset_hold(Object *obj, ResetType type) > sizeof(*env->pmsav8.rlar[M_REG_S]) > * cpu->pmsav7_dregion); > } > - } else if (arm_feature(env, ARM_FEATURE_V7)) { > + } else if (arm_feature(env, ARM_FEATURE_M)) { > memset(env->pmsav7.drbar, 0, > sizeof(*env->pmsav7.drbar) * cpu->pmsav7_dregion); > memset(env->pmsav7.drsr, 0, > @@ -2454,7 +2454,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > } > > if (arm_feature(env, ARM_FEATURE_PMSA) && > - arm_feature(env, ARM_FEATURE_V7)) { > + arm_feature(env, ARM_FEATURE_M)) { > uint32_t nr = cpu->pmsav7_dregion; > > if (nr > 0xff) { These changes don't look correct -- they will break the handling of R-profile PMSAv7 CPUs like the Cortex-R5. > diff --git a/target/arm/ptw.c b/target/arm/ptw.c > index dd40268397..fa771907e3 100644 > --- a/target/arm/ptw.c > +++ b/target/arm/ptw.c > @@ -2383,6 +2383,13 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx, > return regime_sctlr(env, mmu_idx) & SCTLR_BR; > } > > +/* armv6m PMSAv6 is mostly compatible with PMSAv7, > + * main difference : > + * - min region size is 256 instead of 32 > + * - TEX can be only 0 (Tex not used by qemu) > + * - no alias register > + * - HardFault instead of MemManage > + */ Our coding style for multiline comments says /* * they look like this, with the opening and closing markers on * lines of their own */ This comment might be better placed at the point below where we call get_phys_addr_psav7(), because it is the explanation for why on M-profile we call that function even if we don't have PMSAv7. > static bool get_phys_addr_pmsav7(CPUARMState *env, > S1Translate *ptw, > uint32_t address, > @@ -2423,11 +2430,19 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, > continue; > } > > - if (!rsize) { > + /* Issue warning for invalid values > + * for armv7-m rsize >= 4 (min 32) > + * for armv6-m rsize >= 7 (min 256) > + */ > + if (!rsize || > + (arm_feature(env, ARM_FEATURE_M) && ( > + rsize < 7 || > + (rsize < 4 && !arm_feature(env, ARM_FEATURE_V7))))) { > qemu_log_mask(LOG_GUEST_ERROR, > - "DRSR[%d]: Rsize field cannot be 0\n", n); > + "DRSR[%d]: Rsize field cannot be %d\n", n, rsize); > continue; > } > + > rsize++; > rmask = (1ull << rsize) - 1; > > @@ -3515,8 +3530,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, > /* PMSAv8 */ > ret = get_phys_addr_pmsav8(env, ptw, address, access_type, > result, fi); > - } else if (arm_feature(env, ARM_FEATURE_V7)) { > - /* PMSAv7 */ > + } else if (arm_feature(env, ARM_FEATURE_V7) || arm_feature(env, ARM_FEATURE_M)) { > + /* PMSAv7 or PMSAv6 */ This is specifically M-profile PMSAv6. R-profile PMSAv6 (which we do not implement) is a bit different. > ret = get_phys_addr_pmsav7(env, ptw, address, access_type, > result, fi); > } else { > diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c > index 58e54578d6..01bc5d4375 100644 > --- a/target/arm/tcg/cpu-v7m.c > +++ b/target/arm/tcg/cpu-v7m.c > @@ -76,6 +76,20 @@ static void cortex_m0_initfn(Object *obj) > cpu->isar.id_isar6 = 0x00000000; > } > > +static void cortex_m0p_initfn(Object *obj) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + > + /* cortex-m0p is a cortex-m0 with > + * vtor and mpu extension > + */ > + cortex_m0_initfn(obj); > + > + cpu->midr = 0x410cc601; > + cpu->pmsav7_dregion = 8; > +} > + > + > static void cortex_m3_initfn(Object *obj) > { > ARMCPU *cpu = ARM_CPU(obj); > @@ -111,6 +125,7 @@ static void cortex_m4_initfn(Object *obj) > set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); > cpu->midr = 0x410fc240; /* r0p0 */ > cpu->pmsav7_dregion = 8; > + /* VFPv4-SP */ > cpu->isar.mvfr0 = 0x10110021; > cpu->isar.mvfr1 = 0x11000011; > cpu->isar.mvfr2 = 0x00000000; > @@ -141,6 +156,7 @@ static void cortex_m7_initfn(Object *obj) > set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); > cpu->midr = 0x411fc272; /* r1p2 */ > cpu->pmsav7_dregion = 8; > + /* VFPv5 DP */ > cpu->isar.mvfr0 = 0x10110221; > cpu->isar.mvfr1 = 0x12000011; > cpu->isar.mvfr2 = 0x00000040; > @@ -173,6 +189,7 @@ static void cortex_m33_initfn(Object *obj) > cpu->midr = 0x410fd213; /* r0p3 */ > cpu->pmsav7_dregion = 16; > cpu->sau_sregion = 8; > + /* VFPv5 DP */ > cpu->isar.mvfr0 = 0x10110021; > cpu->isar.mvfr1 = 0x11000011; > cpu->isar.mvfr2 = 0x00000040; > @@ -209,7 +226,7 @@ static void cortex_m55_initfn(Object *obj) > cpu->revidr = 0; > cpu->pmsav7_dregion = 16; > cpu->sau_sregion = 8; > - /* These are the MVFR* values for the FPU + full MVE configuration */ > + /* These are the MVFR* values for the FPv5-D16-M + full MVE configuration */ > cpu->isar.mvfr0 = 0x10110221; > cpu->isar.mvfr1 = 0x12100211; > cpu->isar.mvfr2 = 0x00000040; These comment additions/changes don't seem to be related to the Cortex-M0+. If you want to make them, put them in a separate commit with its own commit message saying why. > @@ -267,6 +284,8 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data) > static const ARMCPUInfo arm_v7m_cpus[] = { > { .name = "cortex-m0", .initfn = cortex_m0_initfn, > .class_init = arm_v7m_class_init }, > + { .name = "cortex-m0p", .initfn = cortex_m0p_initfn, > + .class_init = arm_v7m_class_init }, > { .name = "cortex-m3", .initfn = cortex_m3_initfn, > .class_init = arm_v7m_class_init }, > { .name = "cortex-m4", .initfn = cortex_m4_initfn, > -- > 2.39.5 thanks -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] target/arm: Add cortex-m0+ support 2024-10-28 15:27 ` Peter Maydell @ 2024-10-29 20:47 ` castet.matthieu 2024-10-30 16:16 ` Peter Maydell 0 siblings, 1 reply; 7+ messages in thread From: castet.matthieu @ 2024-10-29 20:47 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-devel, open list:ARM cores Hi Peter, >On Tue, 22 Oct 2024 at 21:34, Matthieu Castet <castet.matthieu@free.fr> wrote: >> >> Signed-off-by: Matthieu Castet<castet.matthieu@free.fr> > >Hi; thanks for this patch. I have some initial code review comments, >but the change looks broadly correct to me and I don't think >there's anything obvious missing. > >The commit message body here is empty. Commits should >almost always have some description in the body of >what they are doing and why. Thanks for the review. When I sent the patch, I didn't put my last version and some of your comment were already fixed. Sorry for that. > >(You don't need a separate cover-letter email for >single-patch patches, by the way -- only for multi-patch series.) > ok >> --- >> hw/intc/armv7m_nvic.c | 38 +++++++++++++++++++++++++++++++++----- >> target/arm/cpu.c | 4 ++-- >> target/arm/ptw.c | 23 +++++++++++++++++++---- >> target/arm/tcg/cpu-v7m.c | 21 ++++++++++++++++++++- >> 4 files changed, 74 insertions(+), 12 deletions(-) >> >> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c >> index 98f3cf59bc..ed084e9db3 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) */ > >The v6M spec says that it doesn't have the _A* aliases >for RBAR and RASR so we should have a check to make those >goto bad_offset in both read and write functions. ok, I will add the bad_offset to read > >> @@ -1876,6 +1876,14 @@ 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) >> + goto bad_offset; > >Our coding style says all if() statements need braces, even >one-line ones. The indentation here also seems to be off -- >our indent is four-space. > I will fix the code >> + >> + /* do not support size less than 256 */ >> + value &= ~0xe0; > >This line doesn't look like it does what the comment suggests. >A region size of 256 bytes would be a SIZE field of 7, where >the size field is bits [5:1]. This &= ~0xe0 will (unless I've >got confused) zero bits [3:1], which would turn a SIZE of 7 >into an invalid SIZE of 0. The code is checking region address alignement, not region size. ADDR is encoded on bits [31:8] on armv6-m and bits [31:5] on armv7-m. I will update the comment to /* armv6-m do not support region address with alignement * less than 256. Force alignement. */ > >More generally: in QEMU if we're enforcing limits on the >values a register field can hold we typically do it on the >guest-write path. Then the read path can just return the >value in the register, which we already know to be in-range. Ok >> - 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) */ >> + case 0xda0: /* MPU_RASR (v6M/v7M), MPU_RLAR (v8M) */ >> + case 0xda8: /* MPU_RASR_A1 (v6M/v7M), MPU_RLAR_A1 (v8M) */ >> + case 0xdb0: /* MPU_RASR_A2 (v6M/v7M), MPU_RLAR_A2 (v8M) */ >> + case 0xdb8: /* MPU_RASR_A3 (v6M/v7M), MPU_RLAR_A3 (v8M) */ > >v6M doesn't have the RASR_A* aliases, so those lines shouldn't change. ok > >> { >> int region = cpu->env.pmsav7.rnr[attrs.secure]; >> + int rsize; >> >> if (arm_feature(&cpu->env, ARM_FEATURE_V8)) { >> /* PMSAv8M handling of the aliases is different from v7M: >> @@ -1926,6 +1935,25 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, >> return; >> } >> >> + rsize = extract32(value, 1, 5); >> + if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) { >> + if (offset != 0xda0) >> + goto bad_offset; >> + /* for armv6-m rsize >= 7 (min 256) */ >> + if (rsize < 7) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "MPU region size too small %d\n", rsize); >> + return; >> + } >> + } >> + >> + /* for armv7-m rsize >= 4 (min 32) */ >> + if (rsize < 4) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "MPU region size too small %d\n", rsize); >> + return; >> + } > >You don't need to handle and log too-large region >sizes both in the register-set codepath and then again >later when we do the address translation. If you're ruling >them out here then they'll never happen later. Ok, let's handle it here in the write path. > >Duplicating the qemu_log_mask() line is awkward -- maybe >better to have > rsize_min = arm_feature(&s->cpu->env, ARM_FEATURE_V7) ? 4 : 7; > if (rsize < rsize_min) { > qemu_log_mask(...) > } > Ok, I will do that >> + >> if (region >= cpu->pmsav7_dregion) { >> return; >> } >> diff --git a/target/arm/cpu.c b/target/arm/cpu.c >> index 1320fd8c8f..875e3aab69 100644 >> --- a/target/arm/cpu.c >> +++ b/target/arm/cpu.c >> @@ -508,7 +508,7 @@ static void arm_cpu_reset_hold(Object *obj, ResetType type) >> sizeof(*env->pmsav8.rlar[M_REG_S]) >> * cpu->pmsav7_dregion); >> } >> - } else if (arm_feature(env, ARM_FEATURE_V7)) { >> + } else if (arm_feature(env, ARM_FEATURE_M)) { >> memset(env->pmsav7.drbar, 0, >> sizeof(*env->pmsav7.drbar) * cpu->pmsav7_dregion); >> memset(env->pmsav7.drsr, 0, >> @@ -2454,7 +2454,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) >> } >> >> if (arm_feature(env, ARM_FEATURE_PMSA) && >> - arm_feature(env, ARM_FEATURE_V7)) { >> + arm_feature(env, ARM_FEATURE_M)) { >> uint32_t nr = cpu->pmsav7_dregion; >> >> if (nr > 0xff) { > >These changes don't look correct -- they will break the handling >of R-profile PMSAv7 CPUs like the Cortex-R5. You are right, I changed it to : @@ -508,7 +508,8 @@ static void arm_cpu_reset_hold(Object *obj, ResetType type) sizeof(*env->pmsav8.rlar[M_REG_S]) * cpu->pmsav7_dregion); } - } else if (arm_feature(env, ARM_FEATURE_V7)) { + } else if (arm_feature(env, ARM_FEATURE_V7) || + arm_feature(env, ARM_FEATURE_M)) { memset(env->pmsav7.drbar, 0, sizeof(*env->pmsav7.drbar) * cpu->pmsav7_dregion); memset(env->pmsav7.drsr, 0, @@ -2454,7 +2455,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) } if (arm_feature(env, ARM_FEATURE_PMSA) && - arm_feature(env, ARM_FEATURE_V7)) { + (arm_feature(env, ARM_FEATURE_V7) || arm_feature(env, ARM_FEATURE_M))) { uint32_t nr = cpu->pmsav7_dregion; if (nr > 0xff) { Is that correct ? > >> diff --git a/target/arm/ptw.c b/target/arm/ptw.c >> index dd40268397..fa771907e3 100644 >> --- a/target/arm/ptw.c >> +++ b/target/arm/ptw.c >> @@ -2383,6 +2383,13 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx, >> return regime_sctlr(env, mmu_idx) & SCTLR_BR; >> } >> >> +/* armv6m PMSAv6 is mostly compatible with PMSAv7, >> + * main difference : >> + * - min region size is 256 instead of 32 >> + * - TEX can be only 0 (Tex not used by qemu) >> + * - no alias register >> + * - HardFault instead of MemManage >> + */ > >Our coding style for multiline comments says >/* > * they look like this, with the opening and closing markers on > * lines of their own > */ I will changed that. It do not seem to be followed in In hw/intc/armv7m_nvic.c, but my new comment should also follow this rule in this file ? > >This comment might be better placed at the point below where >we call get_phys_addr_psav7(), because it is the explanation >for why on M-profile we call that function even if we don't >have PMSAv7. ok > >> static bool get_phys_addr_pmsav7(CPUARMState *env, >> S1Translate *ptw, >> uint32_t address, >> @@ -2423,11 +2430,19 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, >> continue; >> } >> >> - if (!rsize) { >> + /* Issue warning for invalid values >> + * for armv7-m rsize >= 4 (min 32) >> + * for armv6-m rsize >= 7 (min 256) >> + */ >> + if (!rsize || >> + (arm_feature(env, ARM_FEATURE_M) && ( >> + rsize < 7 || >> + (rsize < 4 && !arm_feature(env, ARM_FEATURE_V7))))) { >> qemu_log_mask(LOG_GUEST_ERROR, >> - "DRSR[%d]: Rsize field cannot be 0\n", n); >> + "DRSR[%d]: Rsize field cannot be %d\n", n, rsize); >> continue; >> } >> + >> rsize++; >> rmask = (1ull << rsize) - 1; I will remove this modification. Size check is done in write. >> >> @@ -3515,8 +3530,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, >> /* PMSAv8 */ >> ret = get_phys_addr_pmsav8(env, ptw, address, access_type, >> result, fi); >> - } else if (arm_feature(env, ARM_FEATURE_V7)) { >> - /* PMSAv7 */ >> + } else if (arm_feature(env, ARM_FEATURE_V7) || arm_feature(env, ARM_FEATURE_M)) { >> + /* PMSAv7 or PMSAv6 */ > >This is specifically M-profile PMSAv6. R-profile PMSAv6 >(which we do not implement) is a bit different. I change the comment to "armv6-m PMSAv6". >> static void cortex_m3_initfn(Object *obj) >> { >> ARMCPU *cpu = ARM_CPU(obj); >> @@ -111,6 +125,7 @@ static void cortex_m4_initfn(Object *obj) >> set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); >> cpu->midr = 0x410fc240; /* r0p0 */ >> cpu->pmsav7_dregion = 8; >> + /* VFPv4-SP */ >> cpu->isar.mvfr0 = 0x10110021; >> cpu->isar.mvfr1 = 0x11000011; >> cpu->isar.mvfr2 = 0x00000000; >> @@ -141,6 +156,7 @@ static void cortex_m7_initfn(Object *obj) >> set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); >> cpu->midr = 0x411fc272; /* r1p2 */ >> cpu->pmsav7_dregion = 8; >> + /* VFPv5 DP */ >> cpu->isar.mvfr0 = 0x10110221; >> cpu->isar.mvfr1 = 0x12000011; >> cpu->isar.mvfr2 = 0x00000040; >> @@ -173,6 +189,7 @@ static void cortex_m33_initfn(Object *obj) >> cpu->midr = 0x410fd213; /* r0p3 */ >> cpu->pmsav7_dregion = 16; >> cpu->sau_sregion = 8; >> + /* VFPv5 DP */ >> cpu->isar.mvfr0 = 0x10110021; >> cpu->isar.mvfr1 = 0x11000011; >> cpu->isar.mvfr2 = 0x00000040; >> @@ -209,7 +226,7 @@ static void cortex_m55_initfn(Object *obj) >> cpu->revidr = 0; >> cpu->pmsav7_dregion = 16; >> cpu->sau_sregion = 8; >> - /* These are the MVFR* values for the FPU + full MVE configuration */ >> + /* These are the MVFR* values for the FPv5-D16-M + full MVE configuration */ >> cpu->isar.mvfr0 = 0x10110221; >> cpu->isar.mvfr1 = 0x12100211; >> cpu->isar.mvfr2 = 0x00000040; > >These comment additions/changes don't seem to be related to the >Cortex-M0+. If you want to make them, put them in a >separate commit with its own commit message saying why. I will remove this comments, and may be send them in extra patch. If everything is ok, I will send a V2 patch with your review. Thanks Matthieu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] target/arm: Add cortex-m0+ support 2024-10-29 20:47 ` castet.matthieu @ 2024-10-30 16:16 ` Peter Maydell 0 siblings, 0 replies; 7+ messages in thread From: Peter Maydell @ 2024-10-30 16:16 UTC (permalink / raw) To: castet.matthieu; +Cc: qemu-devel, open list:ARM cores On Tue, 29 Oct 2024 at 20:47, <castet.matthieu@free.fr> wrote: > >Our coding style for multiline comments says > >/* > > * they look like this, with the opening and closing markers on > > * lines of their own > > */ > > I will changed that. > > It do not seem to be followed in In hw/intc/armv7m_nvic.c, but my new comment should also follow > this rule in this file ? Yes, please. We have quite a lot of older code that was written before we imposed some of the coding style standardisations we now use, so you'll see stuff that doesn't quite follow the rules in files already. Our general policy is that we don't bother to try to tidy up pre-existing code, but where we're adding new code or if we have to touch the code for some other reason we'll clean it up in that local area. (A rough guide is "if the checkpatch script complains about it consider fixing it", though checkpatch is not always correct about its suggestions.) thanks -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <20241022200310.175432-1-castet.matthieu@free.fr>]
* [PATCH 1/1] target/arm: Add cortex-m0+ support [not found] <20241022200310.175432-1-castet.matthieu@free.fr> @ 2024-10-22 20:03 ` Matthieu Castet 2024-10-22 20:10 ` castet.matthieu 0 siblings, 1 reply; 7+ messages in thread From: Matthieu Castet @ 2024-10-22 20:03 UTC (permalink / raw) To: castet.matthieu Cc: Peter Maydell, open list:ARM cores, open list:All patches CC here Signed-off-by: Matthieu Castet<castet.matthieu@free.fr> --- hw/intc/armv7m_nvic.c | 38 +++++++++++++++++++++++++++++++++----- target/arm/cpu.c | 4 ++-- target/arm/ptw.c | 23 +++++++++++++++++++---- target/arm/tcg/cpu-v7m.c | 21 ++++++++++++++++++++- 4 files changed, 74 insertions(+), 12 deletions(-) diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c index 98f3cf59bc..ed084e9db3 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) */ @@ -1876,6 +1876,14 @@ 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) + goto bad_offset; + + /* do not support size less than 256 */ + 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. @@ -1900,12 +1908,13 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, tlb_flush(CPU(cpu)); break; } - 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) */ + case 0xda0: /* MPU_RASR (v6M/v7M), MPU_RLAR (v8M) */ + case 0xda8: /* MPU_RASR_A1 (v6M/v7M), MPU_RLAR_A1 (v8M) */ + case 0xdb0: /* MPU_RASR_A2 (v6M/v7M), MPU_RLAR_A2 (v8M) */ + case 0xdb8: /* MPU_RASR_A3 (v6M/v7M), MPU_RLAR_A3 (v8M) */ { int region = cpu->env.pmsav7.rnr[attrs.secure]; + int rsize; if (arm_feature(&cpu->env, ARM_FEATURE_V8)) { /* PMSAv8M handling of the aliases is different from v7M: @@ -1926,6 +1935,25 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, return; } + rsize = extract32(value, 1, 5); + if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) { + if (offset != 0xda0) + goto bad_offset; + /* for armv6-m rsize >= 7 (min 256) */ + if (rsize < 7) { + qemu_log_mask(LOG_GUEST_ERROR, + "MPU region size too small %d\n", rsize); + return; + } + } + + /* for armv7-m rsize >= 4 (min 32) */ + if (rsize < 4) { + qemu_log_mask(LOG_GUEST_ERROR, + "MPU region size too small %d\n", rsize); + return; + } + if (region >= cpu->pmsav7_dregion) { return; } diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 1320fd8c8f..875e3aab69 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -508,7 +508,7 @@ static void arm_cpu_reset_hold(Object *obj, ResetType type) sizeof(*env->pmsav8.rlar[M_REG_S]) * cpu->pmsav7_dregion); } - } else if (arm_feature(env, ARM_FEATURE_V7)) { + } else if (arm_feature(env, ARM_FEATURE_M)) { memset(env->pmsav7.drbar, 0, sizeof(*env->pmsav7.drbar) * cpu->pmsav7_dregion); memset(env->pmsav7.drsr, 0, @@ -2454,7 +2454,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) } if (arm_feature(env, ARM_FEATURE_PMSA) && - arm_feature(env, ARM_FEATURE_V7)) { + arm_feature(env, ARM_FEATURE_M)) { uint32_t nr = cpu->pmsav7_dregion; if (nr > 0xff) { diff --git a/target/arm/ptw.c b/target/arm/ptw.c index dd40268397..fa771907e3 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -2383,6 +2383,13 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx, return regime_sctlr(env, mmu_idx) & SCTLR_BR; } +/* armv6m PMSAv6 is mostly compatible with PMSAv7, + * main difference : + * - min region size is 256 instead of 32 + * - TEX can be only 0 (Tex not used by qemu) + * - no alias register + * - HardFault instead of MemManage + */ static bool get_phys_addr_pmsav7(CPUARMState *env, S1Translate *ptw, uint32_t address, @@ -2423,11 +2430,19 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, continue; } - if (!rsize) { + /* Issue warning for invalid values + * for armv7-m rsize >= 4 (min 32) + * for armv6-m rsize >= 7 (min 256) + */ + if (!rsize || + (arm_feature(env, ARM_FEATURE_M) && ( + rsize < 7 || + (rsize < 4 && !arm_feature(env, ARM_FEATURE_V7))))) { qemu_log_mask(LOG_GUEST_ERROR, - "DRSR[%d]: Rsize field cannot be 0\n", n); + "DRSR[%d]: Rsize field cannot be %d\n", n, rsize); continue; } + rsize++; rmask = (1ull << rsize) - 1; @@ -3515,8 +3530,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, /* PMSAv8 */ ret = get_phys_addr_pmsav8(env, ptw, address, access_type, result, fi); - } else if (arm_feature(env, ARM_FEATURE_V7)) { - /* PMSAv7 */ + } else if (arm_feature(env, ARM_FEATURE_V7) || arm_feature(env, ARM_FEATURE_M)) { + /* PMSAv7 or PMSAv6 */ ret = get_phys_addr_pmsav7(env, ptw, address, access_type, result, fi); } else { diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c index 58e54578d6..01bc5d4375 100644 --- a/target/arm/tcg/cpu-v7m.c +++ b/target/arm/tcg/cpu-v7m.c @@ -76,6 +76,20 @@ static void cortex_m0_initfn(Object *obj) cpu->isar.id_isar6 = 0x00000000; } +static void cortex_m0p_initfn(Object *obj) +{ + ARMCPU *cpu = ARM_CPU(obj); + + /* cortex-m0p is a cortex-m0 with + * vtor and mpu extension + */ + cortex_m0_initfn(obj); + + cpu->midr = 0x410cc601; + cpu->pmsav7_dregion = 8; +} + + static void cortex_m3_initfn(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); @@ -111,6 +125,7 @@ static void cortex_m4_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); cpu->midr = 0x410fc240; /* r0p0 */ cpu->pmsav7_dregion = 8; + /* VFPv4-SP */ cpu->isar.mvfr0 = 0x10110021; cpu->isar.mvfr1 = 0x11000011; cpu->isar.mvfr2 = 0x00000000; @@ -141,6 +156,7 @@ static void cortex_m7_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); cpu->midr = 0x411fc272; /* r1p2 */ cpu->pmsav7_dregion = 8; + /* VFPv5 DP */ cpu->isar.mvfr0 = 0x10110221; cpu->isar.mvfr1 = 0x12000011; cpu->isar.mvfr2 = 0x00000040; @@ -173,6 +189,7 @@ static void cortex_m33_initfn(Object *obj) cpu->midr = 0x410fd213; /* r0p3 */ cpu->pmsav7_dregion = 16; cpu->sau_sregion = 8; + /* VFPv5 DP */ cpu->isar.mvfr0 = 0x10110021; cpu->isar.mvfr1 = 0x11000011; cpu->isar.mvfr2 = 0x00000040; @@ -209,7 +226,7 @@ static void cortex_m55_initfn(Object *obj) cpu->revidr = 0; cpu->pmsav7_dregion = 16; cpu->sau_sregion = 8; - /* These are the MVFR* values for the FPU + full MVE configuration */ + /* These are the MVFR* values for the FPv5-D16-M + full MVE configuration */ cpu->isar.mvfr0 = 0x10110221; cpu->isar.mvfr1 = 0x12100211; cpu->isar.mvfr2 = 0x00000040; @@ -267,6 +284,8 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data) static const ARMCPUInfo arm_v7m_cpus[] = { { .name = "cortex-m0", .initfn = cortex_m0_initfn, .class_init = arm_v7m_class_init }, + { .name = "cortex-m0p", .initfn = cortex_m0p_initfn, + .class_init = arm_v7m_class_init }, { .name = "cortex-m3", .initfn = cortex_m3_initfn, .class_init = arm_v7m_class_init }, { .name = "cortex-m4", .initfn = cortex_m4_initfn, -- 2.39.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] target/arm: Add cortex-m0+ support 2024-10-22 20:03 ` Matthieu Castet @ 2024-10-22 20:10 ` castet.matthieu 0 siblings, 0 replies; 7+ messages in thread From: castet.matthieu @ 2024-10-22 20:10 UTC (permalink / raw) To: castet matthieu Cc: Peter Maydell, open list:ARM cores, open list:All patches CC here [-- Attachment #1: Type: text/plain, Size: 8092 bytes --] Hello, Please ignore this mail, I was doing some test and didn't realise real email was added. I will resend the real one. Thanks. ----- Mail original ----- De: "Matthieu Castet" <castet.matthieu@free.fr> À: "castet matthieu" <castet.matthieu@free.fr> Cc: "Peter Maydell" <peter.maydell@linaro.org>, "open list:ARM cores" <qemu-arm@nongnu.org>, "open list:All patches CC here" <qemu-devel@nongnu.org> Envoyé: Mardi 22 Octobre 2024 22:03:10 Objet: [PATCH 1/1] target/arm: Add cortex-m0+ support Signed-off-by: Matthieu Castet<castet.matthieu@free.fr> --- hw/intc/armv7m_nvic.c | 38 +++++++++++++++++++++++++++++++++----- target/arm/cpu.c | 4 ++-- target/arm/ptw.c | 23 +++++++++++++++++++---- target/arm/tcg/cpu-v7m.c | 21 ++++++++++++++++++++- 4 files changed, 74 insertions(+), 12 deletions(-) diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c index 98f3cf59bc..ed084e9db3 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) */ @@ -1876,6 +1876,14 @@ 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) + goto bad_offset; + + /* do not support size less than 256 */ + 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. @@ -1900,12 +1908,13 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, tlb_flush(CPU(cpu)); break; } - 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) */ + case 0xda0: /* MPU_RASR (v6M/v7M), MPU_RLAR (v8M) */ + case 0xda8: /* MPU_RASR_A1 (v6M/v7M), MPU_RLAR_A1 (v8M) */ + case 0xdb0: /* MPU_RASR_A2 (v6M/v7M), MPU_RLAR_A2 (v8M) */ + case 0xdb8: /* MPU_RASR_A3 (v6M/v7M), MPU_RLAR_A3 (v8M) */ { int region = cpu->env.pmsav7.rnr[attrs.secure]; + int rsize; if (arm_feature(&cpu->env, ARM_FEATURE_V8)) { /* PMSAv8M handling of the aliases is different from v7M: @@ -1926,6 +1935,25 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value, return; } + rsize = extract32(value, 1, 5); + if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) { + if (offset != 0xda0) + goto bad_offset; + /* for armv6-m rsize >= 7 (min 256) */ + if (rsize < 7) { + qemu_log_mask(LOG_GUEST_ERROR, + "MPU region size too small %d\n", rsize); + return; + } + } + + /* for armv7-m rsize >= 4 (min 32) */ + if (rsize < 4) { + qemu_log_mask(LOG_GUEST_ERROR, + "MPU region size too small %d\n", rsize); + return; + } + if (region >= cpu->pmsav7_dregion) { return; } diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 1320fd8c8f..875e3aab69 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -508,7 +508,7 @@ static void arm_cpu_reset_hold(Object *obj, ResetType type) sizeof(*env->pmsav8.rlar[M_REG_S]) * cpu->pmsav7_dregion); } - } else if (arm_feature(env, ARM_FEATURE_V7)) { + } else if (arm_feature(env, ARM_FEATURE_M)) { memset(env->pmsav7.drbar, 0, sizeof(*env->pmsav7.drbar) * cpu->pmsav7_dregion); memset(env->pmsav7.drsr, 0, @@ -2454,7 +2454,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) } if (arm_feature(env, ARM_FEATURE_PMSA) && - arm_feature(env, ARM_FEATURE_V7)) { + arm_feature(env, ARM_FEATURE_M)) { uint32_t nr = cpu->pmsav7_dregion; if (nr > 0xff) { diff --git a/target/arm/ptw.c b/target/arm/ptw.c index dd40268397..fa771907e3 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -2383,6 +2383,13 @@ static bool pmsav7_use_background_region(ARMCPU *cpu, ARMMMUIdx mmu_idx, return regime_sctlr(env, mmu_idx) & SCTLR_BR; } +/* armv6m PMSAv6 is mostly compatible with PMSAv7, + * main difference : + * - min region size is 256 instead of 32 + * - TEX can be only 0 (Tex not used by qemu) + * - no alias register + * - HardFault instead of MemManage + */ static bool get_phys_addr_pmsav7(CPUARMState *env, S1Translate *ptw, uint32_t address, @@ -2423,11 +2430,19 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, continue; } - if (!rsize) { + /* Issue warning for invalid values + * for armv7-m rsize >= 4 (min 32) + * for armv6-m rsize >= 7 (min 256) + */ + if (!rsize || + (arm_feature(env, ARM_FEATURE_M) && ( + rsize < 7 || + (rsize < 4 && !arm_feature(env, ARM_FEATURE_V7))))) { qemu_log_mask(LOG_GUEST_ERROR, - "DRSR[%d]: Rsize field cannot be 0\n", n); + "DRSR[%d]: Rsize field cannot be %d\n", n, rsize); continue; } + rsize++; rmask = (1ull << rsize) - 1; @@ -3515,8 +3530,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw, /* PMSAv8 */ ret = get_phys_addr_pmsav8(env, ptw, address, access_type, result, fi); - } else if (arm_feature(env, ARM_FEATURE_V7)) { - /* PMSAv7 */ + } else if (arm_feature(env, ARM_FEATURE_V7) || arm_feature(env, ARM_FEATURE_M)) { + /* PMSAv7 or PMSAv6 */ ret = get_phys_addr_pmsav7(env, ptw, address, access_type, result, fi); } else { diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c index 58e54578d6..01bc5d4375 100644 --- a/target/arm/tcg/cpu-v7m.c +++ b/target/arm/tcg/cpu-v7m.c @@ -76,6 +76,20 @@ static void cortex_m0_initfn(Object *obj) cpu->isar.id_isar6 = 0x00000000; } +static void cortex_m0p_initfn(Object *obj) +{ + ARMCPU *cpu = ARM_CPU(obj); + + /* cortex-m0p is a cortex-m0 with + * vtor and mpu extension + */ + cortex_m0_initfn(obj); + + cpu->midr = 0x410cc601; + cpu->pmsav7_dregion = 8; +} + + static void cortex_m3_initfn(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); @@ -111,6 +125,7 @@ static void cortex_m4_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); cpu->midr = 0x410fc240; /* r0p0 */ cpu->pmsav7_dregion = 8; + /* VFPv4-SP */ cpu->isar.mvfr0 = 0x10110021; cpu->isar.mvfr1 = 0x11000011; cpu->isar.mvfr2 = 0x00000000; @@ -141,6 +156,7 @@ static void cortex_m7_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); cpu->midr = 0x411fc272; /* r1p2 */ cpu->pmsav7_dregion = 8; + /* VFPv5 DP */ cpu->isar.mvfr0 = 0x10110221; cpu->isar.mvfr1 = 0x12000011; cpu->isar.mvfr2 = 0x00000040; @@ -173,6 +189,7 @@ static void cortex_m33_initfn(Object *obj) cpu->midr = 0x410fd213; /* r0p3 */ cpu->pmsav7_dregion = 16; cpu->sau_sregion = 8; + /* VFPv5 DP */ cpu->isar.mvfr0 = 0x10110021; cpu->isar.mvfr1 = 0x11000011; cpu->isar.mvfr2 = 0x00000040; @@ -209,7 +226,7 @@ static void cortex_m55_initfn(Object *obj) cpu->revidr = 0; cpu->pmsav7_dregion = 16; cpu->sau_sregion = 8; - /* These are the MVFR* values for the FPU + full MVE configuration */ + /* These are the MVFR* values for the FPv5-D16-M + full MVE configuration */ cpu->isar.mvfr0 = 0x10110221; cpu->isar.mvfr1 = 0x12100211; cpu->isar.mvfr2 = 0x00000040; @@ -267,6 +284,8 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data) static const ARMCPUInfo arm_v7m_cpus[] = { { .name = "cortex-m0", .initfn = cortex_m0_initfn, .class_init = arm_v7m_class_init }, + { .name = "cortex-m0p", .initfn = cortex_m0p_initfn, + .class_init = arm_v7m_class_init }, { .name = "cortex-m3", .initfn = cortex_m3_initfn, .class_init = arm_v7m_class_init }, { .name = "cortex-m4", .initfn = cortex_m4_initfn, -- 2.39.5 [-- Attachment #2: Type: text/html, Size: 14939 bytes --] ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-10-30 16:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-22 20:34 [PATCH 0/1] Add cortex-m0+ support Matthieu Castet
2024-10-22 20:34 ` [PATCH 1/1] target/arm: " Matthieu Castet
2024-10-28 15:27 ` Peter Maydell
2024-10-29 20:47 ` castet.matthieu
2024-10-30 16:16 ` Peter Maydell
[not found] <20241022200310.175432-1-castet.matthieu@free.fr>
2024-10-22 20:03 ` Matthieu Castet
2024-10-22 20:10 ` castet.matthieu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).