* Re: ACPI endianness
2021-10-11 12:19 ` Michael S. Tsirkin
@ 2021-10-11 13:14 ` Jonathan Cameron
2021-10-11 13:27 ` BALATON Zoltan
2021-10-25 15:05 ` BALATON Zoltan
2 siblings, 0 replies; 14+ messages in thread
From: Jonathan Cameron @ 2021-10-11 13:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Igor Mammedov, pbonzini, Philippe Mathieu-Daudé, qemu-devel
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?
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: ACPI endianness
2021-10-11 12:19 ` Michael S. Tsirkin
2021-10-11 13:14 ` Jonathan Cameron
@ 2021-10-11 13:27 ` BALATON Zoltan
2021-10-11 13:34 ` Michael S. Tsirkin
2021-10-25 15:05 ` BALATON Zoltan
2 siblings, 1 reply; 14+ messages in thread
From: BALATON Zoltan @ 2021-10-11 13:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Igor Mammedov, Philippe Mathieu-Daudé, qemu-devel, pbonzini
[-- 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: ACPI endianness
2021-10-11 13:27 ` BALATON Zoltan
@ 2021-10-11 13:34 ` Michael S. Tsirkin
2021-10-11 13:51 ` BALATON Zoltan
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2021-10-11 13:34 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Igor Mammedov, Philippe Mathieu-Daudé, qemu-devel, pbonzini
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: ACPI endianness
2021-10-11 13:34 ` Michael S. Tsirkin
@ 2021-10-11 13:51 ` BALATON Zoltan
2021-10-11 15:55 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: BALATON Zoltan @ 2021-10-11 13:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Igor Mammedov, Philippe Mathieu-Daudé, qemu-devel, pbonzini
[-- 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: ACPI endianness
2021-10-11 13:51 ` BALATON Zoltan
@ 2021-10-11 15:55 ` Michael S. Tsirkin
2021-10-11 17:47 ` BALATON Zoltan
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2021-10-11 15:55 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Igor Mammedov, Philippe Mathieu-Daudé, qemu-devel, pbonzini
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: ACPI endianness
2021-10-11 15:55 ` Michael S. Tsirkin
@ 2021-10-11 17:47 ` BALATON Zoltan
0 siblings, 0 replies; 14+ messages in thread
From: BALATON Zoltan @ 2021-10-11 17:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Igor Mammedov, Philippe Mathieu-Daudé, qemu-devel, pbonzini
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: ACPI endianness
2021-10-11 12:19 ` Michael S. Tsirkin
2021-10-11 13:14 ` Jonathan Cameron
2021-10-11 13:27 ` BALATON Zoltan
@ 2021-10-25 15:05 ` BALATON Zoltan
2021-10-25 17:10 ` Michael S. Tsirkin
2 siblings, 1 reply; 14+ messages in thread
From: BALATON Zoltan @ 2021-10-25 15:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Igor Mammedov, Philippe Mathieu-Daudé, qemu-devel, pbonzini
[-- 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?
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: ACPI endianness
2021-10-25 15:05 ` BALATON Zoltan
@ 2021-10-25 17:10 ` Michael S. Tsirkin
0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2021-10-25 17:10 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Igor Mammedov, Philippe Mathieu-Daudé, qemu-devel, pbonzini
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?
> >
> >
^ permalink raw reply [flat|nested] 14+ messages in thread