Hi Michael! Thanks for reviewing the patch! > we always get LE values from memory subsystem, > not target endian values:
I see. So do you think the patch is correct in eliminating the extra swap (as virtio_config_readw for example already makes a swap)? Thanks, andré On Thu, Jan 9, 2020 at 7:50 AM Michael S. Tsirkin <m...@redhat.com> wrote: > > On Wed, Jan 08, 2020 at 01:16:18PM -0300, Andre Silva wrote: > > Remove the bswap function calls after reading and before writing > > memory bytes in virtio_pci_config_read and virtio_pci_config_write > > because they are reverting back an already swapped bytes. > > > > Consider the table below in the context of virtio_pci_config_read > > function. > > > > Host Target virtio-config-read[wl] > > swap? virtio-is-big-endian? extra bswap? > > Should be Final result Final result ok? > > ----- ------- ------------------------ ----------------------- > > -------------- ----------- -------------- ------------------ > > LE BE s(x) true s(s(x)) > > s(x) x No > > LE LE x false - > > x x Yes > > BE LE s(x) false - > > s(x) s(x) Yes > > BE BE x true s(x) > > x s(x) No > > we always get LE values from memory subsystem, > not target endian values: > > static const MemoryRegionOps virtio_pci_config_ops = { > .read = virtio_pci_config_read, > .write = virtio_pci_config_write, > .impl = { > .min_access_size = 1, > .max_access_size = 4, > }, > .endianness = DEVICE_LITTLE_ENDIAN, > }; > > > This triggers another swap in address_space_ldl_internal > (memory_ldst.inc.c). > > > > In table above, when target is big endian and VirtIO is pre 1.0, > > function virtio_is_big_endian would return true and the extra > > swap would be executed, reverting the previous swap made by > > virtio_config_read[wl]. > > > > The 's(x)' means that a swap function was applied at > > address x. 'LE' is little endian and 'BE' is big endian. The > > 'Final result' column is the returned value from > > virtio_pci_config_read, considering a target Virtio pre 1.0. > > 'x' means that target's value was not swapped in Qemu, 's(x)' means > > that Qemu will use a swapped value. > > > > If we remove the extra swap made in virtio_pci_config_read we will > > have the correct result in any host/target combination, both for > > VirtIO pre 1.0 or later versions. > > > > The same reasoning applies to virtio_pci_config_write. > > > > Signed-off-by: Andre Silva <afscoe...@gmail.com> > > --- > > hw/virtio/virtio-pci.c | 12 ------------ > > 1 file changed, 12 deletions(-) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index c6b47a9c73..4ba9e847f3 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -431,15 +431,9 @@ static uint64_t virtio_pci_config_read(void *opaque, > > hwaddr addr, > > break; > > case 2: > > val = virtio_config_readw(vdev, addr); > > - if (virtio_is_big_endian(vdev)) { > > - val = bswap16(val); > > - } > > break; > > case 4: > > val = virtio_config_readl(vdev, addr); > > - if (virtio_is_big_endian(vdev)) { > > - val = bswap32(val); > > - } > > break; > > } > > return val; > > @@ -465,15 +459,9 @@ static void virtio_pci_config_write(void *opaque, > > hwaddr addr, > > virtio_config_writeb(vdev, addr, val); > > break; > > case 2: > > - if (virtio_is_big_endian(vdev)) { > > - val = bswap16(val); > > - } > > virtio_config_writew(vdev, addr, val); > > break; > > case 4: > > - if (virtio_is_big_endian(vdev)) { > > - val = bswap32(val); > > - } > > virtio_config_writel(vdev, addr, val); > > break; > > } > > -- > > 2.24.1 >