From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48841) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCQ0P-0007hU-OG for qemu-devel@nongnu.org; Tue, 07 Jul 2015 06:21:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCQ0L-0004Nr-Pj for qemu-devel@nongnu.org; Tue, 07 Jul 2015 06:21:37 -0400 Date: Tue, 7 Jul 2015 12:21:25 +0200 From: Thomas Huth Message-ID: <20150707122125.0e58b09e@thh440s> In-Reply-To: <559BA465.50009@ozlabs.ru> References: <1436148670-6592-1-git-send-email-aik@ozlabs.ru> <1436148670-6592-14-git-send-email-aik@ozlabs.ru> <20150707092311.728e2cd7@thh440s> <559BA465.50009@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu v10 13/14] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Michael Roth , qemu-devel@nongnu.org, Gavin Shan , Alex Williamson , qemu-ppc@nongnu.org, David Gibson On Tue, 7 Jul 2015 20:05:25 +1000 Alexey Kardashevskiy wrote: > On 07/07/2015 05:23 PM, Thomas Huth wrote: > > On Mon, 6 Jul 2015 12:11:09 +1000 > > Alexey Kardashevskiy wrote: ... > >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >> index 8eacfd7..0c7ba8c 100644 > >> --- a/hw/vfio/common.c > >> +++ b/hw/vfio/common.c > >> @@ -488,6 +488,76 @@ static void vfio_listener_release(VFIOContainer *container) > >> memory_listener_unregister(&container->iommu_data.type1.listener); > >> } > >> > >> +static void vfio_ram_do_region(VFIOContainer *container, > >> + MemoryRegionSection *section, unsigned long req) > >> +{ > >> + int ret; > >> + struct vfio_iommu_spapr_register_memory reg = { .argsz = sizeof(reg) }; > >> + > >> + if (!memory_region_is_ram(section->mr) || > >> + memory_region_is_skip_dump(section->mr)) { > >> + return; > >> + } > >> + > >> + if (unlikely((section->offset_within_region & (getpagesize() - 1)))) { > >> + error_report("%s received unaligned region", __func__); > >> + return; > >> + } > >> + > >> + reg.vaddr = (__u64) memory_region_get_ram_ptr(section->mr) + > > > > We're in usespace here ... I think it would be better to use uint64_t > > instead of the kernel-type __u64. > > We are calling a kernel here - @reg is a kernel-defined struct. If you grep for __u64 in the QEMU sources, you'll see that hardly anybody is using this type - even if calling ioctls. So for consistency, I'd really suggest to use uint64_t here. > >> @@ -698,14 +768,18 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as) > >> > >> container->iommu_data.type1.initialized = true; > >> > >> - } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) { > >> + } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) || > >> + ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU)) { > >> + bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU); > > > > That "!!" sounds somewhat wrong here. I think you either want to check > > for "ioctl() == 1" (because only in this case you can be sure that v2 > > is supported), or you can simply omit the "!!" because you're 100% sure > > that the ioctl only returns 0 or 1 (and never a negative error code). > > > The host kernel does not return an error on these ioctls, it returns 0 or > 1. And "!!" is shorter than "(bool)". VFIO_CHECK_EXTENSION for Type1 does > exactly the same already. Simply using nothing instead is even shorter than using "!!". The compiler is smart enough to convert from 0 and 1 to bool. "!!" is IMHO quite ugly and should only be used when it is really necessary. > >> @@ -717,19 +791,36 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as) > >> * when container fd is closed so we do not call it explicitly > >> * in this file. > >> */ > >> - ret = ioctl(fd, VFIO_IOMMU_ENABLE); > >> - if (ret) { > >> - error_report("vfio: failed to enable container: %m"); > >> - ret = -errno; > >> - goto free_container_exit; > >> + if (!v2) { > >> + ret = ioctl(fd, VFIO_IOMMU_ENABLE); > >> + if (ret) { > >> + error_report("vfio: failed to enable container: %m"); > >> + ret = -errno; > >> + goto free_container_exit; > >> + } > >> } > >> > >> container->iommu_data.type1.listener = vfio_memory_listener; > >> - container->iommu_data.release = vfio_listener_release; > >> - > >> memory_listener_register(&container->iommu_data.type1.listener, > >> container->space->as); > >> > >> + if (!v2) { > >> + container->iommu_data.release = vfio_listener_release; > >> + } else { > >> + container->iommu_data.release = vfio_spapr_listener_release_v2; > >> + container->iommu_data.register_listener = > >> + vfio_ram_memory_listener; > >> + memory_listener_register(&container->iommu_data.register_listener, > >> + &address_space_memory); > >> + > >> + if (container->iommu_data.ram_reg_error) { > >> + error_report("vfio: RAM memory listener initialization failed for container"); > > > > Line > 80 columns? > > afaik user visible strings are an exception in QEMU and kernel. You're right for the kernel, but AFAIK QEMU (currently still) has a hard limit at 80 columns. Thomas