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(-)
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
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(-) >
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(-) > > >
02.08.2019. 18.05, "Peter Maydell" <peter.maydell@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 > >
ping for Hervé. 22.08.2019. 20.16, "Aleksandar Markovic" <aleksandar.m.mail@gmail.com> је написао/ла: > > > 02.08.2019. 18.05, "Peter Maydell" <peter.maydell@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 > > > >
© 2016 - 2024 Red Hat, Inc.