[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed h
From: |
Peter Maydell |
Subject: |
[Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook |
Date: |
Fri, 2 Aug 2019 17:04:55 +0100 |
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
- [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook,
Peter Maydell <=
- [Qemu-devel] [PATCH 1/3] hw/mips/mips_jazz: Override do_transaction_failed hook, Peter Maydell, 2019/08/02
- [Qemu-devel] [PATCH 2/3] target/mips: Switch to do_transaction_failed() hook, Peter Maydell, 2019/08/02
- [Qemu-devel] [PATCH 3/3] hw/mips/mips_jazz: Remove no-longer-necessary override of do_unassigned_access, Peter Maydell, 2019/08/02
- Re: [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook, Philippe Mathieu-Daudé, 2019/08/02
- Re: [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook, Aleksandar Markovic, 2019/08/22