ping for Hervé.
22.08.2019. 20.16, "Aleksandar Markovic" <aleksandar.m.m...@gmail.com> је написао/ла: > > > 02.08.2019. 18.05, "Peter Maydell" <peter.mayd...@linaro.org> је написао/ла: > > > > 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. > > > > Herve, bonjour. > > As far as I can see these changes are fine. May I ask you for your opinion? Can you run your Jazz tests without regressions with this change? > > Mille mercis, > 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(-) > > > > -- > > 2.20.1 > > > >
