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

[Aleksandar Markovic <aleksandar.m.mail@gmail.com>]

02.08.2019. 18.29, "Philippe Mathieu-Daudé" <philmd@redhat.com> је
написао/ла:
>
> 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.
> >

Since Herve now tested this series, unless somebody objects, I am going to
include it in the next Mips queue, scheduled in next few days.

Thanks to all involved!

Aleksandar

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