Re: [PATCH] hw/acpi: Set memory regions to native endian as a work around

[BALATON Zoltan]

On Tue, 21 Feb 2023, BALATON Zoltan wrote:
> On Tue, 21 Feb 2023, Paolo Bonzini wrote:
> > On 2/21/23 00:25, BALATON Zoltan wrote:
> > > > I think fundamentally you need to check for the condition
> > > > Size < mr->ops->impl.min_access_size in memory_region_dispatch_write
> > > > and then make a read, combine the result with
> > > > the value and make a write.
> > >
> > > I neither know that part nor feel confident enough breaking such low level 
> > > stuff so I think setting the affected regions NATIVE_ENDIAN for now until 
> > > somebody takes care of this is safer and not likely to break anyting (or 
> > > if it does, much less widely and I'm more likely to be able to fix that 
> > > than your proposed changes). So I'd rather let you do that but I'd like 
> > > this fixed one way or another at last.
> >
> > Sorry about not replying.
> >
> > The case of impl.min_access_size < valid.min_access_size is not
> > supported in the memory core.  Until that is done, the correct fix is to
> > fix acpi_pm_evt_ops to have impl.min_access_size == 1, something like
> > this:
> >
> > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > index 6da275c599c6..96eb88fa7e27 100644
> > --- a/hw/acpi/core.c
> > +++ b/hw/acpi/core.c
> > @@ -429,20 +429,35 @@ void acpi_pm1_evt_reset(ACPIREGS *ar)
> > static uint64_t acpi_pm_evt_read(void *opaque, hwaddr addr, unsigned width)
> > {
> >     ACPIREGS *ar = opaque;
> > +    uint16_t val;
> >     switch (addr) {
> >     case 0:
> > -        return acpi_pm1_evt_get_sts(ar);
> > +        val = acpi_pm1_evt_get_sts(ar);
> >     case 2:
> > -        return ar->pm1.evt.en;
> > +        val = ar->pm1.evt.en;
>
> Some break; statements are missing here and above. This patch does not apply, 
> I had to apply it by hand to test it but it does not work. I don't have time 
> now to debug this. My patch works and don't see what else could it break. We 
> already have some devices set to native endian because of this so that could 
> be a usable workaround for now until you can fix it in memory layer.
>
> You can reproduce this with the MorphOS demo iso on pegasos2 with the 
> original board firmware thath isn't free but availeble in an updater here:
> http://web.archive.org/web/20071021223056/http://www.bplan-gmbh.de/up050404/up050404
> and the script to get the rom image from it is here:
> https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2/attach/extract_rom_from_updater
> the MorphOS iso is here:
> https://www.morphos-team.net/morphos-3.17.iso
>
> Then you can run it as:
>
> qemu-system-ppc -M pegasos2 -bios pegasos2.rom -device ati-vga,romfile=""
>  -serial stdio -cdrom morphos-3.17.iso
>
> at the firmware ok prompt type 'boot cd boot.img' then click OK button for 
> keyboard/language and once it booted press right mouse button on the top menu

To get that menu with Shut Down, first Quit the installer then again right 
click or click on background first to get the menu of the Ambient desktop. 
I also see an error from the firmware at the beginning:
Initializing KBD...00000012    FAILED.
when it's broken and it says Done without the hex number when it works. 
(Two other FAILED messages about clock chip is normal as we don't emulate 
that but all others should be green.)

> bar and select Shutdown from the Ambient (first) menu. Click twice on Shutwon 
> button which should exit QEMU but does reboot instead without a fix. Can you 
> please come up with a working fix if my patch is not acceptable.
>
> Thank you,
> BALATON Zoltan
>
> >     default:
> >         return 0;
> >     }
> > +
> > +    if (width == 1) {
> > +        int bitofs = (addr & 1) * 8;
> > +        val >>= bitofs;
> > +    }
> > +    return val;
> > }
> >  static void acpi_pm_evt_write(void *opaque, hwaddr addr, uint64_t val,
> >                               unsigned width)
> > {
> >     ACPIREGS *ar = opaque;
> > +    if (width == 1) {
> > +        int bitofs = (addr & 1) * 8;
> > +        uint16_t old_val = acpi_pm_evt_read(ar, addr, val & ~1);
> > +        uint16_t mask = 0xFF << bitofs;
> > +        val = (old_val & ~mask) | (val << bitofs);
> > +        addr &= ~1;
> > +    }
> > +
> >     switch (addr) {
> >     case 0:
> >         acpi_pm1_evt_write_sts(ar, val);
> > @@ -458,7 +473,7 @@ static void acpi_pm_evt_write(void *opaque, hwaddr 
> > addr, uint64_t val,
> > static const MemoryRegionOps acpi_pm_evt_ops = {
> >     .read = acpi_pm_evt_read,
> >     .write = acpi_pm_evt_write,
> > -    .impl.min_access_size = 2,
> > +    .impl.min_access_size = 1,
> >     .valid.min_access_size = 1,
> >     .valid.max_access_size = 2,
> >     .endianness = DEVICE_LITTLE_ENDIAN,
> >
> >
> > This assumes that the bus is little-endian, i.e. reading the byte at PM_EVT 
> > returns
> > bits 0..7 and reading the byte at PM_EVT+1 returns bits 8..15.
> >
> > If this is incorrect, endianness needs to be changed as well.
> >
> > Paolo