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
