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

[Howard Spoelstra <hsp.cat7@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6183 bytes --]

On Tue, Jun 27, 2023 at 10:15 AM Mark Cave-Ayland <
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>
> 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 believe It is not fixed by the patch set from the 23 of june, I still get
system errors when running Mac OS 9.2 with the mac99 machine after applying
them.
Mac OS 9.2 has required mac99,via=pmu for a long time now to always boot
successfully. (while 9.0.4 requires mac99 to boot, due to an undiagnosed
OHCI USB problem with the specific drivers that ship with it.)  ;-)

Best,
Howard


>
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 265c0bbd8d..e55f938da7 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -26,6 +26,7 @@
>   #include "qemu/osdep.h"
>   #include "qapi/error.h"
>   #include "qemu/module.h"
> +#include "hw/misc/unimp.h"
>   #include "hw/misc/macio/cuda.h"
>   #include "hw/pci/pci.h"
>   #include "hw/ppc/mac_dbdma.h"
> @@ -94,6 +95,7 @@ static bool macio_common_realize(PCIDevice *d, Error
> **errp)
>   {
>       MacIOState *s = MACIO(d);
>       SysBusDevice *sbd;
> +    DeviceState *dev;
>
>       if (!qdev_realize(DEVICE(&s->dbdma), BUS(&s->macio_bus), errp)) {
>           return false;
> @@ -102,6 +104,14 @@ static bool macio_common_realize(PCIDevice *d, Error
> **errp)
>       memory_region_add_subregion(&s->bar, 0x08000,
>                                   sysbus_mmio_get_region(sbd, 0));
>
> +    dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> +    qdev_prop_set_string(dev, "name", "screamer");
> +    qdev_prop_set_uint64(dev, "size", 0x1000);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sbd = SYS_BUS_DEVICE(dev);
> +    memory_region_add_subregion(&s->bar, 0x14000,
> +                                sysbus_mmio_get_region(sbd, 0));
> +
>       qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
>       qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
>       qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
> diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
> index 86df2c2b60..1894178a68 100644
> --- a/include/hw/misc/macio/macio.h
> +++ b/include/hw/misc/macio/macio.h
> @@ -109,6 +109,7 @@ struct MacIOState {
>       PMUState pmu;
>       DBDMAState dbdma;
>       ESCCState escc;
> +    MemoryRegion screamer;
>       uint64_t frequency;
>   };
>
>
>
> ATB,
>
> Mark.
>
>

[-- Attachment #2: Type: text/html, Size: 7938 bytes --]