Re: [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table

On 13/02/2019 07:52, Alex Williamson wrote:

On Mon, 11 Feb 2019 18:49:17 +1100
Alexey Kardashevskiy [off-list ref] wrote:

quoted

VFIO TCE IOMMU v2 owns IOMMU tables so when detach a IOMMU group from
a container, we need to unset those from a group so we call unset_window()
so do we unconditionally. We also unset tables when removing a DMA window

Patch looks ok, but this first sentence trails off into a bit of a word
salad.  Care to refine a bit?  Thanks,

Fair comment, sorry for the salad. How about this?

===
VFIO TCE IOMMU v2 owns IOMMU tables. When we detach an IOMMU group from
a container, we need to unset these tables from the group which we do by
calling unset_window(). We also unset tables when removing a DMA window
via the VFIO_IOMMU_SPAPR_TCE_REMOVE ioctl.
===

Alex

quoted

via the VFIO_IOMMU_SPAPR_TCE_REMOVE ioctl.

The window removal checks if the table actually exists (hidden inside
tce_iommu_find_table()) but the group detaching does not so the user
may see duplicating messages:
pci 0009:03     : [PE# fd] Removing DMA window #0
pci 0009:03     : [PE# fd] Removing DMA window #1
pci 0009:03     : [PE# fd] Removing DMA window #0
pci 0009:03     : [PE# fd] Removing DMA window #1

At the moment this is not a problem as the second invocation
of unset_window() writes zeroes to the HW registers again and exits early
as there is no table.

Signed-off-by: Alexey Kardashevskiy <redacted>
---

When doing VFIO PCI hot unplug, first we remove the DMA window and
set container->tables[num] - this is a first couple of messages.
Then we detach the group and then we see another couple of the same
messages which confused myself.
---
 drivers/vfio/vfio_iommu_spapr_tce.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index c424913..8dbb270 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c

@@ -1235,7 +1235,8 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
 	}
 
 	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
-		table_group->ops->unset_window(table_group, i);
+		if (container->tables[i])
+			table_group->ops->unset_window(table_group, i);
 
 	table_group->ops->release_ownership(table_group);
 }

-- 
Alexey