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