Re: [Qemu-devel] [PATCH] vfio: Make BARs native endian

[Alexey Kardashevskiy]

On 06/19/2014 10:42 AM, Alexey Kardashevskiy wrote:
> On 06/19/2014 04:57 AM, Alex Williamson wrote:
>> On Wed, 2014-06-18 at 21:35 +1000, Alexey Kardashevskiy wrote:
>>> Slow BAR access path is used when VFIO fails to mmap() BAR or TCG is used.
>>
>> TCG can use the mmap too
> 
> Oh, right.

I'll remove "tcg" and what else is missing here? I thought the original
commit log already had all answers on the questions you askes (or clarified
:) )
Thanks!

> 
> 
>>
>>> Since this is just a transport between the guest and a device, there is
>>> no need to do endianness swapping.
>>>
>>> This changes BARs to use native endianness. Since non-ROM BARs were
>>> doing byte swapping, we need to remove it so does the patch. No change
>>> in behavior is expected though.
>>>
>>> ROM BARs were declared little endian but byte swapping was not
>>> implemented for them so they never actually worked on big endian systems.
>>> This fixes endiannes for ROM BARs: this declares them native endian and
>>> fixes access sizes.
>>
>> So the only actual behavior change we expect from this is to fix ROM
>> access on big endian where we had an unbalanced set of byte swaps.  BAR
>> access worked because the byte swapping in the read/write handler
>> canceled the byte swapping in QEMU because we declared the BARs as
>> little endian.  Otherwise we're just removing canceling byte swaps for
>> big endian.  Is that correct?
> 
> Yes.
> 
>>
>> Also, as the kernel patch appears to not produce any behavioral change,
>> only eliminates canceling swaps, there's no dependency between patches,
>> right?  Thanks,
> 
> That is right.






> 
>>
>> Alex
>>
>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>> ---
>>>  hw/misc/vfio.c | 41 +++++++++++++++++++++++++++++++----------
>>>  1 file changed, 31 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>>> index 7437c2e..3eb3c71 100644
>>> --- a/hw/misc/vfio.c
>>> +++ b/hw/misc/vfio.c
>>> @@ -1052,10 +1052,10 @@ static void vfio_bar_write(void *opaque, hwaddr 
>>> addr,
>>>          buf.byte = data;
>>>          break;
>>>      case 2:
>>> -        buf.word = cpu_to_le16(data);
>>> +        buf.word = data;
>>>          break;
>>>      case 4:
>>> -        buf.dword = cpu_to_le32(data);
>>> +        buf.dword = data;
>>>          break;
>>>      default:
>>>          hw_error("vfio: unsupported write size, %d bytes", size);
>>> @@ -1112,10 +1112,10 @@ static uint64_t vfio_bar_read(void *opaque,
>>>          data = buf.byte;
>>>          break;
>>>      case 2:
>>> -        data = le16_to_cpu(buf.word);
>>> +        data = buf.word;
>>>          break;
>>>      case 4:
>>> -        data = le32_to_cpu(buf.dword);
>>> +        data = buf.dword;
>>>          break;
>>>      default:
>>>          hw_error("vfio: unsupported read size, %d bytes", size);
>>> @@ -1142,7 +1142,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_LITTLE_ENDIAN,
>>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>>  };
>>>  
>>>  static void vfio_pci_load_rom(VFIODevice *vdev)
>>> @@ -1204,21 +1204,42 @@ 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;
>>> +    } buf;
>>> +    uint64_t data = 0;
>>>  
>>>      /* 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(&val, vdev->rom + addr,
>>> +    memcpy(&buf, 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, val);
>>> +            vdev->host.function, addr, size, data);
>>>  
>>> -    return val;
>>> +    return data;
>>>  }
>>>  
>>>  static void vfio_rom_write(void *opaque, hwaddr addr,
>>> @@ -1229,7 +1250,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_LITTLE_ENDIAN,
>>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>>  };
>>>  
>>>  static bool vfio_blacklist_opt_rom(VFIODevice *vdev)
>>
>>
>>
> 
> 


-- 
Alexey