[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH qemu v16 01/19] vfio: Delay DMA address space list

From: Alexey Kardashevskiy
Subject: Re: [Qemu-ppc] [PATCH qemu v16 01/19] vfio: Delay DMA address space listener release
Date: Fri, 13 May 2016 17:16:48 +1000
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0

On 05/06/2016 08:39 AM, Alex Williamson wrote:
On Wed,  4 May 2016 16:52:13 +1000
Alexey Kardashevskiy <address@hidden> wrote:

This postpones VFIO container deinitialization to let region_del()
callbacks (called via vfio_listener_release) do proper clean up
while the group is still attached to the container.

Any mappings within the container should clean themselves up when the
container is deprivleged by removing the last group in the kernel. Is
the issue that that doesn't happen, which would be a spapr vfio kernel
bug, or that our QEMU side structures get all out of whack if we let
that happen?

My mailbase got corrupted, missed that.

This is mostly for "[PATCH qemu v16 17/19] spapr_iommu, vfio, memory: Notify IOMMU about starting/stopping being used by VFIO", I should have put 01/19 and 02/19 right before 17/19, sorry about that.

Every reboot the spapr machine removes all (i.e. one or two) windows and creates the default one.

I do this by memory_region_del_subregion(iommu_mr) + memory_region_add_subregion(iommu_mr). Which gets translated to VFIO_IOMMU_SPAPR_TCE_REMOVE + VFIO_IOMMU_SPAPR_TCE_CREATE via vfio_memory_listener if there is VFIO; no direct calls from spapr to vfio => cool. During the machine reset, the VFIO device is there with the container and groups attached, at some point with no windows.

Now to VFIO plug/unplug.

When VFIO plug happens, vfio_memory_listener is created, region_add() is called, the hardware window is created (via VFIO_IOMMU_SPAPR_TCE_CREATE). Unplugging should end up doing VFIO_IOMMU_SPAPR_TCE_REMOVE somehow. If region_del() is not called when the container is being destroyed (as before this patchset), then the kernel cleans and destroys windows when close(container->fd) is called or when qemu is killed (and this fd is naturally closed), I hope this answers the comment from 02/19.

So far so good (right?)

However I also have a guest view of the TCE table, this is what the guest sees and this is what emulated PCI devices use. This guest view is either allocated in the KVM (so H_PUT_TCE can be handled quickly right in the host kernel, even in real mode) or userspace (VFIO case).

I generally want the guest view to be in the KVM. However when I plug VFIO, I have to move the table to the userspace. When I unplug VFIO, I want to do the opposite so I need a way to tell spapr that it can move the table. region_del() seemed a natural way of doing this as region_add() is already doing the opposite part.

With this patchset, each IOMMU MR gets a usage counter, region_add() does +1, region_del() does -1 (yeah, not extremely optimal during reset). When the counter goes from 0 to 1, vfio_start() hook is called, when the counter becomes 0 - vfio_stop(). Note that we may have multiple VFIO containers on the same PHB.

Without 01/19 and 02/19, I'll have to repeat region_del()'s counter decrement steps in vfio_disconnect_container(). And I still cannot move counting from region_add() to vfio_connect_container() so there will be asymmetry which I am fine with, I am just checking here - what would be the best approach here?


Signed-off-by: Alexey Kardashevskiy <address@hidden>
 hw/vfio/common.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index fe5ec6a..0b40262 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -921,23 +921,31 @@ static void vfio_disconnect_container(VFIOGroup *group)
     VFIOContainer *container = group->container;

-    if (ioctl(group->fd, VFIO_GROUP_UNSET_CONTAINER, &container->fd)) {
-        error_report("vfio: error disconnecting group %d from container",
-                     group->groupid);
-    }
     QLIST_REMOVE(group, container_next);
+    if (QLIST_EMPTY(&container->group_list)) {
+        VFIOGuestIOMMU *giommu;
+        vfio_listener_release(container);
+        QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
+            memory_region_unregister_iommu_notifier(&giommu->n);
+        }
+    }
     group->container = NULL;
+    if (ioctl(group->fd, VFIO_GROUP_UNSET_CONTAINER, &container->fd)) {
+        error_report("vfio: error disconnecting group %d from container",
+                     group->groupid);
+    }

     if (QLIST_EMPTY(&container->group_list)) {
         VFIOAddressSpace *space = container->space;
         VFIOGuestIOMMU *giommu, *tmp;

-        vfio_listener_release(container);
         QLIST_REMOVE(container, next);

         QLIST_FOREACH_SAFE(giommu, &container->giommu_list, giommu_next, tmp) {
-            memory_region_unregister_iommu_notifier(&giommu->n);
             QLIST_REMOVE(giommu, giommu_next);

I'm not spotting why this is a 2-pass process vs simply moving the
existing QLIST_EMPTY cleanup above the ioctl.  Thanks,


reply via email to

[Prev in Thread] Current Thread [Next in Thread]