ACPI endianness

ACPI endianness

[BALATON Zoltan]

Hello,

I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as part 
of the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c) and 
found that the guest writes to ACPI PM1aCNT register come out with wrong 
endianness but not shure why. I have this:

$ qemu-system-ppc -M pegasos2 -monitor stdio
(qemu) info mtree
[...]
memory-region: pci1-io
   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
[...]
     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr

memory-region: system
   0000000000000000-ffffffffffffffff (prio 0, i/o): system
     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win @pci1-mem 0000000080000000-00000000bfffffff
     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win @pci0-mem 00000000c0000000-00000000dfffffff
     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win @pci0-io 0000000000000000-0000000000ffffff
     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win @pci0-mem 0000000000000000-0000000000ffffff
     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win @pci1-mem 0000000000000000-0000000000ffffff
     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win @pci1-io 0000000000000000-0000000000ffffff
     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win @pci1-mem 00000000ff800000-00000000ffffffff
     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom

The guest (which is big endian PPC and I think wotks on real hardware) 
writes to 0xf05 in the io region which should be the high byte of the 
little endian register but in the acpi code it comes out wrong, instead 
of 0x2800 I get in acpi_pm1_cnt_write: val=0x28

The memory regions involved are:

acpi-cnt (hw/acpi/core.c):

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

via-pm (hw/isa/vt82c686.c):

static const MemoryRegionOps pm_io_ops = {
     .read = pm_io_read,
     .write = pm_io_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .impl = {
         .min_access_size = 1,
         .max_access_size = 1,
     },
};

Also tried removing .impl from pm_io_ops but that does not help. Changing 
acpi_pm_cnt_ops to DEVICE_NATIVE_ENDIAN fixes it but not sure what else 
could that break. Should these ACPI regions be native endian or how else 
to fix this for the vt82xx case?

Regards,
BALATON Zoltan

Re: ACPI endianness

[Michael S. Tsirkin]

On Sun, Oct 10, 2021 at 03:24:32PM +0200, BALATON Zoltan wrote:
> Hello,
> 
> I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as part of
> the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c) and found
> that the guest writes to ACPI PM1aCNT register come out with wrong
> endianness but not shure why. I have this:
> 
> $ qemu-system-ppc -M pegasos2 -monitor stdio
> (qemu) info mtree
> [...]
> memory-region: pci1-io
>   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
> [...]
>     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
>       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
>       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
>       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
> 
> memory-region: system
>   0000000000000000-ffffffffffffffff (prio 0, i/o): system
>     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
>     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win @pci1-mem 0000000080000000-00000000bfffffff
>     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win @pci0-mem 00000000c0000000-00000000dfffffff
>     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
>     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win @pci0-io 0000000000000000-0000000000ffffff
>     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win @pci0-mem 0000000000000000-0000000000ffffff
>     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win @pci1-mem 0000000000000000-0000000000ffffff
>     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win @pci1-io 0000000000000000-0000000000ffffff
>     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win @pci1-mem 00000000ff800000-00000000ffffffff
>     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
> 
> The guest (which is big endian PPC and I think wotks on real hardware)
> writes to 0xf05 in the io region which should be the high byte of the little
> endian register but in the acpi code it comes out wrong, instead of 0x2800 I
> get in acpi_pm1_cnt_write: val=0x28
> 
> The memory regions involved are:
> 
> acpi-cnt (hw/acpi/core.c):
> 
> 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,
> };
> 
> via-pm (hw/isa/vt82c686.c):
> 
> static const MemoryRegionOps pm_io_ops = {
>     .read = pm_io_read,
>     .write = pm_io_write,
>     .endianness = DEVICE_NATIVE_ENDIAN,
>     .impl = {
>         .min_access_size = 1,
>         .max_access_size = 1,
>     },
> };
> 
> Also tried removing .impl from pm_io_ops but that does not help. Changing
> acpi_pm_cnt_ops to DEVICE_NATIVE_ENDIAN fixes it but not sure what else
> could that break. Should these ACPI regions be native endian or how else to
> fix this for the vt82xx case?
> 
> Regards,
> BALATON Zoltan

Does it help if you change via to DEVICE_LITTLE_ENDIAN?

-- 
MST

Re: ACPI endianness

[BALATON Zoltan]

On Sun, 10 Oct 2021, Michael S. Tsirkin wrote:
> On Sun, Oct 10, 2021 at 03:24:32PM +0200, BALATON Zoltan wrote:
>> Hello,
>>
>> I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as part of
>> the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c) and found
>> that the guest writes to ACPI PM1aCNT register come out with wrong
>> endianness but not shure why. I have this:
>>
>> $ qemu-system-ppc -M pegasos2 -monitor stdio
>> (qemu) info mtree
>> [...]
>> memory-region: pci1-io
>>   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
>> [...]
>>     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
>>       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
>>       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
>>       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
>>
>> memory-region: system
>>   0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
>>     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win @pci1-mem 0000000080000000-00000000bfffffff
>>     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win @pci0-mem 00000000c0000000-00000000dfffffff
>>     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
>>     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win @pci0-io 0000000000000000-0000000000ffffff
>>     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win @pci0-mem 0000000000000000-0000000000ffffff
>>     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win @pci1-mem 0000000000000000-0000000000ffffff
>>     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win @pci1-io 0000000000000000-0000000000ffffff
>>     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win @pci1-mem 00000000ff800000-00000000ffffffff
>>     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
>>
>> The guest (which is big endian PPC and I think wotks on real hardware)
>> writes to 0xf05 in the io region which should be the high byte of the little
>> endian register but in the acpi code it comes out wrong, instead of 0x2800 I
>> get in acpi_pm1_cnt_write: val=0x28
>>
>> The memory regions involved are:
>>
>> acpi-cnt (hw/acpi/core.c):
>>
>> 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,
>> };
>>
>> via-pm (hw/isa/vt82c686.c):
>>
>> static const MemoryRegionOps pm_io_ops = {
>>     .read = pm_io_read,
>>     .write = pm_io_write,
>>     .endianness = DEVICE_NATIVE_ENDIAN,
>>     .impl = {
>>         .min_access_size = 1,
>>         .max_access_size = 1,
>>     },
>> };
>>
>> Also tried removing .impl from pm_io_ops but that does not help. Changing
>> acpi_pm_cnt_ops to DEVICE_NATIVE_ENDIAN fixes it but not sure what else
>> could that break. Should these ACPI regions be native endian or how else to
>> fix this for the vt82xx case?
>>
>> Regards,
>> BALATON Zoltan
>
> Does it help if you change via to DEVICE_LITTLE_ENDIAN?

No. That region is just acting as a container for the ACPI regions so they 
can be moved together to the base address settable by the guest (and maybe 
implementing some other regs later if needed but we don't do that now) but 
looks like it's not involved in read/write to acpi regs as its ops are not 
called and the write goes through to the acpi-cnt region. So it also does 
not matter if I change endianness or impl in pm_io_ops in vt82c686.c.

These acpi bits only seem to be used on piix4 and ich9 apart from vt82xx 
and those are probably used on little endian hosts while vt8231 is used on 
pegasos2 which is big endian PPC and vt82c686b on fullong2e which I think 
is little endian MIPS. What does NATIVE_ENDIAN actually mean? It does not 
seem to be documented anywhere. Would it change anything if the ACPI 
regions are changed ro NATIVE_ENDIAN for little endian machines that are 
all the other usages? If not then maybe that's the way to go.

Regards,
BALATON Zoltan

Re: ACPI endianness

[Philippe Mathieu-Daudé]

On 10/10/21 15:24, BALATON Zoltan wrote:
> Hello,
> 
> I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as
> part of the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c)
> and found that the guest writes to ACPI PM1aCNT register come out with
> wrong endianness but not shure why. I have this:
> 
> $ qemu-system-ppc -M pegasos2 -monitor stdio
> (qemu) info mtree
> [...]
> memory-region: pci1-io
>   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
> [...]
>     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
>       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
>       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
>       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
> 
> memory-region: system
>   0000000000000000-ffffffffffffffff (prio 0, i/o): system
>     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
>     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win
> @pci1-mem 0000000080000000-00000000bfffffff
>     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win
> @pci0-mem 00000000c0000000-00000000dfffffff
>     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
>     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win
> @pci0-io 0000000000000000-0000000000ffffff
>     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win
> @pci0-mem 0000000000000000-0000000000ffffff
>     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win
> @pci1-mem 0000000000000000-0000000000ffffff
>     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win
> @pci1-io 0000000000000000-0000000000ffffff
>     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win
> @pci1-mem 00000000ff800000-00000000ffffffff
>     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
> 
> The guest (which is big endian PPC and I think wotks on real hardware)
> writes to 0xf05 in the io region which should be the high byte of the
> little endian register but in the acpi code it comes out wrong, instead
> of 0x2800 I get in acpi_pm1_cnt_write: val=0x28

Looks like a northbridge issue (MV64340).
Does Pegasos2 enables bus swapping?
See hw/pci-host/mv64361.c:585:

static void warn_swap_bit(uint64_t val)
{
    if ((val & 0x3000000ULL) >> 24 != 1) {
        qemu_log_mask(LOG_UNIMP, "%s: Data swap not implemented", __func__);
    }
}

Re: ACPI endianness

[BALATON Zoltan]

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

On Mon, 11 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/10/21 15:24, BALATON Zoltan wrote:
>> Hello,
>>
>> I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as
>> part of the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c)
>> and found that the guest writes to ACPI PM1aCNT register come out with
>> wrong endianness but not shure why. I have this:
>>
>> $ qemu-system-ppc -M pegasos2 -monitor stdio
>> (qemu) info mtree
>> [...]
>> memory-region: pci1-io
>>   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
>> [...]
>>     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
>>       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
>>       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
>>       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
>>
>> memory-region: system
>>   0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
>>     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win
>> @pci1-mem 0000000080000000-00000000bfffffff
>>     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win
>> @pci0-mem 00000000c0000000-00000000dfffffff
>>     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
>>     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win
>> @pci0-io 0000000000000000-0000000000ffffff
>>     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win
>> @pci0-mem 0000000000000000-0000000000ffffff
>>     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win
>> @pci1-mem 0000000000000000-0000000000ffffff
>>     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win
>> @pci1-io 0000000000000000-0000000000ffffff
>>     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win
>> @pci1-mem 00000000ff800000-00000000ffffffff
>>     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
>>
>> The guest (which is big endian PPC and I think wotks on real hardware)
>> writes to 0xf05 in the io region which should be the high byte of the
>> little endian register but in the acpi code it comes out wrong, instead
>> of 0x2800 I get in acpi_pm1_cnt_write: val=0x28
>
> Looks like a northbridge issue (MV64340).
> Does Pegasos2 enables bus swapping?
> See hw/pci-host/mv64361.c:585:
>
> static void warn_swap_bit(uint64_t val)
> {
>    if ((val & 0x3000000ULL) >> 24 != 1) {
>        qemu_log_mask(LOG_UNIMP, "%s: Data swap not implemented", __func__);
>    }
> }

No, guests don't use this feature just byteswap themselves and write 
little endian values to the mapped io region. (Or in this case just write 
one byte of the 16 bit value, it specifically writes 0x28 to 0xf05.) 
That's why I think the device model should not byteswap itself so the acpi 
regions should be declared native endian and let the guest handle it. Some 
mentions I've found say that native endian means don't byteswap, little 
endian means byteswap if vcpu is big endian and big endian means byteswap 
if vcpu is little endian. Since guests already account for this and write 
little endian values to these regions there's no need to byteswap in 
device model and changing to native endian should not affect little endian 
machines so unless there's a better argument I'll try sending a patch.

Regards,
BALATON Zoltan

Re: ACPI endianness

[Michael S. Tsirkin]

On Mon, Oct 11, 2021 at 12:13:55PM +0200, BALATON Zoltan wrote:
> On Mon, 11 Oct 2021, Philippe Mathieu-Daudé wrote:
> > On 10/10/21 15:24, BALATON Zoltan wrote:
> > > Hello,
> > > 
> > > I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as
> > > part of the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c)
> > > and found that the guest writes to ACPI PM1aCNT register come out with
> > > wrong endianness but not shure why. I have this:
> > > 
> > > $ qemu-system-ppc -M pegasos2 -monitor stdio
> > > (qemu) info mtree
> > > [...]
> > > memory-region: pci1-io
> > >   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
> > > [...]
> > >     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
> > >       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
> > >       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
> > >       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
> > > 
> > > memory-region: system
> > >   0000000000000000-ffffffffffffffff (prio 0, i/o): system
> > >     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
> > >     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win
> > > @pci1-mem 0000000080000000-00000000bfffffff
> > >     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win
> > > @pci0-mem 00000000c0000000-00000000dfffffff
> > >     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
> > >     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win
> > > @pci0-io 0000000000000000-0000000000ffffff
> > >     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win
> > > @pci0-mem 0000000000000000-0000000000ffffff
> > >     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win
> > > @pci1-mem 0000000000000000-0000000000ffffff
> > >     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win
> > > @pci1-io 0000000000000000-0000000000ffffff
> > >     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win
> > > @pci1-mem 00000000ff800000-00000000ffffffff
> > >     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
> > > 
> > > The guest (which is big endian PPC and I think wotks on real hardware)
> > > writes to 0xf05 in the io region which should be the high byte of the
> > > little endian register but in the acpi code it comes out wrong, instead
> > > of 0x2800 I get in acpi_pm1_cnt_write: val=0x28
> > 
> > Looks like a northbridge issue (MV64340).
> > Does Pegasos2 enables bus swapping?
> > See hw/pci-host/mv64361.c:585:
> > 
> > static void warn_swap_bit(uint64_t val)
> > {
> >    if ((val & 0x3000000ULL) >> 24 != 1) {
> >        qemu_log_mask(LOG_UNIMP, "%s: Data swap not implemented", __func__);
> >    }
> > }
> 
> No, guests don't use this feature just byteswap themselves and write little
> endian values to the mapped io region. (Or in this case just write one byte
> of the 16 bit value, it specifically writes 0x28 to 0xf05.) That's why I
> think the device model should not byteswap itself so the acpi regions should
> be declared native endian and let the guest handle it. Some mentions I've
> found say that native endian means don't byteswap, little endian means
> byteswap if vcpu is big endian and big endian means byteswap if vcpu is
> little endian. Since guests already account for this and write little endian
> values to these regions there's no need to byteswap in device model and
> changing to native endian should not affect little endian machines so unless
> there's a better argument I'll try sending a patch.
> 
> Regards,
> BALATON Zoltan

native endian means endian-ness is open-coded in the device handler
itself.  I think you just found a bug in memory core.

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


Because of that     .impl.min_access_size = 2,
the 1 byte write should be converted to a 2 byte
read+2 byte write.

docs/devel/memory.rst just says:
- .impl.min_access_size, .impl.max_access_size define the access sizes
  (in bytes) supported by the *implementation*; other access sizes will be
  emulated using the ones available.  For example a 4-byte write will be
  emulated using four 1-byte writes, if .impl.max_access_size = 1.



Instead what we have is:

MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
                                         hwaddr addr,
                                         uint64_t data,
                                         MemOp op,
                                         MemTxAttrs attrs)
{
    unsigned size = memop_size(op);

    if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
        unassigned_mem_write(mr, addr, data, size);
        return MEMTX_DECODE_ERROR;
    }

    adjust_endianness(mr, &data, op);


---


static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op)
{
    if ((op & MO_BSWAP) != devend_memop(mr->ops->endianness)) {
        switch (op & MO_SIZE) {
        case MO_8:
            break;
        case MO_16:
            *data = bswap16(*data);
            break;
        case MO_32:
            *data = bswap32(*data);
            break;
        case MO_64:
            *data = bswap64(*data);
            break;
        default:
            g_assert_not_reached();
        }
    }
}

so the byte swap is based on size before extending it to
.impl.min_access_size, not after.

Also, no read happens which I suspect is also a bug,
but could be harmless ...

Paolo, any feedback here?

-- 
MST

Re: ACPI endianness

[Jonathan Cameron]

On Mon, 11 Oct 2021 08:19:01 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Oct 11, 2021 at 12:13:55PM +0200, BALATON Zoltan wrote:
> > On Mon, 11 Oct 2021, Philippe Mathieu-Daudé wrote:  
> > > On 10/10/21 15:24, BALATON Zoltan wrote:  
> > > > Hello,
> > > > 
> > > > I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as
> > > > part of the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c)
> > > > and found that the guest writes to ACPI PM1aCNT register come out with
> > > > wrong endianness but not shure why. I have this:
> > > > 
> > > > $ qemu-system-ppc -M pegasos2 -monitor stdio
> > > > (qemu) info mtree
> > > > [...]
> > > > memory-region: pci1-io
> > > >   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
> > > > [...]
> > > >     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
> > > >       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
> > > >       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
> > > >       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
> > > > 
> > > > memory-region: system
> > > >   0000000000000000-ffffffffffffffff (prio 0, i/o): system
> > > >     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
> > > >     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win
> > > > @pci1-mem 0000000080000000-00000000bfffffff
> > > >     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win
> > > > @pci0-mem 00000000c0000000-00000000dfffffff
> > > >     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
> > > >     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win
> > > > @pci0-io 0000000000000000-0000000000ffffff
> > > >     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win
> > > > @pci0-mem 0000000000000000-0000000000ffffff
> > > >     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win
> > > > @pci1-mem 0000000000000000-0000000000ffffff
> > > >     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win
> > > > @pci1-io 0000000000000000-0000000000ffffff
> > > >     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win
> > > > @pci1-mem 00000000ff800000-00000000ffffffff
> > > >     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
> > > > 
> > > > The guest (which is big endian PPC and I think wotks on real hardware)
> > > > writes to 0xf05 in the io region which should be the high byte of the
> > > > little endian register but in the acpi code it comes out wrong, instead
> > > > of 0x2800 I get in acpi_pm1_cnt_write: val=0x28  
> > > 
> > > Looks like a northbridge issue (MV64340).
> > > Does Pegasos2 enables bus swapping?
> > > See hw/pci-host/mv64361.c:585:
> > > 
> > > static void warn_swap_bit(uint64_t val)
> > > {
> > >    if ((val & 0x3000000ULL) >> 24 != 1) {
> > >        qemu_log_mask(LOG_UNIMP, "%s: Data swap not implemented", __func__);
> > >    }
> > > }  
> > 
> > No, guests don't use this feature just byteswap themselves and write little
> > endian values to the mapped io region. (Or in this case just write one byte
> > of the 16 bit value, it specifically writes 0x28 to 0xf05.) That's why I
> > think the device model should not byteswap itself so the acpi regions should
> > be declared native endian and let the guest handle it. Some mentions I've
> > found say that native endian means don't byteswap, little endian means
> > byteswap if vcpu is big endian and big endian means byteswap if vcpu is
> > little endian. Since guests already account for this and write little endian
> > values to these regions there's no need to byteswap in device model and
> > changing to native endian should not affect little endian machines so unless
> > there's a better argument I'll try sending a patch.
> > 
> > Regards,
> > BALATON Zoltan  
> 
> native endian means endian-ness is open-coded in the device handler
> itself.  I think you just found a bug in memory core.
> 
> 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,
> };
> 
> 
> Because of that     .impl.min_access_size = 2,
> the 1 byte write should be converted to a 2 byte
> read+2 byte write.
> 
> docs/devel/memory.rst just says:
> - .impl.min_access_size, .impl.max_access_size define the access sizes
>   (in bytes) supported by the *implementation*; other access sizes will be
>   emulated using the ones available.  For example a 4-byte write will be
>   emulated using four 1-byte writes, if .impl.max_access_size = 1.
> 
> 
> 
> Instead what we have is:
> 
> MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
>                                          hwaddr addr,
>                                          uint64_t data,
>                                          MemOp op,
>                                          MemTxAttrs attrs)
> {
>     unsigned size = memop_size(op);
> 
>     if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
>         unassigned_mem_write(mr, addr, data, size);
>         return MEMTX_DECODE_ERROR;
>     }
> 
>     adjust_endianness(mr, &data, op);
> 
> 
> ---
> 
> 
> static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op)
> {
>     if ((op & MO_BSWAP) != devend_memop(mr->ops->endianness)) {
>         switch (op & MO_SIZE) {
>         case MO_8:
>             break;
>         case MO_16:
>             *data = bswap16(*data);
>             break;
>         case MO_32:
>             *data = bswap32(*data);
>             break;
>         case MO_64:
>             *data = bswap64(*data);
>             break;
>         default:
>             g_assert_not_reached();
>         }
>     }
> }
> 
> so the byte swap is based on size before extending it to
> .impl.min_access_size, not after.
> 
> Also, no read happens which I suspect is also a bug,
> but could be harmless ...

The lack of read modify write looks like this again:
https://lore.kernel.org/qemu-devel/20210513124737.00002b2d@Huawei.com/

byte swap part is new to me though...

> 
> Paolo, any feedback here?
> 

Re: ACPI endianness

[BALATON Zoltan]

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

On Mon, 11 Oct 2021, Michael S. Tsirkin wrote:
> On Mon, Oct 11, 2021 at 12:13:55PM +0200, BALATON Zoltan wrote:
>> On Mon, 11 Oct 2021, Philippe Mathieu-Daudé wrote:
>>> On 10/10/21 15:24, BALATON Zoltan wrote:
>>>> Hello,
>>>>
>>>> I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as
>>>> part of the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c)
>>>> and found that the guest writes to ACPI PM1aCNT register come out with
>>>> wrong endianness but not shure why. I have this:
>>>>
>>>> $ qemu-system-ppc -M pegasos2 -monitor stdio
>>>> (qemu) info mtree
>>>> [...]
>>>> memory-region: pci1-io
>>>>   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
>>>> [...]
>>>>     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
>>>>       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
>>>>       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
>>>>       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
>>>>
>>>> memory-region: system
>>>>   0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>>>     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
>>>>     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win
>>>> @pci1-mem 0000000080000000-00000000bfffffff
>>>>     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win
>>>> @pci0-mem 00000000c0000000-00000000dfffffff
>>>>     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
>>>>     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win
>>>> @pci0-io 0000000000000000-0000000000ffffff
>>>>     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win
>>>> @pci0-mem 0000000000000000-0000000000ffffff
>>>>     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win
>>>> @pci1-mem 0000000000000000-0000000000ffffff
>>>>     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win
>>>> @pci1-io 0000000000000000-0000000000ffffff
>>>>     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win
>>>> @pci1-mem 00000000ff800000-00000000ffffffff
>>>>     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
>>>>
>>>> The guest (which is big endian PPC and I think wotks on real hardware)
>>>> writes to 0xf05 in the io region which should be the high byte of the
>>>> little endian register but in the acpi code it comes out wrong, instead
>>>> of 0x2800 I get in acpi_pm1_cnt_write: val=0x28
>>>
>>> Looks like a northbridge issue (MV64340).
>>> Does Pegasos2 enables bus swapping?
>>> See hw/pci-host/mv64361.c:585:
>>>
>>> static void warn_swap_bit(uint64_t val)
>>> {
>>>    if ((val & 0x3000000ULL) >> 24 != 1) {
>>>        qemu_log_mask(LOG_UNIMP, "%s: Data swap not implemented", __func__);
>>>    }
>>> }
>>
>> No, guests don't use this feature just byteswap themselves and write little
>> endian values to the mapped io region. (Or in this case just write one byte
>> of the 16 bit value, it specifically writes 0x28 to 0xf05.) That's why I
>> think the device model should not byteswap itself so the acpi regions should
>> be declared native endian and let the guest handle it. Some mentions I've
>> found say that native endian means don't byteswap, little endian means
>> byteswap if vcpu is big endian and big endian means byteswap if vcpu is
>> little endian. Since guests already account for this and write little endian
>> values to these regions there's no need to byteswap in device model and
>> changing to native endian should not affect little endian machines so unless
>> there's a better argument I'll try sending a patch.
>>
>> Regards,
>> BALATON Zoltan
>
> native endian means endian-ness is open-coded in the device handler
> itself.  I think you just found a bug in memory core.
>
> 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,
> };
>
>
> Because of that     .impl.min_access_size = 2,
> the 1 byte write should be converted to a 2 byte
> read+2 byte write.
>
> docs/devel/memory.rst just says:
> - .impl.min_access_size, .impl.max_access_size define the access sizes
>  (in bytes) supported by the *implementation*; other access sizes will be
>  emulated using the ones available.  For example a 4-byte write will be
>  emulated using four 1-byte writes, if .impl.max_access_size = 1.
>
>
>
> Instead what we have is:
>
> MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
>                                         hwaddr addr,
>                                         uint64_t data,
>                                         MemOp op,
>                                         MemTxAttrs attrs)
> {
>    unsigned size = memop_size(op);
>
>    if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
>        unassigned_mem_write(mr, addr, data, size);
>        return MEMTX_DECODE_ERROR;
>    }
>
>    adjust_endianness(mr, &data, op);
>
>
> ---
>
>
> static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op)
> {
>    if ((op & MO_BSWAP) != devend_memop(mr->ops->endianness)) {
>        switch (op & MO_SIZE) {
>        case MO_8:
>            break;
>        case MO_16:
>            *data = bswap16(*data);
>            break;
>        case MO_32:
>            *data = bswap32(*data);
>            break;
>        case MO_64:
>            *data = bswap64(*data);
>            break;
>        default:
>            g_assert_not_reached();
>        }
>    }
> }
>
> so the byte swap is based on size before extending it to
> .impl.min_access_size, not after.

OK thanks for the analysis, I'll wait what comes out of this.

> Also, no read happens which I suspect is also a bug,
> but could be harmless ...

I did not check if a read happens or not but generally I think that may be 
another source of bugs if the memory core changes writes to read + write 
because for some devices a read might have side effects (like changing 
bits, clearing interrupt status, whatever) so the emulation would behave 
differently than real hardware if a guest tries to write such a register. 
So this may have problems either way even if this particular case is not 
affected by that and could just be "fixed" by declaring the regions native 
endian to disable this part of memory core or fixing that up to do byte 
swapping back and forth. I don't mind either way as long as it works.

Regards,
BALATON Zoltan

Re: ACPI endianness

[Michael S. Tsirkin]

On Mon, Oct 11, 2021 at 03:27:44PM +0200, BALATON Zoltan wrote:
> On Mon, 11 Oct 2021, Michael S. Tsirkin wrote:
> > On Mon, Oct 11, 2021 at 12:13:55PM +0200, BALATON Zoltan wrote:
> > > On Mon, 11 Oct 2021, Philippe Mathieu-Daudé wrote:
> > > > On 10/10/21 15:24, BALATON Zoltan wrote:
> > > > > Hello,
> > > > > 
> > > > > I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as
> > > > > part of the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c)
> > > > > and found that the guest writes to ACPI PM1aCNT register come out with
> > > > > wrong endianness but not shure why. I have this:
> > > > > 
> > > > > $ qemu-system-ppc -M pegasos2 -monitor stdio
> > > > > (qemu) info mtree
> > > > > [...]
> > > > > memory-region: pci1-io
> > > > >   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
> > > > > [...]
> > > > >     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
> > > > >       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
> > > > >       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
> > > > >       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
> > > > > 
> > > > > memory-region: system
> > > > >   0000000000000000-ffffffffffffffff (prio 0, i/o): system
> > > > >     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
> > > > >     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win
> > > > > @pci1-mem 0000000080000000-00000000bfffffff
> > > > >     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win
> > > > > @pci0-mem 00000000c0000000-00000000dfffffff
> > > > >     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
> > > > >     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win
> > > > > @pci0-io 0000000000000000-0000000000ffffff
> > > > >     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win
> > > > > @pci0-mem 0000000000000000-0000000000ffffff
> > > > >     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win
> > > > > @pci1-mem 0000000000000000-0000000000ffffff
> > > > >     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win
> > > > > @pci1-io 0000000000000000-0000000000ffffff
> > > > >     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win
> > > > > @pci1-mem 00000000ff800000-00000000ffffffff
> > > > >     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
> > > > > 
> > > > > The guest (which is big endian PPC and I think wotks on real hardware)
> > > > > writes to 0xf05 in the io region which should be the high byte of the
> > > > > little endian register but in the acpi code it comes out wrong, instead
> > > > > of 0x2800 I get in acpi_pm1_cnt_write: val=0x28
> > > > 
> > > > Looks like a northbridge issue (MV64340).
> > > > Does Pegasos2 enables bus swapping?
> > > > See hw/pci-host/mv64361.c:585:
> > > > 
> > > > static void warn_swap_bit(uint64_t val)
> > > > {
> > > >    if ((val & 0x3000000ULL) >> 24 != 1) {
> > > >        qemu_log_mask(LOG_UNIMP, "%s: Data swap not implemented", __func__);
> > > >    }
> > > > }
> > > 
> > > No, guests don't use this feature just byteswap themselves and write little
> > > endian values to the mapped io region. (Or in this case just write one byte
> > > of the 16 bit value, it specifically writes 0x28 to 0xf05.) That's why I
> > > think the device model should not byteswap itself so the acpi regions should
> > > be declared native endian and let the guest handle it. Some mentions I've
> > > found say that native endian means don't byteswap, little endian means
> > > byteswap if vcpu is big endian and big endian means byteswap if vcpu is
> > > little endian. Since guests already account for this and write little endian
> > > values to these regions there's no need to byteswap in device model and
> > > changing to native endian should not affect little endian machines so unless
> > > there's a better argument I'll try sending a patch.
> > > 
> > > Regards,
> > > BALATON Zoltan
> > 
> > native endian means endian-ness is open-coded in the device handler
> > itself.  I think you just found a bug in memory core.
> > 
> > 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,
> > };
> > 
> > 
> > Because of that     .impl.min_access_size = 2,
> > the 1 byte write should be converted to a 2 byte
> > read+2 byte write.
> > 
> > docs/devel/memory.rst just says:
> > - .impl.min_access_size, .impl.max_access_size define the access sizes
> >  (in bytes) supported by the *implementation*; other access sizes will be
> >  emulated using the ones available.  For example a 4-byte write will be
> >  emulated using four 1-byte writes, if .impl.max_access_size = 1.
> > 
> > 
> > 
> > Instead what we have is:
> > 
> > MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
> >                                         hwaddr addr,
> >                                         uint64_t data,
> >                                         MemOp op,
> >                                         MemTxAttrs attrs)
> > {
> >    unsigned size = memop_size(op);
> > 
> >    if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
> >        unassigned_mem_write(mr, addr, data, size);
> >        return MEMTX_DECODE_ERROR;
> >    }
> > 
> >    adjust_endianness(mr, &data, op);
> > 
> > 
> > ---
> > 
> > 
> > static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op)
> > {
> >    if ((op & MO_BSWAP) != devend_memop(mr->ops->endianness)) {
> >        switch (op & MO_SIZE) {
> >        case MO_8:
> >            break;
> >        case MO_16:
> >            *data = bswap16(*data);
> >            break;
> >        case MO_32:
> >            *data = bswap32(*data);
> >            break;
> >        case MO_64:
> >            *data = bswap64(*data);
> >            break;
> >        default:
> >            g_assert_not_reached();
> >        }
> >    }
> > }
> > 
> > so the byte swap is based on size before extending it to
> > .impl.min_access_size, not after.
> 
> OK thanks for the analysis, I'll wait what comes out of this.
> 
> > Also, no read happens which I suspect is also a bug,
> > but could be harmless ...
> 
> I did not check if a read happens or not but generally I think that may be
> another source of bugs if the memory core changes writes to read + write
> because for some devices a read might have side effects


IMHO if reads have side effects then devices should not set .impl.min_access_size
... but given we did not previously do the read, maybe we should keep
it that way at least for the time being.


> (like changing bits,
> clearing interrupt status, whatever) so the emulation would behave
> differently than real hardware if a guest tries to write such a register. So
> this may have problems either way even if this particular case is not
> affected by that and could just be "fixed" by declaring the regions native
> endian to disable this part of memory core or fixing that up to do byte
> swapping back and forth. I don't mind either way as long as it works.
> 
> Regards,
> BALATON Zoltan

Re: ACPI endianness

[BALATON Zoltan]

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

On Mon, 11 Oct 2021, Michael S. Tsirkin wrote:
> On Mon, Oct 11, 2021 at 03:27:44PM +0200, BALATON Zoltan wrote:
>> On Mon, 11 Oct 2021, Michael S. Tsirkin wrote:
>>> On Mon, Oct 11, 2021 at 12:13:55PM +0200, BALATON Zoltan wrote:
>>>> On Mon, 11 Oct 2021, Philippe Mathieu-Daudé wrote:
>>>>> On 10/10/21 15:24, BALATON Zoltan wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as
>>>>>> part of the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c)
>>>>>> and found that the guest writes to ACPI PM1aCNT register come out with
>>>>>> wrong endianness but not shure why. I have this:
>>>>>>
>>>>>> $ qemu-system-ppc -M pegasos2 -monitor stdio
>>>>>> (qemu) info mtree
>>>>>> [...]
>>>>>> memory-region: pci1-io
>>>>>>   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
>>>>>> [...]
>>>>>>     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
>>>>>>       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
>>>>>>       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
>>>>>>       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
>>>>>>
>>>>>> memory-region: system
>>>>>>   0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>>>>>     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
>>>>>>     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win
>>>>>> @pci1-mem 0000000080000000-00000000bfffffff
>>>>>>     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win
>>>>>> @pci0-mem 00000000c0000000-00000000dfffffff
>>>>>>     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
>>>>>>     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win
>>>>>> @pci0-io 0000000000000000-0000000000ffffff
>>>>>>     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win
>>>>>> @pci0-mem 0000000000000000-0000000000ffffff
>>>>>>     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win
>>>>>> @pci1-mem 0000000000000000-0000000000ffffff
>>>>>>     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win
>>>>>> @pci1-io 0000000000000000-0000000000ffffff
>>>>>>     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win
>>>>>> @pci1-mem 00000000ff800000-00000000ffffffff
>>>>>>     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
>>>>>>
>>>>>> The guest (which is big endian PPC and I think wotks on real hardware)
>>>>>> writes to 0xf05 in the io region which should be the high byte of the
>>>>>> little endian register but in the acpi code it comes out wrong, instead
>>>>>> of 0x2800 I get in acpi_pm1_cnt_write: val=0x28
>>>>>
>>>>> Looks like a northbridge issue (MV64340).
>>>>> Does Pegasos2 enables bus swapping?
>>>>> See hw/pci-host/mv64361.c:585:
>>>>>
>>>>> static void warn_swap_bit(uint64_t val)
>>>>> {
>>>>>    if ((val & 0x3000000ULL) >> 24 != 1) {
>>>>>        qemu_log_mask(LOG_UNIMP, "%s: Data swap not implemented", __func__);
>>>>>    }
>>>>> }
>>>>
>>>> No, guests don't use this feature just byteswap themselves and write little
>>>> endian values to the mapped io region. (Or in this case just write one byte
>>>> of the 16 bit value, it specifically writes 0x28 to 0xf05.) That's why I
>>>> think the device model should not byteswap itself so the acpi regions should
>>>> be declared native endian and let the guest handle it. Some mentions I've
>>>> found say that native endian means don't byteswap, little endian means
>>>> byteswap if vcpu is big endian and big endian means byteswap if vcpu is
>>>> little endian. Since guests already account for this and write little endian
>>>> values to these regions there's no need to byteswap in device model and
>>>> changing to native endian should not affect little endian machines so unless
>>>> there's a better argument I'll try sending a patch.
>>>>
>>>> Regards,
>>>> BALATON Zoltan
>>>
>>> native endian means endian-ness is open-coded in the device handler
>>> itself.  I think you just found a bug in memory core.
>>>
>>> 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,
>>> };
>>>
>>>
>>> Because of that     .impl.min_access_size = 2,
>>> the 1 byte write should be converted to a 2 byte
>>> read+2 byte write.
>>>
>>> docs/devel/memory.rst just says:
>>> - .impl.min_access_size, .impl.max_access_size define the access sizes
>>>  (in bytes) supported by the *implementation*; other access sizes will be
>>>  emulated using the ones available.  For example a 4-byte write will be
>>>  emulated using four 1-byte writes, if .impl.max_access_size = 1.
>>>
>>>
>>>
>>> Instead what we have is:
>>>
>>> MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
>>>                                         hwaddr addr,
>>>                                         uint64_t data,
>>>                                         MemOp op,
>>>                                         MemTxAttrs attrs)
>>> {
>>>    unsigned size = memop_size(op);
>>>
>>>    if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
>>>        unassigned_mem_write(mr, addr, data, size);
>>>        return MEMTX_DECODE_ERROR;
>>>    }
>>>
>>>    adjust_endianness(mr, &data, op);
>>>
>>>
>>> ---
>>>
>>>
>>> static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op)
>>> {
>>>    if ((op & MO_BSWAP) != devend_memop(mr->ops->endianness)) {
>>>        switch (op & MO_SIZE) {
>>>        case MO_8:
>>>            break;
>>>        case MO_16:
>>>            *data = bswap16(*data);
>>>            break;
>>>        case MO_32:
>>>            *data = bswap32(*data);
>>>            break;
>>>        case MO_64:
>>>            *data = bswap64(*data);
>>>            break;
>>>        default:
>>>            g_assert_not_reached();
>>>        }
>>>    }
>>> }
>>>
>>> so the byte swap is based on size before extending it to
>>> .impl.min_access_size, not after.
>>
>> OK thanks for the analysis, I'll wait what comes out of this.
>>
>>> Also, no read happens which I suspect is also a bug,
>>> but could be harmless ...
>>
>> I did not check if a read happens or not but generally I think that may be
>> another source of bugs if the memory core changes writes to read + write
>> because for some devices a read might have side effects
>
>
> IMHO if reads have side effects then devices should not set .impl.min_access_size

If all this was documented somewhere then you could expect people know how 
to use it. I could not find any info on how to correctly use these so had 
to guess most of the time. Maybe it's somewhere there but not an obvious 
place. The code implementing this is not something that most people read 
just expect it to work. Could be useful to make a section about it in the 
docs. Maybe some words about it might help in here: 
https://www.qemu.org/docs/master/devel/memory.html

> ... but given we did not previously do the read, maybe we should keep
> it that way at least for the time being.

How do you know there was no read before this write? Did you check it? 
I've only added a printf in the write method and saw the value was swapped 
but did not check if there was a read before that. There are no traces in 
these methods so maybe I would not see it unless adding a printf there too.

Regards,
BALATON Zoltan

Re: ACPI endianness

[Michael S. Tsirkin]

On Mon, Oct 11, 2021 at 03:51:01PM +0200, BALATON Zoltan wrote:
> > ... but given we did not previously do the read, maybe we should keep
> > it that way at least for the time being.
> 
> How do you know there was no read before this write? Did you check it? I've
> only added a printf in the write method and saw the value was swapped but
> did not check if there was a read before that. There are no traces in these
> methods so maybe I would not see it unless adding a printf there too.

All I am saying is that qemu did not convert a write into
a read+write.

Re: ACPI endianness

[BALATON Zoltan]

On Mon, 11 Oct 2021, Michael S. Tsirkin wrote:
> On Mon, Oct 11, 2021 at 03:51:01PM +0200, BALATON Zoltan wrote:
>>> ... but given we did not previously do the read, maybe we should keep
>>> it that way at least for the time being.
>>
>> How do you know there was no read before this write? Did you check it? I've
>> only added a printf in the write method and saw the value was swapped but
>> did not check if there was a read before that. There are no traces in these
>> methods so maybe I would not see it unless adding a printf there too.
>
> All I am saying is that qemu did not convert a write into
> a read+write.

OK confirmed that by disabling pm_io_space_update() in hw/isa/vt82c686.c 
so the via-pm region is never mapped and then modifying the log messages 
for invalid accesses (patch sent separately) I get:

~ # poweroff
Sent SIGKILL to all processes
Requesting system poweroff
pci_cfg_write vt82c686b-usb-uhci 12:3 @0xc0 <- 0x8f00
pci_cfg_write vt82c686b-usb-uhci 12:2 @0xc0 <- 0x8f00
[   16.498465] reboot: Power down
Invalid write at addr 0xFE000F05, size 1, region '(null)', reason: rejected
Invalid write at addr 0xF05, size 1, region '(null)', reason: rejected

So the guest only tries to write one byte but not sure if the read 
generated within memory.c would show up in these logs. I suspect if you 
fixed that you'd get all sorts of problems with other device models so the 
less likely to break anything fix would be to go back to native endian for 
acpi but I wait for what you come up with. I'd like this to be fixed one 
way or another for 6.2 and fixing endianness would probably be enough for 
that.

Regards,
BALATON Zoltan

Re: ACPI endianness

[BALATON Zoltan]

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

On Mon, 11 Oct 2021, Michael S. Tsirkin wrote:
> On Mon, Oct 11, 2021 at 12:13:55PM +0200, BALATON Zoltan wrote:
>> On Mon, 11 Oct 2021, Philippe Mathieu-Daudé wrote:
>>> On 10/10/21 15:24, BALATON Zoltan wrote:
>>>> Hello,
>>>>
>>>> I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as
>>>> part of the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c)
>>>> and found that the guest writes to ACPI PM1aCNT register come out with
>>>> wrong endianness but not shure why. I have this:
>>>>
>>>> $ qemu-system-ppc -M pegasos2 -monitor stdio
>>>> (qemu) info mtree
>>>> [...]
>>>> memory-region: pci1-io
>>>>   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
>>>> [...]
>>>>     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
>>>>       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
>>>>       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
>>>>       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
>>>>
>>>> memory-region: system
>>>>   0000000000000000-ffffffffffffffff (prio 0, i/o): system
>>>>     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
>>>>     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win
>>>> @pci1-mem 0000000080000000-00000000bfffffff
>>>>     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win
>>>> @pci0-mem 00000000c0000000-00000000dfffffff
>>>>     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
>>>>     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win
>>>> @pci0-io 0000000000000000-0000000000ffffff
>>>>     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win
>>>> @pci0-mem 0000000000000000-0000000000ffffff
>>>>     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win
>>>> @pci1-mem 0000000000000000-0000000000ffffff
>>>>     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win
>>>> @pci1-io 0000000000000000-0000000000ffffff
>>>>     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win
>>>> @pci1-mem 00000000ff800000-00000000ffffffff
>>>>     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
>>>>
>>>> The guest (which is big endian PPC and I think wotks on real hardware)
>>>> writes to 0xf05 in the io region which should be the high byte of the
>>>> little endian register but in the acpi code it comes out wrong, instead
>>>> of 0x2800 I get in acpi_pm1_cnt_write: val=0x28
>>>
>>> Looks like a northbridge issue (MV64340).
>>> Does Pegasos2 enables bus swapping?
>>> See hw/pci-host/mv64361.c:585:
>>>
>>> static void warn_swap_bit(uint64_t val)
>>> {
>>>    if ((val & 0x3000000ULL) >> 24 != 1) {
>>>        qemu_log_mask(LOG_UNIMP, "%s: Data swap not implemented", __func__);
>>>    }
>>> }
>>
>> No, guests don't use this feature just byteswap themselves and write little
>> endian values to the mapped io region. (Or in this case just write one byte
>> of the 16 bit value, it specifically writes 0x28 to 0xf05.) That's why I
>> think the device model should not byteswap itself so the acpi regions should
>> be declared native endian and let the guest handle it. Some mentions I've
>> found say that native endian means don't byteswap, little endian means
>> byteswap if vcpu is big endian and big endian means byteswap if vcpu is
>> little endian. Since guests already account for this and write little endian
>> values to these regions there's no need to byteswap in device model and
>> changing to native endian should not affect little endian machines so unless
>> there's a better argument I'll try sending a patch.
>>
>> Regards,
>> BALATON Zoltan

Ping? Should I submit a patch changing these acpi regions to NATIVE_ENDIAN 
for now as a work around for 6.2 or is there a chance this could be fixed 
in some other way before the freeze?

Regards,
BALATON Zoltan

> native endian means endian-ness is open-coded in the device handler
> itself.  I think you just found a bug in memory core.
>
> 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,
> };
>
>
> Because of that     .impl.min_access_size = 2,
> the 1 byte write should be converted to a 2 byte
> read+2 byte write.
>
> docs/devel/memory.rst just says:
> - .impl.min_access_size, .impl.max_access_size define the access sizes
>  (in bytes) supported by the *implementation*; other access sizes will be
>  emulated using the ones available.  For example a 4-byte write will be
>  emulated using four 1-byte writes, if .impl.max_access_size = 1.
>
>
>
> Instead what we have is:
>
> MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
>                                         hwaddr addr,
>                                         uint64_t data,
>                                         MemOp op,
>                                         MemTxAttrs attrs)
> {
>    unsigned size = memop_size(op);
>
>    if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
>        unassigned_mem_write(mr, addr, data, size);
>        return MEMTX_DECODE_ERROR;
>    }
>
>    adjust_endianness(mr, &data, op);
>
>
> ---
>
>
> static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op)
> {
>    if ((op & MO_BSWAP) != devend_memop(mr->ops->endianness)) {
>        switch (op & MO_SIZE) {
>        case MO_8:
>            break;
>        case MO_16:
>            *data = bswap16(*data);
>            break;
>        case MO_32:
>            *data = bswap32(*data);
>            break;
>        case MO_64:
>            *data = bswap64(*data);
>            break;
>        default:
>            g_assert_not_reached();
>        }
>    }
> }
>
> so the byte swap is based on size before extending it to
> .impl.min_access_size, not after.
>
> Also, no read happens which I suspect is also a bug,
> but could be harmless ...
>
> Paolo, any feedback here?
>
>

Re: ACPI endianness

[Michael S. Tsirkin]

On Mon, Oct 25, 2021 at 05:05:46PM +0200, BALATON Zoltan wrote:
> On Mon, 11 Oct 2021, Michael S. Tsirkin wrote:
> > On Mon, Oct 11, 2021 at 12:13:55PM +0200, BALATON Zoltan wrote:
> > > On Mon, 11 Oct 2021, Philippe Mathieu-Daudé wrote:
> > > > On 10/10/21 15:24, BALATON Zoltan wrote:
> > > > > Hello,
> > > > > 
> > > > > I'm trying to fix shutdown and reboot on pegasos2 which uses ACPI as
> > > > > part of the VIA VT8231 (similar to and modelled in hw/isa/vt82c686b.c)
> > > > > and found that the guest writes to ACPI PM1aCNT register come out with
> > > > > wrong endianness but not shure why. I have this:
> > > > > 
> > > > > $ qemu-system-ppc -M pegasos2 -monitor stdio
> > > > > (qemu) info mtree
> > > > > [...]
> > > > > memory-region: pci1-io
> > > > >   0000000000000000-000000000000ffff (prio 0, i/o): pci1-io
> > > > > [...]
> > > > >     0000000000000f00-0000000000000f7f (prio 0, i/o): via-pm
> > > > >       0000000000000f00-0000000000000f03 (prio 0, i/o): acpi-evt
> > > > >       0000000000000f04-0000000000000f05 (prio 0, i/o): acpi-cnt
> > > > >       0000000000000f08-0000000000000f0b (prio 0, i/o): acpi-tmr
> > > > > 
> > > > > memory-region: system
> > > > >   0000000000000000-ffffffffffffffff (prio 0, i/o): system
> > > > >     0000000000000000-000000001fffffff (prio 0, ram): pegasos2.ram
> > > > >     0000000080000000-00000000bfffffff (prio 0, i/o): alias pci1-mem0-win
> > > > > @pci1-mem 0000000080000000-00000000bfffffff
> > > > >     00000000c0000000-00000000dfffffff (prio 0, i/o): alias pci0-mem0-win
> > > > > @pci0-mem 00000000c0000000-00000000dfffffff
> > > > >     00000000f1000000-00000000f100ffff (prio 0, i/o): mv64361
> > > > >     00000000f8000000-00000000f8ffffff (prio 0, i/o): alias pci0-io-win
> > > > > @pci0-io 0000000000000000-0000000000ffffff
> > > > >     00000000f9000000-00000000f9ffffff (prio 0, i/o): alias pci0-mem1-win
> > > > > @pci0-mem 0000000000000000-0000000000ffffff
> > > > >     00000000fd000000-00000000fdffffff (prio 0, i/o): alias pci1-mem1-win
> > > > > @pci1-mem 0000000000000000-0000000000ffffff
> > > > >     00000000fe000000-00000000feffffff (prio 0, i/o): alias pci1-io-win
> > > > > @pci1-io 0000000000000000-0000000000ffffff
> > > > >     00000000ff800000-00000000ffffffff (prio 0, i/o): alias pci1-mem3-win
> > > > > @pci1-mem 00000000ff800000-00000000ffffffff
> > > > >     00000000fff00000-00000000fff7ffff (prio 0, rom): pegasos2.rom
> > > > > 
> > > > > The guest (which is big endian PPC and I think wotks on real hardware)
> > > > > writes to 0xf05 in the io region which should be the high byte of the
> > > > > little endian register but in the acpi code it comes out wrong, instead
> > > > > of 0x2800 I get in acpi_pm1_cnt_write: val=0x28
> > > > 
> > > > Looks like a northbridge issue (MV64340).
> > > > Does Pegasos2 enables bus swapping?
> > > > See hw/pci-host/mv64361.c:585:
> > > > 
> > > > static void warn_swap_bit(uint64_t val)
> > > > {
> > > >    if ((val & 0x3000000ULL) >> 24 != 1) {
> > > >        qemu_log_mask(LOG_UNIMP, "%s: Data swap not implemented", __func__);
> > > >    }
> > > > }
> > > 
> > > No, guests don't use this feature just byteswap themselves and write little
> > > endian values to the mapped io region. (Or in this case just write one byte
> > > of the 16 bit value, it specifically writes 0x28 to 0xf05.) That's why I
> > > think the device model should not byteswap itself so the acpi regions should
> > > be declared native endian and let the guest handle it. Some mentions I've
> > > found say that native endian means don't byteswap, little endian means
> > > byteswap if vcpu is big endian and big endian means byteswap if vcpu is
> > > little endian. Since guests already account for this and write little endian
> > > values to these regions there's no need to byteswap in device model and
> > > changing to native endian should not affect little endian machines so unless
> > > there's a better argument I'll try sending a patch.
> > > 
> > > Regards,
> > > BALATON Zoltan
> 
> Ping? Should I submit a patch changing these acpi regions to NATIVE_ENDIAN
> for now as a work around for 6.2 or is there a chance this could be fixed in
> some other way before the freeze?
> 
> Regards,
> BALATON Zoltan

Paolo, did you say you will look into fixing this?
We don't want more NATIVE_ENDIAN things, do we?


> > native endian means endian-ness is open-coded in the device handler
> > itself.  I think you just found a bug in memory core.
> > 
> > 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,
> > };
> > 
> > 
> > Because of that     .impl.min_access_size = 2,
> > the 1 byte write should be converted to a 2 byte
> > read+2 byte write.
> > 
> > docs/devel/memory.rst just says:
> > - .impl.min_access_size, .impl.max_access_size define the access sizes
> >  (in bytes) supported by the *implementation*; other access sizes will be
> >  emulated using the ones available.  For example a 4-byte write will be
> >  emulated using four 1-byte writes, if .impl.max_access_size = 1.
> > 
> > 
> > 
> > Instead what we have is:
> > 
> > MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
> >                                         hwaddr addr,
> >                                         uint64_t data,
> >                                         MemOp op,
> >                                         MemTxAttrs attrs)
> > {
> >    unsigned size = memop_size(op);
> > 
> >    if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
> >        unassigned_mem_write(mr, addr, data, size);
> >        return MEMTX_DECODE_ERROR;
> >    }
> > 
> >    adjust_endianness(mr, &data, op);
> > 
> > 
> > ---
> > 
> > 
> > static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op)
> > {
> >    if ((op & MO_BSWAP) != devend_memop(mr->ops->endianness)) {
> >        switch (op & MO_SIZE) {
> >        case MO_8:
> >            break;
> >        case MO_16:
> >            *data = bswap16(*data);
> >            break;
> >        case MO_32:
> >            *data = bswap32(*data);
> >            break;
> >        case MO_64:
> >            *data = bswap64(*data);
> >            break;
> >        default:
> >            g_assert_not_reached();
> >        }
> >    }
> > }
> > 
> > so the byte swap is based on size before extending it to
> > .impl.min_access_size, not after.
> > 
> > Also, no read happens which I suspect is also a bug,
> > but could be harmless ...
> > 
> > Paolo, any feedback here?
> > 
> >