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 <address@hidden> 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 <address@hidden>
> ---
>  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                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

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