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

* [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook
@ 2019-08-02 16:04 Peter Maydell
  2019-08-02 16:04 ` [Qemu-devel] [PATCH 1/3] hw/mips/mips_jazz: Override " Peter Maydell
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Peter Maydell @ 2019-08-02 16:04 UTC (permalink / raw)
  To: qemu-devel
  Cc: Aleksandar Rikalo, Hervé Poussineau, Aleksandar Markovic,
	Aurelien Jarno

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(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 14+ messages in thread