From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47101) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WxpP1-0005tU-3f for qemu-devel@nongnu.org; Thu, 19 Jun 2014 23:22:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WxpOv-0007fQ-JI for qemu-devel@nongnu.org; Thu, 19 Jun 2014 23:22:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50708) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WxpOv-0007fB-BU for qemu-devel@nongnu.org; Thu, 19 Jun 2014 23:22:05 -0400 Message-ID: <1403234516.3707.279.camel@ul30vt.home> From: Alex Williamson Date: Thu, 19 Jun 2014 21:21:56 -0600 In-Reply-To: <53A242B1.3050402@ozlabs.ru> References: <1403091321-31721-1-git-send-email-aik@ozlabs.ru> <1403117875.3707.184.camel@ul30vt.home> <53A231DC.2040004@ozlabs.ru> <53A242B1.3050402@ozlabs.ru> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] vfio: Make BARs native endian List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: qemu-devel@nongnu.org, Nikunj A Dadhania , Alexander Graf On Thu, 2014-06-19 at 11:53 +1000, Alexey Kardashevskiy wrote: > 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 > :) ) Yeah, it seems like it's correct otherwise and the patch itself looks ok. Thanks, Alex > >>> 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 > >>> --- > >>> 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) > >> > >> > >> > > > > > >