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

[BALATON Zoltan <balaton@eik.bme.hu>]

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 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
>
>