[Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook

Peter Maydell posted 3 patches 5 years, 3 months ago
Test asan passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20181210165636.28366-1-peter.maydell@linaro.org
Maintainers: Laurent Vivier <laurent@vivier.eu>
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(-)
[Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook
Posted by Peter Maydell 5 years, 3 months ago
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


Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook
Posted by Mark Cave-Ayland 5 years, 3 months ago
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.

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook
Posted by Peter Maydell 5 years, 3 months ago
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

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook
Posted by Thomas Huth 5 years, 3 months ago
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

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook
Posted by Laurent Vivier 5 years, 3 months ago
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

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook
Posted by Peter Maydell 4 years, 10 months ago
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

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook
Posted by Laurent Vivier 4 years, 10 months ago
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

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook
Posted by Peter Maydell 4 years, 10 months ago
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

Re: [Qemu-devel] [RFC 0/3] target/m68k: convert to transaction_failed hook
Posted by Laurent Vivier 4 years, 10 months ago
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