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

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

[BALATON Zoltan]

When using ACPI on big endian machine (such as ppc/pegasos2 which has
a VT8231 south bridge with ACPI) writes to ACPI registers come out
byte swapped. This may be caused by a bug in memory subsystem but
until that is fixed setting the ACPI memory regions to native endian
makes it usable for big endian machines. This fixes ACPI shutdown with
pegasos2 when using the board firmware for now.
This could be reverted when the memory layer is fixed.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/acpi/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 1e004d0078..543e4a7875 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -461,7 +461,7 @@ static const MemoryRegionOps acpi_pm_evt_ops = {
     .impl.min_access_size = 2,
     .valid.min_access_size = 1,
     .valid.max_access_size = 2,
-    .endianness = DEVICE_LITTLE_ENDIAN,
+    .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
 void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
@@ -531,7 +531,7 @@ static const MemoryRegionOps acpi_pm_tmr_ops = {
     .impl.min_access_size = 4,
     .valid.min_access_size = 1,
     .valid.max_access_size = 4,
-    .endianness = DEVICE_LITTLE_ENDIAN,
+    .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
 void acpi_pm_tmr_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
@@ -608,7 +608,7 @@ static const MemoryRegionOps acpi_pm_cnt_ops = {
     .impl.min_access_size = 2,
     .valid.min_access_size = 1,
     .valid.max_access_size = 2,
-    .endianness = DEVICE_LITTLE_ENDIAN,
+    .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
 void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
-- 
2.30.2

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

[Michael S. Tsirkin]

On Mon, Nov 08, 2021 at 02:05:42PM +0100, BALATON Zoltan wrote:
> When using ACPI on big endian machine (such as ppc/pegasos2 which has
> a VT8231 south bridge with ACPI) writes to ACPI registers come out
> byte swapped. This may be caused by a bug in memory subsystem but
> until that is fixed setting the ACPI memory regions to native endian
> makes it usable for big endian machines. This fixes ACPI shutdown with
> pegasos2 when using the board firmware for now.
> This could be reverted when the memory layer is fixed.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>


Paolo, could you weight in on whether we can fix it properly
in the memory core? I suspect it's not a good idea to switch
to native without adding a bunch of byteswaps all
over the place ...

> ---
>  hw/acpi/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 1e004d0078..543e4a7875 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -461,7 +461,7 @@ static const MemoryRegionOps acpi_pm_evt_ops = {
>      .impl.min_access_size = 2,
>      .valid.min_access_size = 1,
>      .valid.max_access_size = 2,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
>  void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
> @@ -531,7 +531,7 @@ static const MemoryRegionOps acpi_pm_tmr_ops = {
>      .impl.min_access_size = 4,
>      .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
>  void acpi_pm_tmr_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
> @@ -608,7 +608,7 @@ static const MemoryRegionOps acpi_pm_cnt_ops = {
>      .impl.min_access_size = 2,
>      .valid.min_access_size = 1,
>      .valid.max_access_size = 2,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
>  void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
> -- 
> 2.30.2

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

[Paolo Bonzini]

On 11/8/21 14:05, BALATON Zoltan wrote:
> When using ACPI on big endian machine (such as ppc/pegasos2 which has
> a VT8231 south bridge with ACPI) writes to ACPI registers come out
> byte swapped. This may be caused by a bug in memory subsystem but
> until that is fixed setting the ACPI memory regions to native endian
> makes it usable for big endian machines. This fixes ACPI shutdown with
> pegasos2 when using the board firmware for now.
> This could be reverted when the memory layer is fixed.

What is the path to the swapped writes?  Even just a backtrace might be 
enough to understand what's going on, and especially where the bug is.

Paolo

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/acpi/core.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 1e004d0078..543e4a7875 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -461,7 +461,7 @@ static const MemoryRegionOps acpi_pm_evt_ops = {
>       .impl.min_access_size = 2,
>       .valid.min_access_size = 1,
>       .valid.max_access_size = 2,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
>   };
>   
>   void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
> @@ -531,7 +531,7 @@ static const MemoryRegionOps acpi_pm_tmr_ops = {
>       .impl.min_access_size = 4,
>       .valid.min_access_size = 1,
>       .valid.max_access_size = 4,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
>   };
>   
>   void acpi_pm_tmr_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
> @@ -608,7 +608,7 @@ static const MemoryRegionOps acpi_pm_cnt_ops = {
>       .impl.min_access_size = 2,
>       .valid.min_access_size = 1,
>       .valid.max_access_size = 2,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
>   };
>   
>   void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
> 

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

[Paolo Bonzini]

On 11/8/21 15:30, Paolo Bonzini wrote:
> On 11/8/21 14:05, BALATON Zoltan wrote:
>> When using ACPI on big endian machine (such as ppc/pegasos2 which has
>> a VT8231 south bridge with ACPI) writes to ACPI registers come out
>> byte swapped. This may be caused by a bug in memory subsystem but
>> until that is fixed setting the ACPI memory regions to native endian
>> makes it usable for big endian machines. This fixes ACPI shutdown with
>> pegasos2 when using the board firmware for now.
>> This could be reverted when the memory layer is fixed.
> 
> What is the path to the swapped writes?  Even just a backtrace might be 
> enough to understand what's going on, and especially where the bug is.

Ok, Michael pointed me at 
https://lore.kernel.org/all/20211011080528-mutt-send-email-mst@kernel.org/.

Paolo

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

[BALATON Zoltan]

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

On Mon, 8 Nov 2021, Paolo Bonzini wrote:
> On 11/8/21 15:30, Paolo Bonzini wrote:
>> On 11/8/21 14:05, BALATON Zoltan wrote:
>>> When using ACPI on big endian machine (such as ppc/pegasos2 which has
>>> a VT8231 south bridge with ACPI) writes to ACPI registers come out
>>> byte swapped. This may be caused by a bug in memory subsystem but
>>> until that is fixed setting the ACPI memory regions to native endian
>>> makes it usable for big endian machines. This fixes ACPI shutdown with
>>> pegasos2 when using the board firmware for now.
>>> This could be reverted when the memory layer is fixed.
>> 
>> What is the path to the swapped writes?  Even just a backtrace might be 
>> enough to understand what's going on, and especially where the bug is.
>
> Ok, Michael pointed me at 
> https://lore.kernel.org/all/20211011080528-mutt-send-email-mst@kernel.org/.

I was about to say that here's the original thread:

https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01972.html

and here's the backtrace:

#0  acpi_pm1_cnt_write (val=40, ar=0x55555695f340) at ../hw/acpi/core.c:556
#1  acpi_pm_cnt_write (opaque=0x55555695f340, addr=1, val=40, width=2) at ../hw/acpi/core.c:602
#2  0x0000555555b3a82f in memory_region_write_accessor
     (mr=mr@entry=0x55555695f590, addr=1, value=value@entry=0x7fffefffdd08, size=size@entry=2, shift=<optimized out>, mask=mask@entry=65535, attrs=...)
     at ../softmmu/memory.c:492
#3  0x0000555555b3813e in access_with_adjusted_size
     (addr=addr@entry=1, value=value@entry=0x7fffefffdd08, size=size@entry=1, access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn=
     0x555555b3a7b0 <memory_region_write_accessor>, mr=0x55555695f590, attrs=...) at ../softmmu/memory.c:554
#4  0x0000555555b3c449 in memory_region_dispatch_write (mr=mr@entry=0x55555695f590, addr=1, data=<optimized out>, op=<optimized out>, attrs=attrs@entry=...)
     at ../softmmu/memory.c:1511
#5  0x0000555555b2c121 in flatview_write_continue
     (fv=fv@entry=0x7fff84d23b30, addr=addr@entry=4261416709, attrs=attrs@entry=..., ptr=ptr@entry=0x7fffefffdec0, len=len@entry=1, addr1=<optimized out>,
      l=<optimized out>, mr=0x55555695f590) at host-utils.h:165
#6  0x0000555555b2c399 in flatview_write (len=1, buf=0x7fffefffdec0, attrs=..., addr=4261416709, fv=0x7fff84d23b30) at ../softmmu/physmem.c:2822
#7  subpage_write (opaque=<optimized out>, addr=<optimized out>, value=<optimized out>, len=1, attrs=...) at ../softmmu/physmem.c:2488
#8  0x0000555555b380de in access_with_adjusted_size
     (addr=addr@entry=3845, value=value@entry=0x7fffefffdf88, size=size@entry=1, access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn=
     0x555555b3aa80 <memory_region_write_with_attrs_accessor>, mr=0x7fff84710bb0, attrs=...) at ../softmmu/memory.c:549
#9  0x0000555555b3c449 in memory_region_dispatch_write (mr=mr@entry=0x7fff84710bb0, addr=addr@entry=3845, data=<optimized out>, data@entry=40, op=op@entry=MO_8, attrs=...)
     at ../softmmu/memory.c:1511
#10 0x0000555555c07b4c in io_writex
     (env=env@entry=0x55555666a820, iotlbentry=iotlbentry@entry=0x7fff843367f0, mmu_idx=1, val=val@entry=40, addr=addr@entry=4261416709,
      retaddr=retaddr@entry=140736116523268, op=MO_8) at ../accel/tcg/cputlb.c:1420
#11 0x0000555555c0b5df in store_helper (op=MO_8, retaddr=<optimized out>, oi=<optimized out>, val=40, addr=4261416709, env=0x55555666a820) at ../accel/tcg/cputlb.c:2355
#12 full_stb_mmu (env=0x55555666a820, addr=4261416709, val=40, oi=<optimized out>, retaddr=140736116523268) at ../accel/tcg/cputlb.c:2404
#13 0x00007fffae3b8104 in code_gen_buffer ()
#14 0x0000555555bfcfab in cpu_tb_exec (cpu=cpu@entry=0x555556661360, itb=itb@entry=0x7fffae3b7fc0 <code_gen_buffer+56197011>, tb_exit=tb_exit@entry=0x7fffefffe668)
     at ../accel/tcg/cpu-exec.c:357
#15 0x0000555555bfe089 in cpu_loop_exec_tb (tb_exit=0x7fffefffe668, last_tb=<synthetic pointer>, tb=0x7fffae3b7fc0 <code_gen_buffer+56197011>, cpu=0x555556661360)
     at ../accel/tcg/cpu-exec.c:833
#16 cpu_exec (cpu=cpu@entry=0x555556661360) at ../accel/tcg/cpu-exec.c:992
#17 0x0000555555c1bba0 in tcg_cpus_exec (cpu=cpu@entry=0x555556661360) at ../accel/tcg/tcg-accel-ops.c:67
#18 0x0000555555c1c3d7 in rr_cpu_thread_fn (arg=arg@entry=0x555556661360) at ../accel/tcg/tcg-accel-ops-rr.c:214
#19 0x0000555555d5c049 in qemu_thread_start (args=0x7fffefffe750) at ../util/qemu-thread-posix.c:556
#20 0x00007ffff6a95dea in start_thread () at /lib64/libpthread.so.0
#21 0x00007ffff69c8fdf in clone () at /lib64/libc.so.6

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

[BALATON Zoltan]

On Mon, 8 Nov 2021, Michael S. Tsirkin wrote:
> On Mon, Nov 08, 2021 at 02:05:42PM +0100, BALATON Zoltan wrote:
>> When using ACPI on big endian machine (such as ppc/pegasos2 which has
>> a VT8231 south bridge with ACPI) writes to ACPI registers come out
>> byte swapped. This may be caused by a bug in memory subsystem but
>> until that is fixed setting the ACPI memory regions to native endian
>> makes it usable for big endian machines. This fixes ACPI shutdown with
>> pegasos2 when using the board firmware for now.
>> This could be reverted when the memory layer is fixed.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
>
> Paolo, could you weight in on whether we can fix it properly
> in the memory core? I suspect it's not a good idea to switch
> to native without adding a bunch of byteswaps all
> over the place ...

In fact I think switching to native is less likely to break anything else 
than fixing it in the memory core because AFAIK NATIVE_ENDIAN means no 
byte swap at all while LITTLE_ENDIAN means byte swap if vcpu is big endian 
so this would only change the pegasos2 case as other ACPI users are little 
endian machines where this change has no effect. You may want to consider 
this when trying a last minute fix but I'm OK with whatever solution that 
fixes the original problem.

Regards,
BALATON Zoltan

>> ---
>>  hw/acpi/core.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
>> index 1e004d0078..543e4a7875 100644
>> --- a/hw/acpi/core.c
>> +++ b/hw/acpi/core.c
>> @@ -461,7 +461,7 @@ static const MemoryRegionOps acpi_pm_evt_ops = {
>>      .impl.min_access_size = 2,
>>      .valid.min_access_size = 1,
>>      .valid.max_access_size = 2,
>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>  };
>>
>>  void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
>> @@ -531,7 +531,7 @@ static const MemoryRegionOps acpi_pm_tmr_ops = {
>>      .impl.min_access_size = 4,
>>      .valid.min_access_size = 1,
>>      .valid.max_access_size = 4,
>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>  };
>>
>>  void acpi_pm_tmr_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
>> @@ -608,7 +608,7 @@ static const MemoryRegionOps acpi_pm_cnt_ops = {
>>      .impl.min_access_size = 2,
>>      .valid.min_access_size = 1,
>>      .valid.max_access_size = 2,
>> -    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>  };
>>
>>  void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
>> --
>> 2.30.2
>
>

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

[BALATON Zoltan]

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

On Mon, 8 Nov 2021, BALATON Zoltan wrote:
> On Mon, 8 Nov 2021, Paolo Bonzini wrote:
>> On 11/8/21 15:30, Paolo Bonzini wrote:
>>> On 11/8/21 14:05, BALATON Zoltan wrote:
>>>> When using ACPI on big endian machine (such as ppc/pegasos2 which has
>>>> a VT8231 south bridge with ACPI) writes to ACPI registers come out
>>>> byte swapped. This may be caused by a bug in memory subsystem but
>>>> until that is fixed setting the ACPI memory regions to native endian
>>>> makes it usable for big endian machines. This fixes ACPI shutdown with
>>>> pegasos2 when using the board firmware for now.
>>>> This could be reverted when the memory layer is fixed.
>>> 
>>> What is the path to the swapped writes?  Even just a backtrace might be 
>>> enough to understand what's going on, and especially where the bug is.
>> 
>> Ok, Michael pointed me at 
>> https://lore.kernel.org/all/20211011080528-mutt-send-email-mst@kernel.org/.

Ping? I haven't seen an alternative fix yet. If you don't have time now 
this could be postponed to next version with the native endian work around 
for now.

Regards,
BALATON Zoltan

> I was about to say that here's the original thread:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01972.html
>
> and here's the backtrace:
>
> #0  acpi_pm1_cnt_write (val=40, ar=0x55555695f340) at ../hw/acpi/core.c:556
> #1  acpi_pm_cnt_write (opaque=0x55555695f340, addr=1, val=40, width=2) at 
> ../hw/acpi/core.c:602
> #2  0x0000555555b3a82f in memory_region_write_accessor
>    (mr=mr@entry=0x55555695f590, addr=1, value=value@entry=0x7fffefffdd08, 
> size=size@entry=2, shift=<optimized out>, mask=mask@entry=65535, attrs=...)
>    at ../softmmu/memory.c:492
> #3  0x0000555555b3813e in access_with_adjusted_size
>    (addr=addr@entry=1, value=value@entry=0x7fffefffdd08, size=size@entry=1, 
> access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn=
>    0x555555b3a7b0 <memory_region_write_accessor>, mr=0x55555695f590, 
> attrs=...) at ../softmmu/memory.c:554
> #4  0x0000555555b3c449 in memory_region_dispatch_write 
> (mr=mr@entry=0x55555695f590, addr=1, data=<optimized out>, op=<optimized 
> out>, attrs=attrs@entry=...)
>    at ../softmmu/memory.c:1511
> #5  0x0000555555b2c121 in flatview_write_continue
>    (fv=fv@entry=0x7fff84d23b30, addr=addr@entry=4261416709, 
> attrs=attrs@entry=..., ptr=ptr@entry=0x7fffefffdec0, len=len@entry=1, 
> addr1=<optimized out>,
>     l=<optimized out>, mr=0x55555695f590) at host-utils.h:165
> #6  0x0000555555b2c399 in flatview_write (len=1, buf=0x7fffefffdec0, 
> attrs=..., addr=4261416709, fv=0x7fff84d23b30) at ../softmmu/physmem.c:2822
> #7  subpage_write (opaque=<optimized out>, addr=<optimized out>, 
> value=<optimized out>, len=1, attrs=...) at ../softmmu/physmem.c:2488
> #8  0x0000555555b380de in access_with_adjusted_size
>    (addr=addr@entry=3845, value=value@entry=0x7fffefffdf88, 
> size=size@entry=1, access_size_min=<optimized out>, 
> access_size_max=<optimized out>, access_fn=
>    0x555555b3aa80 <memory_region_write_with_attrs_accessor>, 
> mr=0x7fff84710bb0, attrs=...) at ../softmmu/memory.c:549
> #9  0x0000555555b3c449 in memory_region_dispatch_write 
> (mr=mr@entry=0x7fff84710bb0, addr=addr@entry=3845, data=<optimized out>, 
> data@entry=40, op=op@entry=MO_8, attrs=...)
>    at ../softmmu/memory.c:1511
> #10 0x0000555555c07b4c in io_writex
>    (env=env@entry=0x55555666a820, 
> iotlbentry=iotlbentry@entry=0x7fff843367f0, mmu_idx=1, val=val@entry=40, 
> addr=addr@entry=4261416709,
>     retaddr=retaddr@entry=140736116523268, op=MO_8) at 
> ../accel/tcg/cputlb.c:1420
> #11 0x0000555555c0b5df in store_helper (op=MO_8, retaddr=<optimized out>, 
> oi=<optimized out>, val=40, addr=4261416709, env=0x55555666a820) at 
> ../accel/tcg/cputlb.c:2355
> #12 full_stb_mmu (env=0x55555666a820, addr=4261416709, val=40, oi=<optimized 
> out>, retaddr=140736116523268) at ../accel/tcg/cputlb.c:2404
> #13 0x00007fffae3b8104 in code_gen_buffer ()
> #14 0x0000555555bfcfab in cpu_tb_exec (cpu=cpu@entry=0x555556661360, 
> itb=itb@entry=0x7fffae3b7fc0 <code_gen_buffer+56197011>, 
> tb_exit=tb_exit@entry=0x7fffefffe668)
>    at ../accel/tcg/cpu-exec.c:357
> #15 0x0000555555bfe089 in cpu_loop_exec_tb (tb_exit=0x7fffefffe668, 
> last_tb=<synthetic pointer>, tb=0x7fffae3b7fc0 <code_gen_buffer+56197011>, 
> cpu=0x555556661360)
>    at ../accel/tcg/cpu-exec.c:833
> #16 cpu_exec (cpu=cpu@entry=0x555556661360) at ../accel/tcg/cpu-exec.c:992
> #17 0x0000555555c1bba0 in tcg_cpus_exec (cpu=cpu@entry=0x555556661360) at 
> ../accel/tcg/tcg-accel-ops.c:67
> #18 0x0000555555c1c3d7 in rr_cpu_thread_fn (arg=arg@entry=0x555556661360) at 
> ../accel/tcg/tcg-accel-ops-rr.c:214
> #19 0x0000555555d5c049 in qemu_thread_start (args=0x7fffefffe750) at 
> ../util/qemu-thread-posix.c:556
> #20 0x00007ffff6a95dea in start_thread () at /lib64/libpthread.so.0
> #21 0x00007ffff69c8fdf in clone () at /lib64/libc.so.6
>

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

[Michael S. Tsirkin]

ping

On Mon, Nov 08, 2021 at 08:33:01AM -0500, Michael S. Tsirkin wrote:
> On Mon, Nov 08, 2021 at 02:05:42PM +0100, BALATON Zoltan wrote:
> > When using ACPI on big endian machine (such as ppc/pegasos2 which has
> > a VT8231 south bridge with ACPI) writes to ACPI registers come out
> > byte swapped. This may be caused by a bug in memory subsystem but
> > until that is fixed setting the ACPI memory regions to native endian
> > makes it usable for big endian machines. This fixes ACPI shutdown with
> > pegasos2 when using the board firmware for now.
> > This could be reverted when the memory layer is fixed.
> > 
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> 
> 
> Paolo, could you weight in on whether we can fix it properly
> in the memory core? I suspect it's not a good idea to switch
> to native without adding a bunch of byteswaps all
> over the place ...
> 
> > ---
> >  hw/acpi/core.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > index 1e004d0078..543e4a7875 100644
> > --- a/hw/acpi/core.c
> > +++ b/hw/acpi/core.c
> > @@ -461,7 +461,7 @@ static const MemoryRegionOps acpi_pm_evt_ops = {
> >      .impl.min_access_size = 2,
> >      .valid.min_access_size = 1,
> >      .valid.max_access_size = 2,
> > -    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> >  };
> >  
> >  void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
> > @@ -531,7 +531,7 @@ static const MemoryRegionOps acpi_pm_tmr_ops = {
> >      .impl.min_access_size = 4,
> >      .valid.min_access_size = 1,
> >      .valid.max_access_size = 4,
> > -    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> >  };
> >  
> >  void acpi_pm_tmr_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
> > @@ -608,7 +608,7 @@ static const MemoryRegionOps acpi_pm_cnt_ops = {
> >      .impl.min_access_size = 2,
> >      .valid.min_access_size = 1,
> >      .valid.max_access_size = 2,
> > -    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> >  };
> >  
> >  void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
> > -- 
> > 2.30.2

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

[Michael S. Tsirkin]

On Sat, Nov 13, 2021 at 07:47:20PM +0100, BALATON Zoltan wrote:
> On Mon, 8 Nov 2021, BALATON Zoltan wrote:
> > On Mon, 8 Nov 2021, Paolo Bonzini wrote:
> > > On 11/8/21 15:30, Paolo Bonzini wrote:
> > > > On 11/8/21 14:05, BALATON Zoltan wrote:
> > > > > When using ACPI on big endian machine (such as ppc/pegasos2 which has
> > > > > a VT8231 south bridge with ACPI) writes to ACPI registers come out
> > > > > byte swapped. This may be caused by a bug in memory subsystem but
> > > > > until that is fixed setting the ACPI memory regions to native endian
> > > > > makes it usable for big endian machines. This fixes ACPI shutdown with
> > > > > pegasos2 when using the board firmware for now.
> > > > > This could be reverted when the memory layer is fixed.
> > > > 
> > > > What is the path to the swapped writes?  Even just a backtrace
> > > > might be enough to understand what's going on, and especially
> > > > where the bug is.
> > > 
> > > Ok, Michael pointed me at https://lore.kernel.org/all/20211011080528-mutt-send-email-mst@kernel.org/.
> 
> Ping? I haven't seen an alternative fix yet. If you don't have time now this
> could be postponed to next version with the native endian work around for
> now.
> 
> Regards,
> BALATON Zoltan

Paolo, ping?

> > I was about to say that here's the original thread:
> > 
> > https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01972.html
> > 
> > and here's the backtrace:
> > 
> > #0  acpi_pm1_cnt_write (val=40, ar=0x55555695f340) at ../hw/acpi/core.c:556
> > #1  acpi_pm_cnt_write (opaque=0x55555695f340, addr=1, val=40, width=2)
> > at ../hw/acpi/core.c:602
> > #2  0x0000555555b3a82f in memory_region_write_accessor
> >    (mr=mr@entry=0x55555695f590, addr=1,
> > value=value@entry=0x7fffefffdd08, size=size@entry=2, shift=<optimized
> > out>, mask=mask@entry=65535, attrs=...)
> >    at ../softmmu/memory.c:492
> > #3  0x0000555555b3813e in access_with_adjusted_size
> >    (addr=addr@entry=1, value=value@entry=0x7fffefffdd08,
> > size=size@entry=1, access_size_min=<optimized out>,
> > access_size_max=<optimized out>, access_fn=
> >    0x555555b3a7b0 <memory_region_write_accessor>, mr=0x55555695f590,
> > attrs=...) at ../softmmu/memory.c:554
> > #4  0x0000555555b3c449 in memory_region_dispatch_write
> > (mr=mr@entry=0x55555695f590, addr=1, data=<optimized out>, op=<optimized
> > out>, attrs=attrs@entry=...)
> >    at ../softmmu/memory.c:1511
> > #5  0x0000555555b2c121 in flatview_write_continue
> >    (fv=fv@entry=0x7fff84d23b30, addr=addr@entry=4261416709,
> > attrs=attrs@entry=..., ptr=ptr@entry=0x7fffefffdec0, len=len@entry=1,
> > addr1=<optimized out>,
> >     l=<optimized out>, mr=0x55555695f590) at host-utils.h:165
> > #6  0x0000555555b2c399 in flatview_write (len=1, buf=0x7fffefffdec0,
> > attrs=..., addr=4261416709, fv=0x7fff84d23b30) at
> > ../softmmu/physmem.c:2822
> > #7  subpage_write (opaque=<optimized out>, addr=<optimized out>,
> > value=<optimized out>, len=1, attrs=...) at ../softmmu/physmem.c:2488
> > #8  0x0000555555b380de in access_with_adjusted_size
> >    (addr=addr@entry=3845, value=value@entry=0x7fffefffdf88,
> > size=size@entry=1, access_size_min=<optimized out>,
> > access_size_max=<optimized out>, access_fn=
> >    0x555555b3aa80 <memory_region_write_with_attrs_accessor>,
> > mr=0x7fff84710bb0, attrs=...) at ../softmmu/memory.c:549
> > #9  0x0000555555b3c449 in memory_region_dispatch_write
> > (mr=mr@entry=0x7fff84710bb0, addr=addr@entry=3845, data=<optimized out>,
> > data@entry=40, op=op@entry=MO_8, attrs=...)
> >    at ../softmmu/memory.c:1511
> > #10 0x0000555555c07b4c in io_writex
> >    (env=env@entry=0x55555666a820,
> > iotlbentry=iotlbentry@entry=0x7fff843367f0, mmu_idx=1, val=val@entry=40,
> > addr=addr@entry=4261416709,
> >     retaddr=retaddr@entry=140736116523268, op=MO_8) at
> > ../accel/tcg/cputlb.c:1420
> > #11 0x0000555555c0b5df in store_helper (op=MO_8, retaddr=<optimized
> > out>, oi=<optimized out>, val=40, addr=4261416709, env=0x55555666a820)
> > at ../accel/tcg/cputlb.c:2355
> > #12 full_stb_mmu (env=0x55555666a820, addr=4261416709, val=40,
> > oi=<optimized out>, retaddr=140736116523268) at
> > ../accel/tcg/cputlb.c:2404
> > #13 0x00007fffae3b8104 in code_gen_buffer ()
> > #14 0x0000555555bfcfab in cpu_tb_exec (cpu=cpu@entry=0x555556661360,
> > itb=itb@entry=0x7fffae3b7fc0 <code_gen_buffer+56197011>,
> > tb_exit=tb_exit@entry=0x7fffefffe668)
> >    at ../accel/tcg/cpu-exec.c:357
> > #15 0x0000555555bfe089 in cpu_loop_exec_tb (tb_exit=0x7fffefffe668,
> > last_tb=<synthetic pointer>, tb=0x7fffae3b7fc0
> > <code_gen_buffer+56197011>, cpu=0x555556661360)
> >    at ../accel/tcg/cpu-exec.c:833
> > #16 cpu_exec (cpu=cpu@entry=0x555556661360) at ../accel/tcg/cpu-exec.c:992
> > #17 0x0000555555c1bba0 in tcg_cpus_exec (cpu=cpu@entry=0x555556661360)
> > at ../accel/tcg/tcg-accel-ops.c:67
> > #18 0x0000555555c1c3d7 in rr_cpu_thread_fn
> > (arg=arg@entry=0x555556661360) at ../accel/tcg/tcg-accel-ops-rr.c:214
> > #19 0x0000555555d5c049 in qemu_thread_start (args=0x7fffefffe750) at
> > ../util/qemu-thread-posix.c:556
> > #20 0x00007ffff6a95dea in start_thread () at /lib64/libpthread.so.0
> > #21 0x00007ffff69c8fdf in clone () at /lib64/libc.so.6
> > 

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

[Michael S. Tsirkin]

On Wed, Jan 19, 2022 at 04:19:14AM -0500, Michael S. Tsirkin wrote:
> On Sat, Nov 13, 2021 at 07:47:20PM +0100, BALATON Zoltan wrote:
> > On Mon, 8 Nov 2021, BALATON Zoltan wrote:
> > > On Mon, 8 Nov 2021, Paolo Bonzini wrote:
> > > > On 11/8/21 15:30, Paolo Bonzini wrote:
> > > > > On 11/8/21 14:05, BALATON Zoltan wrote:
> > > > > > When using ACPI on big endian machine (such as ppc/pegasos2 which has
> > > > > > a VT8231 south bridge with ACPI) writes to ACPI registers come out
> > > > > > byte swapped. This may be caused by a bug in memory subsystem but
> > > > > > until that is fixed setting the ACPI memory regions to native endian
> > > > > > makes it usable for big endian machines. This fixes ACPI shutdown with
> > > > > > pegasos2 when using the board firmware for now.
> > > > > > This could be reverted when the memory layer is fixed.
> > > > > 
> > > > > What is the path to the swapped writes?  Even just a backtrace
> > > > > might be enough to understand what's going on, and especially
> > > > > where the bug is.
> > > > 
> > > > Ok, Michael pointed me at https://lore.kernel.org/all/20211011080528-mutt-send-email-mst@kernel.org/.
> > 
> > Ping? I haven't seen an alternative fix yet. If you don't have time now this
> > could be postponed to next version with the native endian work around for
> > now.
> > 
> > Regards,
> > BALATON Zoltan
> 
> Paolo, ping?

ping

> > > I was about to say that here's the original thread:
> > > 
> > > https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01972.html
> > > 
> > > and here's the backtrace:
> > > 
> > > #0  acpi_pm1_cnt_write (val=40, ar=0x55555695f340) at ../hw/acpi/core.c:556
> > > #1  acpi_pm_cnt_write (opaque=0x55555695f340, addr=1, val=40, width=2)
> > > at ../hw/acpi/core.c:602
> > > #2  0x0000555555b3a82f in memory_region_write_accessor
> > >    (mr=mr@entry=0x55555695f590, addr=1,
> > > value=value@entry=0x7fffefffdd08, size=size@entry=2, shift=<optimized
> > > out>, mask=mask@entry=65535, attrs=...)
> > >    at ../softmmu/memory.c:492
> > > #3  0x0000555555b3813e in access_with_adjusted_size
> > >    (addr=addr@entry=1, value=value@entry=0x7fffefffdd08,
> > > size=size@entry=1, access_size_min=<optimized out>,
> > > access_size_max=<optimized out>, access_fn=
> > >    0x555555b3a7b0 <memory_region_write_accessor>, mr=0x55555695f590,
> > > attrs=...) at ../softmmu/memory.c:554
> > > #4  0x0000555555b3c449 in memory_region_dispatch_write
> > > (mr=mr@entry=0x55555695f590, addr=1, data=<optimized out>, op=<optimized
> > > out>, attrs=attrs@entry=...)
> > >    at ../softmmu/memory.c:1511
> > > #5  0x0000555555b2c121 in flatview_write_continue
> > >    (fv=fv@entry=0x7fff84d23b30, addr=addr@entry=4261416709,
> > > attrs=attrs@entry=..., ptr=ptr@entry=0x7fffefffdec0, len=len@entry=1,
> > > addr1=<optimized out>,
> > >     l=<optimized out>, mr=0x55555695f590) at host-utils.h:165
> > > #6  0x0000555555b2c399 in flatview_write (len=1, buf=0x7fffefffdec0,
> > > attrs=..., addr=4261416709, fv=0x7fff84d23b30) at
> > > ../softmmu/physmem.c:2822
> > > #7  subpage_write (opaque=<optimized out>, addr=<optimized out>,
> > > value=<optimized out>, len=1, attrs=...) at ../softmmu/physmem.c:2488
> > > #8  0x0000555555b380de in access_with_adjusted_size
> > >    (addr=addr@entry=3845, value=value@entry=0x7fffefffdf88,
> > > size=size@entry=1, access_size_min=<optimized out>,
> > > access_size_max=<optimized out>, access_fn=
> > >    0x555555b3aa80 <memory_region_write_with_attrs_accessor>,
> > > mr=0x7fff84710bb0, attrs=...) at ../softmmu/memory.c:549
> > > #9  0x0000555555b3c449 in memory_region_dispatch_write
> > > (mr=mr@entry=0x7fff84710bb0, addr=addr@entry=3845, data=<optimized out>,
> > > data@entry=40, op=op@entry=MO_8, attrs=...)
> > >    at ../softmmu/memory.c:1511
> > > #10 0x0000555555c07b4c in io_writex
> > >    (env=env@entry=0x55555666a820,
> > > iotlbentry=iotlbentry@entry=0x7fff843367f0, mmu_idx=1, val=val@entry=40,
> > > addr=addr@entry=4261416709,
> > >     retaddr=retaddr@entry=140736116523268, op=MO_8) at
> > > ../accel/tcg/cputlb.c:1420
> > > #11 0x0000555555c0b5df in store_helper (op=MO_8, retaddr=<optimized
> > > out>, oi=<optimized out>, val=40, addr=4261416709, env=0x55555666a820)
> > > at ../accel/tcg/cputlb.c:2355
> > > #12 full_stb_mmu (env=0x55555666a820, addr=4261416709, val=40,
> > > oi=<optimized out>, retaddr=140736116523268) at
> > > ../accel/tcg/cputlb.c:2404
> > > #13 0x00007fffae3b8104 in code_gen_buffer ()
> > > #14 0x0000555555bfcfab in cpu_tb_exec (cpu=cpu@entry=0x555556661360,
> > > itb=itb@entry=0x7fffae3b7fc0 <code_gen_buffer+56197011>,
> > > tb_exit=tb_exit@entry=0x7fffefffe668)
> > >    at ../accel/tcg/cpu-exec.c:357
> > > #15 0x0000555555bfe089 in cpu_loop_exec_tb (tb_exit=0x7fffefffe668,
> > > last_tb=<synthetic pointer>, tb=0x7fffae3b7fc0
> > > <code_gen_buffer+56197011>, cpu=0x555556661360)
> > >    at ../accel/tcg/cpu-exec.c:833
> > > #16 cpu_exec (cpu=cpu@entry=0x555556661360) at ../accel/tcg/cpu-exec.c:992
> > > #17 0x0000555555c1bba0 in tcg_cpus_exec (cpu=cpu@entry=0x555556661360)
> > > at ../accel/tcg/tcg-accel-ops.c:67
> > > #18 0x0000555555c1c3d7 in rr_cpu_thread_fn
> > > (arg=arg@entry=0x555556661360) at ../accel/tcg/tcg-accel-ops-rr.c:214
> > > #19 0x0000555555d5c049 in qemu_thread_start (args=0x7fffefffe750) at
> > > ../util/qemu-thread-posix.c:556
> > > #20 0x00007ffff6a95dea in start_thread () at /lib64/libpthread.so.0
> > > #21 0x00007ffff69c8fdf in clone () at /lib64/libc.so.6
> > > 
> 

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

[BALATON Zoltan]

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

On Tue, 22 Feb 2022, Michael S. Tsirkin wrote:
> On Wed, Jan 19, 2022 at 04:19:14AM -0500, Michael S. Tsirkin wrote:
>> On Sat, Nov 13, 2021 at 07:47:20PM +0100, BALATON Zoltan wrote:
>>> On Mon, 8 Nov 2021, BALATON Zoltan wrote:
>>>> On Mon, 8 Nov 2021, Paolo Bonzini wrote:
>>>>> On 11/8/21 15:30, Paolo Bonzini wrote:
>>>>>> On 11/8/21 14:05, BALATON Zoltan wrote:
>>>>>>> When using ACPI on big endian machine (such as ppc/pegasos2 which has
>>>>>>> a VT8231 south bridge with ACPI) writes to ACPI registers come out
>>>>>>> byte swapped. This may be caused by a bug in memory subsystem but
>>>>>>> until that is fixed setting the ACPI memory regions to native endian
>>>>>>> makes it usable for big endian machines. This fixes ACPI shutdown with
>>>>>>> pegasos2 when using the board firmware for now.
>>>>>>> This could be reverted when the memory layer is fixed.
>>>>>>
>>>>>> What is the path to the swapped writes?  Even just a backtrace
>>>>>> might be enough to understand what's going on, and especially
>>>>>> where the bug is.
>>>>>
>>>>> Ok, Michael pointed me at https://lore.kernel.org/all/20211011080528-mutt-send-email-mst@kernel.org/.
>>>
>>> Ping? I haven't seen an alternative fix yet. If you don't have time now this
>>> could be postponed to next version with the native endian work around for
>>> now.
>>>
>>> Regards,
>>> BALATON Zoltan
>>
>> Paolo, ping?
>
> ping

Can this be fixed please or my proposed workaround taken until it will be? 
Original patch I've sent is here:
http://patchew.org/QEMU/20211108130934.59B48748F52@zero.eik.bme.hu/

I hope to make pegasos2 more usable in next release so maybe more people 
will want to use it soon.

Regards,
BALATON Zoltan

>>>> I was about to say that here's the original thread:
>>>>
>>>> https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01972.html
>>>>
>>>> and here's the backtrace:
>>>>
>>>> #0  acpi_pm1_cnt_write (val=40, ar=0x55555695f340) at ../hw/acpi/core.c:556
>>>> #1  acpi_pm_cnt_write (opaque=0x55555695f340, addr=1, val=40, width=2)
>>>> at ../hw/acpi/core.c:602
>>>> #2  0x0000555555b3a82f in memory_region_write_accessor
>>>>    (mr=mr@entry=0x55555695f590, addr=1,
>>>> value=value@entry=0x7fffefffdd08, size=size@entry=2, shift=<optimized
>>>> out>, mask=mask@entry=65535, attrs=...)
>>>>    at ../softmmu/memory.c:492
>>>> #3  0x0000555555b3813e in access_with_adjusted_size
>>>>    (addr=addr@entry=1, value=value@entry=0x7fffefffdd08,
>>>> size=size@entry=1, access_size_min=<optimized out>,
>>>> access_size_max=<optimized out>, access_fn=
>>>>    0x555555b3a7b0 <memory_region_write_accessor>, mr=0x55555695f590,
>>>> attrs=...) at ../softmmu/memory.c:554
>>>> #4  0x0000555555b3c449 in memory_region_dispatch_write
>>>> (mr=mr@entry=0x55555695f590, addr=1, data=<optimized out>, op=<optimized
>>>> out>, attrs=attrs@entry=...)
>>>>    at ../softmmu/memory.c:1511
>>>> #5  0x0000555555b2c121 in flatview_write_continue
>>>>    (fv=fv@entry=0x7fff84d23b30, addr=addr@entry=4261416709,
>>>> attrs=attrs@entry=..., ptr=ptr@entry=0x7fffefffdec0, len=len@entry=1,
>>>> addr1=<optimized out>,
>>>>     l=<optimized out>, mr=0x55555695f590) at host-utils.h:165
>>>> #6  0x0000555555b2c399 in flatview_write (len=1, buf=0x7fffefffdec0,
>>>> attrs=..., addr=4261416709, fv=0x7fff84d23b30) at
>>>> ../softmmu/physmem.c:2822
>>>> #7  subpage_write (opaque=<optimized out>, addr=<optimized out>,
>>>> value=<optimized out>, len=1, attrs=...) at ../softmmu/physmem.c:2488
>>>> #8  0x0000555555b380de in access_with_adjusted_size
>>>>    (addr=addr@entry=3845, value=value@entry=0x7fffefffdf88,
>>>> size=size@entry=1, access_size_min=<optimized out>,
>>>> access_size_max=<optimized out>, access_fn=
>>>>    0x555555b3aa80 <memory_region_write_with_attrs_accessor>,
>>>> mr=0x7fff84710bb0, attrs=...) at ../softmmu/memory.c:549
>>>> #9  0x0000555555b3c449 in memory_region_dispatch_write
>>>> (mr=mr@entry=0x7fff84710bb0, addr=addr@entry=3845, data=<optimized out>,
>>>> data@entry=40, op=op@entry=MO_8, attrs=...)
>>>>    at ../softmmu/memory.c:1511
>>>> #10 0x0000555555c07b4c in io_writex
>>>>    (env=env@entry=0x55555666a820,
>>>> iotlbentry=iotlbentry@entry=0x7fff843367f0, mmu_idx=1, val=val@entry=40,
>>>> addr=addr@entry=4261416709,
>>>>     retaddr=retaddr@entry=140736116523268, op=MO_8) at
>>>> ../accel/tcg/cputlb.c:1420
>>>> #11 0x0000555555c0b5df in store_helper (op=MO_8, retaddr=<optimized
>>>> out>, oi=<optimized out>, val=40, addr=4261416709, env=0x55555666a820)
>>>> at ../accel/tcg/cputlb.c:2355
>>>> #12 full_stb_mmu (env=0x55555666a820, addr=4261416709, val=40,
>>>> oi=<optimized out>, retaddr=140736116523268) at
>>>> ../accel/tcg/cputlb.c:2404
>>>> #13 0x00007fffae3b8104 in code_gen_buffer ()
>>>> #14 0x0000555555bfcfab in cpu_tb_exec (cpu=cpu@entry=0x555556661360,
>>>> itb=itb@entry=0x7fffae3b7fc0 <code_gen_buffer+56197011>,
>>>> tb_exit=tb_exit@entry=0x7fffefffe668)
>>>>    at ../accel/tcg/cpu-exec.c:357
>>>> #15 0x0000555555bfe089 in cpu_loop_exec_tb (tb_exit=0x7fffefffe668,
>>>> last_tb=<synthetic pointer>, tb=0x7fffae3b7fc0
>>>> <code_gen_buffer+56197011>, cpu=0x555556661360)
>>>>    at ../accel/tcg/cpu-exec.c:833
>>>> #16 cpu_exec (cpu=cpu@entry=0x555556661360) at ../accel/tcg/cpu-exec.c:992
>>>> #17 0x0000555555c1bba0 in tcg_cpus_exec (cpu=cpu@entry=0x555556661360)
>>>> at ../accel/tcg/tcg-accel-ops.c:67
>>>> #18 0x0000555555c1c3d7 in rr_cpu_thread_fn
>>>> (arg=arg@entry=0x555556661360) at ../accel/tcg/tcg-accel-ops-rr.c:214
>>>> #19 0x0000555555d5c049 in qemu_thread_start (args=0x7fffefffe750) at
>>>> ../util/qemu-thread-posix.c:556
>>>> #20 0x00007ffff6a95dea in start_thread () at /lib64/libpthread.so.0
>>>> #21 0x00007ffff69c8fdf in clone () at /lib64/libc.so.6
>>>>
>>
>
>

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

[Michael S. Tsirkin]

On Mon, Feb 20, 2023 at 07:24:59PM +0100, BALATON Zoltan wrote:
> On Tue, 22 Feb 2022, Michael S. Tsirkin wrote:
> > On Wed, Jan 19, 2022 at 04:19:14AM -0500, Michael S. Tsirkin wrote:
> > > On Sat, Nov 13, 2021 at 07:47:20PM +0100, BALATON Zoltan wrote:
> > > > On Mon, 8 Nov 2021, BALATON Zoltan wrote:
> > > > > On Mon, 8 Nov 2021, Paolo Bonzini wrote:
> > > > > > On 11/8/21 15:30, Paolo Bonzini wrote:
> > > > > > > On 11/8/21 14:05, BALATON Zoltan wrote:
> > > > > > > > When using ACPI on big endian machine (such as ppc/pegasos2 which has
> > > > > > > > a VT8231 south bridge with ACPI) writes to ACPI registers come out
> > > > > > > > byte swapped. This may be caused by a bug in memory subsystem but
> > > > > > > > until that is fixed setting the ACPI memory regions to native endian
> > > > > > > > makes it usable for big endian machines. This fixes ACPI shutdown with
> > > > > > > > pegasos2 when using the board firmware for now.
> > > > > > > > This could be reverted when the memory layer is fixed.
> > > > > > > 
> > > > > > > What is the path to the swapped writes?  Even just a backtrace
> > > > > > > might be enough to understand what's going on, and especially
> > > > > > > where the bug is.
> > > > > > 
> > > > > > Ok, Michael pointed me at https://lore.kernel.org/all/20211011080528-mutt-send-email-mst@kernel.org/.
> > > > 
> > > > Ping? I haven't seen an alternative fix yet. If you don't have time now this
> > > > could be postponed to next version with the native endian work around for
> > > > now.
> > > > 
> > > > Regards,
> > > > BALATON Zoltan
> > > 
> > > Paolo, ping?
> > 
> > ping
> 
> Can this be fixed please or my proposed workaround taken until it will be?
> Original patch I've sent is here:
> http://patchew.org/QEMU/20211108130934.59B48748F52@zero.eik.bme.hu/
> 
> I hope to make pegasos2 more usable in next release so maybe more people
> will want to use it soon.
> 
> Regards,
> BALATON Zoltan

Any chance of fixing it in memory core? No one else seems to care.


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 was about to say that here's the original thread:
> > > > > 
> > > > > https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01972.html
> > > > > 
> > > > > and here's the backtrace:
> > > > > 
> > > > > #0  acpi_pm1_cnt_write (val=40, ar=0x55555695f340) at ../hw/acpi/core.c:556
> > > > > #1  acpi_pm_cnt_write (opaque=0x55555695f340, addr=1, val=40, width=2)
> > > > > at ../hw/acpi/core.c:602
> > > > > #2  0x0000555555b3a82f in memory_region_write_accessor
> > > > >    (mr=mr@entry=0x55555695f590, addr=1,
> > > > > value=value@entry=0x7fffefffdd08, size=size@entry=2, shift=<optimized
> > > > > out>, mask=mask@entry=65535, attrs=...)
> > > > >    at ../softmmu/memory.c:492
> > > > > #3  0x0000555555b3813e in access_with_adjusted_size
> > > > >    (addr=addr@entry=1, value=value@entry=0x7fffefffdd08,
> > > > > size=size@entry=1, access_size_min=<optimized out>,
> > > > > access_size_max=<optimized out>, access_fn=
> > > > >    0x555555b3a7b0 <memory_region_write_accessor>, mr=0x55555695f590,
> > > > > attrs=...) at ../softmmu/memory.c:554
> > > > > #4  0x0000555555b3c449 in memory_region_dispatch_write
> > > > > (mr=mr@entry=0x55555695f590, addr=1, data=<optimized out>, op=<optimized
> > > > > out>, attrs=attrs@entry=...)
> > > > >    at ../softmmu/memory.c:1511
> > > > > #5  0x0000555555b2c121 in flatview_write_continue
> > > > >    (fv=fv@entry=0x7fff84d23b30, addr=addr@entry=4261416709,
> > > > > attrs=attrs@entry=..., ptr=ptr@entry=0x7fffefffdec0, len=len@entry=1,
> > > > > addr1=<optimized out>,
> > > > >     l=<optimized out>, mr=0x55555695f590) at host-utils.h:165
> > > > > #6  0x0000555555b2c399 in flatview_write (len=1, buf=0x7fffefffdec0,
> > > > > attrs=..., addr=4261416709, fv=0x7fff84d23b30) at
> > > > > ../softmmu/physmem.c:2822
> > > > > #7  subpage_write (opaque=<optimized out>, addr=<optimized out>,
> > > > > value=<optimized out>, len=1, attrs=...) at ../softmmu/physmem.c:2488
> > > > > #8  0x0000555555b380de in access_with_adjusted_size
> > > > >    (addr=addr@entry=3845, value=value@entry=0x7fffefffdf88,
> > > > > size=size@entry=1, access_size_min=<optimized out>,
> > > > > access_size_max=<optimized out>, access_fn=
> > > > >    0x555555b3aa80 <memory_region_write_with_attrs_accessor>,
> > > > > mr=0x7fff84710bb0, attrs=...) at ../softmmu/memory.c:549
> > > > > #9  0x0000555555b3c449 in memory_region_dispatch_write
> > > > > (mr=mr@entry=0x7fff84710bb0, addr=addr@entry=3845, data=<optimized out>,
> > > > > data@entry=40, op=op@entry=MO_8, attrs=...)
> > > > >    at ../softmmu/memory.c:1511
> > > > > #10 0x0000555555c07b4c in io_writex
> > > > >    (env=env@entry=0x55555666a820,
> > > > > iotlbentry=iotlbentry@entry=0x7fff843367f0, mmu_idx=1, val=val@entry=40,
> > > > > addr=addr@entry=4261416709,
> > > > >     retaddr=retaddr@entry=140736116523268, op=MO_8) at
> > > > > ../accel/tcg/cputlb.c:1420
> > > > > #11 0x0000555555c0b5df in store_helper (op=MO_8, retaddr=<optimized
> > > > > out>, oi=<optimized out>, val=40, addr=4261416709, env=0x55555666a820)
> > > > > at ../accel/tcg/cputlb.c:2355
> > > > > #12 full_stb_mmu (env=0x55555666a820, addr=4261416709, val=40,
> > > > > oi=<optimized out>, retaddr=140736116523268) at
> > > > > ../accel/tcg/cputlb.c:2404
> > > > > #13 0x00007fffae3b8104 in code_gen_buffer ()
> > > > > #14 0x0000555555bfcfab in cpu_tb_exec (cpu=cpu@entry=0x555556661360,
> > > > > itb=itb@entry=0x7fffae3b7fc0 <code_gen_buffer+56197011>,
> > > > > tb_exit=tb_exit@entry=0x7fffefffe668)
> > > > >    at ../accel/tcg/cpu-exec.c:357
> > > > > #15 0x0000555555bfe089 in cpu_loop_exec_tb (tb_exit=0x7fffefffe668,
> > > > > last_tb=<synthetic pointer>, tb=0x7fffae3b7fc0
> > > > > <code_gen_buffer+56197011>, cpu=0x555556661360)
> > > > >    at ../accel/tcg/cpu-exec.c:833
> > > > > #16 cpu_exec (cpu=cpu@entry=0x555556661360) at ../accel/tcg/cpu-exec.c:992
> > > > > #17 0x0000555555c1bba0 in tcg_cpus_exec (cpu=cpu@entry=0x555556661360)
> > > > > at ../accel/tcg/tcg-accel-ops.c:67
> > > > > #18 0x0000555555c1c3d7 in rr_cpu_thread_fn
> > > > > (arg=arg@entry=0x555556661360) at ../accel/tcg/tcg-accel-ops-rr.c:214
> > > > > #19 0x0000555555d5c049 in qemu_thread_start (args=0x7fffefffe750) at
> > > > > ../util/qemu-thread-posix.c:556
> > > > > #20 0x00007ffff6a95dea in start_thread () at /lib64/libpthread.so.0
> > > > > #21 0x00007ffff69c8fdf in clone () at /lib64/libc.so.6
> > > > > 
> > > 
> > 
> > 

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

[BALATON Zoltan]

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

On Mon, 20 Feb 2023, Michael S. Tsirkin wrote:
> On Mon, Feb 20, 2023 at 07:24:59PM +0100, BALATON Zoltan wrote:
>> On Tue, 22 Feb 2022, Michael S. Tsirkin wrote:
>>> On Wed, Jan 19, 2022 at 04:19:14AM -0500, Michael S. Tsirkin wrote:
>>>> On Sat, Nov 13, 2021 at 07:47:20PM +0100, BALATON Zoltan wrote:
>>>>> On Mon, 8 Nov 2021, BALATON Zoltan wrote:
>>>>>> On Mon, 8 Nov 2021, Paolo Bonzini wrote:
>>>>>>> On 11/8/21 15:30, Paolo Bonzini wrote:
>>>>>>>> On 11/8/21 14:05, BALATON Zoltan wrote:
>>>>>>>>> When using ACPI on big endian machine (such as ppc/pegasos2 which has
>>>>>>>>> a VT8231 south bridge with ACPI) writes to ACPI registers come out
>>>>>>>>> byte swapped. This may be caused by a bug in memory subsystem but
>>>>>>>>> until that is fixed setting the ACPI memory regions to native endian
>>>>>>>>> makes it usable for big endian machines. This fixes ACPI shutdown with
>>>>>>>>> pegasos2 when using the board firmware for now.
>>>>>>>>> This could be reverted when the memory layer is fixed.
>>>>>>>>
>>>>>>>> What is the path to the swapped writes?  Even just a backtrace
>>>>>>>> might be enough to understand what's going on, and especially
>>>>>>>> where the bug is.
>>>>>>>
>>>>>>> Ok, Michael pointed me at https://lore.kernel.org/all/20211011080528-mutt-send-email-mst@kernel.org/.
>>>>>
>>>>> Ping? I haven't seen an alternative fix yet. If you don't have time now this
>>>>> could be postponed to next version with the native endian work around for
>>>>> now.
>>>>>
>>>>> Regards,
>>>>> BALATON Zoltan
>>>>
>>>> Paolo, ping?
>>>
>>> ping
>>
>> Can this be fixed please or my proposed workaround taken until it will be?
>> Original patch I've sent is here:
>> http://patchew.org/QEMU/20211108130934.59B48748F52@zero.eik.bme.hu/
>>
>> I hope to make pegasos2 more usable in next release so maybe more people
>> will want to use it soon.
>>
>> Regards,
>> BALATON Zoltan
>
> Any chance of fixing it in memory core? No one else seems to care.
>
>
> 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.

Regards,
BALATON Zoltan

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

[Paolo Bonzini]

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

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

[BALATON Zoltan]

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

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

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

[Paolo Bonzini]

On 2/21/23 13:48, BALATON Zoltan wrote:
> My patch works and don't see what else could it break.

I strongly suspect that your patch, while fixing access to one byte of 
the (2-byte) registers, breaks access to the other byte.

Thanks for the reproduction instructions, I'll take a look.

Paolo

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

[BALATON Zoltan]

On Tue, 21 Feb 2023, Paolo Bonzini wrote:
> On 2/21/23 13:48, BALATON Zoltan wrote:
>> My patch works and don't see what else could it break.
>
> I strongly suspect that your patch, while fixing access to one byte of the 
> (2-byte) registers, breaks access to the other byte.
>
> Thanks for the reproduction instructions, I'll take a look.

Any chance this can still be fixed? As a bugfix we may have one more week 
maybe.

Regards,
BALATON Zoltan

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

[Michael S. Tsirkin]

On Tue, Feb 21, 2023 at 03:02:41PM +0100, Paolo Bonzini wrote:
> On 2/21/23 13:48, BALATON Zoltan wrote:
> > My patch works and don't see what else could it break.
> 
> I strongly suspect that your patch, while fixing access to one byte of the
> (2-byte) registers, breaks access to the other byte.
> 
> Thanks for the reproduction instructions, I'll take a look.
> 
> Paolo

ping?

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

[Michael S. Tsirkin]

On Thu, Mar 02, 2023 at 02:42:17PM +0100, BALATON Zoltan wrote:
> On Tue, 21 Feb 2023, Paolo Bonzini wrote:
> > On 2/21/23 13:48, BALATON Zoltan wrote:
> > > My patch works and don't see what else could it break.
> > 
> > I strongly suspect that your patch, while fixing access to one byte of
> > the (2-byte) registers, breaks access to the other byte.
> > 
> > Thanks for the reproduction instructions, I'll take a look.
> 
> Any chance this can still be fixed? As a bugfix we may have one more week
> maybe.
> 
> Regards,
> BALATON Zoltan

I'm not holding my breath for a fix ATM but I am also not really willing
to change this for everyone and risk breaking weird configs such as BE
hosts.

How about propagating a flag from pegasos saying "enable memory core bug
workaround"?

-- 
MST

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

[Paolo Bonzini]

On 2/21/23 13:55, BALATON Zoltan wrote:
> 
> 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.)

Ok, I've reproduced it.  The mouse is a bit flaky but using the keyboard 
for everything except right clicking works better.

Paolo

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

[BALATON Zoltan]

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

On Mon, 6 Mar 2023, Paolo Bonzini wrote:
> On 2/21/23 13:55, BALATON Zoltan wrote:
>> 
>> 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.)
>
> Ok, I've reproduced it.  The mouse is a bit flaky but using the keyboard for 
> everything except right clicking works better.

If you use ati-vga try -device ati-vga,romfile="",guest_hwcursor=true to 
get less jumpy mouse or -vga none -device sm501 instead of -device ati-vga 
which may also work better. (The default for ati-vga is host hardware 
cursor but if the pointer acceleration settings on host vs. guest are very 
different it may jump around; that's a general problem with QEMU ui mouse 
handling so can't do much about that in device model).

Regards,
BALATON Zoltan

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

[Paolo Bonzini]

On 3/6/23 23:56, Paolo Bonzini wrote:
> On 2/21/23 13:55, BALATON Zoltan wrote:
>>
>> 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.)
> 
> Ok, I've reproduced it.  The mouse is a bit flaky but using the keyboard 
> for everything except right clicking works better.

Now the OS doesn't boot anymore, it doesn't get to the point where it 
initializes the VGA.  I got some quick logs with .impl.min_access_size 
to 1, to understand what the firmware (but not the OS) does.  With this 
at least I could confirm that your patch is wrong:

cnt 1 1 write 80
evt 3 1 write 1         // enable timer
evt 0 2 read
evt 0 2 write 1         // just writes again the same value
evt 1 1 write
evt 1 1 write 0

Since you have both 1-size and 2-size writes, and the 2-byte 
reads/writes are done with byte swapping instructions lhbrx and sthbrx, 
your patch would cause 0x100 to be read and written on the third and 
fourth lines.

Likewise, any 4-byte read of the timer port would be byte swapped.

The solution is a patch similar to mine, applied to both evt and cnt; 
while perhaps tmr can be left as is and only accept 4-byte reads, I 
don't know.  I'll try to come up with something that can be tested at 
least with the firmware.

Paolo

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

[BALATON Zoltan]

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

On Tue, 7 Mar 2023, Paolo Bonzini wrote:
> On 3/6/23 23:56, Paolo Bonzini wrote:
>> On 2/21/23 13:55, BALATON Zoltan wrote:
>>> 
>>> 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.)
>> 
>> Ok, I've reproduced it.  The mouse is a bit flaky but using the keyboard 
>> for everything except right clicking works better.
>
> Now the OS doesn't boot anymore, it doesn't get to the point where it 
> initializes the VGA.

Sorry, that is a known breakage from a recent series that's not yet fixed. 
This patch should fix it:

http://patchew.org/QEMU/cover.1677628524.git.balaton@eik.bme.hu/cdfb3c5a42e505450f6803124f27856434c5b298.1677628524.git.balaton@eik.bme.hu/

or another proposed altenative is here:

http://patchew.org/QEMU/20230304114043.121024-1-shentey@gmail.com/20230304114043.121024-2-shentey@gmail.com/

Either of the above should work.

> I got some quick logs with .impl.min_access_size to 1, 
> to understand what the firmware (but not the OS) does.  With this at least I 
> could confirm that your patch is wrong:
>
> cnt 1 1 write 80
> evt 3 1 write 1         // enable timer
> evt 0 2 read
> evt 0 2 write 1         // just writes again the same value
> evt 1 1 write
> evt 1 1 write 0
>
> Since you have both 1-size and 2-size writes, and the 2-byte reads/writes are 
> done with byte swapping instructions lhbrx and sthbrx, your patch would cause 
> 0x100 to be read and written on the third and fourth lines.
>
> Likewise, any 4-byte read of the timer port would be byte swapped.
>
> The solution is a patch similar to mine, applied to both evt and cnt; while 
> perhaps tmr can be left as is and only accept 4-byte reads, I don't know. 
> I'll try to come up with something that can be tested at least with the 
> firmware.

I'm not sure I follow what you mean so I'd need a patch to see then I can 
test it with the clients I run on pegasos2.

Regards,
BALATON Zoltan

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

[Paolo Bonzini]

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

Il mar 7 mar 2023, 01:06 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:

> I'm not sure I follow what you mean so I'd need a patch to see then I can
> test it with the clients I run on pegasos2.


Do you have a spec, or pointer to the morphos kernel sources, to figure out
how the hardware works?

Paolo


> Regards,
> BALATON Zoltan

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

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

[BALATON Zoltan]

On Tue, 7 Mar 2023, Paolo Bonzini wrote:
> Il mar 7 mar 2023, 01:06 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:
>> I'm not sure I follow what you mean so I'd need a patch to see then I can
>> test it with the clients I run on pegasos2.
>
> Do you have a spec, or pointer to the morphos kernel sources, to figure out
> how the hardware works?

No, that's closed source and only available as a demo iso but it's known 
to work on real hardware and freely downloadable so is a good test. (AFAIK 
those who developed MorphOS had close connection to those who wrote the 
firmware for Pegasos II.) Maybe the VT8231 datasheet or similar parts (we 
only emulate VT82C686B and VT8231 in QEMU) has some info on this.

Regards,
BALATON Zoltan

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

[Paolo Bonzini]

On 3/7/23 11:01, BALATON Zoltan wrote:
>>
>>> I'm not sure I follow what you mean so I'd need a patch to see then I 
>>> can
>>> test it with the clients I run on pegasos2.
>>
>> Do you have a spec, or pointer to the morphos kernel sources, to 
>> figure out how the hardware works?
> 
> No, that's closed source and only available as a demo iso but it's known 
> to work on real hardware and freely downloadable so is a good test. 
> (AFAIK those who developed MorphOS had close connection to those who 
> wrote the firmware for Pegasos II.) Maybe the VT8231 datasheet or 
> similar parts (we only emulate VT82C686B and VT8231 in QEMU) has some 
> info on this.

I agree it's a good test, but it's not clear what it means to do 
sub-word writes to the register.

For example, in the dump I posted you have:

evt 3 1 write 1      // enable timer
evt 0 2 read
evt 0 2 write 1      // just writes again the same value, clearing sts

It's important to note that the comments are just my guess.

Before even looking at the effect of the write, the trace tells us that 
your patch is incomplete.  With both current QEMU and your patch it 
would do nothing because addr is not 0 or 2; but since the firmware of 
your machine does use addr == 3, you need to handle it.  In other words, 
before looking at this trace, I was wary of accepting your patch because 
it made no sense to me; but I couldn't exclude that I was missing 
something.  Now, instead, I am certain it shouldn't be accepted.

I am quite confident that the second guess is correct, because "write 
the same value that was read" only makes sense for evt_sts and not for 
evt_en.  We learnt something: no matter what bus this device sits on, it 
does not flip bit 1 of the address for subword writes.  As I mentioned 
yesterday, we also observe that the load and store use lhbrx and sthbrx. 
  Assuming this is not a harmless bug in the firmware, this means the 
device is indeed little endian.

This means that the first write is almost certainly to evt_en.  On a 
little-endian machine the write would set bit 8 of evt.en (power 
button), but what about a big-endian machine like yours?  Should it set 
bit 0 or bit 8?  If it's bit 0 (which resides at offset 2 according to 
the device), who flips the low bit of the address?  Why is bit 0 flipped 
but not bit 1?

You simply cannot fix the emulation of this machine until you can answer 
the above questions.  If there is no source and no spec, the two ways to 
do it are:

- get a real machine, and figure out whether the write to offset 3 
corresponds to the PM timer or the power button.

- continue the trace up to the point the OS runs, and see if you get 
some clues similar to the one above that placed evt_sts at offset 2.

Paolo

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

[BALATON Zoltan]

On Tue, 7 Mar 2023, Paolo Bonzini wrote:
> On 3/7/23 11:01, BALATON Zoltan wrote:
>>>> I'm not sure I follow what you mean so I'd need a patch to see then I can
>>>> test it with the clients I run on pegasos2.
>>> 
>>> Do you have a spec, or pointer to the morphos kernel sources, to figure 
>>> out how the hardware works?
>> 
>> No, that's closed source and only available as a demo iso but it's known to 
>> work on real hardware and freely downloadable so is a good test. (AFAIK 
>> those who developed MorphOS had close connection to those who wrote the 
>> firmware for Pegasos II.) Maybe the VT8231 datasheet or similar parts (we 
>> only emulate VT82C686B and VT8231 in QEMU) has some info on this.
>
> I agree it's a good test, but it's not clear what it means to do sub-word 
> writes to the register.
>
> For example, in the dump I posted you have:
>
> evt 3 1 write 1      // enable timer
> evt 0 2 read
> evt 0 2 write 1      // just writes again the same value, clearing sts
>
> It's important to note that the comments are just my guess.
>
> Before even looking at the effect of the write, the trace tells us that your 
> patch is incomplete.  With both current QEMU and your patch it would do 
> nothing because addr is not 0 or 2; but since the firmware of your machine 
> does use addr == 3, you need to handle it.  In other words, before looking at 
> this trace, I was wary of accepting your patch because it made no sense to 
> me; but I couldn't exclude that I was missing something.  Now, instead, I am 
> certain it shouldn't be accepted.

Well at least that question is cleared then.

> I am quite confident that the second guess is correct, because "write the 
> same value that was read" only makes sense for evt_sts and not for evt_en.

It could also make sense if the guest is trying to flip a bit that's 
already set or cleared. It could be that the reset value on QEMU is not 
the same as on hardware or the firmware just runs the same routine on 
reset and cold start and wants to make sure a bit is in a state it wants 
it even if it already is that way so it could read value, unconditionally 
apply mask which in our case does not change the value and write it back.

> We learnt something: no matter what bus this device sits on, it does not flip 
> bit 1 of the address for subword writes.  As I mentioned yesterday, we also 
> observe that the load and store use lhbrx and sthbrx.  Assuming this is not a 
> harmless bug in the firmware, this means the device is indeed little endian.

AFAIU the device is little endian (VT8231 is a PCI device and primarily a 
PC part so it's expected to be little endian) but the guest also knows 
this and writes byte swaped values to it. But since the memory region is 
set to LITTLE_ENDIAN and we're on a big endian CPU QEMU will byte swap it 
again which it should not do as the guest already did that. Setting it to 
NATIVE_ENDIAN solves this and it oes not break anything for little endian 
machines on little endian hosts for sure. The only part I'm not sure about 
is big endian hosts which I don't have any to test.

> This means that the first write is almost certainly to evt_en.  On a 
> little-endian machine the write would set bit 8 of evt.en (power button), but 
> what about a big-endian machine like yours?  Should it set bit 0 or bit 8? 
> If it's bit 0 (which resides at offset 2 according to the device), who flips 
> the low bit of the address?  Why is bit 0 flipped but not bit 1?

I think the guest already writes byte swapped value so it should work the 
same as on little endian machine but the QEMU memory layer gets in the 
way.

> You simply cannot fix the emulation of this machine until you can answer the 
> above questions.  If there is no source and no spec, the two ways to do it 
> are:
>
> - get a real machine, and figure out whether the write to offset 3 
> corresponds to the PM timer or the power button.

I don't have real machine but know somebody who does but I'm not sure what 
to ast to test on it. Can you describe it what you want to see or maybe 
write a sctipt for the fimrware to test it (if you're familiar enough with 
Forth for that). I can try to find some more info on this but so far I was 
concentrating on other bigger issues. This is a minor annoyance but would 
be nice to fix eventually.

Regards,
BALATON Zoltan

> - continue the trace up to the point the OS runs, and see if you get some 
> clues similar to the one above that placed evt_sts at offset 2.

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

[Paolo Bonzini]

On 3/7/23 13:54, BALATON Zoltan wrote:
>> evt 3 1 write 1      // enable timer
>> evt 0 2 read
>> evt 0 2 write 1      // just writes again the same value, clearing sts
>>
>> I am quite confident that the second guess is correct, because "write 
>> the same value that was read" only makes sense for evt_sts and not for 
>> evt_en.
> 
> It could also make sense if the guest is trying to flip a bit that's 
> already set or cleared.

No, I checked what the guest actually does and it's read followed 
immediately by a write, with no other ALU values in the middle.

> AFAIU the device is little endian (VT8231 is a PCI device and primarily 
> a PC part so it's expected to be little endian) but the guest also knows 
> this and writes byte swaped values to it. But since the memory region is 
> set to LITTLE_ENDIAN and we're on a big endian CPU QEMU will byte swap 
> it again which it should not do as the guest already did that.

It's the opposite.

The CPU first swaps the value that was in the register, when it executes 
sthbrx instructions.  With DEVICE_LITTLE_ENDIAN, QEMU does the second 
swap and restores the value that was in the register.  With 
DEVICE_NATIVE_ENDIAN it happens to fix the cases that matter for your 
testcase, but it breaks others.

>> This means that the first write is almost certainly to evt_en.  On a 
>> little-endian machine the write would set bit 8 of evt.en (power 
>> button), but what about a big-endian machine like yours?  Should it 
>> set bit 0 or bit 8? If it's bit 0 (which resides at offset 2 according 
>> to the device), who flips the low bit of the address?  Why is bit 0 
>> flipped but not bit 1?
> 
> I think the guest already writes byte swapped value so it should work 
> the same as on little endian machine but the QEMU memory layer gets in 
> the way.

The write in question is "evt addr=3 size=1 value=1" so it's a one-byte 
write.  There's no byte swapping involved here, rather the question is 
how the addresses are interpreted.

>> - get a real machine, and figure out whether the write to offset 3 
>> corresponds to the PM timer or the power button.
> 
> I don't have real machine but know somebody who does but I'm not sure 
> what to ast to test on it. Can you describe it what you want to see or 
> maybe write a sctipt for the fimrware to test it (if you're familiar 
> enough with Forth for that). I can try to find some more info on this 
> but so far I was concentrating on other bigger issues. This is a minor 
> annoyance but would be nice to fix eventually.

I didn't even have an idea that Forth was involved, honestly, or how to 
write Forth code for this machine that I barely know exists. :)

>> - continue the trace up to the point the OS runs, and see if you get 
>> some clues similar to the one above that placed evt_sts at offset 2.

I'll try this once the machine is back in shape.

Paolo

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

[BALATON Zoltan]

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

On Tue, 7 Mar 2023, Paolo Bonzini wrote:
> On 3/7/23 13:54, BALATON Zoltan wrote:
>>> evt 3 1 write 1      // enable timer
>>> evt 0 2 read
>>> evt 0 2 write 1      // just writes again the same value, clearing sts
>>> 
>>> I am quite confident that the second guess is correct, because "write the 
>>> same value that was read" only makes sense for evt_sts and not for evt_en.
>> 
>> It could also make sense if the guest is trying to flip a bit that's 
>> already set or cleared.
>
> No, I checked what the guest actually does and it's read followed immediately 
> by a write, with no other ALU values in the middle.
>
>> AFAIU the device is little endian (VT8231 is a PCI device and primarily a 
>> PC part so it's expected to be little endian) but the guest also knows this 
>> and writes byte swaped values to it. But since the memory region is set to 
>> LITTLE_ENDIAN and we're on a big endian CPU QEMU will byte swap it again 
>> which it should not do as the guest already did that.
>
> It's the opposite.
>
> The CPU first swaps the value that was in the register, when it executes 
> sthbrx instructions.  With DEVICE_LITTLE_ENDIAN, QEMU does the second swap 
> and restores the value that was in the register.  With DEVICE_NATIVE_ENDIAN 
> it happens to fix the cases that matter for your testcase, but it breaks 
> others.
>
>>> This means that the first write is almost certainly to evt_en.  On a 
>>> little-endian machine the write would set bit 8 of evt.en (power button), 
>>> but what about a big-endian machine like yours?  Should it set bit 0 or 
>>> bit 8? If it's bit 0 (which resides at offset 2 according to the device), 
>>> who flips the low bit of the address?  Why is bit 0 flipped but not bit 1?
>> 
>> I think the guest already writes byte swapped value so it should work the 
>> same as on little endian machine but the QEMU memory layer gets in the way.
>
> The write in question is "evt addr=3 size=1 value=1" so it's a one-byte 
> write.  There's no byte swapping involved here, rather the question is how 
> the addresses are interpreted.
>
>>> - get a real machine, and figure out whether the write to offset 3 
>>> corresponds to the PM timer or the power button.
>> 
>> I don't have real machine but know somebody who does but I'm not sure what 
>> to ast to test on it. Can you describe it what you want to see or maybe 
>> write a sctipt for the fimrware to test it (if you're familiar enough with 
>> Forth for that). I can try to find some more info on this but so far I was 
>> concentrating on other bigger issues. This is a minor annoyance but would 
>> be nice to fix eventually.
>
> I didn't even have an idea that Forth was involved, honestly, or how to write 
> Forth code for this machine that I barely know exists. :)

It's supposed to be CHRP compatible which mandates OpenFirmware so it 
should work about the same as similar IBM and Apple machines but those are 
also kind of obscure. If you can describe what you want tested I can try 
to make that a script and ask somebody to run it on real machine but I'm 
not sure what to test.

Regards,
BALATON Zoltan

>>> - continue the trace up to the point the OS runs, and see if you get some 
>>> clues similar to the one above that placed evt_sts at offset 2.
>
> I'll try this once the machine is back in shape.
>
> Paolo
>
>

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

[Paolo Bonzini]

On 3/7/23 16:21, BALATON Zoltan wrote:
>> I didn't even have an idea that Forth was involved, honestly, or how 
>> to write Forth code for this machine that I barely know exists. 😄
> 
> It's supposed to be CHRP compatible which mandates OpenFirmware so it 
> should work about the same as similar IBM and Apple machines but those 
> are also kind of obscure. If you can describe what you want tested I can 
> try to make that a script and ask somebody to run it on real machine but 
> I'm not sure what to test.

For example:

- read the PMTIMER, check that it increases when read as little endian

- read individual bytes of the PMTIMER and check which increase faster


Also, since we've probably identified the EN and STS registers:

- write 0 to the supposed 16-bit EN register

- write 1 to the supposed 16-bit EN register

- check that it reads back as 1 (the STS register would not)

- read individual bytes of the 16-bit EN register and check which is the 
low and which is the high one

- write 0x100 to the supposed 16-bit EN register

- check that it reads back as 0x100

- read individual bytes of the 16-bit EN register and confirm that the 
one you identified as "low" now reads 0 and the one you identified as 
"high" now reads 1


Or:

- read the (supposed) 16-bit STS register

- wait for the STS register to be non-zero

- write the read value to the STS register

- read again and check that it's zero

- wait for the STS register to be non-zero

- read the individual bytes of the 16-bit STS register and check which 
is the low and which is the high one

- wait for the STS register to be non-zero

- for each individual byte of the STS register, write it back, and check 
that it now reads to zero

Part of this should already work in QEMU.

Paolo

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

[Michael S. Tsirkin]

On Tue, Feb 21, 2023 at 09:30:34AM +0100, Paolo Bonzini wrote:
> The case of impl.min_access_size < valid.min_access_size is not
> supported in the memory core.

Did you mean > here?

-- 
MST

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

[BALATON Zoltan]

Hello,

Finally could get to write some test script and have somebody with a real 
machine run that so I'm coming back to this with those results.

On Tue, 7 Mar 2023, Paolo Bonzini wrote:
> On 3/7/23 11:01, BALATON Zoltan wrote:
>>>> I'm not sure I follow what you mean so I'd need a patch to see then I can
>>>> test it with the clients I run on pegasos2.
>>> 
>>> Do you have a spec, or pointer to the morphos kernel sources, to figure 
>>> out how the hardware works?
>> 
>> No, that's closed source and only available as a demo iso but it's known to 
>> work on real hardware and freely downloadable so is a good test. (AFAIK 
>> those who developed MorphOS had close connection to those who wrote the 
>> firmware for Pegasos II.) Maybe the VT8231 datasheet or similar parts (we 
>> only emulate VT82C686B and VT8231 in QEMU) has some info on this.
>
> I agree it's a good test, but it's not clear what it means to do sub-word 
> writes to the register.

Looking at the VT8231 data sheet I think it means to set bits related to 
soft-off when it writes 0x28 to upper byte of the control reg. This seems 
pretty obvious.

> For example, in the dump I posted you have:
>
> evt 3 1 write 1      // enable timer
> evt 0 2 read
> evt 0 2 write 1      // just writes again the same value, clearing sts
>
> It's important to note that the comments are just my guess.
>
> Before even looking at the effect of the write, the trace tells us that your 
> patch is incomplete.  With both current QEMU and your patch it would do 
> nothing because addr is not 0 or 2; but since the firmware of your machine 
> does use addr == 3, you need to handle it.  In other words, before looking at 
> this trace, I was wary of accepting your patch because it made no sense to 
> me; but I couldn't exclude that I was missing something.  Now, instead, I am 
> certain it shouldn't be accepted.

Testing on real machine also confirms that the patch was wrong as it 
breaks access to regs even if it happens to fix the unaligned write. Since 
we don't emulate most of it anyway and guests probably don't use it other 
than for power control the breakage does not appeared though.

> I am quite confident that the second guess is correct, because "write the 
> same value that was read" only makes sense for evt_sts and not for evt_en.

You don't have to guess which reg is which, check the data sheet instead:

Power Management I/O-Space Registers
Basic Power Management Control and Status
I/O Offset 1-0 - Power Management Status ................. RWC
I/O Offset 3-2 - Power Management Enable .................. RW
I/O Offset 5-4 - Power Management Control .................RW
I/O Offset 0B-08 - Power Management Timer............... RW

> We learnt something: no matter what bus this device sits on, it does not flip 
> bit 1 of the address for subword writes.  As I mentioned yesterday, we also 
> observe that the load and store use lhbrx and sthbrx.  Assuming this is not a 
> harmless bug in the firmware, this means the device is indeed little endian.
>
> This means that the first write is almost certainly to evt_en.  On a 
> little-endian machine the write would set bit 8 of evt.en (power button), but 
> what about a big-endian machine like yours?  Should it set bit 0 or bit 8? 
> If it's bit 0 (which resides at offset 2 according to the device), who flips 
> the low bit of the address?  Why is bit 0 flipped but not bit 1?
>
> You simply cannot fix the emulation of this machine until you can answer the 
> above questions.  If there is no source and no spec, the two ways to do it 
> are:
>
> - get a real machine, and figure out whether the write to offset 3 
> corresponds to the PM timer or the power button.
>
> - continue the trace up to the point the OS runs, and see if you get some 
> clues similar to the one above that placed evt_sts at offset 2.

So I wrote a script to dump the PM registers of VT8231 with different 
access sizes and compare that to QEMU. The script and results are attached here:
https://osdn.net/projects/qmiga/ticket/47971

The results I think show that:
- VIA chip is little endian and mapped as such on pegasos2
- proposed patch is indeed wrong
- unaligned access is not handled correctly in acpi_pm_cnt_write() which 
now I think is the place where this should be fixed
- QEMU runs the PM timer in 32 bit while the corresponding bit is set to 
24 bit mode (VT8231 has a bit to set this in PCI config reg 0x41), not 
sure if this causes any problem and if QEMU has a 24 bit mode

What do you think?

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

On Mon, 1 May 2023, BALATON Zoltan wrote:
> - unaligned access is not handled correctly in acpi_pm_cnt_write() which now 
> I think is the place where this should be fixed

And also when running the test script on QEMU:

acpi_pm_cnt_read: 0 2 -> 0x80
acpi_pm_cnt_read: 1 2 -> 0x80

where the second should return 0 so these ops are either wrong or the 
memory layer does not do what's intended for:

static const MemoryRegionOps acpi_pm_cnt_ops = {
     .read = acpi_pm_cnt_read,
     .write = acpi_pm_cnt_write,
     .impl.min_access_size = 2,
     .valid.min_access_size = 1,
     .valid.max_access_size = 2,
     .endianness = DEVICE_LITTLE_ENDIAN,
};

I could patch this up in acpi_pm_cnt_read() abd acpi_pm_cnt_write() but 
how this is supposed to work? Do these functions have to handle unaligned 
access or the impl/valid settings should take care of that?

Regards,
BALATON Zoltan