[Qemu-devel] [PATCH 0/8] Implement ARM external abort handling

[Qemu-devel] [PATCH 0/8] Implement ARM external abort handling

[Peter Maydell]

Following recent list discussion
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00063.html

here's a patchseries which defines a new API for handling CPU memory
transaction failures at the right level in the memory subsystem code,
and implements it for ARM so that we can generate prefetch abort and
data abort exceptions for external aborts.

The first 3 patches here implement the core code support
for the new cpu_transaction_failed_hook.

The next 2 patches add support for turning it off on
a per-board basis, and use that to go back to RAZ/WI
on the legacy ARM board models, because right now we rely
on that so that guest code doesn't blow up when it touches
a device we don't have a model for yet. (We leave the
support enabled for 'virt' and 'mps2'.)

Finally the last 3 patches do some cleanup and then
implement the new hook for ARM.

(I have not as yet audited the target/arm code to check that
we correctly handle failed transactions in all the places
that C code does physical address accesses; but since those
lookups don't generate exceptions today, the series leaves
behaviour there no worse off than they were before.)

thanks
-- PMM

Peter Maydell (8):
  memory.h: Move MemTxResult type to memattrs.h
  cpu: Define new cpu_transaction_failed() hook
  cputlb: Support generating CPU exceptions on memory transaction
    failures
  boards.h: Define new flag ignore_memory_transaction_failures
  hw/arm: Set ignore_memory_transaction_failures for most ARM boards
  target/arm: Factor out fault delivery code
  target/arm: Allow deliver_fault() caller to specify EA bit
  target/arm: Implement new do_transaction_failed hook

 include/exec/memattrs.h |  10 ++++
 include/exec/memory.h   |  10 ----
 include/hw/boards.h     |  11 ++++
 include/qom/cpu.h       |  26 ++++++++
 softmmu_template.h      |   4 +-
 target/arm/internals.h  |  12 ++++
 accel/tcg/cputlb.c      |  32 +++++++++-
 hw/arm/aspeed.c         |   3 +
 hw/arm/collie.c         |   1 +
 hw/arm/cubieboard.c     |   1 +
 hw/arm/digic_boards.c   |   1 +
 hw/arm/exynos4_boards.c |   2 +
 hw/arm/gumstix.c        |   2 +
 hw/arm/highbank.c       |   2 +
 hw/arm/imx25_pdk.c      |   1 +
 hw/arm/integratorcp.c   |   1 +
 hw/arm/kzm.c            |   1 +
 hw/arm/mainstone.c      |   1 +
 hw/arm/musicpal.c       |   1 +
 hw/arm/netduino2.c      |   1 +
 hw/arm/nseries.c        |   2 +
 hw/arm/omap_sx1.c       |   2 +
 hw/arm/palm.c           |   1 +
 hw/arm/raspi.c          |   1 +
 hw/arm/realview.c       |   4 ++
 hw/arm/sabrelite.c      |   1 +
 hw/arm/spitz.c          |   4 ++
 hw/arm/stellaris.c      |   2 +
 hw/arm/tosa.c           |   1 +
 hw/arm/versatilepb.c    |   2 +
 hw/arm/vexpress.c       |   1 +
 hw/arm/xilinx_zynq.c    |   1 +
 hw/arm/xlnx-ep108.c     |   2 +
 hw/arm/z2.c             |   1 +
 qom/cpu.c               |   7 +++
 target/arm/cpu.c        |   1 +
 target/arm/op_helper.c  | 155 +++++++++++++++++++++++++++++++-----------------
 37 files changed, 243 insertions(+), 68 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/8] memory.h: Move MemTxResult type to memattrs.h

[Peter Maydell]

Move the MemTxResult type to memattrs.h. We're going to want to
use it in cpu/qom.h, which doesn't want to include all of
memory.h. In practice MemTxResult and MemTxAttrs are pretty
closely linked since both are used for the new-style
read_with_attrs and write_with_attrs callbacks, so memattrs.h
is a reasonable home for this rather than creating a whole
new header file for it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memattrs.h | 10 ++++++++++
 include/exec/memory.h   | 10 ----------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index e601061..d4a1642 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -46,4 +46,14 @@ typedef struct MemTxAttrs {
  */
 #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
 
+/* New-style MMIO accessors can indicate that the transaction failed.
+ * A zero (MEMTX_OK) response means success; anything else is a failure
+ * of some kind. The memory subsystem will bitwise-OR together results
+ * if it is synthesizing an operation from multiple smaller accesses.
+ */
+#define MEMTX_OK 0
+#define MEMTX_ERROR             (1U << 0) /* device returned an error */
+#define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
+typedef uint32_t MemTxResult;
+
 #endif
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 400dd44..1dcd312 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -112,16 +112,6 @@ static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
     n->end = end;
 }
 
-/* New-style MMIO accessors can indicate that the transaction failed.
- * A zero (MEMTX_OK) response means success; anything else is a failure
- * of some kind. The memory subsystem will bitwise-OR together results
- * if it is synthesizing an operation from multiple smaller accesses.
- */
-#define MEMTX_OK 0
-#define MEMTX_ERROR             (1U << 0) /* device returned an error */
-#define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
-typedef uint32_t MemTxResult;
-
 /*
  * Memory region callbacks
  */
-- 
2.7.4

[Qemu-devel] [PATCH 2/8] cpu: Define new cpu_transaction_failed() hook

[Peter Maydell]

Currently we have a rather half-baked setup for allowing CPUs to
generate exceptions on accesses to invalid memory: the CPU has a
cpu_unassigned_access() hook which the memory system calls in
unassigned_mem_write() and unassigned_mem_read() if the current_cpu
pointer is non-NULL.  This was originally designed before we
implemented the MemTxResult type that allows memory operations to
report a success or failure code, which is why the hook is called
right at the bottom of the memory system.  The major problem with
this is that it means that the hook can be called even when the
access was not actually done by the CPU: for instance if the CPU
writes to a DMA engine register which causes the DMA engine to begin
a transaction which has been set up by the guest to operate on
invalid memory then this will casue the CPU to take an exception
incorrectly.  Another minor problem is that currently if a device
returns a transaction error then this won't turn into a CPU exception
at all.

The right way to do this is to have allow the CPU to respond
to memory system transaction failures at the point where the
CPU specific code calls into the memory system.

Define a new QOM CPU method and utility function
cpu_transaction_failed() which is called in these cases.
The functionality here overlaps with the existing
cpu_unassigned_access() because individual target CPUs will
need some work to convert them to the new system. When this
transition is complete we can remove the old cpu_unassigned_access()
code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qom/cpu.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 25eefea..fc54d55 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -85,8 +85,10 @@ struct TranslationBlock;
  * @has_work: Callback for checking if there is work to do.
  * @do_interrupt: Callback for interrupt handling.
  * @do_unassigned_access: Callback for unassigned access handling.
+ * (this is deprecated: new targets should use do_transaction_failed instead)
  * @do_unaligned_access: Callback for unaligned access handling, if
  * the target defines #ALIGNED_ONLY.
+ * @do_transaction_failed: Callback for handling failed memory transactions
  * @virtio_is_big_endian: Callback to return %true if a CPU which supports
  * runtime configurable endianness is currently big-endian. Non-configurable
  * CPUs can use the default implementation of this method. This method should
@@ -153,6 +155,10 @@ typedef struct CPUClass {
     void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
                                 MMUAccessType access_type,
                                 int mmu_idx, uintptr_t retaddr);
+    void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr,
+                                  unsigned size, MMUAccessType access_type,
+                                  int mmu_idx, MemTxAttrs attrs,
+                                  MemTxResult response, uintptr_t retaddr);
     bool (*virtio_is_big_endian)(CPUState *cpu);
     int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
                            uint8_t *buf, int len, bool is_write);
@@ -837,6 +843,21 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
 
     cc->do_unaligned_access(cpu, addr, access_type, mmu_idx, retaddr);
 }
+
+static inline void cpu_transaction_failed(CPUState *cpu, hwaddr physaddr,
+                                          vaddr addr, unsigned size,
+                                          MMUAccessType access_type,
+                                          int mmu_idx, MemTxAttrs attrs,
+                                          MemTxResult response,
+                                          uintptr_t retaddr)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->do_transaction_failed) {
+        cc->do_transaction_failed(cpu, physaddr, addr, size, access_type,
+                                  mmu_idx, attrs, response, retaddr);
+    }
+}
 #endif
 
 #endif /* NEED_CPU_H */
-- 
2.7.4

[Qemu-devel] [PATCH 3/8] cputlb: Support generating CPU exceptions on memory transaction failures

[Peter Maydell]

Call the new cpu_transaction_failed() hook at the places where
CPU generated code interacts with the memory system:
 io_readx()
 io_writex()
 get_page_addr_code()

Any access from C code (eg via cpu_physical_memory_rw(),
address_space_rw(), ld/st_*_phys()) will *not* trigger CPU exceptions
via cpu_transaction_failed().  Handling for transactions failures for
this kind of call should be done by using a function which returns a
MemTxResult and treating the failure case appropriately in the
calling code.

In an ideal world we would not generate CPU exceptions for
instruction fetch failures in get_page_addr_code() but instead wait
until the code translation process tried a load and it failed;
however that change would require too great a restructuring and
redesign to attempt at this point.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 softmmu_template.h |  4 ++--
 accel/tcg/cputlb.c | 32 ++++++++++++++++++++++++++++++--
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/softmmu_template.h b/softmmu_template.h
index 4a2b665..d756329 100644
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -101,7 +101,7 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
                                               uintptr_t retaddr)
 {
     CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
-    return io_readx(env, iotlbentry, addr, retaddr, DATA_SIZE);
+    return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, DATA_SIZE);
 }
 #endif
 
@@ -262,7 +262,7 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env,
                                           uintptr_t retaddr)
 {
     CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
-    return io_writex(env, iotlbentry, val, addr, retaddr, DATA_SIZE);
+    return io_writex(env, iotlbentry, mmu_idx, val, addr, retaddr, DATA_SIZE);
 }
 
 void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 85635ae..e72415a 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -747,6 +747,7 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
 }
 
 static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
+                         int mmu_idx,
                          target_ulong addr, uintptr_t retaddr, int size)
 {
     CPUState *cpu = ENV_GET_CPU(env);
@@ -754,6 +755,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->attrs);
     uint64_t val;
     bool locked = false;
+    MemTxResult r;
 
     physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
     cpu->mem_io_pc = retaddr;
@@ -767,7 +769,12 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
         qemu_mutex_lock_iothread();
         locked = true;
     }
-    memory_region_dispatch_read(mr, physaddr, &val, size, iotlbentry->attrs);
+    r = memory_region_dispatch_read(mr, physaddr,
+                                    &val, size, iotlbentry->attrs);
+    if (r != MEMTX_OK) {
+        cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_LOAD,
+                               mmu_idx, iotlbentry->attrs, r, retaddr);
+    }
     if (locked) {
         qemu_mutex_unlock_iothread();
     }
@@ -776,6 +783,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
 }
 
 static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
+                      int mmu_idx,
                       uint64_t val, target_ulong addr,
                       uintptr_t retaddr, int size)
 {
@@ -783,6 +791,7 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     hwaddr physaddr = iotlbentry->addr;
     MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->attrs);
     bool locked = false;
+    MemTxResult r;
 
     physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
     if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
@@ -795,7 +804,12 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
         qemu_mutex_lock_iothread();
         locked = true;
     }
-    memory_region_dispatch_write(mr, physaddr, val, size, iotlbentry->attrs);
+    r = memory_region_dispatch_write(mr, physaddr,
+                                     val, size, iotlbentry->attrs);
+    if (r != MEMTX_OK) {
+        cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_STORE,
+                               mmu_idx, iotlbentry->attrs, r, retaddr);
+    }
     if (locked) {
         qemu_mutex_unlock_iothread();
     }
@@ -845,6 +859,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
     MemoryRegion *mr;
     CPUState *cpu = ENV_GET_CPU(env);
     CPUIOTLBEntry *iotlbentry;
+    hwaddr physaddr;
 
     index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = cpu_mmu_index(env, true);
@@ -868,6 +883,19 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
         }
         qemu_mutex_unlock_iothread();
 
+        /* Give the new-style cpu_transaction_failed() hook first chance
+         * to handle this.
+         * This is not the ideal place to detect and generate CPU
+         * exceptions for instruction fetch failure (for instance
+         * we don't know the length of the access that the CPU would
+         * use, and it would be better to go ahead and try the access
+         * and use the MemTXResult it produced). However it is the
+         * simplest place we have currently available for the check.
+         */
+        physaddr = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
+        cpu_transaction_failed(cpu, physaddr, addr, 0, MMU_INST_FETCH, mmu_idx,
+                               iotlbentry->attrs, MEMTX_DECODE_ERROR, 0);
+
         cpu_unassigned_access(cpu, addr, false, true, 0, 4);
         /* The CPU's unassigned access hook might have longjumped out
          * with an exception. If it didn't (or there was no hook) then
-- 
2.7.4

[Qemu-devel] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Peter Maydell]

Define a new MachineClass field ignore_memory_transaction_failures.
If this is flag is true then the CPU will ignore memory transaction
failures which should cause the CPU to take an exception due to an
access to an unassigned physical address; the transaction will
instead return zero (for a read) or be ignored (for a write).  This
should be set only by legacy board models which rely on the old
RAZ/WI behaviour for handling devices that QEMU does not yet model.
New board models should instead use "unimplemented-device" for all
memory ranges where the guest will attempt to probe for a device that
QEMU doesn't implement and a stub device is required.

We need this for ARM boards, where we're about to implement support for
generating external aborts on memory transaction failures. Too many
of our legacy board models rely on the RAZ/WI behaviour and we
would break currently working guests when their "probe for device"
code provoked an external abort rather than a RAZ.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/boards.h | 11 +++++++++++
 include/qom/cpu.h   |  7 ++++++-
 qom/cpu.c           |  7 +++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 3363dd1..7f044d1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -131,6 +131,16 @@ typedef struct {
  *    size than the target architecture's minimum. (Attempting to create
  *    such a CPU will fail.) Note that changing this is a migration
  *    compatibility break for the machine.
+ * @ignore_memory_transaction_failures:
+ *    If this is flag is true then the CPU will ignore memory transaction
+ *    failures which should cause the CPU to take an exception due to an
+ *    access to an unassigned physical address; the transaction will instead
+ *    return zero (for a read) or be ignored (for a write). This should be
+ *    set only by legacy board models which rely on the old RAZ/WI behaviour
+ *    for handling devices that QEMU does not yet model. New board models
+ *    should instead use "unimplemented-device" for all memory ranges where
+ *    the guest will attempt to probe for a device that QEMU doesn't
+ *    implement and a stub device is required.
  */
 struct MachineClass {
     /*< private >*/
@@ -171,6 +181,7 @@ struct MachineClass {
     bool rom_file_has_mr;
     int minimum_page_bits;
     bool has_hotpluggable_cpus;
+    bool ignore_memory_transaction_failures;
     int numa_mem_align_shift;
     void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
                                  int nb_nodes, ram_addr_t size);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index fc54d55..8cff86f 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -311,6 +311,9 @@ struct qemu_work_item;
  * @trace_dstate_delayed: Delayed changes to trace_dstate (includes all changes
  *                        to @trace_dstate).
  * @trace_dstate: Dynamic tracing state of events for this vCPU (bitmask).
+ * @ignore_memory_transaction_failures: Cached copy of the MachineState
+ *    flag of the same name: allows the board to suppress calling of the
+ *    CPU do_transaction_failed hook function.
  *
  * State of one CPU core or thread.
  */
@@ -397,6 +400,8 @@ struct CPUState {
      */
     bool throttle_thread_scheduled;
 
+    bool ignore_memory_transaction_failures;
+
     /* Note that this is accessed at the start of every TB via a negative
        offset from AREG0.  Leave this field at the end so as to make the
        (absolute value) offset as small as possible.  This reduces code
@@ -853,7 +858,7 @@ static inline void cpu_transaction_failed(CPUState *cpu, hwaddr physaddr,
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
 
-    if (cc->do_transaction_failed) {
+    if (!cpu->ignore_memory_transaction_failures && cc->do_transaction_failed) {
         cc->do_transaction_failed(cpu, physaddr, addr, size, access_type,
                                   mmu_idx, attrs, response, retaddr);
     }
diff --git a/qom/cpu.c b/qom/cpu.c
index 4f38db0..d8dcf64 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -29,6 +29,7 @@
 #include "exec/cpu-common.h"
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
+#include "hw/boards.h"
 #include "hw/qdev-properties.h"
 #include "trace-root.h"
 
@@ -360,6 +361,12 @@ static void cpu_common_parse_features(const char *typename, char *features,
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
+    Object *machine = qdev_get_machine();
+    ObjectClass *oc = object_get_class(machine);
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    cpu->ignore_memory_transaction_failures =
+        mc->ignore_memory_transaction_failures;
 
     if (dev->hotplugged) {
         cpu_synchronize_post_init(cpu);
-- 
2.7.4

[Qemu-devel] [PATCH 5/8] hw/arm: Set ignore_memory_transaction_failures for most ARM boards

[Peter Maydell]

Set the MachineClass flag ignore_memory_transaction_failures
for almost all ARM boards. This means they retain the legacy
behaviour that accesses to unimplemented addresses will RAZ/WI
rather than aborting, when a subsequent commit adds support
for external aborts.

The exceptions are:
 * virt -- we know that guests won't try to prod devices
   that we don't describe in the device tree or ACPI tables
 * mps2 -- this board was written to use unimplemented-device
   for all the ranges with devices we don't yet handle

New boards should not set the flag, but instead be written
like the mps2.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/aspeed.c         | 3 +++
 hw/arm/collie.c         | 1 +
 hw/arm/cubieboard.c     | 1 +
 hw/arm/digic_boards.c   | 1 +
 hw/arm/exynos4_boards.c | 2 ++
 hw/arm/gumstix.c        | 2 ++
 hw/arm/highbank.c       | 2 ++
 hw/arm/imx25_pdk.c      | 1 +
 hw/arm/integratorcp.c   | 1 +
 hw/arm/kzm.c            | 1 +
 hw/arm/mainstone.c      | 1 +
 hw/arm/musicpal.c       | 1 +
 hw/arm/netduino2.c      | 1 +
 hw/arm/nseries.c        | 2 ++
 hw/arm/omap_sx1.c       | 2 ++
 hw/arm/palm.c           | 1 +
 hw/arm/raspi.c          | 1 +
 hw/arm/realview.c       | 4 ++++
 hw/arm/sabrelite.c      | 1 +
 hw/arm/spitz.c          | 4 ++++
 hw/arm/stellaris.c      | 2 ++
 hw/arm/tosa.c           | 1 +
 hw/arm/versatilepb.c    | 2 ++
 hw/arm/vexpress.c       | 1 +
 hw/arm/xilinx_zynq.c    | 1 +
 hw/arm/xlnx-ep108.c     | 2 ++
 hw/arm/z2.c             | 1 +
 27 files changed, 43 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 0c5635f..ab895ad 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -270,6 +270,7 @@ static void palmetto_bmc_class_init(ObjectClass *oc, void *data)
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo palmetto_bmc_type = {
@@ -302,6 +303,7 @@ static void ast2500_evb_class_init(ObjectClass *oc, void *data)
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo ast2500_evb_type = {
@@ -326,6 +328,7 @@ static void romulus_bmc_class_init(ObjectClass *oc, void *data)
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo romulus_bmc_type = {
diff --git a/hw/arm/collie.c b/hw/arm/collie.c
index 2e69531..8830192 100644
--- a/hw/arm/collie.c
+++ b/hw/arm/collie.c
@@ -64,6 +64,7 @@ static void collie_machine_init(MachineClass *mc)
 {
     mc->desc = "Sharp SL-5500 (Collie) PDA (SA-1110)";
     mc->init = collie_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("collie", collie_machine_init)
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index b98e1c4..32f1edd 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -86,6 +86,7 @@ static void cubieboard_machine_init(MachineClass *mc)
     mc->init = cubieboard_init;
     mc->block_default_type = IF_IDE;
     mc->units_per_default_bus = 1;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("cubieboard", cubieboard_machine_init)
diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index 520c8e9..9f11dcd 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -155,6 +155,7 @@ static void canon_a1100_machine_init(MachineClass *mc)
 {
     mc->desc = "Canon PowerShot A1100 IS";
     mc->init = &canon_a1100_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("canon-a1100", canon_a1100_machine_init)
diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
index 7c03ed3..f1441ec 100644
--- a/hw/arm/exynos4_boards.c
+++ b/hw/arm/exynos4_boards.c
@@ -189,6 +189,7 @@ static void nuri_class_init(ObjectClass *oc, void *data)
     mc->desc = "Samsung NURI board (Exynos4210)";
     mc->init = nuri_init;
     mc->max_cpus = EXYNOS4210_NCPUS;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo nuri_type = {
@@ -204,6 +205,7 @@ static void smdkc210_class_init(ObjectClass *oc, void *data)
     mc->desc = "Samsung SMDKC210 board (Exynos4210)";
     mc->init = smdkc210_init;
     mc->max_cpus = EXYNOS4210_NCPUS;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo smdkc210_type = {
diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c
index d59d9ba..092ce36 100644
--- a/hw/arm/gumstix.c
+++ b/hw/arm/gumstix.c
@@ -128,6 +128,7 @@ static void connex_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Gumstix Connex (PXA255)";
     mc->init = connex_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo connex_type = {
@@ -142,6 +143,7 @@ static void verdex_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Gumstix Verdex (PXA270)";
     mc->init = verdex_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo verdex_type = {
diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 20e60f1..0d222fe 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -410,6 +410,7 @@ static void highbank_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_IDE;
     mc->units_per_default_bus = 1;
     mc->max_cpus = 4;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo highbank_type = {
@@ -427,6 +428,7 @@ static void midway_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_IDE;
     mc->units_per_default_bus = 1;
     mc->max_cpus = 4;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo midway_type = {
diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
index 7d42c74..9f3ee14 100644
--- a/hw/arm/imx25_pdk.c
+++ b/hw/arm/imx25_pdk.c
@@ -148,6 +148,7 @@ static void imx25_pdk_machine_init(MachineClass *mc)
 {
     mc->desc = "ARM i.MX25 PDK board (ARM926)";
     mc->init = imx25_pdk_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("imx25-pdk", imx25_pdk_machine_init)
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index d9530ed..d603af9 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -681,6 +681,7 @@ static void integratorcp_machine_init(MachineClass *mc)
 {
     mc->desc = "ARM Integrator/CP (ARM926EJ-S)";
     mc->init = integratorcp_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("integratorcp", integratorcp_machine_init)
diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
index 3ed6577..f9c2228 100644
--- a/hw/arm/kzm.c
+++ b/hw/arm/kzm.c
@@ -142,6 +142,7 @@ static void kzm_machine_init(MachineClass *mc)
 {
     mc->desc = "ARM KZM Emulation Baseboard (ARM1136)";
     mc->init = kzm_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("kzm", kzm_machine_init)
diff --git a/hw/arm/mainstone.c b/hw/arm/mainstone.c
index fb268e6..637f52c 100644
--- a/hw/arm/mainstone.c
+++ b/hw/arm/mainstone.c
@@ -196,6 +196,7 @@ static void mainstone2_machine_init(MachineClass *mc)
 {
     mc->desc = "Mainstone II (PXA27x)";
     mc->init = mainstone_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("mainstone", mainstone2_machine_init)
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 7e8ab31..fcf6224 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1718,6 +1718,7 @@ static void musicpal_machine_init(MachineClass *mc)
 {
     mc->desc = "Marvell 88w8618 / MusicPal (ARM926EJ-S)";
     mc->init = musicpal_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("musicpal", musicpal_machine_init)
diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index 3cfe332..9d34d4c 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -45,6 +45,7 @@ static void netduino2_machine_init(MachineClass *mc)
 {
     mc->desc = "Netduino 2 Machine";
     mc->init = netduino2_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("netduino2", netduino2_machine_init)
diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index 503a3b6..a32ac82 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -1425,6 +1425,7 @@ static void n800_class_init(ObjectClass *oc, void *data)
     mc->desc = "Nokia N800 tablet aka. RX-34 (OMAP2420)";
     mc->init = n800_init;
     mc->default_boot_order = "";
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo n800_type = {
@@ -1440,6 +1441,7 @@ static void n810_class_init(ObjectClass *oc, void *data)
     mc->desc = "Nokia N810 tablet aka. RX-44 (OMAP2420)";
     mc->init = n810_init;
     mc->default_boot_order = "";
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo n810_type = {
diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
index 9809106..4535617 100644
--- a/hw/arm/omap_sx1.c
+++ b/hw/arm/omap_sx1.c
@@ -223,6 +223,7 @@ static void sx1_machine_v2_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Siemens SX1 (OMAP310) V2";
     mc->init = sx1_init_v2;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo sx1_machine_v2_type = {
@@ -237,6 +238,7 @@ static void sx1_machine_v1_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Siemens SX1 (OMAP310) V1";
     mc->init = sx1_init_v1;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo sx1_machine_v1_type = {
diff --git a/hw/arm/palm.c b/hw/arm/palm.c
index 64cf8ca..bf070a2 100644
--- a/hw/arm/palm.c
+++ b/hw/arm/palm.c
@@ -274,6 +274,7 @@ static void palmte_machine_init(MachineClass *mc)
 {
     mc->desc = "Palm Tungsten|E aka. Cheetah PDA (OMAP310)";
     mc->init = palmte_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("cheetah", palmte_machine_init)
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 32cdc98..5941c9f 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -168,5 +168,6 @@ static void raspi2_machine_init(MachineClass *mc)
     mc->no_cdrom = 1;
     mc->max_cpus = BCM2836_NCPUS;
     mc->default_ram_size = 1024 * 1024 * 1024;
+    mc->ignore_memory_transaction_failures = true;
 };
 DEFINE_MACHINE("raspi2", raspi2_machine_init)
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 76ff557..f1b261f 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -396,6 +396,7 @@ static void realview_eb_class_init(ObjectClass *oc, void *data)
     mc->desc = "ARM RealView Emulation Baseboard (ARM926EJ-S)";
     mc->init = realview_eb_init;
     mc->block_default_type = IF_SCSI;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo realview_eb_type = {
@@ -412,6 +413,7 @@ static void realview_eb_mpcore_class_init(ObjectClass *oc, void *data)
     mc->init = realview_eb_mpcore_init;
     mc->block_default_type = IF_SCSI;
     mc->max_cpus = 4;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo realview_eb_mpcore_type = {
@@ -426,6 +428,7 @@ static void realview_pb_a8_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "ARM RealView Platform Baseboard for Cortex-A8";
     mc->init = realview_pb_a8_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo realview_pb_a8_type = {
@@ -441,6 +444,7 @@ static void realview_pbx_a9_class_init(ObjectClass *oc, void *data)
     mc->desc = "ARM RealView Platform Baseboard Explore for Cortex-A9";
     mc->init = realview_pbx_a9_init;
     mc->max_cpus = 4;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo realview_pbx_a9_type = {
diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
index 4e7ac8c..ee140e5 100644
--- a/hw/arm/sabrelite.c
+++ b/hw/arm/sabrelite.c
@@ -122,6 +122,7 @@ static void sabrelite_machine_init(MachineClass *mc)
     mc->desc = "Freescale i.MX6 Quad SABRE Lite Board (Cortex A9)";
     mc->init = sabrelite_init;
     mc->max_cpus = FSL_IMX6_NUM_CPUS;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("sabrelite", sabrelite_machine_init)
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 7f588ce..6406421 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -983,6 +983,7 @@ static void akitapda_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Sharp SL-C1000 (Akita) PDA (PXA270)";
     mc->init = akita_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo akitapda_type = {
@@ -998,6 +999,7 @@ static void spitzpda_class_init(ObjectClass *oc, void *data)
     mc->desc = "Sharp SL-C3000 (Spitz) PDA (PXA270)";
     mc->init = spitz_init;
     mc->block_default_type = IF_IDE;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo spitzpda_type = {
@@ -1013,6 +1015,7 @@ static void borzoipda_class_init(ObjectClass *oc, void *data)
     mc->desc = "Sharp SL-C3100 (Borzoi) PDA (PXA270)";
     mc->init = borzoi_init;
     mc->block_default_type = IF_IDE;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo borzoipda_type = {
@@ -1028,6 +1031,7 @@ static void terrierpda_class_init(ObjectClass *oc, void *data)
     mc->desc = "Sharp SL-C3200 (Terrier) PDA (PXA270)";
     mc->init = terrier_init;
     mc->block_default_type = IF_IDE;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo terrierpda_type = {
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 408c1a1..b3aad23 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1453,6 +1453,7 @@ static void lm3s811evb_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Stellaris LM3S811EVB";
     mc->init = lm3s811evb_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo lm3s811evb_type = {
@@ -1467,6 +1468,7 @@ static void lm3s6965evb_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Stellaris LM3S6965EVB";
     mc->init = lm3s6965evb_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo lm3s6965evb_type = {
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 8b757ff..1134cf7 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -263,6 +263,7 @@ static void tosapda_machine_init(MachineClass *mc)
     mc->desc = "Sharp SL-6000 (Tosa) PDA (PXA255)";
     mc->init = tosa_init;
     mc->block_default_type = IF_IDE;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("tosa", tosapda_machine_init)
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index b0e9f5b..76664e4 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -403,6 +403,7 @@ static void versatilepb_class_init(ObjectClass *oc, void *data)
     mc->desc = "ARM Versatile/PB (ARM926EJ-S)";
     mc->init = vpb_init;
     mc->block_default_type = IF_SCSI;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo versatilepb_type = {
@@ -418,6 +419,7 @@ static void versatileab_class_init(ObjectClass *oc, void *data)
     mc->desc = "ARM Versatile/AB (ARM926EJ-S)";
     mc->init = vab_init;
     mc->block_default_type = IF_SCSI;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static const TypeInfo versatileab_type = {
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 528c65d..9be1833 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -750,6 +750,7 @@ static void vexpress_class_init(ObjectClass *oc, void *data)
     mc->desc = "ARM Versatile Express";
     mc->init = vexpress_common_init;
     mc->max_cpus = 4;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 static void vexpress_a9_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 6b11a75..9883215 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -324,6 +324,7 @@ static void zynq_machine_init(MachineClass *mc)
     mc->init = zynq_init;
     mc->max_cpus = 1;
     mc->no_sdcard = 1;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("xilinx-zynq-a9", zynq_machine_init)
diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
index 860780a..c339cd4 100644
--- a/hw/arm/xlnx-ep108.c
+++ b/hw/arm/xlnx-ep108.c
@@ -122,6 +122,7 @@ static void xlnx_ep108_machine_init(MachineClass *mc)
     mc->init = xlnx_ep108_init;
     mc->block_default_type = IF_IDE;
     mc->units_per_default_bus = 1;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("xlnx-ep108", xlnx_ep108_machine_init)
@@ -132,6 +133,7 @@ static void xlnx_zcu102_machine_init(MachineClass *mc)
     mc->init = xlnx_ep108_init;
     mc->block_default_type = IF_IDE;
     mc->units_per_default_bus = 1;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("xlnx-zcu102", xlnx_zcu102_machine_init)
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index 1607cbd..417bc1a 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -370,6 +370,7 @@ static void z2_machine_init(MachineClass *mc)
 {
     mc->desc = "Zipit Z2 (PXA27x)";
     mc->init = z2_init;
+    mc->ignore_memory_transaction_failures = true;
 }
 
 DEFINE_MACHINE("z2", z2_machine_init)
-- 
2.7.4

[Qemu-devel] [PATCH 6/8] target/arm: Factor out fault delivery code

[Peter Maydell]

We currently have some similar code in tlb_fill() and in
arm_cpu_do_unaligned_access() for delivering a data abort or prefetch
abort.  We're also going to want to do the same thing to handle
external aborts.  Factor out the common code into a new function
deliver_fault().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/op_helper.c | 110 +++++++++++++++++++++++++------------------------
 1 file changed, 57 insertions(+), 53 deletions(-)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 2a85666..aa52a98 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -115,6 +115,51 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
     return syn;
 }
 
+static void deliver_fault(ARMCPU *cpu, vaddr addr, MMUAccessType access_type,
+                          uint32_t fsr, uint32_t fsc, ARMMMUFaultInfo *fi)
+{
+    CPUARMState *env = &cpu->env;
+    int target_el;
+    bool same_el;
+    uint32_t syn, exc;
+
+    target_el = exception_target_el(env);
+    if (fi->stage2) {
+        target_el = 2;
+        env->cp15.hpfar_el2 = extract64(fi->s2addr, 12, 47) << 4;
+    }
+    same_el = (arm_current_el(env) == target_el);
+
+    if (fsc == 0x3f) {
+        /* Caller doesn't have a long-format fault status code. This
+         * should only happen if this fault will never actually be reported
+         * to an EL that uses a syndrome register. Check that here.
+         * 0x3f is a (currently) reserved FSR code, in case the constructed
+         * syndrome does leak into the guest somehow.
+         */
+        assert(target_el != 2 && !arm_el_is_aa64(env, target_el));
+    }
+
+    if (access_type == MMU_INST_FETCH) {
+        syn = syn_insn_abort(same_el, 0, fi->s1ptw, fsc);
+        exc = EXCP_PREFETCH_ABORT;
+    } else {
+        syn = merge_syn_data_abort(env->exception.syndrome, target_el,
+                                   same_el, fi->s1ptw,
+                                   access_type == MMU_DATA_STORE,
+                                   fsc);
+        if (access_type == MMU_DATA_STORE
+            && arm_feature(env, ARM_FEATURE_V6)) {
+            fsr |= (1 << 11);
+        }
+        exc = EXCP_DATA_ABORT;
+    }
+
+    env->exception.vaddress = addr;
+    env->exception.fsr = fsr;
+    raise_exception(env, exc, syn, target_el);
+}
+
 /* try to fill the TLB and return an exception if error. If retaddr is
  * NULL, it means that the function was called in C code (i.e. not
  * from generated code or from helper.c)
@@ -129,23 +174,13 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
     ret = arm_tlb_fill(cs, addr, access_type, mmu_idx, &fsr, &fi);
     if (unlikely(ret)) {
         ARMCPU *cpu = ARM_CPU(cs);
-        CPUARMState *env = &cpu->env;
-        uint32_t syn, exc, fsc;
-        unsigned int target_el;
-        bool same_el;
+        uint32_t fsc;
 
         if (retaddr) {
             /* now we have a real cpu fault */
             cpu_restore_state(cs, retaddr);
         }
 
-        target_el = exception_target_el(env);
-        if (fi.stage2) {
-            target_el = 2;
-            env->cp15.hpfar_el2 = extract64(fi.s2addr, 12, 47) << 4;
-        }
-        same_el = arm_current_el(env) == target_el;
-
         if (fsr & (1 << 9)) {
             /* LPAE format fault status register : bottom 6 bits are
              * status code in the same form as needed for syndrome
@@ -153,34 +188,15 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
             fsc = extract32(fsr, 0, 6);
         } else {
             /* Short format FSR : this fault will never actually be reported
-             * to an EL that uses a syndrome register. Check that here,
-             * and use a (currently) reserved FSR code in case the constructed
-             * syndrome does leak into the guest somehow.
+             * to an EL that uses a syndrome register. Use a (currently)
+             * reserved FSR code in case the constructed syndrome does leak
+             * into the guest somehow. deliver_fault will assert that
+             * we don't target an EL using the syndrome.
              */
-            assert(target_el != 2 && !arm_el_is_aa64(env, target_el));
             fsc = 0x3f;
         }
 
-        /* For insn and data aborts we assume there is no instruction syndrome
-         * information; this is always true for exceptions reported to EL1.
-         */
-        if (access_type == MMU_INST_FETCH) {
-            syn = syn_insn_abort(same_el, 0, fi.s1ptw, fsc);
-            exc = EXCP_PREFETCH_ABORT;
-        } else {
-            syn = merge_syn_data_abort(env->exception.syndrome, target_el,
-                                       same_el, fi.s1ptw,
-                                       access_type == MMU_DATA_STORE, fsc);
-            if (access_type == MMU_DATA_STORE
-                && arm_feature(env, ARM_FEATURE_V6)) {
-                fsr |= (1 << 11);
-            }
-            exc = EXCP_DATA_ABORT;
-        }
-
-        env->exception.vaddress = addr;
-        env->exception.fsr = fsr;
-        raise_exception(env, exc, syn, target_el);
+        deliver_fault(cpu, addr, access_type, fsr, fsc, &fi);
     }
 }
 
@@ -191,9 +207,8 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-    int target_el;
-    bool same_el;
-    uint32_t syn;
+    uint32_t fsr, fsc;
+    ARMMMUFaultInfo fi = {};
     ARMMMUIdx arm_mmu_idx = core_to_arm_mmu_idx(env, mmu_idx);
 
     if (retaddr) {
@@ -201,28 +216,17 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
         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_s1_regime_using_lpae_format(env, arm_mmu_idx)) {
-        env->exception.fsr = (1 << 9) | 0x21;
+        fsr = (1 << 9) | 0x21;
     } else {
-        env->exception.fsr = 0x1;
-    }
-
-    if (access_type == MMU_DATA_STORE && arm_feature(env, ARM_FEATURE_V6)) {
-        env->exception.fsr |= (1 << 11);
+        fsr = 0x1;
     }
+    fsc = 0x21;
 
-    syn = merge_syn_data_abort(env->exception.syndrome, target_el,
-                               same_el, 0, access_type == MMU_DATA_STORE,
-                               0x21);
-    raise_exception(env, EXCP_DATA_ABORT, syn, target_el);
+    deliver_fault(cpu, vaddr, access_type, fsr, fsc, &fi);
 }
 
 #endif /* !defined(CONFIG_USER_ONLY) */
-- 
2.7.4

[Qemu-devel] [PATCH 7/8] target/arm: Allow deliver_fault() caller to specify EA bit

[Peter Maydell]

For external aborts, we will want to be able to specify the EA
(external abort type) bit in the syndrome field.  Allow callers of
deliver_fault() to do that by adding a field to ARMMMUFaultInfo which
we use when constructing the syndrome values.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h |  2 ++
 target/arm/op_helper.c | 10 +++++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 1f6efef..a3adbd8 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -448,12 +448,14 @@ void arm_handle_psci_call(ARMCPU *cpu);
  * @s2addr: Address that caused a fault at stage 2
  * @stage2: True if we faulted at stage 2
  * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
+ * @ea: True if we should set the EA (external abort type) bit in syndrome
  */
 typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
 struct ARMMMUFaultInfo {
     target_ulong s2addr;
     bool stage2;
     bool s1ptw;
+    bool ea;
 };
 
 /* Do a page table walk and add page to TLB if possible */
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index aa52a98..7eac272 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -80,7 +80,7 @@ uint32_t HELPER(neon_tbl)(CPUARMState *env, uint32_t ireg, uint32_t def,
 
 static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
                                             unsigned int target_el,
-                                            bool same_el,
+                                            bool same_el, bool ea,
                                             bool s1ptw, bool is_write,
                                             int fsc)
 {
@@ -99,7 +99,7 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
      */
     if (!(template_syn & ARM_EL_ISV) || target_el != 2 || s1ptw) {
         syn = syn_data_abort_no_iss(same_el,
-                                    0, 0, s1ptw, is_write, fsc);
+                                    ea, 0, s1ptw, is_write, fsc);
     } else {
         /* Fields: IL, ISV, SAS, SSE, SRT, SF and AR come from the template
          * syndrome created at translation time.
@@ -107,7 +107,7 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
          */
         syn = syn_data_abort_with_iss(same_el,
                                       0, 0, 0, 0, 0,
-                                      0, 0, s1ptw, is_write, fsc,
+                                      ea, 0, s1ptw, is_write, fsc,
                                       false);
         /* Merge the runtime syndrome with the template syndrome.  */
         syn |= template_syn;
@@ -141,11 +141,11 @@ static void deliver_fault(ARMCPU *cpu, vaddr addr, MMUAccessType access_type,
     }
 
     if (access_type == MMU_INST_FETCH) {
-        syn = syn_insn_abort(same_el, 0, fi->s1ptw, fsc);
+        syn = syn_insn_abort(same_el, fi->ea, fi->s1ptw, fsc);
         exc = EXCP_PREFETCH_ABORT;
     } else {
         syn = merge_syn_data_abort(env->exception.syndrome, target_el,
-                                   same_el, fi->s1ptw,
+                                   same_el, fi->ea, fi->s1ptw,
                                    access_type == MMU_DATA_STORE,
                                    fsc);
         if (access_type == MMU_DATA_STORE
-- 
2.7.4

[Qemu-devel] [PATCH 8/8] target/arm: Implement new do_transaction_failed hook

[Peter Maydell]

Implement the new do_transaction_failed hook for ARM, which should
cause the CPU to take a prefetch abort or data abort.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h | 10 ++++++++++
 target/arm/cpu.c       |  1 +
 target/arm/op_helper.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index a3adbd8..13bb001 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -471,6 +471,16 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
                                  MMUAccessType access_type,
                                  int mmu_idx, uintptr_t retaddr);
 
+/* arm_cpu_do_transaction_failed: handle a memory system error response
+ * (eg "no device/memory present at address") by raising an external abort
+ * exception
+ */
+void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                   vaddr addr, unsigned size,
+                                   MMUAccessType access_type,
+                                   int mmu_idx, MemTxAttrs attrs,
+                                   MemTxResult response, uintptr_t retaddr);
+
 /* Call the EL change hook if one has been registered */
 static inline void arm_call_el_change_hook(ARMCPU *cpu)
 {
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 05c038b..6baede0 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1670,6 +1670,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
 #else
     cc->do_interrupt = arm_cpu_do_interrupt;
     cc->do_unaligned_access = arm_cpu_do_unaligned_access;
+    cc->do_transaction_failed = arm_cpu_do_transaction_failed;
     cc->get_phys_page_attrs_debug = arm_cpu_get_phys_page_attrs_debug;
     cc->asidx_from_attrs = arm_asidx_from_attrs;
     cc->vmsd = &vmstate_arm_cpu;
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 7eac272..54b6dd8 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -229,6 +229,49 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
     deliver_fault(cpu, vaddr, access_type, fsr, fsc, &fi);
 }
 
+/* arm_cpu_do_transaction_failed: handle a memory system error response
+ * (eg "no device/memory present at address") by raising an external abort
+ * exception
+ */
+void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                   vaddr addr, unsigned size,
+                                   MMUAccessType access_type,
+                                   int mmu_idx, MemTxAttrs attrs,
+                                   MemTxResult response, uintptr_t retaddr)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    uint32_t fsr, fsc;
+    ARMMMUFaultInfo fi = {};
+    ARMMMUIdx arm_mmu_idx = core_to_arm_mmu_idx(env, mmu_idx);
+
+    if (retaddr) {
+        /* now we have a real cpu fault */
+        cpu_restore_state(cs, retaddr);
+    }
+
+    /* The EA bit in syndromes and fault status registers is an
+     * IMPDEF classification of external aborts. ARM implementations
+     * usually use this to indicate AXI bus Decode error (0) or
+     * Slave error (1); in QEMU we follow that.
+     */
+    fi.ea = (response != MEMTX_DECODE_ERROR);
+
+    /* The fault status register format depends on whether we're using
+     * the LPAE long descriptor format, or the short descriptor format.
+     */
+    if (arm_s1_regime_using_lpae_format(env, arm_mmu_idx)) {
+        /* long descriptor form, STATUS 0b010000: synchronous ext abort */
+        fsr = (fi.ea << 12) | (1 << 9) | 0x10;
+    } else {
+        /* short descriptor form, FSR 0b01000 : synchronous ext abort */
+        fsr = (fi.ea << 12) | 0x8;
+    }
+    fsc = 0x10;
+
+    deliver_fault(cpu, addr, access_type, fsr, fsc, &fi);
+}
+
 #endif /* !defined(CONFIG_USER_ONLY) */
 
 uint32_t HELPER(add_setq)(CPUARMState *env, uint32_t a, uint32_t b)
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/8] memory.h: Move MemTxResult type to memattrs.h

[Richard Henderson]

On 08/04/2017 10:20 AM, Peter Maydell wrote:
> Move the MemTxResult type to memattrs.h. We're going to want to
> use it in cpu/qom.h, which doesn't want to include all of
> memory.h. In practice MemTxResult and MemTxAttrs are pretty
> closely linked since both are used for the new-style
> read_with_attrs and write_with_attrs callbacks, so memattrs.h
> is a reasonable home for this rather than creating a whole
> new header file for it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/memattrs.h | 10 ++++++++++
>  include/exec/memory.h   | 10 ----------
>  2 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Philippe Mathieu-Daudé]

On 08/04/2017 02:20 PM, Peter Maydell wrote:
> Define a new MachineClass field ignore_memory_transaction_failures.
> If this is flag is true then the CPU will ignore memory transaction
> failures which should cause the CPU to take an exception due to an
> access to an unassigned physical address; the transaction will
> instead return zero (for a read) or be ignored (for a write).  This
> should be set only by legacy board models which rely on the old
> RAZ/WI behaviour for handling devices that QEMU does not yet model.
> New board models should instead use "unimplemented-device" for all
> memory ranges where the guest will attempt to probe for a device that
> QEMU doesn't implement and a stub device is required.

This is a very good idea. At least it will help understanding why not 
all firmwares compiled for the same board can boot.

Since create_unimplemented_device() register overlapped with low 
priority, why not register it as default device directly, over the whole 
address space?

> 
> We need this for ARM boards, where we're about to implement support for
> generating external aborts on memory transaction failures. Too many
> of our legacy board models rely on the RAZ/WI behaviour and we
> would break currently working guests when their "probe for device"
> code provoked an external abort rather than a RAZ.

I think some firmware will give some surprises, those probing device is 
not here and expect RAZ/WI. I remember some fw probing PCI space, or 
enumerating CS this way for ex.

RAZ/WI is a bus-feature, this is also bus-dependent to reply with abort 
or behave RAZ/WI. Maybe the effort should be done on how model/use buses 
in QEMU? Bus device would be an alias of unimplemented_device, which 
current purpose is more debugging than avoiding unassigned physical 
access aborts.

I'm pretty sure this library setup probes for unassigned access 
installing an handler and checking it got hit, in this case (ab)using 
unimplemented_device would prevent this firmware to boot:
http://www.ti.com/ww/en/functional_safety/safeti/index.html
(I might have self-answered my first question)

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/boards.h | 11 +++++++++++
>   include/qom/cpu.h   |  7 ++++++-
>   qom/cpu.c           |  7 +++++++
>   3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3363dd1..7f044d1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -131,6 +131,16 @@ typedef struct {
>    *    size than the target architecture's minimum. (Attempting to create
>    *    such a CPU will fail.) Note that changing this is a migration
>    *    compatibility break for the machine.
> + * @ignore_memory_transaction_failures:
> + *    If this is flag is true then the CPU will ignore memory transaction
> + *    failures which should cause the CPU to take an exception due to an
> + *    access to an unassigned physical address; the transaction will instead
> + *    return zero (for a read) or be ignored (for a write). This should be
> + *    set only by legacy board models which rely on the old RAZ/WI behaviour
> + *    for handling devices that QEMU does not yet model. New board models
> + *    should instead use "unimplemented-device" for all memory ranges where
> + *    the guest will attempt to probe for a device that QEMU doesn't
> + *    implement and a stub device is required.
>    */
>   struct MachineClass {
>       /*< private >*/
> @@ -171,6 +181,7 @@ struct MachineClass {
>       bool rom_file_has_mr;
>       int minimum_page_bits;
>       bool has_hotpluggable_cpus;
> +    bool ignore_memory_transaction_failures;
>       int numa_mem_align_shift;
>       void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>                                    int nb_nodes, ram_addr_t size);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index fc54d55..8cff86f 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -311,6 +311,9 @@ struct qemu_work_item;
>    * @trace_dstate_delayed: Delayed changes to trace_dstate (includes all changes
>    *                        to @trace_dstate).
>    * @trace_dstate: Dynamic tracing state of events for this vCPU (bitmask).
> + * @ignore_memory_transaction_failures: Cached copy of the MachineState
> + *    flag of the same name: allows the board to suppress calling of the
> + *    CPU do_transaction_failed hook function.
>    *
>    * State of one CPU core or thread.
>    */
> @@ -397,6 +400,8 @@ struct CPUState {
>        */
>       bool throttle_thread_scheduled;
>   
> +    bool ignore_memory_transaction_failures;
> +
>       /* Note that this is accessed at the start of every TB via a negative
>          offset from AREG0.  Leave this field at the end so as to make the
>          (absolute value) offset as small as possible.  This reduces code
> @@ -853,7 +858,7 @@ static inline void cpu_transaction_failed(CPUState *cpu, hwaddr physaddr,
>   {
>       CPUClass *cc = CPU_GET_CLASS(cpu);
>   
> -    if (cc->do_transaction_failed) {
> +    if (!cpu->ignore_memory_transaction_failures && cc->do_transaction_failed) {
>           cc->do_transaction_failed(cpu, physaddr, addr, size, access_type,
>                                     mmu_idx, attrs, response, retaddr);
>       }
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 4f38db0..d8dcf64 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -29,6 +29,7 @@
>   #include "exec/cpu-common.h"
>   #include "qemu/error-report.h"
>   #include "sysemu/sysemu.h"
> +#include "hw/boards.h"
>   #include "hw/qdev-properties.h"
>   #include "trace-root.h"
>   
> @@ -360,6 +361,12 @@ static void cpu_common_parse_features(const char *typename, char *features,
>   static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>   {
>       CPUState *cpu = CPU(dev);
> +    Object *machine = qdev_get_machine();
> +    ObjectClass *oc = object_get_class(machine);
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    cpu->ignore_memory_transaction_failures =
> +        mc->ignore_memory_transaction_failures;
>   
>       if (dev->hotplugged) {
>           cpu_synchronize_post_init(cpu);
> 

Re: [Qemu-devel] [PATCH 2/8] cpu: Define new cpu_transaction_failed() hook

[Richard Henderson]

On 08/04/2017 10:20 AM, Peter Maydell wrote:
> Currently we have a rather half-baked setup for allowing CPUs to
> generate exceptions on accesses to invalid memory: the CPU has a
> cpu_unassigned_access() hook which the memory system calls in
> unassigned_mem_write() and unassigned_mem_read() if the current_cpu
> pointer is non-NULL.  This was originally designed before we
> implemented the MemTxResult type that allows memory operations to
> report a success or failure code, which is why the hook is called
> right at the bottom of the memory system.  The major problem with
> this is that it means that the hook can be called even when the
> access was not actually done by the CPU: for instance if the CPU
> writes to a DMA engine register which causes the DMA engine to begin
> a transaction which has been set up by the guest to operate on
> invalid memory then this will casue the CPU to take an exception
> incorrectly.  Another minor problem is that currently if a device
> returns a transaction error then this won't turn into a CPU exception
> at all.
> 
> The right way to do this is to have allow the CPU to respond
> to memory system transaction failures at the point where the
> CPU specific code calls into the memory system.
> 
> Define a new QOM CPU method and utility function
> cpu_transaction_failed() which is called in these cases.
> The functionality here overlaps with the existing
> cpu_unassigned_access() because individual target CPUs will
> need some work to convert them to the new system. When this
> transition is complete we can remove the old cpu_unassigned_access()
> code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/qom/cpu.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Richard Henderson]

On 08/04/2017 11:09 AM, Philippe Mathieu-Daudé wrote:
> Since create_unimplemented_device() register overlapped with low priority, why
> not register it as default device directly, over the whole address space?

That's a good suggestion.  It makes more sense to me than adding a flag on the
MachineClass.


r~

Re: [Qemu-devel] [PATCH 6/8] target/arm: Factor out fault delivery code

[Richard Henderson]

On 08/04/2017 10:20 AM, Peter Maydell wrote:
> +    if (fsc == 0x3f) {
> +        /* Caller doesn't have a long-format fault status code. This
> +         * should only happen if this fault will never actually be reported
> +         * to an EL that uses a syndrome register. Check that here.
> +         * 0x3f is a (currently) reserved FSR code, in case the constructed
> +         * syndrome does leak into the guest somehow.
> +         */
> +        assert(target_el != 2 && !arm_el_is_aa64(env, target_el));
> +    }

I see that this is just code movement, but there appears to be a typo in the
comment, confusing fsc vs fsr and the 0x3f reserved value.

Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 7/8] target/arm: Allow deliver_fault() caller to specify EA bit

[Richard Henderson]

On 08/04/2017 10:20 AM, Peter Maydell wrote:
> For external aborts, we will want to be able to specify the EA
> (external abort type) bit in the syndrome field.  Allow callers of
> deliver_fault() to do that by adding a field to ARMMMUFaultInfo which
> we use when constructing the syndrome values.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/internals.h |  2 ++
>  target/arm/op_helper.c | 10 +++++-----
>  2 files changed, 7 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 8/8] target/arm: Implement new do_transaction_failed hook

[Richard Henderson]

On 08/04/2017 10:20 AM, Peter Maydell wrote:
> Implement the new do_transaction_failed hook for ARM, which should
> cause the CPU to take a prefetch abort or data abort.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/internals.h | 10 ++++++++++
>  target/arm/cpu.c       |  1 +
>  target/arm/op_helper.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/8] memory.h: Move MemTxResult type to memattrs.h

[Edgar E. Iglesias]

On Fri, Aug 04, 2017 at 06:20:42PM +0100, Peter Maydell wrote:
> Move the MemTxResult type to memattrs.h. We're going to want to
> use it in cpu/qom.h, which doesn't want to include all of
> memory.h. In practice MemTxResult and MemTxAttrs are pretty
> closely linked since both are used for the new-style
> read_with_attrs and write_with_attrs callbacks, so memattrs.h
> is a reasonable home for this rather than creating a whole
> new header file for it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  include/exec/memattrs.h | 10 ++++++++++
>  include/exec/memory.h   | 10 ----------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index e601061..d4a1642 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -46,4 +46,14 @@ typedef struct MemTxAttrs {
>   */
>  #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
>  
> +/* New-style MMIO accessors can indicate that the transaction failed.
> + * A zero (MEMTX_OK) response means success; anything else is a failure
> + * of some kind. The memory subsystem will bitwise-OR together results
> + * if it is synthesizing an operation from multiple smaller accesses.
> + */
> +#define MEMTX_OK 0
> +#define MEMTX_ERROR             (1U << 0) /* device returned an error */
> +#define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
> +typedef uint32_t MemTxResult;
> +
>  #endif
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 400dd44..1dcd312 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -112,16 +112,6 @@ static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
>      n->end = end;
>  }
>  
> -/* New-style MMIO accessors can indicate that the transaction failed.
> - * A zero (MEMTX_OK) response means success; anything else is a failure
> - * of some kind. The memory subsystem will bitwise-OR together results
> - * if it is synthesizing an operation from multiple smaller accesses.
> - */
> -#define MEMTX_OK 0
> -#define MEMTX_ERROR             (1U << 0) /* device returned an error */
> -#define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
> -typedef uint32_t MemTxResult;
> -
>  /*
>   * Memory region callbacks
>   */
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/8] cpu: Define new cpu_transaction_failed() hook

[Edgar E. Iglesias]

On Fri, Aug 04, 2017 at 06:20:43PM +0100, Peter Maydell wrote:
> Currently we have a rather half-baked setup for allowing CPUs to
> generate exceptions on accesses to invalid memory: the CPU has a
> cpu_unassigned_access() hook which the memory system calls in
> unassigned_mem_write() and unassigned_mem_read() if the current_cpu
> pointer is non-NULL.  This was originally designed before we
> implemented the MemTxResult type that allows memory operations to
> report a success or failure code, which is why the hook is called
> right at the bottom of the memory system.  The major problem with
> this is that it means that the hook can be called even when the
> access was not actually done by the CPU: for instance if the CPU
> writes to a DMA engine register which causes the DMA engine to begin
> a transaction which has been set up by the guest to operate on
> invalid memory then this will casue the CPU to take an exception
> incorrectly.  Another minor problem is that currently if a device
> returns a transaction error then this won't turn into a CPU exception
> at all.
> 
> The right way to do this is to have allow the CPU to respond
> to memory system transaction failures at the point where the
> CPU specific code calls into the memory system.
> 
> Define a new QOM CPU method and utility function
> cpu_transaction_failed() which is called in these cases.
> The functionality here overlaps with the existing
> cpu_unassigned_access() because individual target CPUs will
> need some work to convert them to the new system. When this
> transition is complete we can remove the old cpu_unassigned_access()
> code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> ---
>  include/qom/cpu.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 25eefea..fc54d55 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -85,8 +85,10 @@ struct TranslationBlock;
>   * @has_work: Callback for checking if there is work to do.
>   * @do_interrupt: Callback for interrupt handling.
>   * @do_unassigned_access: Callback for unassigned access handling.
> + * (this is deprecated: new targets should use do_transaction_failed instead)
>   * @do_unaligned_access: Callback for unaligned access handling, if
>   * the target defines #ALIGNED_ONLY.
> + * @do_transaction_failed: Callback for handling failed memory transactions

Looks OK but I wonder if there you might want to clarify that this is a
bus/slave failure and not a failure within the CPU (e.g not an MMU fault).

Anyway:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



>   * @virtio_is_big_endian: Callback to return %true if a CPU which supports
>   * runtime configurable endianness is currently big-endian. Non-configurable
>   * CPUs can use the default implementation of this method. This method should
> @@ -153,6 +155,10 @@ typedef struct CPUClass {
>      void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>                                  MMUAccessType access_type,
>                                  int mmu_idx, uintptr_t retaddr);
> +    void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr,
> +                                  unsigned size, MMUAccessType access_type,
> +                                  int mmu_idx, MemTxAttrs attrs,
> +                                  MemTxResult response, uintptr_t retaddr);
>      bool (*virtio_is_big_endian)(CPUState *cpu);
>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                             uint8_t *buf, int len, bool is_write);
> @@ -837,6 +843,21 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
>  
>      cc->do_unaligned_access(cpu, addr, access_type, mmu_idx, retaddr);
>  }
> +
> +static inline void cpu_transaction_failed(CPUState *cpu, hwaddr physaddr,
> +                                          vaddr addr, unsigned size,
> +                                          MMUAccessType access_type,
> +                                          int mmu_idx, MemTxAttrs attrs,
> +                                          MemTxResult response,
> +                                          uintptr_t retaddr)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->do_transaction_failed) {
> +        cc->do_transaction_failed(cpu, physaddr, addr, size, access_type,
> +                                  mmu_idx, attrs, response, retaddr);
> +    }
> +}
>  #endif
>  
>  #endif /* NEED_CPU_H */
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/8] cpu: Define new cpu_transaction_failed() hook

[Edgar E. Iglesias]

On Fri, Aug 04, 2017 at 06:20:43PM +0100, Peter Maydell wrote:
> Currently we have a rather half-baked setup for allowing CPUs to
> generate exceptions on accesses to invalid memory: the CPU has a
> cpu_unassigned_access() hook which the memory system calls in
> unassigned_mem_write() and unassigned_mem_read() if the current_cpu
> pointer is non-NULL.  This was originally designed before we
> implemented the MemTxResult type that allows memory operations to
> report a success or failure code, which is why the hook is called
> right at the bottom of the memory system.  The major problem with
> this is that it means that the hook can be called even when the
> access was not actually done by the CPU: for instance if the CPU
> writes to a DMA engine register which causes the DMA engine to begin
> a transaction which has been set up by the guest to operate on
> invalid memory then this will casue the CPU to take an exception
> incorrectly.  Another minor problem is that currently if a device
> returns a transaction error then this won't turn into a CPU exception
> at all.
> 
> The right way to do this is to have allow the CPU to respond
> to memory system transaction failures at the point where the
> CPU specific code calls into the memory system.
> 
> Define a new QOM CPU method and utility function
> cpu_transaction_failed() which is called in these cases.
> The functionality here overlaps with the existing
> cpu_unassigned_access() because individual target CPUs will
> need some work to convert them to the new system. When this
> transition is complete we can remove the old cpu_unassigned_access()
> code.

BTW, a question. I don't know of any from memory but does any arch
have the ability to report the payload that failed for stores?
I guess it's easy enough to add that if needed though.


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/qom/cpu.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 25eefea..fc54d55 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -85,8 +85,10 @@ struct TranslationBlock;
>   * @has_work: Callback for checking if there is work to do.
>   * @do_interrupt: Callback for interrupt handling.
>   * @do_unassigned_access: Callback for unassigned access handling.
> + * (this is deprecated: new targets should use do_transaction_failed instead)
>   * @do_unaligned_access: Callback for unaligned access handling, if
>   * the target defines #ALIGNED_ONLY.
> + * @do_transaction_failed: Callback for handling failed memory transactions
>   * @virtio_is_big_endian: Callback to return %true if a CPU which supports
>   * runtime configurable endianness is currently big-endian. Non-configurable
>   * CPUs can use the default implementation of this method. This method should
> @@ -153,6 +155,10 @@ typedef struct CPUClass {
>      void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>                                  MMUAccessType access_type,
>                                  int mmu_idx, uintptr_t retaddr);
> +    void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr,
> +                                  unsigned size, MMUAccessType access_type,
> +                                  int mmu_idx, MemTxAttrs attrs,
> +                                  MemTxResult response, uintptr_t retaddr);
>      bool (*virtio_is_big_endian)(CPUState *cpu);
>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                             uint8_t *buf, int len, bool is_write);
> @@ -837,6 +843,21 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
>  
>      cc->do_unaligned_access(cpu, addr, access_type, mmu_idx, retaddr);
>  }
> +
> +static inline void cpu_transaction_failed(CPUState *cpu, hwaddr physaddr,
> +                                          vaddr addr, unsigned size,
> +                                          MMUAccessType access_type,
> +                                          int mmu_idx, MemTxAttrs attrs,
> +                                          MemTxResult response,
> +                                          uintptr_t retaddr)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->do_transaction_failed) {
> +        cc->do_transaction_failed(cpu, physaddr, addr, size, access_type,
> +                                  mmu_idx, attrs, response, retaddr);
> +    }
> +}
>  #endif
>  
>  #endif /* NEED_CPU_H */
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/8] cputlb: Support generating CPU exceptions on memory transaction failures

[Edgar E. Iglesias]

On Fri, Aug 04, 2017 at 06:20:44PM +0100, Peter Maydell wrote:
> Call the new cpu_transaction_failed() hook at the places where
> CPU generated code interacts with the memory system:
>  io_readx()
>  io_writex()
>  get_page_addr_code()
> 
> Any access from C code (eg via cpu_physical_memory_rw(),
> address_space_rw(), ld/st_*_phys()) will *not* trigger CPU exceptions
> via cpu_transaction_failed().  Handling for transactions failures for
> this kind of call should be done by using a function which returns a
> MemTxResult and treating the failure case appropriately in the
> calling code.
> 
> In an ideal world we would not generate CPU exceptions for
> instruction fetch failures in get_page_addr_code() but instead wait
> until the code translation process tried a load and it failed;
> however that change would require too great a restructuring and
> redesign to attempt at this point.

You're right but onsidering the lack of models for I caches and
prefetching, I don't think that matters much...

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  softmmu_template.h |  4 ++--
>  accel/tcg/cputlb.c | 32 ++++++++++++++++++++++++++++++--
>  2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/softmmu_template.h b/softmmu_template.h
> index 4a2b665..d756329 100644
> --- a/softmmu_template.h
> +++ b/softmmu_template.h
> @@ -101,7 +101,7 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
>                                                uintptr_t retaddr)
>  {
>      CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
> -    return io_readx(env, iotlbentry, addr, retaddr, DATA_SIZE);
> +    return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, DATA_SIZE);
>  }
>  #endif
>  
> @@ -262,7 +262,7 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env,
>                                            uintptr_t retaddr)
>  {
>      CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
> -    return io_writex(env, iotlbentry, val, addr, retaddr, DATA_SIZE);
> +    return io_writex(env, iotlbentry, mmu_idx, val, addr, retaddr, DATA_SIZE);
>  }
>  
>  void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 85635ae..e72415a 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -747,6 +747,7 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
>  }
>  
>  static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
> +                         int mmu_idx,
>                           target_ulong addr, uintptr_t retaddr, int size)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> @@ -754,6 +755,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>      MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->attrs);
>      uint64_t val;
>      bool locked = false;
> +    MemTxResult r;
>  
>      physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
>      cpu->mem_io_pc = retaddr;
> @@ -767,7 +769,12 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>          qemu_mutex_lock_iothread();
>          locked = true;
>      }
> -    memory_region_dispatch_read(mr, physaddr, &val, size, iotlbentry->attrs);
> +    r = memory_region_dispatch_read(mr, physaddr,
> +                                    &val, size, iotlbentry->attrs);
> +    if (r != MEMTX_OK) {
> +        cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_LOAD,
> +                               mmu_idx, iotlbentry->attrs, r, retaddr);
> +    }
>      if (locked) {
>          qemu_mutex_unlock_iothread();
>      }
> @@ -776,6 +783,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>  }
>  
>  static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
> +                      int mmu_idx,
>                        uint64_t val, target_ulong addr,
>                        uintptr_t retaddr, int size)
>  {
> @@ -783,6 +791,7 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>      hwaddr physaddr = iotlbentry->addr;
>      MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->attrs);
>      bool locked = false;
> +    MemTxResult r;
>  
>      physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
>      if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
> @@ -795,7 +804,12 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>          qemu_mutex_lock_iothread();
>          locked = true;
>      }
> -    memory_region_dispatch_write(mr, physaddr, val, size, iotlbentry->attrs);
> +    r = memory_region_dispatch_write(mr, physaddr,
> +                                     val, size, iotlbentry->attrs);
> +    if (r != MEMTX_OK) {
> +        cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_STORE,
> +                               mmu_idx, iotlbentry->attrs, r, retaddr);
> +    }
>      if (locked) {
>          qemu_mutex_unlock_iothread();
>      }
> @@ -845,6 +859,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>      MemoryRegion *mr;
>      CPUState *cpu = ENV_GET_CPU(env);
>      CPUIOTLBEntry *iotlbentry;
> +    hwaddr physaddr;
>  
>      index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>      mmu_idx = cpu_mmu_index(env, true);
> @@ -868,6 +883,19 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>          }
>          qemu_mutex_unlock_iothread();
>  
> +        /* Give the new-style cpu_transaction_failed() hook first chance
> +         * to handle this.
> +         * This is not the ideal place to detect and generate CPU
> +         * exceptions for instruction fetch failure (for instance
> +         * we don't know the length of the access that the CPU would
> +         * use, and it would be better to go ahead and try the access
> +         * and use the MemTXResult it produced). However it is the
> +         * simplest place we have currently available for the check.
> +         */
> +        physaddr = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
> +        cpu_transaction_failed(cpu, physaddr, addr, 0, MMU_INST_FETCH, mmu_idx,
> +                               iotlbentry->attrs, MEMTX_DECODE_ERROR, 0);
> +
>          cpu_unassigned_access(cpu, addr, false, true, 0, 4);
>          /* The CPU's unassigned access hook might have longjumped out
>           * with an exception. If it didn't (or there was no hook) then
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Edgar E. Iglesias]

On Fri, Aug 04, 2017 at 06:20:45PM +0100, Peter Maydell wrote:
> Define a new MachineClass field ignore_memory_transaction_failures.
> If this is flag is true then the CPU will ignore memory transaction
> failures which should cause the CPU to take an exception due to an
> access to an unassigned physical address; the transaction will
> instead return zero (for a read) or be ignored (for a write).  This
> should be set only by legacy board models which rely on the old
> RAZ/WI behaviour for handling devices that QEMU does not yet model.
> New board models should instead use "unimplemented-device" for all
> memory ranges where the guest will attempt to probe for a device that
> QEMU doesn't implement and a stub device is required.
> 
> We need this for ARM boards, where we're about to implement support for
> generating external aborts on memory transaction failures. Too many
> of our legacy board models rely on the RAZ/WI behaviour and we
> would break currently working guests when their "probe for device"
> code provoked an external abort rather than a RAZ.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  include/hw/boards.h | 11 +++++++++++
>  include/qom/cpu.h   |  7 ++++++-
>  qom/cpu.c           |  7 +++++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3363dd1..7f044d1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -131,6 +131,16 @@ typedef struct {
>   *    size than the target architecture's minimum. (Attempting to create
>   *    such a CPU will fail.) Note that changing this is a migration
>   *    compatibility break for the machine.
> + * @ignore_memory_transaction_failures:
> + *    If this is flag is true then the CPU will ignore memory transaction
> + *    failures which should cause the CPU to take an exception due to an
> + *    access to an unassigned physical address; the transaction will instead
> + *    return zero (for a read) or be ignored (for a write). This should be
> + *    set only by legacy board models which rely on the old RAZ/WI behaviour
> + *    for handling devices that QEMU does not yet model. New board models
> + *    should instead use "unimplemented-device" for all memory ranges where
> + *    the guest will attempt to probe for a device that QEMU doesn't
> + *    implement and a stub device is required.
>   */
>  struct MachineClass {
>      /*< private >*/
> @@ -171,6 +181,7 @@ struct MachineClass {
>      bool rom_file_has_mr;
>      int minimum_page_bits;
>      bool has_hotpluggable_cpus;
> +    bool ignore_memory_transaction_failures;
>      int numa_mem_align_shift;
>      void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>                                   int nb_nodes, ram_addr_t size);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index fc54d55..8cff86f 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -311,6 +311,9 @@ struct qemu_work_item;
>   * @trace_dstate_delayed: Delayed changes to trace_dstate (includes all changes
>   *                        to @trace_dstate).
>   * @trace_dstate: Dynamic tracing state of events for this vCPU (bitmask).
> + * @ignore_memory_transaction_failures: Cached copy of the MachineState
> + *    flag of the same name: allows the board to suppress calling of the
> + *    CPU do_transaction_failed hook function.
>   *
>   * State of one CPU core or thread.
>   */
> @@ -397,6 +400,8 @@ struct CPUState {
>       */
>      bool throttle_thread_scheduled;
>  
> +    bool ignore_memory_transaction_failures;
> +
>      /* Note that this is accessed at the start of every TB via a negative
>         offset from AREG0.  Leave this field at the end so as to make the
>         (absolute value) offset as small as possible.  This reduces code
> @@ -853,7 +858,7 @@ static inline void cpu_transaction_failed(CPUState *cpu, hwaddr physaddr,
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> -    if (cc->do_transaction_failed) {
> +    if (!cpu->ignore_memory_transaction_failures && cc->do_transaction_failed) {
>          cc->do_transaction_failed(cpu, physaddr, addr, size, access_type,
>                                    mmu_idx, attrs, response, retaddr);
>      }
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 4f38db0..d8dcf64 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -29,6 +29,7 @@
>  #include "exec/cpu-common.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/boards.h"
>  #include "hw/qdev-properties.h"
>  #include "trace-root.h"
>  
> @@ -360,6 +361,12 @@ static void cpu_common_parse_features(const char *typename, char *features,
>  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cpu = CPU(dev);
> +    Object *machine = qdev_get_machine();
> +    ObjectClass *oc = object_get_class(machine);
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    cpu->ignore_memory_transaction_failures =
> +        mc->ignore_memory_transaction_failures;
>  
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(cpu);
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/8] hw/arm: Set ignore_memory_transaction_failures for most ARM boards

[Edgar E. Iglesias]

On Fri, Aug 04, 2017 at 06:20:46PM +0100, Peter Maydell wrote:
> Set the MachineClass flag ignore_memory_transaction_failures
> for almost all ARM boards. This means they retain the legacy
> behaviour that accesses to unimplemented addresses will RAZ/WI
> rather than aborting, when a subsequent commit adds support
> for external aborts.
> 
> The exceptions are:
>  * virt -- we know that guests won't try to prod devices
>    that we don't describe in the device tree or ACPI tables
>  * mps2 -- this board was written to use unimplemented-device
>    for all the ranges with devices we don't yet handle
> 
> New boards should not set the flag, but instead be written
> like the mps2.

For the Xilinx boards:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/aspeed.c         | 3 +++
>  hw/arm/collie.c         | 1 +
>  hw/arm/cubieboard.c     | 1 +
>  hw/arm/digic_boards.c   | 1 +
>  hw/arm/exynos4_boards.c | 2 ++
>  hw/arm/gumstix.c        | 2 ++
>  hw/arm/highbank.c       | 2 ++
>  hw/arm/imx25_pdk.c      | 1 +
>  hw/arm/integratorcp.c   | 1 +
>  hw/arm/kzm.c            | 1 +
>  hw/arm/mainstone.c      | 1 +
>  hw/arm/musicpal.c       | 1 +
>  hw/arm/netduino2.c      | 1 +
>  hw/arm/nseries.c        | 2 ++
>  hw/arm/omap_sx1.c       | 2 ++
>  hw/arm/palm.c           | 1 +
>  hw/arm/raspi.c          | 1 +
>  hw/arm/realview.c       | 4 ++++
>  hw/arm/sabrelite.c      | 1 +
>  hw/arm/spitz.c          | 4 ++++
>  hw/arm/stellaris.c      | 2 ++
>  hw/arm/tosa.c           | 1 +
>  hw/arm/versatilepb.c    | 2 ++
>  hw/arm/vexpress.c       | 1 +
>  hw/arm/xilinx_zynq.c    | 1 +
>  hw/arm/xlnx-ep108.c     | 2 ++
>  hw/arm/z2.c             | 1 +
>  27 files changed, 43 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 0c5635f..ab895ad 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -270,6 +270,7 @@ static void palmetto_bmc_class_init(ObjectClass *oc, void *data)
>      mc->no_floppy = 1;
>      mc->no_cdrom = 1;
>      mc->no_parallel = 1;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo palmetto_bmc_type = {
> @@ -302,6 +303,7 @@ static void ast2500_evb_class_init(ObjectClass *oc, void *data)
>      mc->no_floppy = 1;
>      mc->no_cdrom = 1;
>      mc->no_parallel = 1;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo ast2500_evb_type = {
> @@ -326,6 +328,7 @@ static void romulus_bmc_class_init(ObjectClass *oc, void *data)
>      mc->no_floppy = 1;
>      mc->no_cdrom = 1;
>      mc->no_parallel = 1;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo romulus_bmc_type = {
> diff --git a/hw/arm/collie.c b/hw/arm/collie.c
> index 2e69531..8830192 100644
> --- a/hw/arm/collie.c
> +++ b/hw/arm/collie.c
> @@ -64,6 +64,7 @@ static void collie_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Sharp SL-5500 (Collie) PDA (SA-1110)";
>      mc->init = collie_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("collie", collie_machine_init)
> diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
> index b98e1c4..32f1edd 100644
> --- a/hw/arm/cubieboard.c
> +++ b/hw/arm/cubieboard.c
> @@ -86,6 +86,7 @@ static void cubieboard_machine_init(MachineClass *mc)
>      mc->init = cubieboard_init;
>      mc->block_default_type = IF_IDE;
>      mc->units_per_default_bus = 1;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("cubieboard", cubieboard_machine_init)
> diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
> index 520c8e9..9f11dcd 100644
> --- a/hw/arm/digic_boards.c
> +++ b/hw/arm/digic_boards.c
> @@ -155,6 +155,7 @@ static void canon_a1100_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Canon PowerShot A1100 IS";
>      mc->init = &canon_a1100_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("canon-a1100", canon_a1100_machine_init)
> diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
> index 7c03ed3..f1441ec 100644
> --- a/hw/arm/exynos4_boards.c
> +++ b/hw/arm/exynos4_boards.c
> @@ -189,6 +189,7 @@ static void nuri_class_init(ObjectClass *oc, void *data)
>      mc->desc = "Samsung NURI board (Exynos4210)";
>      mc->init = nuri_init;
>      mc->max_cpus = EXYNOS4210_NCPUS;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo nuri_type = {
> @@ -204,6 +205,7 @@ static void smdkc210_class_init(ObjectClass *oc, void *data)
>      mc->desc = "Samsung SMDKC210 board (Exynos4210)";
>      mc->init = smdkc210_init;
>      mc->max_cpus = EXYNOS4210_NCPUS;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo smdkc210_type = {
> diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c
> index d59d9ba..092ce36 100644
> --- a/hw/arm/gumstix.c
> +++ b/hw/arm/gumstix.c
> @@ -128,6 +128,7 @@ static void connex_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "Gumstix Connex (PXA255)";
>      mc->init = connex_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo connex_type = {
> @@ -142,6 +143,7 @@ static void verdex_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "Gumstix Verdex (PXA270)";
>      mc->init = verdex_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo verdex_type = {
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 20e60f1..0d222fe 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -410,6 +410,7 @@ static void highbank_class_init(ObjectClass *oc, void *data)
>      mc->block_default_type = IF_IDE;
>      mc->units_per_default_bus = 1;
>      mc->max_cpus = 4;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo highbank_type = {
> @@ -427,6 +428,7 @@ static void midway_class_init(ObjectClass *oc, void *data)
>      mc->block_default_type = IF_IDE;
>      mc->units_per_default_bus = 1;
>      mc->max_cpus = 4;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo midway_type = {
> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
> index 7d42c74..9f3ee14 100644
> --- a/hw/arm/imx25_pdk.c
> +++ b/hw/arm/imx25_pdk.c
> @@ -148,6 +148,7 @@ static void imx25_pdk_machine_init(MachineClass *mc)
>  {
>      mc->desc = "ARM i.MX25 PDK board (ARM926)";
>      mc->init = imx25_pdk_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("imx25-pdk", imx25_pdk_machine_init)
> diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
> index d9530ed..d603af9 100644
> --- a/hw/arm/integratorcp.c
> +++ b/hw/arm/integratorcp.c
> @@ -681,6 +681,7 @@ static void integratorcp_machine_init(MachineClass *mc)
>  {
>      mc->desc = "ARM Integrator/CP (ARM926EJ-S)";
>      mc->init = integratorcp_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("integratorcp", integratorcp_machine_init)
> diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
> index 3ed6577..f9c2228 100644
> --- a/hw/arm/kzm.c
> +++ b/hw/arm/kzm.c
> @@ -142,6 +142,7 @@ static void kzm_machine_init(MachineClass *mc)
>  {
>      mc->desc = "ARM KZM Emulation Baseboard (ARM1136)";
>      mc->init = kzm_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("kzm", kzm_machine_init)
> diff --git a/hw/arm/mainstone.c b/hw/arm/mainstone.c
> index fb268e6..637f52c 100644
> --- a/hw/arm/mainstone.c
> +++ b/hw/arm/mainstone.c
> @@ -196,6 +196,7 @@ static void mainstone2_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Mainstone II (PXA27x)";
>      mc->init = mainstone_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("mainstone", mainstone2_machine_init)
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 7e8ab31..fcf6224 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -1718,6 +1718,7 @@ static void musicpal_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Marvell 88w8618 / MusicPal (ARM926EJ-S)";
>      mc->init = musicpal_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("musicpal", musicpal_machine_init)
> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> index 3cfe332..9d34d4c 100644
> --- a/hw/arm/netduino2.c
> +++ b/hw/arm/netduino2.c
> @@ -45,6 +45,7 @@ static void netduino2_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Netduino 2 Machine";
>      mc->init = netduino2_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("netduino2", netduino2_machine_init)
> diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
> index 503a3b6..a32ac82 100644
> --- a/hw/arm/nseries.c
> +++ b/hw/arm/nseries.c
> @@ -1425,6 +1425,7 @@ static void n800_class_init(ObjectClass *oc, void *data)
>      mc->desc = "Nokia N800 tablet aka. RX-34 (OMAP2420)";
>      mc->init = n800_init;
>      mc->default_boot_order = "";
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo n800_type = {
> @@ -1440,6 +1441,7 @@ static void n810_class_init(ObjectClass *oc, void *data)
>      mc->desc = "Nokia N810 tablet aka. RX-44 (OMAP2420)";
>      mc->init = n810_init;
>      mc->default_boot_order = "";
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo n810_type = {
> diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
> index 9809106..4535617 100644
> --- a/hw/arm/omap_sx1.c
> +++ b/hw/arm/omap_sx1.c
> @@ -223,6 +223,7 @@ static void sx1_machine_v2_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "Siemens SX1 (OMAP310) V2";
>      mc->init = sx1_init_v2;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo sx1_machine_v2_type = {
> @@ -237,6 +238,7 @@ static void sx1_machine_v1_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "Siemens SX1 (OMAP310) V1";
>      mc->init = sx1_init_v1;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo sx1_machine_v1_type = {
> diff --git a/hw/arm/palm.c b/hw/arm/palm.c
> index 64cf8ca..bf070a2 100644
> --- a/hw/arm/palm.c
> +++ b/hw/arm/palm.c
> @@ -274,6 +274,7 @@ static void palmte_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Palm Tungsten|E aka. Cheetah PDA (OMAP310)";
>      mc->init = palmte_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("cheetah", palmte_machine_init)
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 32cdc98..5941c9f 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -168,5 +168,6 @@ static void raspi2_machine_init(MachineClass *mc)
>      mc->no_cdrom = 1;
>      mc->max_cpus = BCM2836_NCPUS;
>      mc->default_ram_size = 1024 * 1024 * 1024;
> +    mc->ignore_memory_transaction_failures = true;
>  };
>  DEFINE_MACHINE("raspi2", raspi2_machine_init)
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index 76ff557..f1b261f 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -396,6 +396,7 @@ static void realview_eb_class_init(ObjectClass *oc, void *data)
>      mc->desc = "ARM RealView Emulation Baseboard (ARM926EJ-S)";
>      mc->init = realview_eb_init;
>      mc->block_default_type = IF_SCSI;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo realview_eb_type = {
> @@ -412,6 +413,7 @@ static void realview_eb_mpcore_class_init(ObjectClass *oc, void *data)
>      mc->init = realview_eb_mpcore_init;
>      mc->block_default_type = IF_SCSI;
>      mc->max_cpus = 4;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo realview_eb_mpcore_type = {
> @@ -426,6 +428,7 @@ static void realview_pb_a8_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "ARM RealView Platform Baseboard for Cortex-A8";
>      mc->init = realview_pb_a8_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo realview_pb_a8_type = {
> @@ -441,6 +444,7 @@ static void realview_pbx_a9_class_init(ObjectClass *oc, void *data)
>      mc->desc = "ARM RealView Platform Baseboard Explore for Cortex-A9";
>      mc->init = realview_pbx_a9_init;
>      mc->max_cpus = 4;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo realview_pbx_a9_type = {
> diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
> index 4e7ac8c..ee140e5 100644
> --- a/hw/arm/sabrelite.c
> +++ b/hw/arm/sabrelite.c
> @@ -122,6 +122,7 @@ static void sabrelite_machine_init(MachineClass *mc)
>      mc->desc = "Freescale i.MX6 Quad SABRE Lite Board (Cortex A9)";
>      mc->init = sabrelite_init;
>      mc->max_cpus = FSL_IMX6_NUM_CPUS;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("sabrelite", sabrelite_machine_init)
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 7f588ce..6406421 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -983,6 +983,7 @@ static void akitapda_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "Sharp SL-C1000 (Akita) PDA (PXA270)";
>      mc->init = akita_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo akitapda_type = {
> @@ -998,6 +999,7 @@ static void spitzpda_class_init(ObjectClass *oc, void *data)
>      mc->desc = "Sharp SL-C3000 (Spitz) PDA (PXA270)";
>      mc->init = spitz_init;
>      mc->block_default_type = IF_IDE;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo spitzpda_type = {
> @@ -1013,6 +1015,7 @@ static void borzoipda_class_init(ObjectClass *oc, void *data)
>      mc->desc = "Sharp SL-C3100 (Borzoi) PDA (PXA270)";
>      mc->init = borzoi_init;
>      mc->block_default_type = IF_IDE;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo borzoipda_type = {
> @@ -1028,6 +1031,7 @@ static void terrierpda_class_init(ObjectClass *oc, void *data)
>      mc->desc = "Sharp SL-C3200 (Terrier) PDA (PXA270)";
>      mc->init = terrier_init;
>      mc->block_default_type = IF_IDE;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo terrierpda_type = {
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 408c1a1..b3aad23 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -1453,6 +1453,7 @@ static void lm3s811evb_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "Stellaris LM3S811EVB";
>      mc->init = lm3s811evb_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo lm3s811evb_type = {
> @@ -1467,6 +1468,7 @@ static void lm3s6965evb_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "Stellaris LM3S6965EVB";
>      mc->init = lm3s6965evb_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo lm3s6965evb_type = {
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index 8b757ff..1134cf7 100644
> --- a/hw/arm/tosa.c
> +++ b/hw/arm/tosa.c
> @@ -263,6 +263,7 @@ static void tosapda_machine_init(MachineClass *mc)
>      mc->desc = "Sharp SL-6000 (Tosa) PDA (PXA255)";
>      mc->init = tosa_init;
>      mc->block_default_type = IF_IDE;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("tosa", tosapda_machine_init)
> diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
> index b0e9f5b..76664e4 100644
> --- a/hw/arm/versatilepb.c
> +++ b/hw/arm/versatilepb.c
> @@ -403,6 +403,7 @@ static void versatilepb_class_init(ObjectClass *oc, void *data)
>      mc->desc = "ARM Versatile/PB (ARM926EJ-S)";
>      mc->init = vpb_init;
>      mc->block_default_type = IF_SCSI;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo versatilepb_type = {
> @@ -418,6 +419,7 @@ static void versatileab_class_init(ObjectClass *oc, void *data)
>      mc->desc = "ARM Versatile/AB (ARM926EJ-S)";
>      mc->init = vab_init;
>      mc->block_default_type = IF_SCSI;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static const TypeInfo versatileab_type = {
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 528c65d..9be1833 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -750,6 +750,7 @@ static void vexpress_class_init(ObjectClass *oc, void *data)
>      mc->desc = "ARM Versatile Express";
>      mc->init = vexpress_common_init;
>      mc->max_cpus = 4;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  static void vexpress_a9_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 6b11a75..9883215 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -324,6 +324,7 @@ static void zynq_machine_init(MachineClass *mc)
>      mc->init = zynq_init;
>      mc->max_cpus = 1;
>      mc->no_sdcard = 1;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("xilinx-zynq-a9", zynq_machine_init)
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> index 860780a..c339cd4 100644
> --- a/hw/arm/xlnx-ep108.c
> +++ b/hw/arm/xlnx-ep108.c
> @@ -122,6 +122,7 @@ static void xlnx_ep108_machine_init(MachineClass *mc)
>      mc->init = xlnx_ep108_init;
>      mc->block_default_type = IF_IDE;
>      mc->units_per_default_bus = 1;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("xlnx-ep108", xlnx_ep108_machine_init)
> @@ -132,6 +133,7 @@ static void xlnx_zcu102_machine_init(MachineClass *mc)
>      mc->init = xlnx_ep108_init;
>      mc->block_default_type = IF_IDE;
>      mc->units_per_default_bus = 1;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("xlnx-zcu102", xlnx_zcu102_machine_init)
> diff --git a/hw/arm/z2.c b/hw/arm/z2.c
> index 1607cbd..417bc1a 100644
> --- a/hw/arm/z2.c
> +++ b/hw/arm/z2.c
> @@ -370,6 +370,7 @@ static void z2_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Zipit Z2 (PXA27x)";
>      mc->init = z2_init;
> +    mc->ignore_memory_transaction_failures = true;
>  }
>  
>  DEFINE_MACHINE("z2", z2_machine_init)
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/8] target/arm: Factor out fault delivery code

[Edgar E. Iglesias]

On Fri, Aug 04, 2017 at 06:20:47PM +0100, Peter Maydell wrote:
> We currently have some similar code in tlb_fill() and in
> arm_cpu_do_unaligned_access() for delivering a data abort or prefetch
> abort.  We're also going to want to do the same thing to handle
> external aborts.  Factor out the common code into a new function
> deliver_fault().

I found this a bit hard to read but I think it looks OK :-)
Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/op_helper.c | 110 +++++++++++++++++++++++++------------------------
>  1 file changed, 57 insertions(+), 53 deletions(-)
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 2a85666..aa52a98 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -115,6 +115,51 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
>      return syn;
>  }
>  
> +static void deliver_fault(ARMCPU *cpu, vaddr addr, MMUAccessType access_type,
> +                          uint32_t fsr, uint32_t fsc, ARMMMUFaultInfo *fi)
> +{
> +    CPUARMState *env = &cpu->env;
> +    int target_el;
> +    bool same_el;
> +    uint32_t syn, exc;
> +
> +    target_el = exception_target_el(env);
> +    if (fi->stage2) {
> +        target_el = 2;
> +        env->cp15.hpfar_el2 = extract64(fi->s2addr, 12, 47) << 4;
> +    }
> +    same_el = (arm_current_el(env) == target_el);
> +
> +    if (fsc == 0x3f) {
> +        /* Caller doesn't have a long-format fault status code. This
> +         * should only happen if this fault will never actually be reported
> +         * to an EL that uses a syndrome register. Check that here.
> +         * 0x3f is a (currently) reserved FSR code, in case the constructed
> +         * syndrome does leak into the guest somehow.
> +         */
> +        assert(target_el != 2 && !arm_el_is_aa64(env, target_el));
> +    }
> +
> +    if (access_type == MMU_INST_FETCH) {
> +        syn = syn_insn_abort(same_el, 0, fi->s1ptw, fsc);
> +        exc = EXCP_PREFETCH_ABORT;
> +    } else {
> +        syn = merge_syn_data_abort(env->exception.syndrome, target_el,
> +                                   same_el, fi->s1ptw,
> +                                   access_type == MMU_DATA_STORE,
> +                                   fsc);
> +        if (access_type == MMU_DATA_STORE
> +            && arm_feature(env, ARM_FEATURE_V6)) {
> +            fsr |= (1 << 11);
> +        }
> +        exc = EXCP_DATA_ABORT;
> +    }
> +
> +    env->exception.vaddress = addr;
> +    env->exception.fsr = fsr;
> +    raise_exception(env, exc, syn, target_el);
> +}
> +
>  /* try to fill the TLB and return an exception if error. If retaddr is
>   * NULL, it means that the function was called in C code (i.e. not
>   * from generated code or from helper.c)
> @@ -129,23 +174,13 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>      ret = arm_tlb_fill(cs, addr, access_type, mmu_idx, &fsr, &fi);
>      if (unlikely(ret)) {
>          ARMCPU *cpu = ARM_CPU(cs);
> -        CPUARMState *env = &cpu->env;
> -        uint32_t syn, exc, fsc;
> -        unsigned int target_el;
> -        bool same_el;
> +        uint32_t fsc;
>  
>          if (retaddr) {
>              /* now we have a real cpu fault */
>              cpu_restore_state(cs, retaddr);
>          }
>  
> -        target_el = exception_target_el(env);
> -        if (fi.stage2) {
> -            target_el = 2;
> -            env->cp15.hpfar_el2 = extract64(fi.s2addr, 12, 47) << 4;
> -        }
> -        same_el = arm_current_el(env) == target_el;
> -
>          if (fsr & (1 << 9)) {
>              /* LPAE format fault status register : bottom 6 bits are
>               * status code in the same form as needed for syndrome
> @@ -153,34 +188,15 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>              fsc = extract32(fsr, 0, 6);
>          } else {
>              /* Short format FSR : this fault will never actually be reported
> -             * to an EL that uses a syndrome register. Check that here,
> -             * and use a (currently) reserved FSR code in case the constructed
> -             * syndrome does leak into the guest somehow.
> +             * to an EL that uses a syndrome register. Use a (currently)
> +             * reserved FSR code in case the constructed syndrome does leak
> +             * into the guest somehow. deliver_fault will assert that
> +             * we don't target an EL using the syndrome.
>               */
> -            assert(target_el != 2 && !arm_el_is_aa64(env, target_el));
>              fsc = 0x3f;
>          }
>  
> -        /* For insn and data aborts we assume there is no instruction syndrome
> -         * information; this is always true for exceptions reported to EL1.
> -         */
> -        if (access_type == MMU_INST_FETCH) {
> -            syn = syn_insn_abort(same_el, 0, fi.s1ptw, fsc);
> -            exc = EXCP_PREFETCH_ABORT;
> -        } else {
> -            syn = merge_syn_data_abort(env->exception.syndrome, target_el,
> -                                       same_el, fi.s1ptw,
> -                                       access_type == MMU_DATA_STORE, fsc);
> -            if (access_type == MMU_DATA_STORE
> -                && arm_feature(env, ARM_FEATURE_V6)) {
> -                fsr |= (1 << 11);
> -            }
> -            exc = EXCP_DATA_ABORT;
> -        }
> -
> -        env->exception.vaddress = addr;
> -        env->exception.fsr = fsr;
> -        raise_exception(env, exc, syn, target_el);
> +        deliver_fault(cpu, addr, access_type, fsr, fsc, &fi);
>      }
>  }
>  
> @@ -191,9 +207,8 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> -    int target_el;
> -    bool same_el;
> -    uint32_t syn;
> +    uint32_t fsr, fsc;
> +    ARMMMUFaultInfo fi = {};
>      ARMMMUIdx arm_mmu_idx = core_to_arm_mmu_idx(env, mmu_idx);
>  
>      if (retaddr) {
> @@ -201,28 +216,17 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>          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_s1_regime_using_lpae_format(env, arm_mmu_idx)) {
> -        env->exception.fsr = (1 << 9) | 0x21;
> +        fsr = (1 << 9) | 0x21;
>      } else {
> -        env->exception.fsr = 0x1;
> -    }
> -
> -    if (access_type == MMU_DATA_STORE && arm_feature(env, ARM_FEATURE_V6)) {
> -        env->exception.fsr |= (1 << 11);
> +        fsr = 0x1;
>      }
> +    fsc = 0x21;
>  
> -    syn = merge_syn_data_abort(env->exception.syndrome, target_el,
> -                               same_el, 0, access_type == MMU_DATA_STORE,
> -                               0x21);
> -    raise_exception(env, EXCP_DATA_ABORT, syn, target_el);
> +    deliver_fault(cpu, vaddr, access_type, fsr, fsc, &fi);
>  }
>  
>  #endif /* !defined(CONFIG_USER_ONLY) */
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 8/8] target/arm: Implement new do_transaction_failed hook

[Edgar E. Iglesias]

On Fri, Aug 04, 2017 at 06:20:49PM +0100, Peter Maydell wrote:
> Implement the new do_transaction_failed hook for ARM, which should
> cause the CPU to take a prefetch abort or data abort.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target/arm/internals.h | 10 ++++++++++
>  target/arm/cpu.c       |  1 +
>  target/arm/op_helper.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index a3adbd8..13bb001 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -471,6 +471,16 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>                                   MMUAccessType access_type,
>                                   int mmu_idx, uintptr_t retaddr);
>  
> +/* arm_cpu_do_transaction_failed: handle a memory system error response
> + * (eg "no device/memory present at address") by raising an external abort
> + * exception
> + */
> +void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> +                                   vaddr addr, unsigned size,
> +                                   MMUAccessType access_type,
> +                                   int mmu_idx, MemTxAttrs attrs,
> +                                   MemTxResult response, uintptr_t retaddr);
> +
>  /* Call the EL change hook if one has been registered */
>  static inline void arm_call_el_change_hook(ARMCPU *cpu)
>  {
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 05c038b..6baede0 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1670,6 +1670,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>  #else
>      cc->do_interrupt = arm_cpu_do_interrupt;
>      cc->do_unaligned_access = arm_cpu_do_unaligned_access;
> +    cc->do_transaction_failed = arm_cpu_do_transaction_failed;
>      cc->get_phys_page_attrs_debug = arm_cpu_get_phys_page_attrs_debug;
>      cc->asidx_from_attrs = arm_asidx_from_attrs;
>      cc->vmsd = &vmstate_arm_cpu;
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 7eac272..54b6dd8 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -229,6 +229,49 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>      deliver_fault(cpu, vaddr, access_type, fsr, fsc, &fi);
>  }
>  
> +/* arm_cpu_do_transaction_failed: handle a memory system error response
> + * (eg "no device/memory present at address") by raising an external abort
> + * exception
> + */
> +void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> +                                   vaddr addr, unsigned size,
> +                                   MMUAccessType access_type,
> +                                   int mmu_idx, MemTxAttrs attrs,
> +                                   MemTxResult response, uintptr_t retaddr)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    uint32_t fsr, fsc;
> +    ARMMMUFaultInfo fi = {};
> +    ARMMMUIdx arm_mmu_idx = core_to_arm_mmu_idx(env, mmu_idx);
> +
> +    if (retaddr) {
> +        /* now we have a real cpu fault */
> +        cpu_restore_state(cs, retaddr);
> +    }
> +
> +    /* The EA bit in syndromes and fault status registers is an
> +     * IMPDEF classification of external aborts. ARM implementations
> +     * usually use this to indicate AXI bus Decode error (0) or
> +     * Slave error (1); in QEMU we follow that.
> +     */
> +    fi.ea = (response != MEMTX_DECODE_ERROR);
> +
> +    /* The fault status register format depends on whether we're using
> +     * the LPAE long descriptor format, or the short descriptor format.
> +     */
> +    if (arm_s1_regime_using_lpae_format(env, arm_mmu_idx)) {
> +        /* long descriptor form, STATUS 0b010000: synchronous ext abort */
> +        fsr = (fi.ea << 12) | (1 << 9) | 0x10;
> +    } else {
> +        /* short descriptor form, FSR 0b01000 : synchronous ext abort */
> +        fsr = (fi.ea << 12) | 0x8;
> +    }
> +    fsc = 0x10;
> +
> +    deliver_fault(cpu, addr, access_type, fsr, fsc, &fi);
> +}
> +
>  #endif /* !defined(CONFIG_USER_ONLY) */
>  
>  uint32_t HELPER(add_setq)(CPUARMState *env, uint32_t a, uint32_t b)
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 7/8] target/arm: Allow deliver_fault() caller to specify EA bit

[Edgar E. Iglesias]

On Fri, Aug 04, 2017 at 06:20:48PM +0100, Peter Maydell wrote:
> For external aborts, we will want to be able to specify the EA
> (external abort type) bit in the syndrome field.  Allow callers of
> deliver_fault() to do that by adding a field to ARMMMUFaultInfo which
> we use when constructing the syndrome values.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target/arm/internals.h |  2 ++
>  target/arm/op_helper.c | 10 +++++-----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 1f6efef..a3adbd8 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -448,12 +448,14 @@ void arm_handle_psci_call(ARMCPU *cpu);
>   * @s2addr: Address that caused a fault at stage 2
>   * @stage2: True if we faulted at stage 2
>   * @s1ptw: True if we faulted at stage 2 while doing a stage 1 page-table walk
> + * @ea: True if we should set the EA (external abort type) bit in syndrome
>   */
>  typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
>  struct ARMMMUFaultInfo {
>      target_ulong s2addr;
>      bool stage2;
>      bool s1ptw;
> +    bool ea;
>  };
>  
>  /* Do a page table walk and add page to TLB if possible */
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index aa52a98..7eac272 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -80,7 +80,7 @@ uint32_t HELPER(neon_tbl)(CPUARMState *env, uint32_t ireg, uint32_t def,
>  
>  static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
>                                              unsigned int target_el,
> -                                            bool same_el,
> +                                            bool same_el, bool ea,
>                                              bool s1ptw, bool is_write,
>                                              int fsc)
>  {
> @@ -99,7 +99,7 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
>       */
>      if (!(template_syn & ARM_EL_ISV) || target_el != 2 || s1ptw) {
>          syn = syn_data_abort_no_iss(same_el,
> -                                    0, 0, s1ptw, is_write, fsc);
> +                                    ea, 0, s1ptw, is_write, fsc);
>      } else {
>          /* Fields: IL, ISV, SAS, SSE, SRT, SF and AR come from the template
>           * syndrome created at translation time.
> @@ -107,7 +107,7 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
>           */
>          syn = syn_data_abort_with_iss(same_el,
>                                        0, 0, 0, 0, 0,
> -                                      0, 0, s1ptw, is_write, fsc,
> +                                      ea, 0, s1ptw, is_write, fsc,
>                                        false);
>          /* Merge the runtime syndrome with the template syndrome.  */
>          syn |= template_syn;
> @@ -141,11 +141,11 @@ static void deliver_fault(ARMCPU *cpu, vaddr addr, MMUAccessType access_type,
>      }
>  
>      if (access_type == MMU_INST_FETCH) {
> -        syn = syn_insn_abort(same_el, 0, fi->s1ptw, fsc);
> +        syn = syn_insn_abort(same_el, fi->ea, fi->s1ptw, fsc);
>          exc = EXCP_PREFETCH_ABORT;
>      } else {
>          syn = merge_syn_data_abort(env->exception.syndrome, target_el,
> -                                   same_el, fi->s1ptw,
> +                                   same_el, fi->ea, fi->s1ptw,
>                                     access_type == MMU_DATA_STORE,
>                                     fsc);
>          if (access_type == MMU_DATA_STORE
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Peter Maydell]

On 4 August 2017 at 20:23, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 08/04/2017 11:09 AM, Philippe Mathieu-Daudé wrote:
>> Since create_unimplemented_device() register overlapped with low priority, why
>> not register it as default device directly, over the whole address space?
>
> That's a good suggestion.  It makes more sense to me than adding a flag on the
> MachineClass.

Yeah, I did think about implementing it that way, but...

That wouldn't handle the case of a device model directly
returning a MEMTX_ERROR, or a transaction dispatched to
a memory region whose MemoryRegionOps valid settings
prohibit it (eg byte accesses to a word-access-only device),
or accesses to a MemoryRegion that was created by passing
a NULL MemoryRegionOps pointer to memory_region_init_io
(I dunno why you'd do that but some code does).

In short, there are lots of ways the memory subsystem might
end up returning a transaction error -- this mechanism
ensures that none of them start generating exceptions
when they previously did not, and is (I hope) easy to
review in the sense of being sure that it does what it
intends to do without the need to audit a lot of corner
cases.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Peter Maydell]

On 4 August 2017 at 19:09, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 08/04/2017 02:20 PM, Peter Maydell wrote:
>> We need this for ARM boards, where we're about to implement support for
>> generating external aborts on memory transaction failures. Too many
>> of our legacy board models rely on the RAZ/WI behaviour and we
>> would break currently working guests when their "probe for device"
>> code provoked an external abort rather than a RAZ.

> I think some firmware will give some surprises, those probing device is not
> here and expect RAZ/WI. I remember some fw probing PCI space, or enumerating
> CS this way for ex.

PCI space is funny anyway because IIRC the PCI spec mandates
RAZ/WI (which is handled by QEMU's PCI implementation I think,
it doesn't fall out to the memory system's unmapped-address
handling).

That said: yes, possibly some guest code really wants the fault
(indeed the motivation for this patchset was having some test
guest code which wanted to see the faults), but that guest code
won't work on QEMU today, so if it doesn't boot on QEMU with
this patchsets that's not a regression. We can then (as the
issue arises) look at fixing whatever particular board model
it is to properly model or stub out all its devices so we
can boot that guest code without breaking existing working
guest code.

> RAZ/WI is a bus-feature, this is also bus-dependent to reply with abort or
> behave RAZ/WI. Maybe the effort should be done on how model/use buses in
> QEMU? Bus device would be an alias of unimplemented_device, which current
> purpose is more debugging than avoiding unassigned physical access aborts.

You can model this kind of bus-dependent behaviour by having
the bus register a background region which implements the
default behaviour that is desired. (That way accesses to
that part of the address space don't ever respond with
a transaction error, which is what's happening on hardware
where the bus doesn't report errors.)

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/8] cpu: Define new cpu_transaction_failed() hook

[Peter Maydell]

On 5 August 2017 at 02:06, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Fri, Aug 04, 2017 at 06:20:43PM +0100, Peter Maydell wrote:
>> @@ -85,8 +85,10 @@ struct TranslationBlock;
>>   * @has_work: Callback for checking if there is work to do.
>>   * @do_interrupt: Callback for interrupt handling.
>>   * @do_unassigned_access: Callback for unassigned access handling.
>> + * (this is deprecated: new targets should use do_transaction_failed instead)
>>   * @do_unaligned_access: Callback for unaligned access handling, if
>>   * the target defines #ALIGNED_ONLY.
>> + * @do_transaction_failed: Callback for handling failed memory transactions
>
> Looks OK but I wonder if there you might want to clarify that this is a
> bus/slave failure and not a failure within the CPU (e.g not an MMU fault).

Yes, we could add
"(ie bus faults or external aborts; not MMU faults)"
just to clarify.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/8] cpu: Define new cpu_transaction_failed() hook

[Peter Maydell]

On 5 August 2017 at 02:12, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> BTW, a question. I don't know of any from memory but does any arch
> have the ability to report the payload that failed for stores?
> I guess it's easy enough to add that if needed though.

I think maybe m68k bus fault stack frames have
store payload data? The description of them is pretty
complicated though and I'm not sure how much of the
frame is "stuff we actually need to emulate" vs "data
that's only important if your implementation pipelines
instruction execution"...

As you say, we can easily add a 'uint64_t data' (only valid
when access_type == MMU_DATA_STORE), either now or later.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/8] memory.h: Move MemTxResult type to memattrs.h

[Alistair Francis]

On Fri, Aug 4, 2017 at 5:59 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Fri, Aug 04, 2017 at 06:20:42PM +0100, Peter Maydell wrote:
>> Move the MemTxResult type to memattrs.h. We're going to want to
>> use it in cpu/qom.h, which doesn't want to include all of
>> memory.h. In practice MemTxResult and MemTxAttrs are pretty
>> closely linked since both are used for the new-style
>> read_with_attrs and write_with_attrs callbacks, so memattrs.h
>> is a reasonable home for this rather than creating a whole
>> new header file for it.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

>
>
>> ---
>>  include/exec/memattrs.h | 10 ++++++++++
>>  include/exec/memory.h   | 10 ----------
>>  2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
>> index e601061..d4a1642 100644
>> --- a/include/exec/memattrs.h
>> +++ b/include/exec/memattrs.h
>> @@ -46,4 +46,14 @@ typedef struct MemTxAttrs {
>>   */
>>  #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
>>
>> +/* New-style MMIO accessors can indicate that the transaction failed.
>> + * A zero (MEMTX_OK) response means success; anything else is a failure
>> + * of some kind. The memory subsystem will bitwise-OR together results
>> + * if it is synthesizing an operation from multiple smaller accesses.
>> + */
>> +#define MEMTX_OK 0
>> +#define MEMTX_ERROR             (1U << 0) /* device returned an error */
>> +#define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
>> +typedef uint32_t MemTxResult;
>> +
>>  #endif
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 400dd44..1dcd312 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -112,16 +112,6 @@ static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
>>      n->end = end;
>>  }
>>
>> -/* New-style MMIO accessors can indicate that the transaction failed.
>> - * A zero (MEMTX_OK) response means success; anything else is a failure
>> - * of some kind. The memory subsystem will bitwise-OR together results
>> - * if it is synthesizing an operation from multiple smaller accesses.
>> - */
>> -#define MEMTX_OK 0
>> -#define MEMTX_ERROR             (1U << 0) /* device returned an error */
>> -#define MEMTX_DECODE_ERROR      (1U << 1) /* nothing at that address */
>> -typedef uint32_t MemTxResult;
>> -
>>  /*
>>   * Memory region callbacks
>>   */
>> --
>> 2.7.4
>>
>>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Peter Maydell]

On 5 August 2017 at 11:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 August 2017 at 20:23, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> On 08/04/2017 11:09 AM, Philippe Mathieu-Daudé wrote:
>>> Since create_unimplemented_device() register overlapped with low priority, why
>>> not register it as default device directly, over the whole address space?
>>
>> That's a good suggestion.  It makes more sense to me than adding a flag on the
>> MachineClass.
>
> Yeah, I did think about implementing it that way, but...
>
> That wouldn't handle the case of a device model directly
> returning a MEMTX_ERROR, or a transaction dispatched to
> a memory region whose MemoryRegionOps valid settings
> prohibit it (eg byte accesses to a word-access-only device),
> or accesses to a MemoryRegion that was created by passing
> a NULL MemoryRegionOps pointer to memory_region_init_io
> (I dunno why you'd do that but some code does).
>
> In short, there are lots of ways the memory subsystem might
> end up returning a transaction error -- this mechanism
> ensures that none of them start generating exceptions
> when they previously did not, and is (I hope) easy to
> review in the sense of being sure that it does what it
> intends to do without the need to audit a lot of corner
> cases.

So, this question (should we have a board flag to disable reporting
of tx failures to the CPU hook, or use unimplemented_device as a
sort of background region) seems to be the main unanswered question
for this series. I think (as outlined above) that the board flag
is simpler and safer; are people happy for me to put this series
in target-arm.next with that approach, or should I rethink this bit?

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Philippe Mathieu-Daudé]

Hi Peter,

On 08/17/2017 07:25 AM, Peter Maydell wrote:
> On 5 August 2017 at 11:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 4 August 2017 at 20:23, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>> On 08/04/2017 11:09 AM, Philippe Mathieu-Daudé wrote:
>>>> Since create_unimplemented_device() register overlapped with low priority, why
>>>> not register it as default device directly, over the whole address space?
>>>
>>> That's a good suggestion.  It makes more sense to me than adding a flag on the
>>> MachineClass.
>>
>> Yeah, I did think about implementing it that way, but...
>>
>> That wouldn't handle the case of a device model directly
>> returning a MEMTX_ERROR, or a transaction dispatched to
>> a memory region whose MemoryRegionOps valid settings
>> prohibit it (eg byte accesses to a word-access-only device),
>> or accesses to a MemoryRegion that was created by passing
>> a NULL MemoryRegionOps pointer to memory_region_init_io
>> (I dunno why you'd do that but some code does).
>>
>> In short, there are lots of ways the memory subsystem might
>> end up returning a transaction error -- this mechanism
>> ensures that none of them start generating exceptions
>> when they previously did not, and is (I hope) easy to
>> review in the sense of being sure that it does what it
>> intends to do without the need to audit a lot of corner
>> cases.
> 
> So, this question (should we have a board flag to disable reporting
> of tx failures to the CPU hook, or use unimplemented_device as a
> sort of background region) seems to be the main unanswered question
> for this series. I think (as outlined above) that the board flag
> is simpler and safer; are people happy for me to put this series
> in target-arm.next with that approach, or should I rethink this bit?
As remarked previously in this thread, the current QEMU behavior on 
transaction error isn't always matching real hardware.
Matching correctly throwing errors is likely to break various current users.

If we are worried about being backward compatible, defaulting background 
region to unimp() won't throw any transaction error.

Since the default is no transaction error, users who expect hardware 
error can implement the correct behavior, per region/bus transaction errors.

I'm somehow afraid that "ignore_memory_transaction_failures" ends up 
like the "cannot_instantiate_with_device_add_yet" flag - a hard to 
remove kludge outliving his purpose.

Anyway I'm not unhappy with this approach, but I'd be very happy to have 
unimp() covering the whole background region.

Regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Peter Maydell]

On 22 August 2017 at 04:45, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 08/17/2017 07:25 AM, Peter Maydell wrote:
>>
>> On 5 August 2017 at 11:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>> So, this question (should we have a board flag to disable reporting
>> of tx failures to the CPU hook, or use unimplemented_device as a
>> sort of background region) seems to be the main unanswered question
>> for this series. I think (as outlined above) that the board flag
>> is simpler and safer; are people happy for me to put this series
>> in target-arm.next with that approach, or should I rethink this bit?
>
> As remarked previously in this thread, the current QEMU behavior on
> transaction error isn't always matching real hardware.
> Matching correctly throwing errors is likely to break various
> current users.

Yes, hence this patchseries keeping the wrong but back compatible
behaviour...

> If we are worried about being backward compatible, defaulting background
> region to unimp() won't throw any transaction error.

As I said, it will, for the cases of device model directly
returning a MEMTX_ERROR, or a transaction dispatched to
a memory region whose MemoryRegionOps valid settings
prohibit it (eg byte accesses to a word-access-only device), etc.
The only simple way to guarantee that we don't generate exceptions
on transaction errors is to cause the hook not to be called
(or to have the hook decide to do nothing, I suppose).

> I'm somehow afraid that "ignore_memory_transaction_failures" ends up like
> the "cannot_instantiate_with_device_add_yet" flag - a hard to remove kludge
> outliving his purpose.

I agree that it's going to be around for a long time, possibly
forever, but that's life when we have so many old boards.
Any approach we take is almost certainly going to be hanging
around forever.

> Anyway I'm not unhappy with this approach, but I'd be very happy to have
> unimp() covering the whole background region.

I think this would be a reasonable approach for converting
boards away from this ignore_memory_transaction_failures hook
on a board-by-board basis but you'd still want to test some
common guest software for each conversion.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Richard Henderson]

On 22 August 2017 at 09:36, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 August 2017 at 04:45, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> If we are worried about being backward compatible, defaulting background
>> region to unimp() won't throw any transaction error.
>
> As I said, it will, for the cases of device model directly
> returning a MEMTX_ERROR, or a transaction dispatched to
> a memory region whose MemoryRegionOps valid settings
> prohibit it (eg byte accesses to a word-access-only device), etc.
> The only simple way to guarantee that we don't generate exceptions
> on transaction errors is to cause the hook not to be called
> (or to have the hook decide to do nothing, I suppose).
>
>> I'm somehow afraid that "ignore_memory_transaction_failures" ends up like
>> the "cannot_instantiate_with_device_add_yet" flag - a hard to remove kludge
>> outliving his purpose.
>
> I agree that it's going to be around for a long time, possibly
> forever, but that's life when we have so many old boards.
> Any approach we take is almost certainly going to be hanging
> around forever.
>
>> Anyway I'm not unhappy with this approach, but I'd be very happy to have
>> unimp() covering the whole background region.
>
> I think this would be a reasonable approach for converting
> boards away from this ignore_memory_transaction_failures hook
> on a board-by-board basis but you'd still want to test some
> common guest software for each conversion.

Peter and I had a chat on IRC about this.

While I think the background region idea is tempting, it does nothing to help
legacy boards once devices start signaling their own errors.  Therefore some
sort of flag is the only reasonable solution.


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/8] boards.h: Define new flag ignore_memory_transaction_failures

[Peter Maydell]

On 24 August 2017 at 21:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Peter and I had a chat on IRC about this.
>
> While I think the background region idea is tempting, it does nothing to help
> legacy boards once devices start signaling their own errors.  Therefore some
> sort of flag is the only reasonable solution.

Thanks; I'm going to put this series into target-arm.next as-is
(with the couple of minor comment tweaks suggested in review
applied to it), ready for the post-2.10-release pull request.

-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/8] cputlb: Support generating CPU exceptions on memory transaction failures

[Peter Maydell]

On 4 August 2017 at 18:20, Peter Maydell <peter.maydell@linaro.org> wrote:
> Call the new cpu_transaction_failed() hook at the places where
> CPU generated code interacts with the memory system:
>  io_readx()
>  io_writex()
>  get_page_addr_code()

> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c

>  static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
> +                      int mmu_idx,
>                        uint64_t val, target_ulong addr,
>                        uintptr_t retaddr, int size)
>  {
> @@ -783,6 +791,7 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>      hwaddr physaddr = iotlbentry->addr;
>      MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->attrs);
>      bool locked = false;
> +    MemTxResult r;
>
>      physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
>      if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
> @@ -795,7 +804,12 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>          qemu_mutex_lock_iothread();
>          locked = true;
>      }
> -    memory_region_dispatch_write(mr, physaddr, val, size, iotlbentry->attrs);
> +    r = memory_region_dispatch_write(mr, physaddr,
> +                                     val, size, iotlbentry->attrs);
> +    if (r != MEMTX_OK) {
> +        cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_STORE,
> +                               mmu_idx, iotlbentry->attrs, r, retaddr);
> +    }

I was looking at a bug that involved stepping through this function,
and it turns out that the value in the variable "physaddr" here is
not in fact the physical address of the access. It's just the offset
into the memory region.

This doesn't matter for Arm or Alpha, which don't actually need the
physaddr for reporting the exceptions, and those are the only
current implementations of the transaction_failed hook.
But we should fix this so it doesn't bite us when we do eventually
have a cpu that needs the physaddr...

If we have a CPUIOTLBEntry how do we get back to the physaddr for it?

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/8] cputlb: Support generating CPU exceptions on memory transaction failures

[Paolo Bonzini]

On 13/12/2017 17:39, Peter Maydell wrote:
> I was looking at a bug that involved stepping through this function,
> and it turns out that the value in the variable "physaddr" here is
> not in fact the physical address of the access. It's just the offset
> into the memory region.
> 
> This doesn't matter for Arm or Alpha, which don't actually need the
> physaddr for reporting the exceptions, and those are the only
> current implementations of the transaction_failed hook.
> But we should fix this so it doesn't bite us when we do eventually
> have a cpu that needs the physaddr...
> 
> If we have a CPUIOTLBEntry how do we get back to the physaddr for it?

iotlb_to_region only gets the MemoryRegion from the MemoryRegionSection,
but you could actually make it return the whole MRS.  Then you can sum
the MRS's offset_with_address_space.

Thanks,

Paolo