From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>, qemu-devel@nongnu.org
Cc: "Paul Burton" <pburton@wavecomp.com>,
"Maciej W. Rozycki" <macro@linux-mips.org>,
"Aleksandar Rikalo" <arikalo@wavecomp.com>,
"James Hogan" <jhogan@kernel.org>,
"linux-mips@vger.kernel.org" <linux-mips@vger.kernel.org>,
"Hervé Poussineau" <hpoussin@reactos.org>,
"Aleksandar Markovic" <amarkovic@wavecomp.com>,
"Aurelien Jarno" <aurelien@aurel32.net>
Subject: Re: [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook
Date: Fri, 2 Aug 2019 18:29:12 +0200 [thread overview]
Message-ID: <565ed74a-5c6b-c1eb-035e-3eb981487de5@redhat.com> (raw)
In-Reply-To: <20190802160458.25681-1-peter.maydell@linaro.org>
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(-)
>
next prev parent reply other threads:[~2019-08-02 16:29 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-02 16:04 [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook Peter Maydell
2019-08-02 16:04 ` [Qemu-devel] [PATCH 1/3] hw/mips/mips_jazz: Override " Peter Maydell
2019-08-02 16:20 ` Philippe Mathieu-Daudé
2019-09-09 21:41 ` Hervé Poussineau
2019-08-02 16:04 ` [Qemu-devel] [PATCH 2/3] target/mips: Switch to do_transaction_failed() hook Peter Maydell
2019-08-02 16:29 ` Philippe Mathieu-Daudé
2019-09-09 21:41 ` Hervé Poussineau
2019-08-02 16:04 ` [Qemu-devel] [PATCH 3/3] hw/mips/mips_jazz: Remove no-longer-necessary override of do_unassigned_access Peter Maydell
2019-08-02 16:30 ` Philippe Mathieu-Daudé
2019-09-09 21:42 ` Hervé Poussineau
2019-08-02 16:29 ` Philippe Mathieu-Daudé [this message]
2019-09-11 7:22 ` [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook Aleksandar Markovic
2019-08-22 18:16 ` Aleksandar Markovic
2019-09-09 10:30 ` Aleksandar Markovic
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=565ed74a-5c6b-c1eb-035e-3eb981487de5@redhat.com \
--to=philmd@redhat.com \
--cc=amarkovic@wavecomp.com \
--cc=arikalo@wavecomp.com \
--cc=aurelien@aurel32.net \
--cc=hpoussin@reactos.org \
--cc=jhogan@kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=macro@linux-mips.org \
--cc=pburton@wavecomp.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).