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 <address@hidden>
> ---
>   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);