[Qemu-devel] [PATCH 0/2] vfio: Another try to fix ROM BAR endianness

[Qemu-devel] [PATCH 0/2] vfio: Another try to fix ROM BAR endianness

[Alexey Kardashevskiy]

I did some tests on LE/BE guest/host combinations and came up with
these 2 patches. Please comment. Thanks.


Alexey Kardashevskiy (1):
  Revert "vfio: Make BARs native endian"

Nikunj A Dadhania (1):
  vfio: make rom read endian sensitive

 hw/misc/vfio.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH 1/2] Revert "vfio: Make BARs native endian"

[Alexey Kardashevskiy]

This reverts commit c40708176a6b52b73bec14796b7c71b882ceb102.

The idea not to swap bytes at all did not work out as MMIO interface
is defined as target host endian and it is always big-endian for PPC64
so the original part broke PPC64 guests on LE hosts (x86/ppc64le).

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/misc/vfio.c | 41 ++++++++++-------------------------------
 1 file changed, 10 insertions(+), 31 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 40dcaa6..22ebcbd 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -1098,10 +1098,10 @@ static void vfio_bar_write(void *opaque, hwaddr addr,
         buf.byte = data;
         break;
     case 2:
-        buf.word = data;
+        buf.word = cpu_to_le16(data);
         break;
     case 4:
-        buf.dword = data;
+        buf.dword = cpu_to_le32(data);
         break;
     default:
         hw_error("vfio: unsupported write size, %d bytes", size);
@@ -1158,10 +1158,10 @@ static uint64_t vfio_bar_read(void *opaque,
         data = buf.byte;
         break;
     case 2:
-        data = buf.word;
+        data = le16_to_cpu(buf.word);
         break;
     case 4:
-        data = buf.dword;
+        data = le32_to_cpu(buf.dword);
         break;
     default:
         hw_error("vfio: unsupported read size, %d bytes", size);
@@ -1188,7 +1188,7 @@ static uint64_t vfio_bar_read(void *opaque,
 static const MemoryRegionOps vfio_bar_ops = {
     .read = vfio_bar_read,
     .write = vfio_bar_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static void vfio_pci_load_rom(VFIODevice *vdev)
@@ -1250,42 +1250,21 @@ static void vfio_pci_load_rom(VFIODevice *vdev)
 static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
 {
     VFIODevice *vdev = opaque;
-    union {
-        uint8_t byte;
-        uint16_t word;
-        uint32_t dword;
-        uint64_t qword;
-    } buf;
-    uint64_t data = 0;
+    uint64_t val = ((uint64_t)1 << (size * 8)) - 1;
 
     /* Load the ROM lazily when the guest tries to read it */
     if (unlikely(!vdev->rom && !vdev->rom_read_failed)) {
         vfio_pci_load_rom(vdev);
     }
 
-    memcpy(&buf, vdev->rom + addr,
+    memcpy(&val, vdev->rom + addr,
            (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0);
 
-    switch (size) {
-    case 1:
-        data = buf.byte;
-        break;
-    case 2:
-        data = buf.word;
-        break;
-    case 4:
-        data = buf.dword;
-        break;
-    default:
-        hw_error("vfio: unsupported read size, %d bytes", size);
-        break;
-    }
-
     DPRINTF("%s(%04x:%02x:%02x.%x, 0x%"HWADDR_PRIx", 0x%x) = 0x%"PRIx64"\n",
             __func__, vdev->host.domain, vdev->host.bus, vdev->host.slot,
-            vdev->host.function, addr, size, data);
+            vdev->host.function, addr, size, val);
 
-    return data;
+    return val;
 }
 
 static void vfio_rom_write(void *opaque, hwaddr addr,
@@ -1296,7 +1275,7 @@ static void vfio_rom_write(void *opaque, hwaddr addr,
 static const MemoryRegionOps vfio_rom_ops = {
     .read = vfio_rom_read,
     .write = vfio_rom_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static bool vfio_blacklist_opt_rom(VFIODevice *vdev)
-- 
2.0.0

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] Revert "vfio: Make BARs native endian"

[Greg Kurz]

On Tue,  9 Sep 2014 21:34:46 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> This reverts commit c40708176a6b52b73bec14796b7c71b882ceb102.
> 
> The idea not to swap bytes at all did not work out as MMIO interface
> is defined as target host endian and it is always big-endian for PPC64

s/target host endian/target endian/

> so the original part broke PPC64 guests on LE hosts (x86/ppc64le).
> 

What about a simpler description ?

The resulting code wrongly assumed target and host endianness are the same.

> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/misc/vfio.c | 41 ++++++++++-------------------------------
>  1 file changed, 10 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 40dcaa6..22ebcbd 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -1098,10 +1098,10 @@ static void vfio_bar_write(void *opaque, hwaddr addr,
>          buf.byte = data;
>          break;
>      case 2:
> -        buf.word = data;
> +        buf.word = cpu_to_le16(data);
>          break;
>      case 4:
> -        buf.dword = data;
> +        buf.dword = cpu_to_le32(data);
>          break;
>      default:
>          hw_error("vfio: unsupported write size, %d bytes", size);
> @@ -1158,10 +1158,10 @@ static uint64_t vfio_bar_read(void *opaque,
>          data = buf.byte;
>          break;
>      case 2:
> -        data = buf.word;
> +        data = le16_to_cpu(buf.word);
>          break;
>      case 4:
> -        data = buf.dword;
> +        data = le32_to_cpu(buf.dword);
>          break;
>      default:
>          hw_error("vfio: unsupported read size, %d bytes", size);
> @@ -1188,7 +1188,7 @@ static uint64_t vfio_bar_read(void *opaque,
>  static const MemoryRegionOps vfio_bar_ops = {
>      .read = vfio_bar_read,
>      .write = vfio_bar_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> 
>  static void vfio_pci_load_rom(VFIODevice *vdev)
> @@ -1250,42 +1250,21 @@ static void vfio_pci_load_rom(VFIODevice *vdev)
>  static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      VFIODevice *vdev = opaque;
> -    union {
> -        uint8_t byte;
> -        uint16_t word;
> -        uint32_t dword;
> -        uint64_t qword;
> -    } buf;
> -    uint64_t data = 0;
> +    uint64_t val = ((uint64_t)1 << (size * 8)) - 1;
> 
>      /* Load the ROM lazily when the guest tries to read it */
>      if (unlikely(!vdev->rom && !vdev->rom_read_failed)) {
>          vfio_pci_load_rom(vdev);
>      }
> 
> -    memcpy(&buf, vdev->rom + addr,
> +    memcpy(&val, vdev->rom + addr,
>             (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0);
> 
> -    switch (size) {
> -    case 1:
> -        data = buf.byte;
> -        break;
> -    case 2:
> -        data = buf.word;
> -        break;
> -    case 4:
> -        data = buf.dword;
> -        break;
> -    default:
> -        hw_error("vfio: unsupported read size, %d bytes", size);
> -        break;
> -    }
> -
>      DPRINTF("%s(%04x:%02x:%02x.%x, 0x%"HWADDR_PRIx", 0x%x) = 0x%"PRIx64"\n",
>              __func__, vdev->host.domain, vdev->host.bus, vdev->host.slot,
> -            vdev->host.function, addr, size, data);
> +            vdev->host.function, addr, size, val);
> 
> -    return data;
> +    return val;
>  }
> 
>  static void vfio_rom_write(void *opaque, hwaddr addr,
> @@ -1296,7 +1275,7 @@ static void vfio_rom_write(void *opaque, hwaddr addr,
>  static const MemoryRegionOps vfio_rom_ops = {
>      .read = vfio_rom_read,
>      .write = vfio_rom_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> 
>  static bool vfio_blacklist_opt_rom(VFIODevice *vdev)



-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.

[Qemu-devel] [PATCH 2/2] vfio: make rom read endian sensitive

[Alexey Kardashevskiy]

From: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

All memory regions used by VFIO are LITTLE_ENDIAN and they
already take care of endiannes when accessing real device BARs
except ROM - it was broken on BE hosts.

This fixes endiannes for ROM BARs the same way as it is done
for other BARs.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
[aik: added commit log]
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/misc/vfio.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 22ebcbd..d66f3d2 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -1250,7 +1250,13 @@ static void vfio_pci_load_rom(VFIODevice *vdev)
 static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
 {
     VFIODevice *vdev = opaque;
-    uint64_t val = ((uint64_t)1 << (size * 8)) - 1;
+    union {
+        uint8_t byte;
+        uint16_t word;
+        uint32_t dword;
+        uint64_t qword;
+    } val;
+    uint64_t data = 0;
 
     /* Load the ROM lazily when the guest tries to read it */
     if (unlikely(!vdev->rom && !vdev->rom_read_failed)) {
@@ -1260,11 +1266,26 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
     memcpy(&val, vdev->rom + addr,
            (addr < vdev->rom_size) ? MIN(size, vdev->rom_size - addr) : 0);
 
+    switch (size) {
+    case 1:
+        data = val.byte;
+        break;
+    case 2:
+        data = le16_to_cpu(val.word);
+        break;
+    case 4:
+        data = le32_to_cpu(val.dword);
+        break;
+    default:
+        hw_error("vfio: unsupported read size, %d bytes\n", size);
+        break;
+    }
+
     DPRINTF("%s(%04x:%02x:%02x.%x, 0x%"HWADDR_PRIx", 0x%x) = 0x%"PRIx64"\n",
             __func__, vdev->host.domain, vdev->host.bus, vdev->host.slot,
-            vdev->host.function, addr, size, val);
+            vdev->host.function, addr, size, data);
 
-    return val;
+    return data;
 }
 
 static void vfio_rom_write(void *opaque, hwaddr addr,
-- 
2.0.0

Re: [Qemu-devel] [PATCH 0/2] vfio: Another try to fix ROM BAR endianness

[Alexander Graf]

On 09.09.14 13:34, Alexey Kardashevskiy wrote:
> I did some tests on LE/BE guest/host combinations and came up with
> these 2 patches. Please comment. Thanks.

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

Re: [Qemu-devel] [PATCH 0/2] vfio: Another try to fix ROM BAR endianness

[Alex Williamson]

On Tue, 2014-09-09 at 21:34 +1000, Alexey Kardashevskiy wrote:
> I did some tests on LE/BE guest/host combinations and came up with
> these 2 patches. Please comment. Thanks.

Have you tried BE guest/LE host?  IIRC, the g3beige tcg guest on x86
host used to work with a vfio-pci NIC.  The ROM is of course typically
useless in this mode, but it would be nice to make sure we haven't
broken BAR access for that combination.  Do we have any better BE tcg
targets with PCI support these days?  Thanks,

Alex

Re: [Qemu-devel] [PATCH 0/2] vfio: Another try to fix ROM BAR endianness

[Alexey Kardashevskiy]

On 09/12/2014 07:20 AM, Alex Williamson wrote:
> On Tue, 2014-09-09 at 21:34 +1000, Alexey Kardashevskiy wrote:
>> I did some tests on LE/BE guest/host combinations and came up with
>> these 2 patches. Please comment. Thanks.
> 
> Have you tried BE guest/LE host?  

Yes.


> IIRC, the g3beige tcg guest on x86
> host used to work with a vfio-pci NIC.  The ROM is of course typically
> useless in this mode, but it would be nice to make sure we haven't
> broken BAR access for that combination.  Do we have any better BE tcg
> targets with PCI support these days?  Thanks,

pseries? PPC64 TCG can do VFIO.


-- 
Alexey

Re: [Qemu-devel] [PATCH 0/2] vfio: Another try to fix ROM BAR endianness

[Alexey Kardashevskiy]

On 09/12/2014 11:30 AM, Alexey Kardashevskiy wrote:
> On 09/12/2014 07:20 AM, Alex Williamson wrote:
>> On Tue, 2014-09-09 at 21:34 +1000, Alexey Kardashevskiy wrote:
>>> I did some tests on LE/BE guest/host combinations and came up with
>>> these 2 patches. Please comment. Thanks.
>>
>> Have you tried BE guest/LE host?  
> 
> Yes.

Double checked, LE-BE host-guest work in any combination.


>> IIRC, the g3beige tcg guest on x86
>> host used to work with a vfio-pci NIC.  The ROM is of course typically
>> useless in this mode, but it would be nice to make sure we haven't
>> broken BAR access for that combination.  Do we have any better BE tcg
>> targets with PCI support these days?  Thanks,
> 
> pseries? PPC64 TCG can do VFIO.

And checked this too, VFIO works in BE TCG PPC64 guest running on PPC64 LE
host.


-- 
Alexey

Re: [Qemu-devel] [PATCH 0/2] vfio: Another try to fix ROM BAR endianness

[Alex Williamson]

On Thu, 2014-09-18 at 23:55 +1000, Alexey Kardashevskiy wrote:
> On 09/12/2014 11:30 AM, Alexey Kardashevskiy wrote:
> > On 09/12/2014 07:20 AM, Alex Williamson wrote:
> >> On Tue, 2014-09-09 at 21:34 +1000, Alexey Kardashevskiy wrote:
> >>> I did some tests on LE/BE guest/host combinations and came up with
> >>> these 2 patches. Please comment. Thanks.
> >>
> >> Have you tried BE guest/LE host?  
> > 
> > Yes.
> 
> Double checked, LE-BE host-guest work in any combination.
> 
> 
> >> IIRC, the g3beige tcg guest on x86
> >> host used to work with a vfio-pci NIC.  The ROM is of course typically
> >> useless in this mode, but it would be nice to make sure we haven't
> >> broken BAR access for that combination.  Do we have any better BE tcg
> >> targets with PCI support these days?  Thanks,
> > 
> > pseries? PPC64 TCG can do VFIO.
> 
> And checked this too, VFIO works in BE TCG PPC64 guest running on PPC64 LE
> host.

Thanks for the additional testing.  Greg had some comments on patch 1/2,
perhaps you could address them or update the comments.  Thanks,

Alex