[Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook

Peter Maydell posted 3 patches 4 years, 7 months ago
Test docker-clang@ubuntu passed
Test s390x passed
Test asan passed
Test docker-mingw@fedora passed
Test FreeBSD passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190802160458.25681-1-peter.maydell@linaro.org
Maintainers: Aleksandar Rikalo <arikalo@wavecomp.com>, Aurelien Jarno <aurelien@aurel32.net>, Aleksandar Markovic <amarkovic@wavecomp.com>, "Hervé Poussineau" <hpoussin@reactos.org>
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(-)
[Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook
Posted by Peter Maydell 4 years, 7 months ago
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


Re: [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook
Posted by Philippe Mathieu-Daudé 4 years, 7 months ago
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(-)
> 

Re: [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook
Posted by Aleksandar Markovic 4 years, 6 months ago
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(-)
> >
>
Re: [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook
Posted by Aleksandar Markovic 4 years, 7 months ago
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
>
>
Re: [Qemu-devel] [PATCH 0/3] target/mips: Convert to do_transaction_failed hook
Posted by Aleksandar Markovic 4 years, 6 months ago
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
> >
> >