On 05/13/2015 04:32 PM, Gavin Shan wrote:

On Tue, May 12, 2015 at 01:39:00AM +1000, Alexey Kardashevskiy wrote:

quoted

This is a pretty mechanical patch to make next patches simpler.

New tce_iommu_unuse_page() helper does put_page() now but it might skip
that after the memory registering patch applied.

As we are here, this removes unnecessary checks for a value returned
by pfn_to_page() as it cannot possibly return NULL.

This moves tce_iommu_disable() later to let tce_iommu_clear() know if
the container has been enabled because if it has not been, then
put_page() must not be called on TCEs from the TCE table. This situation
is not yet possible but it will after KVM acceleration patchset is
applied.

This changes code to work with physical addresses rather than linear
mapping addresses for better code readability. Following patches will
add an xchg() callback for an IOMMU table which will accept/return
physical addresses (unlike current tce_build()) which will eliminate
redundant conversions.

Signed-off-by: Alexey Kardashevskiy <redacted>
[aw: for the vfio related changes]
Acked-by: Alex Williamson <redacted>
Reviewed-by: David Gibson <redacted>

Reviewed-by: Gavin Shan <redacted>

quoted

---
Changes:
v9:
* changed helpers to work with physical addresses rather than linear
(for simplicity - later ::xchg() will receive physical and avoid
additional convertions)

v6:
* tce_get_hva() returns hva via a pointer
---
drivers/vfio/vfio_iommu_spapr_tce.c | 61 +++++++++++++++++++++++++------------
1 file changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index e21479c..115d5e6 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c

@@ -191,69 +191,90 @@ static void tce_iommu_release(void *iommu_data)

	struct tce_container *container = iommu_data;

	WARN_ON(container->tbl && !container->tbl->it_group);
-	tce_iommu_disable(container);

	if (container->tbl && container->tbl->it_group)
		tce_iommu_detach_group(iommu_data, container->tbl->it_group);

+	tce_iommu_disable(container);
	mutex_destroy(&container->lock);

	kfree(container);
}

+static void tce_iommu_unuse_page(struct tce_container *container,
+		unsigned long oldtce)
+{
+	struct page *page;
+
+	if (!(oldtce & (TCE_PCI_READ | TCE_PCI_WRITE)))
+		return;
+

It might be worthy to have a global helper function in iommu.h to check
if the given TCE entry is empty or not, for better readability. I would
think the helper function is used here and there :-)

The patchset adds one later, called iommu_tce_direction() ;)

In general, I removed TCE_PCI_READ, TCE_PCI_WRITE from everywhere but 
powernv code and used  enum dma_data_direction instead as these bits are 
SPAPR TCE protocol specific and VFIO IOMMU API or 
arch/powerpc/kernel/iommu.c do not receive/handle these bits, they have 
their own, platform independent.


-- 
Alexey