From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46613) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wu0Yh-00027F-CD for qemu-devel@nongnu.org; Mon, 09 Jun 2014 10:28:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wu0Yc-00033V-9W for qemu-devel@nongnu.org; Mon, 09 Jun 2014 10:28:23 -0400 Message-ID: <1402324093.14174.49.camel@ul30vt.home> From: Alex Williamson Date: Mon, 09 Jun 2014 08:28:13 -0600 In-Reply-To: <5393A4F6.50508@ozlabs.ru> References: <1402025693-25096-1-git-send-email-aik@ozlabs.ru> <1402025693-25096-5-git-send-email-aik@ozlabs.ru> <1402073878.14174.12.camel@ul30vt.home> <539244FF.7000007@ozlabs.ru> <5393A4F6.50508@ozlabs.ru> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 4/4] vfio: Enable for SPAPR List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Gavin Shan , Alexander Graf On Sun, 2014-06-08 at 09:49 +1000, Alexey Kardashevskiy wrote: > On 06/07/2014 08:47 AM, Alexey Kardashevskiy wrote: > > On 06/07/2014 02:57 AM, Alex Williamson wrote: > >> On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote: > >>> This turns the sPAPR support on and enables VFIO container use > >>> in the kernel. > >>> > >>> This extends vfio_connect_container to support VFIO_SPAPR_TCE_IOMMU type > >>> in the host kernel. > >>> > >>> This registers a memory listener which sPAPR IOMMU will notify when > >>> executing H_PUT_TCE/etc DMA calls. The listener then will notify the host > >>> kernel about DMA map/unmap operation via VFIO_IOMMU_MAP_DMA/ > >>> VFIO_IOMMU_UNMAP_DMA ioctls. > >>> > >>> This executes VFIO_IOMMU_ENABLE ioctl to make sure that the IOMMU is free > >>> of mappings and can be exclusively given to the user. At the moment SPAPR > >>> is the only platform requiring this call to be implemented. > >>> > >>> Note that the host kernel function implementing VFIO_IOMMU_DISABLE > >>> is called automatically when container's fd is closed so there is > >>> no need to call it explicitly from QEMU. This may change in the future. > >> > >> So you're saying we rely on a behavior that may change in the future... > >> The kernel must do cleanup, it can never rely on userspace to do the > >> right thing or we have a bug. Does that mean we can remove the "may > >> change in the future" part of this comment or is that directed at QEMU's > >> behavior and not the kernel's? > > > > > > I wanted to say that if/when we support PCI hotplug or dynamic IOMMU group > > reconfiguration (we might be able to do so in POWER9 or later, do not know > > details), we will call DISABLE explicitly. > > > Any better? > === > Note that the host kernel function implementing VFIO_IOMMU_DISABLE > is called automatically when container's fd is closed so there is > no need to call it explicitly from QEMU. We may need to call > VFIO_IOMMU_DISABLE explicitely in the future for some sort of dynamic > reconfiguration (PCI hotplug or dynamic IOMMU group management). > === That sounds better. I'd also appreciate a comment in the code where enable is called stating that disable is implicit in closing the container fd. Thanks, Alex > > > >> A comment in the code where we setup the > >> release handler or call ENABLE would be a good idea too. Thanks, > > > > > > > > > >> > >> Alex > >> > >>> Signed-off-by: Alexey Kardashevskiy > >>> --- > >>> Changes: > >>> v8: > >>> * added note about VFIO_IOMMU_DISABLE in the commit log > >>> > >>> v7: > >>> * added more details in commit log > >>> > >>> v5: > >>> * multiple returns converted to gotos > >>> > >>> v4: > >>> * fixed format string to use %m which is a glibc extension: > >>> "Print output of strerror(errno). No argument is required." > >>> --- > >>> hw/misc/vfio.c | 28 ++++++++++++++++++++++++++++ > >>> 1 file changed, 28 insertions(+) > >>> > >>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c > >>> index bb77934..78d2045 100644 > >>> --- a/hw/misc/vfio.c > >>> +++ b/hw/misc/vfio.c > >>> @@ -3650,6 +3650,34 @@ 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)) { > >>> + ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); > >>> + if (ret) { > >>> + error_report("vfio: failed to set group container: %m"); > >>> + ret = -errno; > >>> + goto free_container_exit; > >>> + } > >>> + > >>> + ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_SPAPR_TCE_IOMMU); > >>> + if (ret) { > >>> + error_report("vfio: failed to set iommu for container: %m"); > >>> + ret = -errno; > >>> + goto free_container_exit; > >>> + } > >>> + > >>> + 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); > >>> + > >>> } else { > >>> error_report("vfio: No available IOMMU models"); > >>> ret = -EINVAL; > >> > >> > >> > > > > > >