[Qemu-devel] [PATCH v3 0/7] ARM BE8/BE32 big-endian system-mode fixes (semihosting, gdbstub)

[Qemu-devel] [PATCH v3 0/7] ARM BE8/BE32 big-endian system-mode fixes (semihosting, gdbstub)

[Julian Brown]

This is the third iteration of a series of patches to implement
semihosting/gdbstub support for big-endian ARM system mode. The previous
series started here:

  http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00972.html

I've (hopefully!) addressed all the comments from the second round of
reviews, apologies in advance if I've missed anything.

Thanks,

Julian

Julian Brown (7):
  Add cfgend parameter for ARM CPU selection.
  Honour reset_sctlr EE/B bits during reset.
  Move target_memory_rw_debug function.
  ARM big-endian semihosting support.
  ARM big-endian system-mode gdbstub support.
  Fix Thumb-1 BE32 execution and disassembly.
  ARM BE32 watchpoint fix.

 disas.c                         |   1 +
 exec.c                          |   1 +
 gdbstub.c                       |  11 ----
 hw/arm/boot.c                   |  27 +++++++++
 hw/arm/integratorcp.c           |  19 +++++-
 include/disas/bfd.h             |   7 +++
 include/exec/cpu-all.h          |  22 +++++++
 include/exec/softmmu-arm-semi.h | 131 ++++++++++++++++++++++++++++++++++++++++
 include/qom/cpu.h               |   3 +
 qom/cpu.c                       |   6 ++
 target/arm/arm-semi.c           |   4 +-
 target/arm/arm_ldst.h           |  10 ++-
 target/arm/cpu.c                |  64 ++++++++++++++++++++
 target/arm/cpu.h                |  13 ++++
 target/arm/gdbstub.c            |  42 +++++++++++++
 target/arm/internals.h          |   5 ++
 target/arm/op_helper.c          |  22 +++++++
 17 files changed, 372 insertions(+), 16 deletions(-)
 create mode 100644 include/exec/softmmu-arm-semi.h

-- 
2.8.1

[Qemu-devel] [PATCH v3 1/7] Add cfgend parameter for ARM CPU selection.

[Julian Brown]

The new "cfgend" parameter can be given after the CPU name,
e.g. "-cpu=arm926,cfgend=yes". Unlike earlier versions of the patch,
CPU attribute parsing is left to board initialisation code: for the
"virt" board, this was being done already, and this patch makes the
integratorcp board init do so too.

Signed-off-by: Julian Brown <julian@codesourcery.com>
---
 hw/arm/integratorcp.c | 19 +++++++++++++++++--
 target/arm/cpu.c      | 14 ++++++++++++++
 target/arm/cpu.h      |  7 +++++++
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 039812a..337c689 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -535,27 +535,42 @@ static void integratorcp_init(MachineState *machine)
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
+    char **cpustr;
     ObjectClass *cpu_oc;
+    CPUClass *cc;
     Object *cpuobj;
     ARMCPU *cpu;
+    const char *typename;
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     MemoryRegion *ram_alias = g_new(MemoryRegion, 1);
     qemu_irq pic[32];
     DeviceState *dev, *sic, *icp;
     int i;
+    Error *err = NULL;
 
     if (!cpu_model) {
         cpu_model = "arm926";
     }
 
-    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
+    cpustr = g_strsplit(cpu_model, ",", 2);
+
+    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
     if (!cpu_oc) {
         fprintf(stderr, "Unable to find CPU definition\n");
         exit(1);
     }
+    typename = object_class_get_name(cpu_oc);
+
+    cc = CPU_CLASS(cpu_oc);
+    cc->parse_features(typename, cpustr[1], &err);
+    g_strfreev(cpustr);
+    if (err) {
+        error_report(err);
+        exit(1);
+    }
 
-    cpuobj = object_new(object_class_get_name(cpu_oc));
+    cpuobj = object_new(typename);
 
     /* By default ARM1176 CPUs have EL3 enabled.  This board does not
      * currently support EL3 so the CPU EL3 property is disabled before
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index c29aafe..bdf86de 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -33,6 +33,7 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
+#include "exec/cpu-common.h"
 
 static void arm_cpu_set_pc(CPUState *cs, vaddr value)
 {
@@ -502,6 +503,9 @@ static Property arm_cpu_has_el2_property =
 static Property arm_cpu_has_el3_property =
             DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, true);
 
+static Property arm_cpu_cfgend_property =
+            DEFINE_PROP_BOOL("cfgend", ARMCPU, cfgend, false);
+
 /* use property name "pmu" to match other archs and virt tools */
 static Property arm_cpu_has_pmu_property =
             DEFINE_PROP_BOOL("pmu", ARMCPU, has_pmu, true);
@@ -569,6 +573,8 @@ static void arm_cpu_post_init(Object *obj)
         }
     }
 
+    qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
+                             &error_abort);
 }
 
 static void arm_cpu_finalizefn(Object *obj)
@@ -770,6 +776,14 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
                            address_space_init_shareable(cs->memory,
                                                         "cpu-memory"),
                            ARMASIdx_NS);
+
+    if (cpu->cfgend) {
+        if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
+            cpu->reset_sctlr |= SCTLR_EE;
+        } else {
+            cpu->reset_sctlr |= SCTLR_B;
+        }
+    }
 #endif
 
     qemu_init_vcpu(cs);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 151a5d7..ac8d625 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -669,6 +669,13 @@ struct ARMCPU {
     int gic_vpribits; /* number of virtual priority bits */
     int gic_vprebits; /* number of virtual preemption bits */
 
+    /* Whether the cfgend input is high (i.e. this CPU should reset into
+     * big-endian mode).  This setting isn't used directly: instead it modifies
+     * the reset_sctlr value to have SCTLR_B or SCTLR_EE set, depending on the
+     * architecture version.
+     */
+    bool cfgend;
+
     ARMELChangeHook *el_change_hook;
     void *el_change_hook_opaque;
 };
-- 
2.8.1

Re: [Qemu-devel] [PATCH v3 1/7] Add cfgend parameter for ARM CPU selection.

[Peter Maydell]

On 20 January 2017 at 16:30, Julian Brown <julian@codesourcery.com> wrote:
> The new "cfgend" parameter can be given after the CPU name,
> e.g. "-cpu=arm926,cfgend=yes". Unlike earlier versions of the patch,
> CPU attribute parsing is left to board initialisation code: for the
> "virt" board, this was being done already, and this patch makes the
> integratorcp board init do so too.
>
> Signed-off-by: Julian Brown <julian@codesourcery.com>

This is sufficiently close to OK that I'm going to fix it up
in target-arm.next. Changes I'm making:

 * use error_report_err() rather than error_report() (I suspect this
   is a conflict with some API change that landed recently)
 * move the integratorcp changes to their own patch
 * drop an unnecessary extra #include;
 * move setting of reset_sctlr above registration of cpregs
   so it actually has an effect

That last one is the important one -- otherwise reset_sctlr
has already been copied into the cpregs data and the
changes to it won't be reflected in the values used for
reset. (You currently work around this with patch 3, which
we can then drop as unnecessary.)

thanks
-- PMM

[Qemu-devel] [PATCH v3 2/7] Honour reset_sctlr EE/B bits during reset.

[Julian Brown]

This patch uses the reset value of the SCTLR register (e.g. as modified
by the "cfgend" parameter introduced by the previous patch) to set the
endianness used by QEMU when it is invoked without a kernel or firmware
image. The intended use case is loading code via GDB as follows:

  (gdb) target remote | qemu-arm-system [options] -cpu=cortex-a15,cfgend=yes
  (gdb) load
  (gdb) [...]

The idea being to start a board in a "reset" state in the selected
endianness so that communication with GDB works properly. (The previous
version of the patch used "-kernel /dev/null" for the same purpose:
this version avoids that, and also avoids fiddling with info->endianness
setting.)

Signed-off-by: Julian Brown <julian@codesourcery.com>
---
 hw/arm/boot.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ff621e4..c059bd2 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -646,6 +646,33 @@ static void do_cpu_reset(void *opaque)
                 info->secondary_cpu_reset_hook(cpu, info);
             }
         }
+    } else {
+	/* No kernel image or firmware has been supplied.  Reset to the
+	 * default endianness for the current board (e.g. so that code loaded
+	 * via the gdb stub interface does the right thing).
+	 */
+	if (arm_feature(env, ARM_FEATURE_V7)) {
+	    int i;
+	    if (cpu->reset_sctlr & SCTLR_EE) {
+		env->cp15.sctlr_el[1] |= SCTLR_E0E;
+		for (i = 1; i < 4; ++i) {
+		    env->cp15.sctlr_el[i] |= SCTLR_EE;
+		}
+		env->uncached_cpsr |= CPSR_E;
+	    } else {
+                env->cp15.sctlr_el[1] &= ~SCTLR_E0E;
+                for (i = 1; i < 4; ++i) {
+                    env->cp15.sctlr_el[i] &= ~SCTLR_EE;
+                }
+                env->uncached_cpsr &= ~CPSR_E;
+	    }
+	} else {
+	    if (cpu->reset_sctlr & SCTLR_B) {
+		env->cp15.sctlr_el[1] |= SCTLR_B;
+	    } else {
+		env->cp15.sctlr_el[1] &= ~SCTLR_B;
+	    }
+	}
     }
 }
 
-- 
2.8.1

Re: [Qemu-devel] [PATCH v3 2/7] Honour reset_sctlr EE/B bits during reset.

[Peter Maydell]

On 20 January 2017 at 16:30, Julian Brown <julian@codesourcery.com> wrote:
> This patch uses the reset value of the SCTLR register (e.g. as modified
> by the "cfgend" parameter introduced by the previous patch) to set the
> endianness used by QEMU when it is invoked without a kernel or firmware
> image. The intended use case is loading code via GDB as follows:
>
>   (gdb) target remote | qemu-arm-system [options] -cpu=cortex-a15,cfgend=yes
>   (gdb) load
>   (gdb) [...]
>
> The idea being to start a board in a "reset" state in the selected
> endianness so that communication with GDB works properly. (The previous
> version of the patch used "-kernel /dev/null" for the same purpose:
> this version avoids that, and also avoids fiddling with info->endianness
> setting.)
>
> Signed-off-by: Julian Brown <julian@codesourcery.com>
> ---

This patch is just working around the bug in patch 1 that I
mention in my comments there, so we can drop it entirely.

thanks
-- PMM

[Qemu-devel] [PATCH v3 3/7] Move target_memory_rw_debug function.

[Julian Brown]

This patch moves the target_memory_rw_debug function to
include/exec/cpu-all.h, so that it can be used by the ARM semihosting
code as well as the gdbstub code. (I tried Peter Maydell's suggestion
of include/qom/cpu.h as a location for the function, but that raised
uncomfortably-many dependency problems for my taste).

Signed-off-by: Julian Brown <julian@codesourcery.com>
---
 gdbstub.c              | 11 -----------
 include/exec/cpu-all.h | 22 ++++++++++++++++++++++
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 64f2696..cb77e15 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -44,17 +44,6 @@
 #define GDB_ATTACHED "0"
 #endif
 
-static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
-                                         uint8_t *buf, int len, bool is_write)
-{
-    CPUClass *cc = CPU_GET_CLASS(cpu);
-
-    if (cc->memory_rw_debug) {
-        return cc->memory_rw_debug(cpu, addr, buf, len, is_write);
-    }
-    return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
-}
-
 enum {
     GDB_SIGNAL_0 = 0,
     GDB_SIGNAL_INT = 2,
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index ffe43d5..700b90d 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -347,4 +347,26 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
 
 int cpu_exec(CPUState *cpu);
 
+/**
+  * target_memory_rw_debug:
+  * @cpu: The CPU to use.
+  * @addr: The target address to read or write.
+  * @buf: Host buffer to read or write from or into.
+  * @len: Length of data.
+  * @is_write: True for write, false for read.
+  *
+  * A wrapper for cpu_memory_rw_debug that can be overridden with a
+  * CPUClass-specific version if required.
+  */
+static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
+                                         uint8_t *buf, int len, bool is_write)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->memory_rw_debug) {
+        return cc->memory_rw_debug(cpu, addr, buf, len, is_write);
+    }
+    return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
+}
+
 #endif /* CPU_ALL_H */
-- 
2.8.1

Re: [Qemu-devel] [PATCH v3 3/7] Move target_memory_rw_debug function.

[Peter Maydell]

On 20 January 2017 at 16:30, Julian Brown <julian@codesourcery.com> wrote:
> This patch moves the target_memory_rw_debug function to
> include/exec/cpu-all.h, so that it can be used by the ARM semihosting
> code as well as the gdbstub code. (I tried Peter Maydell's suggestion
> of include/qom/cpu.h as a location for the function, but that raised
> uncomfortably-many dependency problems for my taste).
>
> Signed-off-by: Julian Brown <julian@codesourcery.com>
> ---
>  gdbstub.c              | 11 -----------
>  include/exec/cpu-all.h | 22 ++++++++++++++++++++++
>  2 files changed, 22 insertions(+), 11 deletions(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v3 4/7] ARM big-endian semihosting support.

[Julian Brown]

This patch introduces an ARM-specific version of the memory read/write,
etc. functions used for semihosting, in order to byte-swap (big-endian)
target memory that is to be interpreted by the (little-endian) host.
The target_memory_rw_debug function is used that knows about the
byte-reversal used for BE32 system mode.

Signed-off-by: Julian Brown <julian@codesourcery.com>
---
 include/exec/softmmu-arm-semi.h | 131 ++++++++++++++++++++++++++++++++++++++++
 target/arm/arm-semi.c           |   4 +-
 target/arm/cpu.c                |  24 ++++++++
 target/arm/cpu.h                |   6 ++
 4 files changed, 163 insertions(+), 2 deletions(-)
 create mode 100644 include/exec/softmmu-arm-semi.h

diff --git a/include/exec/softmmu-arm-semi.h b/include/exec/softmmu-arm-semi.h
new file mode 100644
index 0000000..bba9ca6
--- /dev/null
+++ b/include/exec/softmmu-arm-semi.h
@@ -0,0 +1,131 @@
+/*
+ * Helper routines to provide target memory access for ARM semihosting
+ * syscalls in system emulation mode.
+ *
+ * Copyright (c) 2007 CodeSourcery, (c) 2016 Mentor Graphics
+ *
+ * This code is licensed under the GPL
+ */
+
+#ifndef SOFTMMU_ARM_SEMI_H
+#define SOFTMMU_ARM_SEMI_H 1
+
+/* In big-endian mode (either BE8 or BE32), values larger than a byte will be
+ * transferred to/from memory in big-endian format.  Assuming we're on a
+ * little-endian host machine, such values will need to be byteswapped before
+ * and after the host processes them.
+ *
+ * This means that byteswapping will occur *twice* in BE32 mode for
+ * halfword/word reads/writes.
+ */
+
+static inline bool arm_bswap_needed(CPUARMState *env)
+{
+#ifdef HOST_WORDS_BIGENDIAN
+#error HOST_WORDS_BIGENDIAN is not supported for ARM semihosting at the moment.
+#else
+    return arm_sctlr_b(env) || arm_cpsr_e(env);
+#endif
+}
+
+static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr)
+{
+    uint64_t val;
+
+    target_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 8, 0);
+    if (arm_bswap_needed(env)) {
+        return bswap64(val);
+    } else {
+        return val;
+    }
+}
+
+static inline uint32_t softmmu_tget32(CPUArchState *env, target_ulong addr)
+{
+    uint32_t val;
+
+    target_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 4, 0);
+    if (arm_bswap_needed(env)) {
+        return bswap32(val);
+    } else {
+        return val;
+    }
+}
+
+static inline uint32_t softmmu_tget8(CPUArchState *env, target_ulong addr)
+{
+    uint8_t val;
+    target_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 1, 0);
+    return val;
+}
+
+#define get_user_u64(arg, p) ({ arg = softmmu_tget64(env, p); 0; })
+#define get_user_u32(arg, p) ({ arg = softmmu_tget32(env, p) ; 0; })
+#define get_user_u8(arg, p) ({ arg = softmmu_tget8(env, p) ; 0; })
+#define get_user_ual(arg, p) get_user_u32(arg, p)
+
+static inline void softmmu_tput64(CPUArchState *env,
+                                  target_ulong addr, uint64_t val)
+{
+    if (arm_bswap_needed(env)) {
+        val = bswap64(val);
+    }
+    cpu_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 8, 1);
+}
+
+static inline void softmmu_tput32(CPUArchState *env,
+                                  target_ulong addr, uint32_t val)
+{
+    if (arm_bswap_needed(env)) {
+        val = bswap32(val);
+    }
+    target_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 4, 1);
+}
+#define put_user_u64(arg, p) ({ softmmu_tput64(env, p, arg) ; 0; })
+#define put_user_u32(arg, p) ({ softmmu_tput32(env, p, arg) ; 0; })
+#define put_user_ual(arg, p) put_user_u32(arg, p)
+
+static inline void *softmmu_lock_user(CPUArchState *env,
+                                      target_ulong addr, target_ulong len,
+                                      int copy)
+{
+    uint8_t *p;
+    /* TODO: Make this something that isn't fixed size.  */
+    p = malloc(len);
+    if (p && copy) {
+        target_memory_rw_debug(ENV_GET_CPU(env), addr, p, len, 0);
+    }
+    return p;
+}
+#define lock_user(type, p, len, copy) softmmu_lock_user(env, p, len, copy)
+static inline char *softmmu_lock_user_string(CPUArchState *env,
+                                             target_ulong addr)
+{
+    char *p;
+    char *s;
+    uint8_t c;
+    /* TODO: Make this something that isn't fixed size.  */
+    s = p = malloc(1024);
+    if (!s) {
+        return NULL;
+    }
+    do {
+        target_memory_rw_debug(ENV_GET_CPU(env), addr, &c, 1, 0);
+        addr++;
+        *(p++) = c;
+    } while (c);
+    return s;
+}
+#define lock_user_string(p) softmmu_lock_user_string(env, p)
+static inline void softmmu_unlock_user(CPUArchState *env, void *p,
+                                       target_ulong addr, target_ulong len)
+{
+    if (len) {
+        target_memory_rw_debug(ENV_GET_CPU(env), addr, p, len, 1);
+    }
+    free(p);
+}
+
+#define unlock_user(s, args, len) softmmu_unlock_user(env, s, args, len)
+
+#endif
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 7cac873..6b235ad 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -114,7 +114,7 @@ static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
     return code;
 }
 
-#include "exec/softmmu-semi.h"
+#include "exec/softmmu-arm-semi.h"
 #endif
 
 static target_ulong arm_semi_syscall_len;
@@ -187,7 +187,7 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
     /* The size is always stored in big-endian order, extract
        the value. We assume the size always fit in 32 bits.  */
     uint32_t size;
-    cpu_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, (uint8_t *)&size, 4, 0);
+    target_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, (uint8_t *)&size, 4, 0);
     size = be32_to_cpu(size);
     if (is_a64(env)) {
         env->xregs[0] = size;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index bdf86de..74a2fa1 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1571,6 +1571,27 @@ static gchar *arm_gdb_arch_name(CPUState *cs)
     return g_strdup("arm");
 }
 
+#ifndef CONFIG_USER_ONLY
+static int arm_cpu_memory_rw_debug(CPUState *cpu, vaddr address,
+                                   uint8_t *buf, int len, bool is_write)
+{
+    target_ulong addr = address;
+    ARMCPU *armcpu = ARM_CPU(cpu);
+    CPUARMState *env = &armcpu->env;
+
+    if (arm_sctlr_b(env)) {
+        target_ulong i;
+        for (i = 0; i < len; i++) {
+            cpu_memory_rw_debug(cpu, (addr + i) ^ 3, &buf[i], 1, is_write);
+        }
+    } else {
+        cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
+    }
+
+    return 0;
+}
+#endif
+
 static void arm_cpu_class_init(ObjectClass *oc, void *data)
 {
     ARMCPUClass *acc = ARM_CPU_CLASS(oc);
@@ -1588,6 +1609,9 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->has_work = arm_cpu_has_work;
     cc->cpu_exec_interrupt = arm_cpu_exec_interrupt;
     cc->dump_state = arm_cpu_dump_state;
+#if !defined(CONFIG_USER_ONLY)
+    cc->memory_rw_debug = arm_cpu_memory_rw_debug;
+#endif
     cc->set_pc = arm_cpu_set_pc;
     cc->gdb_read_register = arm_cpu_gdb_read_register;
     cc->gdb_write_register = arm_cpu_gdb_write_register;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index ac8d625..efd0de5 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2128,6 +2128,12 @@ static inline bool arm_sctlr_b(CPUARMState *env)
         (env->cp15.sctlr_el[1] & SCTLR_B) != 0;
 }
 
+static inline bool arm_cpsr_e(CPUARMState *env)
+{
+    return arm_feature(env, ARM_FEATURE_V7) &&
+           (env->uncached_cpsr & CPSR_E) != 0;
+}
+
 /* Return true if the processor is in big-endian mode. */
 static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
 {
-- 
2.8.1

Re: [Qemu-devel] [PATCH v3 4/7] ARM big-endian semihosting support.

[Peter Maydell]

On 20 January 2017 at 16:32, Julian Brown <julian@codesourcery.com> wrote:
> This patch introduces an ARM-specific version of the memory read/write,
> etc. functions used for semihosting, in order to byte-swap (big-endian)
> target memory that is to be interpreted by the (little-endian) host.
> The target_memory_rw_debug function is used that knows about the
> byte-reversal used for BE32 system mode.
>
> Signed-off-by: Julian Brown <julian@codesourcery.com>
> ---
>  include/exec/softmmu-arm-semi.h | 131 ++++++++++++++++++++++++++++++++++++++++
>  target/arm/arm-semi.c           |   4 +-
>  target/arm/cpu.c                |  24 ++++++++
>  target/arm/cpu.h                |   6 ++
>  4 files changed, 163 insertions(+), 2 deletions(-)
>  create mode 100644 include/exec/softmmu-arm-semi.h
>
> diff --git a/include/exec/softmmu-arm-semi.h b/include/exec/softmmu-arm-semi.h
> new file mode 100644
> index 0000000..bba9ca6
> --- /dev/null
> +++ b/include/exec/softmmu-arm-semi.h
> @@ -0,0 +1,131 @@
> +/*
> + * Helper routines to provide target memory access for ARM semihosting
> + * syscalls in system emulation mode.
> + *
> + * Copyright (c) 2007 CodeSourcery, (c) 2016 Mentor Graphics
> + *
> + * This code is licensed under the GPL
> + */
> +
> +#ifndef SOFTMMU_ARM_SEMI_H
> +#define SOFTMMU_ARM_SEMI_H 1
> +
> +/* In big-endian mode (either BE8 or BE32), values larger than a byte will be
> + * transferred to/from memory in big-endian format.  Assuming we're on a
> + * little-endian host machine, such values will need to be byteswapped before
> + * and after the host processes them.
> + *
> + * This means that byteswapping will occur *twice* in BE32 mode for
> + * halfword/word reads/writes.
> + */
> +
> +static inline bool arm_bswap_needed(CPUARMState *env)
> +{
> +#ifdef HOST_WORDS_BIGENDIAN
> +#error HOST_WORDS_BIGENDIAN is not supported for ARM semihosting at the moment.
> +#else

This breaks compilation on big-endian systems, right? This needs
to be actually implemented... maybe

    return
#ifdef BSWAP_NEEDED
        1 ^
#endif
        (arm_sctlr_b(env) || arm_cpsr_e(env));

(untested, and there may be a less ugly way to phrase that).

> +    return arm_sctlr_b(env) || arm_cpsr_e(env);
> +#endif
> +}

Also, what about AArch64? Should we just be calling
arm_cpu_data_is_big_endian() here?

> +
> +static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr)
> +{
> +    uint64_t val;
> +
> +    target_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 8, 0);
> +    if (arm_bswap_needed(env)) {
> +        return bswap64(val);
> +    } else {
> +        return val;
> +    }
> +}
> +
> +static inline uint32_t softmmu_tget32(CPUArchState *env, target_ulong addr)
> +{
> +    uint32_t val;
> +
> +    target_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 4, 0);
> +    if (arm_bswap_needed(env)) {
> +        return bswap32(val);
> +    } else {
> +        return val;
> +    }
> +}
> +
> +static inline uint32_t softmmu_tget8(CPUArchState *env, target_ulong addr)
> +{
> +    uint8_t val;
> +    target_memory_rw_debug(ENV_GET_CPU(env), addr, &val, 1, 0);
> +    return val;
> +}
> +
> +#define get_user_u64(arg, p) ({ arg = softmmu_tget64(env, p); 0; })
> +#define get_user_u32(arg, p) ({ arg = softmmu_tget32(env, p) ; 0; })
> +#define get_user_u8(arg, p) ({ arg = softmmu_tget8(env, p) ; 0; })
> +#define get_user_ual(arg, p) get_user_u32(arg, p)
> +
> +static inline void softmmu_tput64(CPUArchState *env,
> +                                  target_ulong addr, uint64_t val)
> +{
> +    if (arm_bswap_needed(env)) {
> +        val = bswap64(val);
> +    }
> +    cpu_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 8, 1);
> +}
> +
> +static inline void softmmu_tput32(CPUArchState *env,
> +                                  target_ulong addr, uint32_t val)
> +{
> +    if (arm_bswap_needed(env)) {
> +        val = bswap32(val);
> +    }
> +    target_memory_rw_debug(ENV_GET_CPU(env), addr, (uint8_t *)&val, 4, 1);
> +}
> +#define put_user_u64(arg, p) ({ softmmu_tput64(env, p, arg) ; 0; })
> +#define put_user_u32(arg, p) ({ softmmu_tput32(env, p, arg) ; 0; })
> +#define put_user_ual(arg, p) put_user_u32(arg, p)
> +
> +static inline void *softmmu_lock_user(CPUArchState *env,
> +                                      target_ulong addr, target_ulong len,
> +                                      int copy)
> +{
> +    uint8_t *p;
> +    /* TODO: Make this something that isn't fixed size.  */
> +    p = malloc(len);
> +    if (p && copy) {
> +        target_memory_rw_debug(ENV_GET_CPU(env), addr, p, len, 0);
> +    }
> +    return p;
> +}
> +#define lock_user(type, p, len, copy) softmmu_lock_user(env, p, len, copy)
> +static inline char *softmmu_lock_user_string(CPUArchState *env,
> +                                             target_ulong addr)
> +{
> +    char *p;
> +    char *s;
> +    uint8_t c;
> +    /* TODO: Make this something that isn't fixed size.  */
> +    s = p = malloc(1024);
> +    if (!s) {
> +        return NULL;
> +    }
> +    do {
> +        target_memory_rw_debug(ENV_GET_CPU(env), addr, &c, 1, 0);
> +        addr++;
> +        *(p++) = c;
> +    } while (c);
> +    return s;
> +}
> +#define lock_user_string(p) softmmu_lock_user_string(env, p)
> +static inline void softmmu_unlock_user(CPUArchState *env, void *p,
> +                                       target_ulong addr, target_ulong len)
> +{
> +    if (len) {
> +        target_memory_rw_debug(ENV_GET_CPU(env), addr, p, len, 1);
> +    }
> +    free(p);
> +}
> +
> +#define unlock_user(s, args, len) softmmu_unlock_user(env, s, args, len)
> +
> +#endif

So this is basically duplicating exec/softmmu-semi.h and then
making some minor tweaks to it, in order to use a custom
function instead of tswap64 and tswap32. I think we can do a bit
better than that without too much extra effort:
have exec/softmmu-semi.h start with

/* Some targets need to provide a custom function for byteswapping data
 * that the semihosting operations access; most can use the default
 * tswap32 and tswap64, though.
 */
#ifndef TSWAP64_FN
#define TSWAP64_FN tswap64
#endif
#ifndef TSWAP32_FN
#define TSWAP32_FN tswap32
#endif

and then have the ARM header define suitable functions and
set the TSWAP32_FN TSWAP64_FN before including softmmu-semi.h.
Not the most elegant abstraction in the world but it saves on
the copy-n-paste.

(make the "s/cpu_memory_rw_debug/target_memory_rw_debug/ in softmmu-semi.h"
change its own patch since that's purely mechanical).

> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 7cac873..6b235ad 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -114,7 +114,7 @@ static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
>      return code;
>  }
>
> -#include "exec/softmmu-semi.h"
> +#include "exec/softmmu-arm-semi.h"
>  #endif
>
>  static target_ulong arm_semi_syscall_len;
> @@ -187,7 +187,7 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
>      /* The size is always stored in big-endian order, extract
>         the value. We assume the size always fit in 32 bits.  */
>      uint32_t size;
> -    cpu_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, (uint8_t *)&size, 4, 0);
> +    target_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, (uint8_t *)&size, 4, 0);
>      size = be32_to_cpu(size);
>      if (is_a64(env)) {
>          env->xregs[0] = size;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index bdf86de..74a2fa1 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1571,6 +1571,27 @@ static gchar *arm_gdb_arch_name(CPUState *cs)
>      return g_strdup("arm");
>  }
>
> +#ifndef CONFIG_USER_ONLY
> +static int arm_cpu_memory_rw_debug(CPUState *cpu, vaddr address,
> +                                   uint8_t *buf, int len, bool is_write)
> +{
> +    target_ulong addr = address;
> +    ARMCPU *armcpu = ARM_CPU(cpu);
> +    CPUARMState *env = &armcpu->env;
> +
> +    if (arm_sctlr_b(env)) {
> +        target_ulong i;
> +        for (i = 0; i < len; i++) {
> +            cpu_memory_rw_debug(cpu, (addr + i) ^ 3, &buf[i], 1, is_write);
> +        }
> +    } else {
> +        cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
> +    }
> +
> +    return 0;
> +}
> +#endif
> +
>  static void arm_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> @@ -1588,6 +1609,9 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>      cc->has_work = arm_cpu_has_work;
>      cc->cpu_exec_interrupt = arm_cpu_exec_interrupt;
>      cc->dump_state = arm_cpu_dump_state;
> +#if !defined(CONFIG_USER_ONLY)
> +    cc->memory_rw_debug = arm_cpu_memory_rw_debug;
> +#endif
>      cc->set_pc = arm_cpu_set_pc;
>      cc->gdb_read_register = arm_cpu_gdb_read_register;
>      cc->gdb_write_register = arm_cpu_gdb_write_register;
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index ac8d625..efd0de5 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2128,6 +2128,12 @@ static inline bool arm_sctlr_b(CPUARMState *env)
>          (env->cp15.sctlr_el[1] & SCTLR_B) != 0;
>  }
>
> +static inline bool arm_cpsr_e(CPUARMState *env)
> +{
> +    return arm_feature(env, ARM_FEATURE_V7) &&
> +           (env->uncached_cpsr & CPSR_E) != 0;
> +}

You don't need to gate this on ARM_FEATURE_V7 -- if
the CPU isn't v7 then the E bit should never get set.
(Compare the check in arm_cpu_data_is_big_endian()).

> +
>  /* Return true if the processor is in big-endian mode. */
>  static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)

thanks
-- PMM

[Qemu-devel] [PATCH v3 5/7] ARM big-endian system-mode gdbstub support.

[Julian Brown]

When debugging a big-endian (either BE8 or BE32) executable, GDB uses
a big-endian byte ordering for its remote protocol.  The gdb stub
code in QEMU needs to interpret data in host (little-endian) order in
arm_cpu_gdb_read_register and arm_cpu_gdb_write_register, so this patch
arranges to byte-swap the data to/from GDB in those cases.

Signed-off-by: Julian Brown <julian@codesourcery.com>
---
 target/arm/gdbstub.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 04c1208..1e9fe68 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -21,6 +21,7 @@
 #include "qemu-common.h"
 #include "cpu.h"
 #include "exec/gdbstub.h"
+#include "exec/softmmu-arm-semi.h"
 
 /* Old gdb always expect FPA registers.  Newer (xml-aware) gdb only expect
    whatever the target description contains.  Due to a historical mishap
@@ -32,10 +33,22 @@ int arm_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
+#ifndef CONFIG_USER_ONLY
+    bool targ_bigendian = arm_bswap_needed(env);
+#endif
 
     if (n < 16) {
         /* Core integer register.  */
+#ifdef CONFIG_USER_ONLY
         return gdb_get_reg32(mem_buf, env->regs[n]);
+#else
+        if (targ_bigendian) {
+            stl_be_p(mem_buf, env->regs[n]);
+        } else {
+            stl_le_p(mem_buf, env->regs[n]);
+        }
+        return 4;
+#endif
     }
     if (n < 24) {
         /* FPA registers.  */
@@ -51,10 +64,28 @@ int arm_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
         if (gdb_has_xml) {
             return 0;
         }
+#ifdef CONFIG_USER_ONLY
         return gdb_get_reg32(mem_buf, 0);
+#else
+        if (targ_bigendian) {
+            stl_be_p(mem_buf, 0);
+        } else {
+            stl_le_p(mem_buf, 0);
+        }
+        return 4;
+#endif
     case 25:
         /* CPSR */
+#ifdef CONFIG_USER_ONLY
         return gdb_get_reg32(mem_buf, cpsr_read(env));
+#else
+        if (targ_bigendian) {
+            stl_be_p(mem_buf, cpsr_read(env));
+        } else {
+            stl_le_p(mem_buf, cpsr_read(env));
+        }
+        return 4;
+#endif
     }
     /* Unknown register.  */
     return 0;
@@ -65,8 +96,19 @@ int arm_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
     uint32_t tmp;
+#ifndef CONFIG_USER_ONLY
+    bool targ_bigendian = arm_bswap_needed(env);
+#endif
 
+#ifdef CONFIG_USER_ONLY
     tmp = ldl_p(mem_buf);
+#else
+    if (targ_bigendian) {
+        tmp = ldl_be_p(mem_buf);
+    } else {
+        tmp = ldl_le_p(mem_buf);
+    }
+#endif
 
     /* Mask out low bit of PC to workaround gdb bugs.  This will probably
        cause problems if we ever implement the Jazelle DBX extensions.  */
-- 
2.8.1

Re: [Qemu-devel] [PATCH v3 5/7] ARM big-endian system-mode gdbstub support.

[Peter Maydell]

On 20 January 2017 at 16:32, Julian Brown <julian@codesourcery.com> wrote:
> When debugging a big-endian (either BE8 or BE32) executable, GDB uses
> a big-endian byte ordering for its remote protocol.  The gdb stub
> code in QEMU needs to interpret data in host (little-endian) order in
> arm_cpu_gdb_read_register and arm_cpu_gdb_write_register, so this patch
> arranges to byte-swap the data to/from GDB in those cases.
>
> Signed-off-by: Julian Brown <julian@codesourcery.com>
> ---
>  target/arm/gdbstub.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 04c1208..1e9fe68 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -21,6 +21,7 @@
>  #include "qemu-common.h"
>  #include "cpu.h"
>  #include "exec/gdbstub.h"
> +#include "exec/softmmu-arm-semi.h"
>
>  /* Old gdb always expect FPA registers.  Newer (xml-aware) gdb only expect
>     whatever the target description contains.  Due to a historical mishap
> @@ -32,10 +33,22 @@ int arm_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> +#ifndef CONFIG_USER_ONLY
> +    bool targ_bigendian = arm_bswap_needed(env);
> +#endif

This is a "what is the state of the CPU right this instant" test,
but surely gdb doesn't flip its protocol definition as the CPU
flips between big and little endian at runtime? I'm not sure
what the right check is but it probably isn't this.

>
>      if (n < 16) {
>          /* Core integer register.  */
> +#ifdef CONFIG_USER_ONLY
>          return gdb_get_reg32(mem_buf, env->regs[n]);
> +#else
> +        if (targ_bigendian) {
> +            stl_be_p(mem_buf, env->regs[n]);
> +        } else {
> +            stl_le_p(mem_buf, env->regs[n]);
> +        }
> +        return 4;
> +#endif

There's probably a phrasing here that avoids the ifdeffery...

thanks
-- PMM

[Qemu-devel] [PATCH v3 6/7] Fix Thumb-1 BE32 execution and disassembly.

[Julian Brown]

Thumb-1 code has some issues in BE32 mode (as currently implemented). In
short, since bytes are swapped within words at load time for BE32
executables, this also swaps pairs of adjacent Thumb-1 instructions.

This patch un-swaps those pairs of instructions again, both for execution,
and for disassembly. (The previous version of the patch always read four
bytes in arm_read_memory_func and then extracted the proper two bytes,
in a probably misguided attempt to match the behaviour of actual hardware
as described by e.g. the ARM9TDMI TRM, section 3.3 "Endian effects for
instruction fetches". It's less complicated to just read the correct
two bytes though.)

Signed-off-by: Julian Brown <julian@codesourcery.com>
---
 disas.c               |  1 +
 include/disas/bfd.h   |  7 +++++++
 target/arm/arm_ldst.h | 10 +++++++++-
 target/arm/cpu.c      | 23 +++++++++++++++++++++++
 4 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/disas.c b/disas.c
index 67f116a..506e56f 100644
--- a/disas.c
+++ b/disas.c
@@ -190,6 +190,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
 
     s.cpu = cpu;
     s.info.read_memory_func = target_read_memory;
+    s.info.read_memory_inner_func = NULL;
     s.info.buffer_vma = code;
     s.info.buffer_length = size;
     s.info.print_address_func = generic_print_address;
diff --git a/include/disas/bfd.h b/include/disas/bfd.h
index 8a3488c..14d266b 100644
--- a/include/disas/bfd.h
+++ b/include/disas/bfd.h
@@ -291,6 +291,7 @@ typedef struct disassemble_info {
      The bottom 16 bits are for the internal use of the disassembler.  */
   unsigned long flags;
 #define INSN_HAS_RELOC	0x80000000
+#define INSN_ARM_BE32	0x00010000
   PTR private_data;
 
   /* Function used to get bytes to disassemble.  MEMADDR is the
@@ -302,6 +303,12 @@ typedef struct disassemble_info {
     (bfd_vma memaddr, bfd_byte *myaddr, int length,
 	     struct disassemble_info *info);
 
+  /* A place to stash the real read_memory_func if read_memory_func wants to
+     do some funky address arithmetic or similar (e.g. for ARM BE32 mode).  */
+  int (*read_memory_inner_func)
+    (bfd_vma memaddr, bfd_byte *myaddr, int length,
+             struct disassemble_info *info);
+
   /* Function which should be called if we get an error that we can't
      recover from.  STATUS is the errno value from read_memory_func and
      MEMADDR is the address that we were trying to read.  INFO is a
diff --git a/target/arm/arm_ldst.h b/target/arm/arm_ldst.h
index a76d89f..01587b3 100644
--- a/target/arm/arm_ldst.h
+++ b/target/arm/arm_ldst.h
@@ -39,7 +39,15 @@ static inline uint32_t arm_ldl_code(CPUARMState *env, target_ulong addr,
 static inline uint16_t arm_lduw_code(CPUARMState *env, target_ulong addr,
                                      bool sctlr_b)
 {
-    uint16_t insn = cpu_lduw_code(env, addr);
+    uint16_t insn;
+#ifndef CONFIG_USER_ONLY
+    /* In big-endian (BE32) mode, adjacent Thumb instructions have been swapped
+       within each word.  Undo that now.  */
+    if (sctlr_b) {
+        addr ^= 2;
+    }
+#endif
+    insn = cpu_lduw_code(env, addr);
     if (bswap_code(sctlr_b)) {
         return bswap16(insn);
     }
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 74a2fa1..4d91d07 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -408,6 +408,21 @@ print_insn_thumb1(bfd_vma pc, disassemble_info *info)
   return print_insn_arm(pc | 1, info);
 }
 
+static int arm_read_memory_func(bfd_vma memaddr, bfd_byte *b,
+                                int length, struct disassemble_info *info)
+{
+    assert(info->read_memory_inner_func);
+    assert((info->flags & INSN_ARM_BE32) == 0 || length == 2 || length == 4);
+
+    if ((info->flags & INSN_ARM_BE32) != 0 && length == 2) {
+        assert(info->endian == BFD_ENDIAN_LITTLE);
+        return info->read_memory_inner_func(memaddr ^ 2, (bfd_byte *)b, 2,
+                                            info);
+    } else {
+        return info->read_memory_inner_func(memaddr, b, length, info);
+    }
+}
+
 static void arm_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
     ARMCPU *ac = ARM_CPU(cpu);
@@ -433,6 +448,14 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info *info)
         info->endian = BFD_ENDIAN_BIG;
 #endif
     }
+    if (info->read_memory_inner_func == NULL) {
+        info->read_memory_inner_func = info->read_memory_func;
+        info->read_memory_func = arm_read_memory_func;
+    }
+    info->flags &= ~INSN_ARM_BE32;
+    if (arm_sctlr_b(env)) {
+        info->flags |= INSN_ARM_BE32;
+    }
 }
 
 static void arm_cpu_initfn(Object *obj)
-- 
2.8.1

Re: [Qemu-devel] [PATCH v3 6/7] Fix Thumb-1 BE32 execution and disassembly.

[Peter Maydell]

On 20 January 2017 at 16:32, Julian Brown <julian@codesourcery.com> wrote:
> Thumb-1 code has some issues in BE32 mode (as currently implemented). In
> short, since bytes are swapped within words at load time for BE32
> executables, this also swaps pairs of adjacent Thumb-1 instructions.
>
> This patch un-swaps those pairs of instructions again, both for execution,
> and for disassembly. (The previous version of the patch always read four
> bytes in arm_read_memory_func and then extracted the proper two bytes,
> in a probably misguided attempt to match the behaviour of actual hardware
> as described by e.g. the ARM9TDMI TRM, section 3.3 "Endian effects for
> instruction fetches". It's less complicated to just read the correct
> two bytes though.)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v3 7/7] ARM BE32 watchpoint fix.

[Julian Brown]

In BE32 mode, sub-word size watchpoints can fail to trigger because the
address of the access is adjusted in the opcode helpers before being
compared with the watchpoint registers.  This patch reverses the address
adjustment before performing the comparison with the help of a new CPUClass
hook.

This version of the patch augments and tidies up comments a little.

Signed-off-by: Julian Brown <julian@codesourcery.com>
---
 exec.c                 |  1 +
 include/qom/cpu.h      |  3 +++
 qom/cpu.c              |  6 ++++++
 target/arm/cpu.c       |  3 +++
 target/arm/internals.h |  5 +++++
 target/arm/op_helper.c | 22 ++++++++++++++++++++++
 6 files changed, 40 insertions(+)

diff --git a/exec.c b/exec.c
index 401a912..b6be381 100644
--- a/exec.c
+++ b/exec.c
@@ -2110,6 +2110,7 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
         return;
     }
     vaddr = (cpu->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
+    vaddr = cc->adjust_watchpoint_address(cpu, vaddr, len);
     QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
         if (cpu_watchpoint_address_matches(wp, vaddr, len)
             && (wp->flags & flags)) {
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 3f79a8e..1d2df57 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -132,6 +132,8 @@ struct TranslationBlock;
  * @cpu_exec_exit: Callback for cpu_exec cleanup.
  * @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec.
  * @disas_set_info: Setup architecture specific components of disassembly info
+ * @adjust_watchpoint_address: Perform a target-specific adjustment to an
+ * address before attempting to match it against watchpoints.
  *
  * Represents a CPU family or model.
  */
@@ -195,6 +197,7 @@ typedef struct CPUClass {
     bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
 
     void (*disas_set_info)(CPUState *cpu, disassemble_info *info);
+    vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
 } CPUClass;
 
 #ifdef HOST_WORDS_BIGENDIAN
diff --git a/qom/cpu.c b/qom/cpu.c
index cee4e6f..b35deb5 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -391,6 +391,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
     return cpu->cpu_index;
 }
 
+static vaddr cpu_adjust_watchpoint_address(CPUState *cpu, vaddr addr, int len)
+{
+    return addr;
+}
+
 static void cpu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -415,6 +420,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->cpu_exec_enter = cpu_common_noop;
     k->cpu_exec_exit = cpu_common_noop;
     k->cpu_exec_interrupt = cpu_common_exec_interrupt;
+    k->adjust_watchpoint_address = cpu_adjust_watchpoint_address;
     dc->realize = cpu_common_realizefn;
     dc->unrealize = cpu_common_unrealizefn;
     /*
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 4d91d07..7a56487 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1656,6 +1656,9 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_stop_before_watchpoint = true;
     cc->debug_excp_handler = arm_debug_excp_handler;
     cc->debug_check_watchpoint = arm_debug_check_watchpoint;
+#if !defined(CONFIG_USER_ONLY)
+    cc->adjust_watchpoint_address = arm_adjust_watchpoint_address;
+#endif
 
     cc->disas_set_info = arm_disas_set_info;
 }
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 3cae5ff..0485583 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -437,6 +437,11 @@ void hw_breakpoint_update_all(ARMCPU *cpu);
 /* Callback function for checking if a watchpoint should trigger. */
 bool arm_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
 
+/* Adjust addresses (in BE32 mode) before testing against watchpoint
+ * addresses.
+ */
+vaddr arm_adjust_watchpoint_address(CPUState *cs, vaddr addr, int len);
+
 /* Callback function for when a watchpoint or breakpoint triggers. */
 void arm_debug_excp_handler(CPUState *cs);
 
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index ba796d8..fb366fd 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -1225,6 +1225,28 @@ bool arm_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
     return check_watchpoints(cpu);
 }
 
+vaddr arm_adjust_watchpoint_address(CPUState *cs, vaddr addr, int len)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    /* In BE32 system mode, target memory is stored byteswapped (on a
+     * little-endian host system), and by the time we reach here (via an
+     * opcode helper) the addresses of subword accesses have been adjusted
+     * to account for that, which means that watchpoints will not match.
+     * Undo the adjustment here.
+     */
+    if (arm_sctlr_b(env)) {
+        if (len == 1) {
+            addr ^= 3;
+        } else if (len == 2) {
+            addr ^= 2;
+        }
+    }
+
+    return addr;
+}
+
 void arm_debug_excp_handler(CPUState *cs)
 {
     /* Called by core code when a watchpoint or breakpoint fires;
-- 
2.8.1

Re: [Qemu-devel] [PATCH v3 7/7] ARM BE32 watchpoint fix.

[Peter Maydell]

On 20 January 2017 at 16:32, Julian Brown <julian@codesourcery.com> wrote:
> In BE32 mode, sub-word size watchpoints can fail to trigger because the
> address of the access is adjusted in the opcode helpers before being
> compared with the watchpoint registers.  This patch reverses the address
> adjustment before performing the comparison with the help of a new CPUClass
> hook.
>
> This version of the patch augments and tidies up comments a little.
>
> Signed-off-by: Julian Brown <julian@codesourcery.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 0/7] ARM BE8/BE32 big-endian system-mode fixes (semihosting, gdbstub)

[Peter Maydell]

On 20 January 2017 at 16:30, Julian Brown <julian@codesourcery.com> wrote:
> This is the third iteration of a series of patches to implement
> semihosting/gdbstub support for big-endian ARM system mode. The previous
> series started here:
>
>   http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg00972.html
>
> I've (hopefully!) addressed all the comments from the second round of
> reviews, apologies in advance if I've missed anything.
>
> Thanks,
>
> Julian
>
> Julian Brown (7):
>   Add cfgend parameter for ARM CPU selection.
>   Honour reset_sctlr EE/B bits during reset.
>   Move target_memory_rw_debug function.
>   ARM big-endian semihosting support.
>   ARM big-endian system-mode gdbstub support.
>   Fix Thumb-1 BE32 execution and disassembly.
>   ARM BE32 watchpoint fix.

So to summarise:
I'm taking patches 1 (with some tweaks), 6 and 7 into target-arm.next.
2 isn't required. 3 is OK but should go in with the patches that
use it. 4 and 5 still need some work.

thanks
-- PMM