Re: [Qemu-devel] [PATCH v2] target-arm: raise exception on misaligned LDREX operands

[Peter Maydell]

On 3 December 2015 at 18:36, Andrew Baumann
<address@hidden> wrote:
> Qemu does not generally perform alignment checks. However, the ARM ARM
> requires implementation of alignment exceptions for a number of cases
> including LDREX, and Windows-on-ARM relies on this.
>
> This change adds plumbing to enable alignment checks on loads using
> MO_ALIGN, a do_unaligned_access hook to raise the exception (data
> abort), and uses the new aligned loads in LDREX (for all but
> single-byte loads).
>
> Signed-off-by: Andrew Baumann <address@hidden>
> ---
> Thanks for the feedback on v1! I wish I had known about (or gone
> looking for) MO_ALIGN sooner...
>
> arm_regime_using_lpae_format() is a no-op wrapper I added to export
> regime_using_lpae_format (which is a static inline). Would it be
> preferable to simply export the existing function, and rename it? If
> so, is this still the correct name to use for the function?

The way you have it seems OK to me.

> +/* Raise a data fault alignment exception for the specified virtual address 
> */
> +void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
> +                                 int is_user, uintptr_t retaddr)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    int target_el;
> +    bool same_el;
> +
> +    if (retaddr) {
> +        /* now we have a real cpu fault */
> +        cpu_restore_state(cs, retaddr);
> +    }
> +
> +    target_el = exception_target_el(env);
> +    same_el = (arm_current_el(env) == target_el);
> +
> +    env->exception.vaddress = vaddr;
> +
> +    /* the DFSR for an alignment fault depends on whether we're using
> +     * the LPAE long descriptor format, or the short descriptor format */
> +    if (arm_regime_using_lpae_format(env, cpu_mmu_index(env, false))) {
> +        env->exception.fsr = 0x21;
> +    } else {
> +        env->exception.fsr = 0x1;
> +    }
> +
> +    raise_exception(env, EXCP_DATA_ABORT,
> +                    syn_data_abort(same_el, 0, 0, 0, 0, 0x21),
> +                    target_el);
> +}

This isn't propagating the 'read or write' information
from is_write into the syndrome and DFSR. You need this minor
tweak:

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index c6995ca..3e5e0d3 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -154,8 +154,12 @@ void arm_cpu_do_unaligned_access(CPUState *cs,
vaddr vaddr, int is_write,
         env->exception.fsr = 0x1;
     }

+    if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
+        env->exception.fsr |= (1 << 11);
+    }
+
     raise_exception(env, EXCP_DATA_ABORT,
-                    syn_data_abort(same_el, 0, 0, 0, 0, 0x21),
+                    syn_data_abort(same_el, 0, 0, 0, is_write == 1, 0x21),
                     target_el);
 }

(compare the similar code in tlb_fill()).

I'm just going to squash that in when I apply this to target-arm.next,
to save you having to respin.

thanks
-- PMM