Re: [Qemu-devel] [PATCH 1/2] virtio-pci: Fix endianness of virtio config

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/10/2012 06:07 PM, Alexander Graf wrote:
> From: Benjamin Herrenschmidt<benh@kernel.crashing.org>
>
> 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>
> Signed-off-by: Alexander Graf<agraf@suse.de>
> [agraf: keep virtio in libhw and determine endianness through a
>          helper function in exec.c]

Both:

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   exec.c          |   14 ++++++++++++++
>   hw/virtio-pci.c |   28 ++++++++++++++++++++++++++--
>   2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index b1d6602..319e867 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -4390,6 +4390,20 @@ tb_page_addr_t get_page_addr_code(CPUState *env1, target_ulong addr)
>       return qemu_ram_addr_from_host_nofail(p);
>   }
>
> +/*
> + * A helper function for the _utterly broken_ virtio device model to find out if
> + * it's running on a big endian machine. Don't do this at home kids!
> + */
> +bool virtio_is_big_endian(void);
> +bool virtio_is_big_endian(void)
> +{
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +    return true;
> +#else
> +    return false;
> +#endif
> +}
> +
>   #define MMUSUFFIX _cmmu
>   #undef GETPC
>   #define GETPC() NULL
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 77b75bc..c8d5b5f 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -89,6 +89,9 @@
>    */
>   #define wmb() do { } while (0)
>
> +/* HACK for virtio to determine if it's running a big endian guest */
> +bool virtio_is_big_endian(void);
> +
>   /* virtio device */
>
>   static void virtio_pci_notify(void *opaque, uint16_t vector)
> @@ -412,20 +415,35 @@ 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);
> +    if (virtio_is_big_endian()) {
> +        /*
> +         * 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);
> +    }
> +    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);
> +    if (virtio_is_big_endian()) {
> +        val = bswap32(val);
> +    }
> +    return val;
>   }
>
>   static void virtio_pci_config_writeb(void *opaque, uint32_t addr, uint32_t val)
> @@ -449,6 +467,9 @@ static void virtio_pci_config_writew(void *opaque, uint32_t addr, uint32_t val)
>           return;
>       }
>       addr -= config;
> +    if (virtio_is_big_endian()) {
> +        val = bswap16(val);
> +    }
>       virtio_config_writew(proxy->vdev, addr, val);
>   }
>
> @@ -461,6 +482,9 @@ static void virtio_pci_config_writel(void *opaque, uint32_t addr, uint32_t val)
>           return;
>       }
>       addr -= config;
> +    if (virtio_is_big_endian()) {
> +        val = bswap32(val);
> +    }
>       virtio_config_writel(proxy->vdev, addr, val);
>   }
>