Re: [PATCH] virtio-pci: Fix endianness of virtio config

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/10/2012 05:35 AM, Benjamin Herrenschmidt wrote:
> The virtio config area in PIO space is a bit special. The initial
> header is little endian but the rest (device specific) is guest
> native endian.
>
> The PIO accessors for PCI on machines that don't have native IO ports
> assume that all PIO is little endian, which works fine for everything
> except the above.
>
> A complicated way to fix it would be to split the BAR into two memory
> regions with different endianess settings, but this isn't practical
> to do, besides, the PIO code doesn't honor region endianness anyway
> (I have a patch for that too but it isn't necessary at this stage).
>
> So I decided to go for the quick fix instead which consists of
> reverting the swap in virtio-pci in selected places, hoping that when
> we eventually do a "v2" of the virtio protocols, we sort that out once
> and for all using a fixed endian setting for everything.
>
> Unfortunately, that mean moving virtio-pci from Makefile.objs to
> Makefile.target so we can use TARGET_WORDS_BIGENDIAN which would
> otherwise be poisoned.
>
> Signed-off-by: Benjamin Herrenschmidt<benh@kernel.crashing.org>
> ---
>   Makefile.objs   |    1 -
>   Makefile.target |    1 +
>   hw/virtio-pci.c |   24 ++++++++++++++++++++++--
>   3 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 4f6d26c..b721fca 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -186,7 +186,6 @@ hw-obj-y >   hw-obj-y += vl.o loader.o
>   hw-obj-$(CONFIG_VIRTIO) += virtio-console.o
>   hw-obj-y += usb-libhw.o
> -hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>   hw-obj-y += fw_cfg.o
>   hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o
>   hw-obj-$(CONFIG_PCI) += msix.o msi.o
> diff --git a/Makefile.target b/Makefile.target
> index ef6834b..03d44c3 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -191,6 +191,7 @@ obj-y = arch_init.o cpus.o monitor.o machine.o gdbstub.o balloon.o ioport.o
>   # need to fix this properly
>   obj-$(CONFIG_NO_PCI) += pci-stub.o
>   obj-$(CONFIG_VIRTIO) += virtio.o virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial-bus.o
> +obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>   obj-y += vhost_net.o
>   obj-$(CONFIG_VHOST_NET) += vhost.o
>   obj-$(CONFIG_REALLY_VIRTFS) += 9pfs/virtio-9p-device.o
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 77b75bc..ca70e42 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -412,20 +412,34 @@ static uint32_t virtio_pci_config_readw(void *opaque, uint32_t addr)
>   {
>       VirtIOPCIProxy *proxy = opaque;
>       uint32_t config = VIRTIO_PCI_CONFIG(&proxy->pci_dev);
> +    uint16_t val;
>       if (addr<  config)
>           return virtio_ioport_read(proxy, addr);
>       addr -= config;
> -    return virtio_config_readw(proxy->vdev, addr);
> +    val = virtio_config_readw(proxy->vdev, addr);
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    /* virtio is odd, ioports are LE but config space is target native
> +     * endian. However, in qemu, all PIO is LE, so we need to re-swap
> +     * on BE targets
> +     */
> +    val = bswap16(val);
> +#endif

I think this is the only reasonable way to do it, but I'd suggest adding 
target_is_bigendian() to arch_init.c that way we wouldn't have to move the 
object from libhw.

Regards,

Anthony Liguori


> +    return val;
>   }
>
>   static uint32_t virtio_pci_config_readl(void *opaque, uint32_t addr)
>   {
>       VirtIOPCIProxy *proxy = opaque;
>       uint32_t config = VIRTIO_PCI_CONFIG(&proxy->pci_dev);
> +    uint32_t val;
>       if (addr<  config)
>           return virtio_ioport_read(proxy, addr);
>       addr -= config;
> -    return virtio_config_readl(proxy->vdev, addr);
> +    val = virtio_config_readl(proxy->vdev, addr);
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap32(val);
> +#endif
> +    return val;
>   }
>
>   static void virtio_pci_config_writeb(void *opaque, uint32_t addr, uint32_t val)
> @@ -449,6 +463,9 @@ static void virtio_pci_config_writew(void *opaque, uint32_t addr, uint32_t val)
>           return;
>       }
>       addr -= config;
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap16(val);
> +#endif
>       virtio_config_writew(proxy->vdev, addr, val);
>   }
>
> @@ -461,6 +478,9 @@ static void virtio_pci_config_writel(void *opaque, uint32_t addr, uint32_t val)
>           return;
>       }
>       addr -= config;
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap32(val);
> +#endif
>       virtio_config_writel(proxy->vdev, addr, val);
>   }
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html