On Tue, 8 Oct 2024 at 18:32, Richard Henderson
<richard.henderson@xxxxxxxxxx> wrote:
>
> On 10/8/24 07:45, Peter Maydell wrote:
> > On Sat, 5 Oct 2024 at 21:06, Richard Henderson
> > <richard.henderson@xxxxxxxxxx> wrote:
> >>
> >> Zero is the safe do-nothing value for callers to use.
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@xxxxxxxxxx>
> >> ---
> >> target/arm/internals.h | 3 ++-
> >> target/arm/ptw.c | 2 +-
> >> target/arm/tcg/m_helper.c | 8 ++++----
> >> target/arm/tcg/tlb_helper.c | 2 +-
> >> 4 files changed, 8 insertions(+), 7 deletions(-)
> >
> >> diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
> >> index 23d7f73035..f7354f3c6e 100644
> >> --- a/target/arm/tcg/m_helper.c
> >> +++ b/target/arm/tcg/m_helper.c
> >> @@ -222,7 +222,7 @@ static bool v7m_stack_write(ARMCPU *cpu, uint32_t addr, uint32_t value,
> >> int exc;
> >> bool exc_secure;
> >>
> >> - if (get_phys_addr(env, addr, MMU_DATA_STORE, mmu_idx, &res, &fi)) {
> >> + if (get_phys_addr(env, addr, MMU_DATA_STORE, 0, mmu_idx, &res, &fi)) {
> >> /* MPU/SAU lookup failed */
> >> if (fi.type == ARMFault_QEMU_SFault) {
> >> if (mode == STACK_LAZYFP) {
> >> @@ -311,7 +311,7 @@ static bool v7m_stack_read(ARMCPU *cpu, uint32_t *dest, uint32_t addr,
> >> bool exc_secure;
> >> uint32_t value;
> >>
> >> - if (get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, &res, &fi)) {
> >> + if (get_phys_addr(env, addr, MMU_DATA_LOAD, 0, mmu_idx, &res, &fi)) {
> >> /* MPU/SAU lookup failed */
> >> if (fi.type == ARMFault_QEMU_SFault) {
> >> qemu_log_mask(CPU_LOG_INT,
> >
> > We do actually know what kind of memory operation we're doing here:
> > it's a 4-byte access. (It should never be unaligned because an M-profile
> > SP can't ever be un-4-aligned, though I forget whether our implementation
> > really enforces that.)
> >
> >> @@ -2009,7 +2009,7 @@ static bool v7m_read_half_insn(ARMCPU *cpu, ARMMMUIdx mmu_idx, bool secure,
> >> "...really SecureFault with SFSR.INVEP\n");
> >> return false;
> >> }
> >> - if (get_phys_addr(env, addr, MMU_INST_FETCH, mmu_idx, &res, &fi)) {
> >> + if (get_phys_addr(env, addr, MMU_INST_FETCH, 0, mmu_idx, &res, &fi)) {
> >> /* the MPU lookup failed */
> >> env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_IACCVIOL_MASK;
> >> armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM, env->v7m.secure);
> >
> > Similarly this is a 16-bit load that in theory should never
> > be possible to be unaligned.
> >
> >> @@ -2045,7 +2045,7 @@ static bool v7m_read_sg_stack_word(ARMCPU *cpu, ARMMMUIdx mmu_idx,
> >> ARMMMUFaultInfo fi = {};
> >> uint32_t value;
> >>
> >> - if (get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, &res, &fi)) {
> >> + if (get_phys_addr(env, addr, MMU_DATA_LOAD, 0, mmu_idx, &res, &fi)) {
> >> /* MPU/SAU lookup failed */
> >> if (fi.type == ARMFault_QEMU_SFault) {
> >> qemu_log_mask(CPU_LOG_INT,
> >
> > and this is another 4-byte load via sp.
> >
> >> diff --git a/target/arm/tcg/tlb_helper.c b/target/arm/tcg/tlb_helper.c
> >> index 885bf4ec14..1d8b7bcaa2 100644
> >> --- a/target/arm/tcg/tlb_helper.c
> >> +++ b/target/arm/tcg/tlb_helper.c
> >> @@ -344,7 +344,7 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >> * return false. Otherwise populate fsr with ARM DFSR/IFSR fault
> >> * register format, and signal the fault.
> >> */
> >> - ret = get_phys_addr(&cpu->env, address, access_type,
> >> + ret = get_phys_addr(&cpu->env, address, access_type, 0,
> >> core_to_arm_mmu_idx(&cpu->env, mmu_idx),
> >> &res, fi);
> >> if (likely(!ret)) {
>
> The question is: if it should be impossible for them to be misaligned, should we pass an
> argument that checks alignment and then (!) potentially raise a guest exception.
>
> I suspect the answer is no.
>
> If it should be impossible, no alignment fault is ever visible to the guest in this
> context, then we should at most assert(), otherwise do nothing.
>
> We *can* pass, e.g. MO_32 or MO_16 for documentation purposes, if you like. Without
> additional adornment, this does not imply alignment enforcement (i.e. MO_ALIGN). But this
> would be functionally indistinguishable from 0 (which I imperfectly documented with "or 0"
> in the function block comments).
Mmm. I think I thought when I started reviewing this series that we might
have a bigger set of "we put in 0 here but is that the right thing?"
callsites. But I think in working through it it turns out there aren't
very many and for all of those "don't check alignment" is the right thing.
thanks
-- PMM
