qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 0/4] target/ppc: Catch invalid real address accesses


From: Cédric Le Goater
Subject: Re: [PATCH 0/4] target/ppc: Catch invalid real address accesses
Date: Wed, 28 Jun 2023 09:02:23 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0

On 6/27/23 22:26, Mark Cave-Ayland wrote:
On 27/06/2023 13:41, Cédric Le Goater wrote:

On 6/27/23 14:05, Howard Spoelstra wrote:


On Tue, Jun 27, 2023 at 1:24 PM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk 
<mailto:mark.cave-ayland@ilande.co.uk>> wrote:

    On 27/06/2023 11:28, Howard Spoelstra wrote:

     > On Tue, Jun 27, 2023 at 10:15 AM Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk <mailto:mark.cave-ayland@ilande.co.uk>
     > <mailto:mark.cave-ayland@ilande.co.uk 
<mailto:mark.cave-ayland@ilande.co.uk>>> wrote:
     >
     >     On 26/06/2023 14:35, Cédric Le Goater wrote:
     >
     >      > On 6/23/23 14:37, Cédric Le Goater wrote:
     >      >> On 6/23/23 11:10, Peter Maydell wrote:
     >      >>> On Fri, 23 Jun 2023 at 09:21, Nicholas Piggin <npiggin@gmail.com 
<mailto:npiggin@gmail.com>
     >     <mailto:npiggin@gmail.com <mailto:npiggin@gmail.com>>> wrote:
     >      >>>>
     >      >>>> ppc has always silently ignored access to real (physical) 
addresses
     >      >>>> with nothing behind it, which can make debugging difficult at 
times.
     >      >>>>
     >      >>>> It looks like the way to handle this is implement the 
transaction
     >      >>>> failed call, which most target architectures do. Notably not 
x86
     >      >>>> though, I wonder why?
     >      >>>
     >      >>> Much of this is historical legacy. QEMU originally had no
     >      >>> concept of "the system outside the CPU returns some kind
     >      >>> of bus error and the CPU raises an exception for it".
     >      >>> This is turn is (I think) because the x86 PC doesn't do
     >      >>> that: you always get back some kind of response, I think
     >      >>> -1 on reads and writes ignored. We added the 
do_transaction_failed
     >      >>> hook largely because we wanted it to give more accurate
     >      >>> emulation of this kind of thing on Arm, but as usual with new
     >      >>> facilities we left the other architectures to do it themselves
     >      >>> if they wanted -- by default the behaviour remained the same.
     >      >>> Some architectures have picked it up; some haven't.
     >      >>>
     >      >>> The main reason it's a bit of a pain to turn the correct
     >      >>> handling on is because often boards don't actually implement
     >      >>> all the devices they're supposed to. For a pile of legacy Arm
     >      >>> boards, especially where we didn't have good test images,
     >      >>> we use the machine flag ignore_memory_transaction_failures to
     >      >>> retain the legacy behaviour. (This isn't great because it's
     >      >>> pretty much going to mean we have that flag set on those
     >      >>> boards forever because nobody is going to care enough to
     >      >>> investigate and test.)
     >      >>>
     >      >>>> Other question is, sometimes I guess it's nice to avoid 
crashing in
     >      >>>> order to try to quickly get past some unimplemented MMIO. 
Maybe a
     >      >>>> command line option or something could turn it off? It should
     >      >>>> probably be a QEMU-wide option if so, so that shouldn't hold 
this
     >      >>>> series up, I can propose a option for that if anybody is 
worried
     >      >>>> about it.
     >      >>>
     >      >>> I would not recommend going any further than maybe setting the
     >      >>> ignore_memory_transaction_failures flag for boards you don't
     >      >>> care about. (But in an ideal world, don't set it and deal with
     >      >>> any bug reports by implementing stub versions of missing 
devices.
     >      >>> Depends how confident you are in your test coverage.)
     >      >>
     >      >> It seems it broke the "mac99" and  powernv10 machines, using the
     >      >> qemu-ppc-boot images which are mostly buildroot. See below for 
logs.
     >      >>
     >      >> Adding Mark for further testing on Mac OS.
     >      >
     >      >
     >      > Mac OS 9.2 fails to boot with a popup saying :
     >      >          Sorry, a system error occured.
     >      >          "Sound Manager"
     >      >            address error
     >      >          To temporarily turn off extensions, restart and
     >      >          hold down the shift key
     >      >
     >      >
     >      > Darwin and Mac OSX look OK.
     >
     >     My guess would be that MacOS 9.2 is trying to access the sound chip 
registers which
     >     isn't implemented in QEMU for the moment (I have a separate screamer 
branch
     >     available, but it's not ready for primetime yet). In theory they 
shouldn't be
     >     accessed at all because the sound device isn't present in the 
OpenBIOS device tree,
     >     but this is all fairly old stuff.
     >
     >     Does implementing the sound registers using a dummy device help at 
all?
     >
     >
     > My uneducated guess is that you stumbled on a longstanding, but 
intermittently
     > occurring, issue specific to Mac OS 9.2 related to sound support over 
USB in Apple
     > monitors.

    I'm not sure I understand this: are there non-standard command line options 
being
    used here other than "qemu-system-ppc -M mac99 -cdrom macos92.iso -boot d"?



It must be my windows host ;-)

qemu-system-ppc.exe -M mac99,via=pmu -cdrom C:\mac-iso\9.2.2.iso -boot d -L 
pc-bios
crashes Mac OS with an address error. (with unpatched and patched builds).

Same on Linux. I get an invalid opcode. QEMU 7.2 work fine though.

C.

That certainly shouldn't happen, and if it worked in 7.2 then there's 
definitely a regression which has crept in there somewhere. I'll try and bisect 
this at some point soon, but feel free to try and beat me ;)

bisect points to :

commit e506ad6a05c806bbef460a7d014a184ff8d707a6
Author: Richard Henderson <richard.henderson@linaro.org>
Date:   Mon Mar 6 04:30:11 2023 +0300

    accel/tcg: Pass last not end to tb_invalidate_phys_range
Pass the address of the last byte to be changed, rather than
    the first address past the last byte.  This avoids overflow
    when the last page of the address space is involved.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
    Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

 include/exec/exec-all.h   |  2 +-
 accel/tcg/tb-maint.c      | 31 ++++++++++++++++---------------
 accel/tcg/translate-all.c |  2 +-
 accel/tcg/user-exec.c     |  2 +-
 softmmu/physmem.c         |  2 +-
 5 files changed, 20 insertions(+), 19 deletions(-)


I think the instruction is fnmadds. Needs more digging.

Thanks,

C.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]