Hi Richard, While testing the future V9, I've some regressions on a custom board using cortex-R5 CPUs. Unaligned data accesses are no longer allowed because of that patch.
I've dug into the various documentation and it seems that R-profile CPUs don't have the same default memory type as A-profile. It depends on a default memory map provided in the R-Profile RM in C1.3 [1], even when PMU is disabled. > Each PMSAv8-32 MPU has an associated default memory map which is used when > the MPU is not enabled. > ... > Table C1-4 and Table C1-5 describe the default memory map defined for the EL1 > MPU. For our case, Table C1-5 can be simplified as: | 0x00000000 – 0x7FFFFFFF Normal | 0x80000000 – 0xBFFFFFFF Device-nGnRE | 0xC0000000 – 0xFFFFFFFF Device-nGnRnE Therefore, we can't blindly enable strict alignment checking solely on SCTLR bits. We should make it depend on the address targeted. But is it possible to know that address in `aprofile_require_alignment` ? with `mmu_idx` ? By the way, are R-Profile CPUs the same as those having the `PMSA` feature ? That could mean we can use the `ARM_FEATURE_PMSA` to deal with that, instead of create a new `ARM_FEATURE_R` Note that the RM I've linked is for ARMv8. But this other link [2] seems to show a similar behavior for arm-v7. cc Jonathan and Ard, though not sure this is the same bug you've reported earlier. Thanks, Clément [1] https://developer.arm.com/documentation/ddi0568/a-c/?lang=en [2] https://developer.arm.com/documentation/ddi0406/cb/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/About-the-PMSA/Enabling-and-disabling-the-MPU?lang=en#BEIJEFCJ On Fri, Mar 1, 2024 at 9:43 PM Richard Henderson <richard.hender...@linaro.org> wrote: > > If translation is disabled, the default memory type is Device, which > requires alignment checking. This is more optimally done early via > the MemOp given to the TCG memory operation. > > Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org> > Reported-by: Idan Horowitz <idan.horow...@gmail.com> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1204 > Signed-off-by: Richard Henderson <richard.hender...@linaro.org> > --- > target/arm/tcg/hflags.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c > index 8e5d35d922..5da1b0fc1d 100644 > --- a/target/arm/tcg/hflags.c > +++ b/target/arm/tcg/hflags.c > @@ -26,6 +26,35 @@ static inline bool fgt_svc(CPUARMState *env, int el) > FIELD_EX64(env->cp15.fgt_exec[FGTREG_HFGITR], HFGITR_EL2, SVC_EL1); > } > > +/* Return true if memory alignment should be enforced. */ > +static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t > sctlr) > +{ > +#ifdef CONFIG_USER_ONLY > + return false; > +#else > + /* Check the alignment enable bit. */ > + if (sctlr & SCTLR_A) { > + return true; > + } > + > + /* > + * If translation is disabled, then the default memory type is > + * Device(-nGnRnE) instead of Normal, which requires that alignment > + * be enforced. Since this affects all ram, it is most efficient > + * to handle this during translation. > + */ > + if (sctlr & SCTLR_M) { > + /* Translation enabled: memory type in PTE via MAIR_ELx. */ > + return false; > + } > + if (el < 2 && (arm_hcr_el2_eff(env) & (HCR_DC | HCR_VM))) { > + /* Stage 2 translation enabled: memory type in PTE. */ > + return false; > + } > + return true; > +#endif > +} > + > static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el, > ARMMMUIdx mmu_idx, > CPUARMTBFlags flags) > @@ -121,8 +150,9 @@ static CPUARMTBFlags rebuild_hflags_a32(CPUARMState *env, > int fp_el, > { > CPUARMTBFlags flags = {}; > int el = arm_current_el(env); > + uint64_t sctlr = arm_sctlr(env, el); > > - if (arm_sctlr(env, el) & SCTLR_A) { > + if (aprofile_require_alignment(env, el, sctlr)) { > DP_TBFLAG_ANY(flags, ALIGN_MEM, 1); > } > > @@ -223,7 +253,7 @@ static CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env, > int el, int fp_el, > > sctlr = regime_sctlr(env, stage1); > > - if (sctlr & SCTLR_A) { > + if (aprofile_require_alignment(env, el, sctlr)) { > DP_TBFLAG_ANY(flags, ALIGN_MEM, 1); > } > > -- > 2.34.1 > >
