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

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

[Alexander Graf]

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]
---
 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);
 }
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/2] virtio: change memcpy to guest reads

[Alexander Graf]

When accessing the device specific virtio config space, we memcpy
the data into a variable in QEMU. At that point we're basically
pulling host endianness into the game which is a really bad idea.

So instead, let's use the target specific load/store helpers for
memory pointers which fetch things in target endianness. The whole
array is already populated in target endianness anyways
(see virtio-blk).

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/virtio.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 81ecc40..74cc038 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -539,7 +539,7 @@ uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
     if (addr > (vdev->config_len - sizeof(val)))
         return (uint32_t)-1;
 
-    memcpy(&val, vdev->config + addr, sizeof(val));
+    val = ldub_p(vdev->config + addr);
     return val;
 }
 
@@ -552,7 +552,7 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr)
     if (addr > (vdev->config_len - sizeof(val)))
         return (uint32_t)-1;
 
-    memcpy(&val, vdev->config + addr, sizeof(val));
+    val = lduw_p(vdev->config + addr);
     return val;
 }
 
@@ -565,7 +565,7 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr)
     if (addr > (vdev->config_len - sizeof(val)))
         return (uint32_t)-1;
 
-    memcpy(&val, vdev->config + addr, sizeof(val));
+    val = ldl_p(vdev->config + addr);
     return val;
 }
 
@@ -576,7 +576,7 @@ void virtio_config_writeb(VirtIODevice *vdev, uint32_t addr, uint32_t data)
     if (addr > (vdev->config_len - sizeof(val)))
         return;
 
-    memcpy(vdev->config + addr, &val, sizeof(val));
+    stb_p(vdev->config + addr, val);
 
     if (vdev->set_config)
         vdev->set_config(vdev, vdev->config);
@@ -589,7 +589,7 @@ void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data)
     if (addr > (vdev->config_len - sizeof(val)))
         return;
 
-    memcpy(vdev->config + addr, &val, sizeof(val));
+    stw_p(vdev->config + addr, val);
 
     if (vdev->set_config)
         vdev->set_config(vdev, vdev->config);
@@ -602,7 +602,7 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data)
     if (addr > (vdev->config_len - sizeof(val)))
         return;
 
-    memcpy(vdev->config + addr, &val, sizeof(val));
+    stl_p(vdev->config + addr, val);
 
     if (vdev->set_config)
         vdev->set_config(vdev, vdev->config);
-- 
1.6.0.2

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

[Anthony Liguori]

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);
>   }
>

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

[Alexander Graf]

On 11.01.2012, at 01:07, Anthony Liguori wrote:

> 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>

Cool :). Queued up in ppc-next.


Alex

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

[Benjamin Herrenschmidt]

On Wed, 2012-01-11 at 01:08 +0100, Alexander Graf wrote:
> On 11.01.2012, at 01:07, Anthony Liguori wrote:
> 
> > 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>
> 
> Cool :). Queued up in ppc-next.

Fix the commit log, you aren't moving it to Makefile.target anymore :-)

Cheers,
Ben.