Re: [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook

[Philippe Mathieu-Daudé]

Cc'ing broader MIPS audience.

On 8/2/19 6:04 PM, Peter Maydell wrote:
> This patchset converts the MIPS target away from the
> old broken do_unassigned_access hook to the new (added in
> 2017...) do_transaction_failed hook.
> 
> The motivation here is:
>  * do_unassigned_access is broken because:
>     + it will be called for any kind of access to physical addresses
>       where there is no assigned device, whether that access is by the
>       CPU or by something else (like a DMA controller!), so it can
>       result in spurious guest CPU exceptions.
>     + It will also get called even when using KVM, when there's nothing
>       useful it can do.
>     + It isn't passed in the return-address within the TCG generated
>       code, so it isn't able to correctly restore the CPU state
>       before generating the exception, and so the exception will
>       often be generated with the wrong faulting guest PC value
>  * there are now only a few targets still using the old hook,
>    so if we can convert them we can delete all the old code
>    and complete this API transation. (Patches for SPARC are on
>    the list; the other user is RISCV, which accidentally
>    implemented the old hook rather than the new one recently.)
> 
> The general approach to the conversion is to check the target for
> load/store-by-physical-address operations which were previously
> implicitly causing exceptions, to see if they now need to explicitly
> check for and handle memory access failures. (The 'git grep' regexes
> in docs/devel/loads-stores.rst are useful here: the API families to
> look for are ld*_phys/st*_phys, address_space_ld/st*, and
> cpu_physical_memory*.)
> 
> For MIPS, there are none of these (the usual place where targets do
> this is hardware page table walks where the page table entries are
> loaded by physical address, and MIPS doesn't seem to have those).
> 
> Code audit out of the way, the actual hook changeover is pretty
> simple.
> 
> The complication here is the MIPS Jazz board, which has some rather
> dubious code that intercepts the do_unassigned_access hook to suppress
> generation of exceptions for invalid accesses due to data accesses,
> while leaving exceptions for invalid instruction fetches in place. I'm
> a bit dubious about whether the behaviour we have implemented here is
> really what the hardware does -- it seems pretty odd to me to not
> generate exceptions for d-side accesses but to generate them for
> i-side accesses, and looking back through git and mailing list history
> this code is here mainly as "put back the behaviour we had before a
> previous commit broke it", and that older behaviour in turn I think is
> more historical-accident than because anybody deliberately checked the
> hardware behaviour and made QEMU work that way. However, I don't have
> any real hardware to do comparative tests on, so this series retains
> the same behaviour we had before on this board, by making it intercept
> the new hook in the same way it did the old one. I've beefed up the
> comment somewhat to indicate what we're doing, why, and why it might
> not be right.
> 
> The patch series is structured in three parts:
>  * make the Jazz board code support CPUs regardless of which
>    of the two hooks they implement
>  * switch the MIPS CPUs over to implementing the new hook
>  * remove the no-longer-needed Jazz board code for the old
>    hook
> (This seemed cleaner to me than squashing the whole thing into
> a single patch that touched core mips code and the jazz board
> at the same time.)
> 
> I have tested this with:
>  * the ARC Multiboot BIOS linked to from the bug
>    https://bugs.launchpad.net/qemu/+bug/1245924 (and which
>    was the test case for needing the hook intercept)
>  * a Linux kernel for the 'mips' mips r4k machine
>  * 'make check'
> Obviously more extensive testing would be useful, but I
> don't have any other test images. I also don't have
> a KVM MIPS host, which would be worth testing to confirm
> that it also still works.
> 
> If anybody happens by some chance to still have a working
> real-hardware Magnum or PICA61 board, we could perhaps test
> how it handles accesses to invalid memory, but I suspect that
> nobody does any more :-)
> 
> thanks
> -- PMM
> 
> 
> Peter Maydell (3):
>   hw/mips/mips_jazz: Override do_transaction_failed hook
>   target/mips: Switch to do_transaction_failed() hook
>   hw/mips/mips_jazz: Remove no-longer-necessary override of
>     do_unassigned_access
> 
>  target/mips/internal.h  |  8 ++++---
>  hw/mips/mips_jazz.c     | 47 +++++++++++++++++++++++++++++------------
>  target/mips/cpu.c       |  2 +-
>  target/mips/op_helper.c | 24 +++++++--------------
>  4 files changed, 47 insertions(+), 34 deletions(-)
>