Re: [Qemu-devel] [PATCH 1/3] hw/mips/mips_jazz: Override do_transaction_failed hook

[Philippe Mathieu-Daudé]

On 8/2/19 6:04 PM, Peter Maydell wrote:
> The MIPS Jazz ('magnum' and 'pica61') boards have some code which
> overrides the CPU's do_unassigned_access hook, so they can intercept
> it and not raise exceptions on data accesses to invalid addresses,
> only for instruction fetches.
> 
> We want to switch MIPS over to using the do_transaction_failed
> hook instead, so add an intercept for that as well, and make
> the board code install whichever hook the CPU is actually using.
> Once we've changed the CPU implementation we can remove the
> redundant code for the old hook.
> 
> Note: I am suspicious that the behaviour as implemented here may not
> be what the hardware really does.  It was added in commit
> 54e755588cf1e90f0b14 to restore the behaviour that was broken by
> commit c658b94f6e8c206c59d.  But prior to commit c658b94f6e8c206c59d
> every MIPS board generated exceptions for instruction access to
> invalid addresses but not for data accesses; and other boards,
> notably Malta, were fixed by making all invalid accesses behave as
> reads-as-zero (see the call to empty_slot_init() in
> mips_malta_init()).  Hardware that raises exceptions for instruction
> access and not data access seems to me to be an unlikely design, and
> it's possible that the right way to emulate this is to make the Jazz
> boards do what we did with Malta (or some variation of that).
> Nonetheless, since I don't have access to real hardware to test
> against I have taken the approach of "make QEMU continue to behave
> the same way it did before this commit".  I have updated the comment
> to correct the parts that are no longer accurate and note that
> the hardware might behave differently.
> 
> The test case for the need for the hook-hijacking is in
> https://bugs.launchpad.net/qemu/+bug/1245924 That BIOS will boot OK
> either with this overriding of both hooks, or with a simple "global
> memory region to ignore bad accesses of all types", so it doesn't
> provide evidence either way, unfortunately.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  hw/mips/mips_jazz.c | 54 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
> index fa8775d4284..c64b4c78809 100644
> --- a/hw/mips/mips_jazz.c
> +++ b/hw/mips/mips_jazz.c
> @@ -123,6 +123,28 @@ static void mips_jazz_do_unassigned_access(CPUState 
> *cpu, hwaddr addr,
>      (*real_do_unassigned_access)(cpu, addr, is_write, is_exec, opaque, size);
>  }
>  
> +static void (*real_do_transaction_failed)(CPUState *cpu, hwaddr physaddr,
> +                                          vaddr addr, unsigned size,
> +                                          MMUAccessType access_type,
> +                                          int mmu_idx, MemTxAttrs attrs,
> +                                          MemTxResult response,
> +                                          uintptr_t retaddr);
> +
> +static void mips_jazz_do_transaction_failed(CPUState *cs, hwaddr physaddr,
> +                                            vaddr addr, unsigned size,
> +                                            MMUAccessType access_type,
> +                                            int mmu_idx, MemTxAttrs attrs,
> +                                            MemTxResult response,
> +                                            uintptr_t retaddr)
> +{
> +    if (access_type != MMU_INST_FETCH) {
> +        /* ignore invalid access (ie do not raise exception) */
> +        return;
> +    }
> +    (*real_do_transaction_failed)(cs, physaddr, addr, size, access_type,
> +                                  mmu_idx, attrs, response, retaddr);
> +}
> +
>  static void mips_jazz_init(MachineState *machine,
>                             enum jazz_model_e jazz_model)
>  {
> @@ -157,16 +179,32 @@ static void mips_jazz_init(MachineState *machine,
>      env = &cpu->env;
>      qemu_register_reset(main_cpu_reset, cpu);
>  
> -    /* Chipset returns 0 in invalid reads and do not raise data exceptions.
> +    /*
> +     * Chipset returns 0 in invalid reads and do not raise data exceptions.
>       * However, we can't simply add a global memory region to catch
> -     * everything, as memory core directly call unassigned_mem_read/write
> -     * on some invalid accesses, which call do_unassigned_access on the
> -     * CPU, which raise an exception.
> -     * Handle that case by hijacking the do_unassigned_access method on
> -     * the CPU, and do not raise exceptions for data access. */
> +     * everything, as this would make all accesses including instruction
> +     * accesses be ignored and not raise exceptions.
> +     * So instead we hijack either the do_unassigned_access method or
> +     * the do_transaction_failed method on the CPU, and do not raise 
> exceptions
> +     * for data access.
> +     *
> +     * NOTE: this behaviour of raising exceptions for bad instruction
> +     * fetches but not bad data accesses was added in commit 54e755588cf1e9
> +     * to restore behaviour broken by c658b94f6e8c206, but it is not clear
> +     * whether the real hardware behaves this way. It is possible that
> +     * real hardware ignores bad instruction fetches as well -- if so then
> +     * we could replace this hijacking of CPU methods with a simple global
> +     * memory region that catches all memory accesses, as we do on Malta.

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> +     */
>      cc = CPU_GET_CLASS(cpu);
> -    real_do_unassigned_access = cc->do_unassigned_access;
> -    cc->do_unassigned_access = mips_jazz_do_unassigned_access;
> +    if (cc->do_unassigned_access) {
> +        real_do_unassigned_access = cc->do_unassigned_access;
> +        cc->do_unassigned_access = mips_jazz_do_unassigned_access;
> +    }
> +    if (cc->do_transaction_failed) {
> +        real_do_transaction_failed = cc->do_transaction_failed;
> +        cc->do_transaction_failed = mips_jazz_do_transaction_failed;
> +    }
>  
>      /* allocate RAM */
>      memory_region_allocate_system_memory(ram, NULL, "mips_jazz.ram",
>