| Message ID | 20181210165636.28366-1-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | target/m68k: convert to transaction_failed hook | expand |
On 2018-12-10 17:56, Peter Maydell wrote: > This patchset converts the m68k target from the deprecated > unassigned_access hook to the new transaction_failed hook. > It's RFC for a couple of reasons: > * it's untested, since I don't have an m68k test image Have a look at the arnewsh 5206 and 5208evb images here: https://web.archive.org/web/20180427080645/http://www.uclinux.org/ports/coldfire/binary.html or https://www.qemu-advent-calendar.org/2018/#day-7 :-) Thomas
On 10/12/2018 16:56, Peter Maydell wrote: > This patchset converts the m68k target from the deprecated > unassigned_access hook to the new transaction_failed hook. > It's RFC for a couple of reasons: > * it's untested, since I don't have an m68k test image > * the second patch just makes "bus error while trying to > read page tables" be treated as a page fault, when it > should probably cause a fault reporting it as a bus error > of some kind > * I don't understand why the old unassigned_access hook > set the ATC bit in the MMU SSW, since the docs I have say > this should be set if the fault happened during a table > search, but cleared if it's just an ordinary bus-errored > data or insn access. Probably this is a pre-existing bug? > > Anyway, I send it out as a skeleton for comments, because > it would be nice to get rid of the old unassigned_access > hook, which is fundamentally broken (it's still used by m68k, > microblaze, mips and sparc). Laurent is really the expert here (my work on the q800 was purely on the device side), however is this also a nudge to see if the unassigned_access hook can be eliminated from sparc too? ;) ATB, Mark.
On Tue, 11 Dec 2018 at 19:13, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 10/12/2018 16:56, Peter Maydell wrote: > > Anyway, I send it out as a skeleton for comments, because > > it would be nice to get rid of the old unassigned_access > > hook, which is fundamentally broken (it's still used by m68k, > > microblaze, mips and sparc). > > Laurent is really the expert here (my work on the q800 was purely on the device > side), however is this also a nudge to see if the unassigned_access hook can be > eliminated from sparc too? ;) It would certainly be great to convert sparc too; it and mips are a little more complicated than these ones, but the principle is the same: * helper functions in target/sparc which call cpu_unassigned_access() should be changed to call some sparc-internal function to raise the right exception * callsites in target/sparc which do loads or stores by physical address should be checked to ensure they do the right thing when a bus error is detected; this usually means changing them to use address_space_* functions and check they return MEMTX_OK. (With the old unassigned_access hook these would result in calls to the hook, which was often the wrong thing anyway. The transaction_failed hook is called only for accesses via the TCG MMU.) The docs/devel/loads-stores.rst docs have some handy regexes for use with 'git grep'; for sparc these catch everything: git grep '\<ldu\?[bwlq]\(_[bl]e\)\?_phys\>' target/sparc/ git grep '\<st[bwlq]\(_[bl]e\)\?_phys\>' target/sparc/ * convert the hook itself: this requires a little fiddling of parameters, and the addition of the cpu_restore_state() call (MIPS has some odd board-specific handling on top of that which will need to be fixed too.) thanks -- PMM
On 10/12/2018 17:56, Peter Maydell wrote: > This patchset converts the m68k target from the deprecated > unassigned_access hook to the new transaction_failed hook. > It's RFC for a couple of reasons: > * it's untested, since I don't have an m68k test image > * the second patch just makes "bus error while trying to > read page tables" be treated as a page fault, when it > should probably cause a fault reporting it as a bus error > of some kind > * I don't understand why the old unassigned_access hook > set the ATC bit in the MMU SSW, since the docs I have say > this should be set if the fault happened during a table > search, but cleared if it's just an ordinary bus-errored > data or insn access. Probably this is a pre-existing bug? > > Anyway, I send it out as a skeleton for comments, because > it would be nice to get rid of the old unassigned_access > hook, which is fundamentally broken (it's still used by m68k, > microblaze, mips and sparc). > > thanks > -- PMM > > Peter Maydell (3): > target/m68k: In dump_address_map() check for memory access failures > target/m68k: In get_physical_address() check for memory access > failures > target/m68k: Switch to transaction_failed hook > > target/m68k/cpu.h | 7 ++-- > target/m68k/cpu.c | 2 +- > target/m68k/helper.c | 84 ++++++++++++++++++++++++++++++++--------- > target/m68k/op_helper.c | 20 ++++------ > 4 files changed, 80 insertions(+), 33 deletions(-) > Tested-by: Laurent Vivier <laurent@vivier.eu> I'll try to review this later... Thanks, Laurent
On Wed, 12 Dec 2018 at 20:43, Laurent Vivier <laurent@vivier.eu> wrote: > > On 10/12/2018 17:56, Peter Maydell wrote: > > This patchset converts the m68k target from the deprecated > > unassigned_access hook to the new transaction_failed hook. > > It's RFC for a couple of reasons: > > * it's untested, since I don't have an m68k test image > > * the second patch just makes "bus error while trying to > > read page tables" be treated as a page fault, when it > > should probably cause a fault reporting it as a bus error > > of some kind > > * I don't understand why the old unassigned_access hook > > set the ATC bit in the MMU SSW, since the docs I have say > > this should be set if the fault happened during a table > > search, but cleared if it's just an ordinary bus-errored > > data or insn access. Probably this is a pre-existing bug? > > > > Anyway, I send it out as a skeleton for comments, because > > it would be nice to get rid of the old unassigned_access > > hook, which is fundamentally broken (it's still used by m68k, > > microblaze, mips and sparc). > > > > thanks > > -- PMM > > > > Peter Maydell (3): > > target/m68k: In dump_address_map() check for memory access failures > > target/m68k: In get_physical_address() check for memory access > > failures > > target/m68k: Switch to transaction_failed hook > > > > target/m68k/cpu.h | 7 ++-- > > target/m68k/cpu.c | 2 +- > > target/m68k/helper.c | 84 ++++++++++++++++++++++++++++++++--------- > > target/m68k/op_helper.c | 20 ++++------ > > 4 files changed, 80 insertions(+), 33 deletions(-) > > > > Tested-by: Laurent Vivier <laurent@vivier.eu> > > I'll try to review this later... Ping! Are we at "later" yet ? :-) I checked with the mbox of the series from https://patchew.org/QEMU/20181210165636.28366-1-peter.maydell@linaro.org/ and it still applies cleanly to master. thanks -- PMM
On 10/12/2018 17:56, Peter Maydell wrote: > This patchset converts the m68k target from the deprecated > unassigned_access hook to the new transaction_failed hook. > It's RFC for a couple of reasons: > * it's untested, since I don't have an m68k test image > * the second patch just makes "bus error while trying to > read page tables" be treated as a page fault, when it > should probably cause a fault reporting it as a bus error > of some kind > * I don't understand why the old unassigned_access hook > set the ATC bit in the MMU SSW, since the docs I have say > this should be set if the fault happened during a table > search, but cleared if it's just an ordinary bus-errored > data or insn access. Probably this is a pre-existing bug? I think you're right. It must be cleared on bus error. Thanks, Laurent
On Fri, 3 May 2019 at 18:12, Laurent Vivier <laurent@vivier.eu> wrote: > > On 10/12/2018 17:56, Peter Maydell wrote: > > This patchset converts the m68k target from the deprecated > > unassigned_access hook to the new transaction_failed hook. > > It's RFC for a couple of reasons: > > * it's untested, since I don't have an m68k test image > > * the second patch just makes "bus error while trying to > > read page tables" be treated as a page fault, when it > > should probably cause a fault reporting it as a bus error > > of some kind > > * I don't understand why the old unassigned_access hook > > set the ATC bit in the MMU SSW, since the docs I have say > > this should be set if the fault happened during a table > > search, but cleared if it's just an ordinary bus-errored > > data or insn access. Probably this is a pre-existing bug? > > I think you're right. It must be cleared on bus error. Thanks for the review of this patchset. Is there anything you want me to do for a v2, or is it ready to be applied ? -- PMM
On 16/05/2019 15:26, Peter Maydell wrote: > On Fri, 3 May 2019 at 18:12, Laurent Vivier <laurent@vivier.eu> wrote: >> >> On 10/12/2018 17:56, Peter Maydell wrote: >>> This patchset converts the m68k target from the deprecated >>> unassigned_access hook to the new transaction_failed hook. >>> It's RFC for a couple of reasons: >>> * it's untested, since I don't have an m68k test image >>> * the second patch just makes "bus error while trying to >>> read page tables" be treated as a page fault, when it >>> should probably cause a fault reporting it as a bus error >>> of some kind >>> * I don't understand why the old unassigned_access hook >>> set the ATC bit in the MMU SSW, since the docs I have say >>> this should be set if the fault happened during a table >>> search, but cleared if it's just an ordinary bus-errored >>> data or insn access. Probably this is a pre-existing bug? >> >> I think you're right. It must be cleared on bus error. > > Thanks for the review of this patchset. Is there anything > you want me to do for a v2, or is it ready to be applied ? It looks good to me. I'm going to add it to my m68k branch and send a PR. Thanks, Laurent
This patchset converts the m68k target from the deprecated unassigned_access hook to the new transaction_failed hook. It's RFC for a couple of reasons: * it's untested, since I don't have an m68k test image * the second patch just makes "bus error while trying to read page tables" be treated as a page fault, when it should probably cause a fault reporting it as a bus error of some kind * I don't understand why the old unassigned_access hook set the ATC bit in the MMU SSW, since the docs I have say this should be set if the fault happened during a table search, but cleared if it's just an ordinary bus-errored data or insn access. Probably this is a pre-existing bug? Anyway, I send it out as a skeleton for comments, because it would be nice to get rid of the old unassigned_access hook, which is fundamentally broken (it's still used by m68k, microblaze, mips and sparc). thanks -- PMM Peter Maydell (3): target/m68k: In dump_address_map() check for memory access failures target/m68k: In get_physical_address() check for memory access failures target/m68k: Switch to transaction_failed hook target/m68k/cpu.h | 7 ++-- target/m68k/cpu.c | 2 +- target/m68k/helper.c | 84 ++++++++++++++++++++++++++++++++--------- target/m68k/op_helper.c | 20 ++++------ 4 files changed, 80 insertions(+), 33 deletions(-) -- 2.19.2