Message ID | 20190801183012.17564-1-peter.maydell@linaro.org (mailing list archive) |
---|---|
Headers | show |
Series | target/sparc: Convert to do_transaction_failed hook | expand |
Mark -- ping? Richard has reviewed this series; do you want more time to test it, or should I just apply it to master if you don't have any other pending sparc patches? thanks -- PMM On Thu, 1 Aug 2019 at 19:30, Peter Maydell <peter.maydell@linaro.org> wrote: > > This patchset converts the SPARC target away from the old > broken do_unassigned_access hook to the new (added in 2017...) > do_transaction_failed hook. In the process it fixes a number > of bugs in corner cases. > > The SPARC ld/st-with-ASI helper functions are odd in that they > make use of the cpu_unassigned_access() function as a way of > raising an MMU fault. We start by making them just directly > call a new sparc_raise_mmu_fault() function so they don't go > through the hook function. This in-passing fixes a bug where > the hook was using GETPC(), which won't work inside a function > several levels deep in the callstack from a helper function. > > The next four patches convert places that were using ld*_phys() > and st*_phys() and thus getting "implicitly causes an exception > via do_unassigned_access if it gets a bus error" behaviour. > We make them all use address_space_ld*/st* functions instead, > and explicitly handle the transaction-failure case. Variously: > * for MMU passthrough, this doesn't change behaviour > * for the MXCC stream source/destination ASI accesses, > this doesn't change behaviour, but the current behaviour > looks a bit weird, so a TODO comment is left in case anybody > wants to chase up the actual right behaviour in future > * for page table walks this fixes a bug where we would take > an exception instead of reporting the page table walk failure > with the correct fault status register information > * for the page table walk in mmu_probe() this fixes a bug where > we would take an exception when we are supposed to just report > failure. Note that the spec says that MMU probe operations > are supposed to set the fault status registers, but we do not; > again I leave this pre-existing bug as a TODO note. > Next, we remove one entirely pointless and unused ldl_phys() > call from dump_mmu(). > > Finally, the last patch can do the very small amount of work to > change over to the new hook function. This will cause all the > "handle error" code paths in the preceding patches to become > active (when a do_unassigned_access hook is present the load > or store functions will never return an error, because the hook > will get called and throw an exception first). > > I have tested that I can boot a sparc32 debian image, and > that sparc64 boots up to the firmware prompt, but this could > certainly use more extensive testing than I have given it. > > (After SPARC, the only remaining unassigned-access-hook users > are RISCV, which crept in while I wasn't looking, and MIPS, > which is doing something complicated with the Jazz board that > I haven't yet investigated.) > > thanks > --PMM > > Peter Maydell (7): > target/sparc: Factor out the body of sparc_cpu_unassigned_access() > target/sparc: Check for transaction failures in MMU passthrough ASIs > target/sparc: Check for transaction failures in MXCC stream ASI > accesses > target/sparc: Correctly handle bus errors in page table walks > target/sparc: Handle bus errors in mmu_probe() > target/sparc: Remove unused ldl_phys from dump_mmu() > target/sparc: Switch to do_transaction_failed() hook > > target/sparc/cpu.h | 8 +- > target/sparc/cpu.c | 2 +- > target/sparc/ldst_helper.c | 319 +++++++++++++++++++++---------------- > target/sparc/mmu_helper.c | 57 +++++-- > 4 files changed, 238 insertions(+), 148 deletions(-) >
On 03/09/2019 14:15, Peter Maydell wrote: > Mark -- ping? Richard has reviewed this series; do you want > more time to test it, or should I just apply it to master > if you don't have any other pending sparc patches? Yes, sorry - it has been on my TODO list for a while to look at this, but I've been quite short of time these past few weeks. I should be able to run it through my SPARC test images before the end of the week if that helps? ATB, Mark.
On Tue, 3 Sep 2019 at 18:00, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 03/09/2019 14:15, Peter Maydell wrote: > > > Mark -- ping? Richard has reviewed this series; do you want > > more time to test it, or should I just apply it to master > > if you don't have any other pending sparc patches? > > Yes, sorry - it has been on my TODO list for a while to look at this, but I've been > quite short of time these past few weeks. I should be able to run it through my SPARC > test images before the end of the week if that helps? Yeah, that would be great. It's not hugely urgent, but I would like to get this refactoring complete some time this release cycle so we can drop the old hooks. thanks -- PMM
On 01/08/2019 19:30, Peter Maydell wrote: > This patchset converts the SPARC target away from the old > broken do_unassigned_access hook to the new (added in 2017...) > do_transaction_failed hook. In the process it fixes a number > of bugs in corner cases. > > The SPARC ld/st-with-ASI helper functions are odd in that they > make use of the cpu_unassigned_access() function as a way of > raising an MMU fault. We start by making them just directly > call a new sparc_raise_mmu_fault() function so they don't go > through the hook function. This in-passing fixes a bug where > the hook was using GETPC(), which won't work inside a function > several levels deep in the callstack from a helper function. > > The next four patches convert places that were using ld*_phys() > and st*_phys() and thus getting "implicitly causes an exception > via do_unassigned_access if it gets a bus error" behaviour. > We make them all use address_space_ld*/st* functions instead, > and explicitly handle the transaction-failure case. Variously: > * for MMU passthrough, this doesn't change behaviour > * for the MXCC stream source/destination ASI accesses, > this doesn't change behaviour, but the current behaviour > looks a bit weird, so a TODO comment is left in case anybody > wants to chase up the actual right behaviour in future > * for page table walks this fixes a bug where we would take > an exception instead of reporting the page table walk failure > with the correct fault status register information > * for the page table walk in mmu_probe() this fixes a bug where > we would take an exception when we are supposed to just report > failure. Note that the spec says that MMU probe operations > are supposed to set the fault status registers, but we do not; > again I leave this pre-existing bug as a TODO note. > Next, we remove one entirely pointless and unused ldl_phys() > call from dump_mmu(). > > Finally, the last patch can do the very small amount of work to > change over to the new hook function. This will cause all the > "handle error" code paths in the preceding patches to become > active (when a do_unassigned_access hook is present the load > or store functions will never return an error, because the hook > will get called and throw an exception first). > > I have tested that I can boot a sparc32 debian image, and > that sparc64 boots up to the firmware prompt, but this could > certainly use more extensive testing than I have given it. > > (After SPARC, the only remaining unassigned-access-hook users > are RISCV, which crept in while I wasn't looking, and MIPS, > which is doing something complicated with the Jazz board that > I haven't yet investigated.) > > thanks > --PMM > > Peter Maydell (7): > target/sparc: Factor out the body of sparc_cpu_unassigned_access() > target/sparc: Check for transaction failures in MMU passthrough ASIs > target/sparc: Check for transaction failures in MXCC stream ASI > accesses > target/sparc: Correctly handle bus errors in page table walks > target/sparc: Handle bus errors in mmu_probe() > target/sparc: Remove unused ldl_phys from dump_mmu() > target/sparc: Switch to do_transaction_failed() hook > > target/sparc/cpu.h | 8 +- > target/sparc/cpu.c | 2 +- > target/sparc/ldst_helper.c | 319 +++++++++++++++++++++---------------- > target/sparc/mmu_helper.c | 57 +++++-- > 4 files changed, 238 insertions(+), 148 deletions(-) I've just run this through my SPARC test images with a fairly recent git master and I don't see any obvious regressions so: Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Thanks for taking the time to do the conversion for the SPARC target. ATB, Mark.
On Fri, 6 Sep 2019 at 15:34, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 01/08/2019 19:30, Peter Maydell wrote: > > > This patchset converts the SPARC target away from the old > > broken do_unassigned_access hook to the new (added in 2017...) > > do_transaction_failed hook. In the process it fixes a number > > of bugs in corner cases. > > Peter Maydell (7): > > target/sparc: Factor out the body of sparc_cpu_unassigned_access() > > target/sparc: Check for transaction failures in MMU passthrough ASIs > > target/sparc: Check for transaction failures in MXCC stream ASI > > accesses > > target/sparc: Correctly handle bus errors in page table walks > > target/sparc: Handle bus errors in mmu_probe() > > target/sparc: Remove unused ldl_phys from dump_mmu() > > target/sparc: Switch to do_transaction_failed() hook > > > > target/sparc/cpu.h | 8 +- > > target/sparc/cpu.c | 2 +- > > target/sparc/ldst_helper.c | 319 +++++++++++++++++++++---------------- > > target/sparc/mmu_helper.c | 57 +++++-- > > 4 files changed, 238 insertions(+), 148 deletions(-) > > I've just run this through my SPARC test images with a fairly recent git master and I > don't see any obvious regressions so: > > Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Thanks for taking the time to do the conversion for the SPARC target. Thanks; I've applied the patchset to master. -- PMM