On Tue, 12 May 2015 01:39:12 +1000
Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
> At the moment writing new TCE value to the IOMMU table fails with EBUSY
> if there is a valid entry already. However PAPR specification allows
> the guest to write new TCE value without clearing it first.
>
> Another problem this patch is addressing is the use of pool locks for
> external IOMMU users such as VFIO. The pool locks are to protect
> DMA page allocator rather than entries and since the host kernel does
> not control what pages are in use, there is no point in pool locks and
> exchange()+put_page(oldtce) is sufficient to avoid possible races.
>
> This adds an exchange() callback to iommu_table_ops which does the same
> thing as set() plus it returns replaced TCE and DMA direction so
> the caller can release the pages afterwards. The exchange() receives
> a physical address unlike set() which receives linear mapping address;
> and returns a physical address as the clear() does.
>
> This implements exchange() for P5IOC2/IODA/IODA2. This adds a requirement
> for a platform to have exchange() implemented in order to support VFIO.
>
> This replaces iommu_tce_build() and iommu_clear_tce() with
> a single iommu_tce_xchg().
>
> This makes sure that TCE permission bits are not set in TCE passed to
> IOMMU API as those are to be calculated by platform code from
> DMA direction.
>
> This moves SetPageDirty() to the IOMMU code to make it work for both
> VFIO ioctl interface in in-kernel TCE acceleration (when it becomes
> available later).
>
> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
> [aw: for the vfio related changes]
> Acked-by: Alex Williamson <alex.william...@redhat.com>
[...]
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index 6275164..1287d49 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -962,10 +962,7 @@ EXPORT_SYMBOL_GPL(iommu_tce_clear_param_check);
> int iommu_tce_put_param_check(struct iommu_table *tbl,
> unsigned long ioba, unsigned long tce)
> {
> - if (!(tce & (TCE_PCI_WRITE | TCE_PCI_READ)))
> - return -EINVAL;
> -
> - if (tce & ~(IOMMU_PAGE_MASK(tbl) | TCE_PCI_WRITE | TCE_PCI_READ))
> + if (tce & ~IOMMU_PAGE_MASK(tbl))
> return -EINVAL;
>
> if (ioba & ~IOMMU_PAGE_MASK(tbl))
> @@ -982,44 +979,16 @@ int iommu_tce_put_param_check(struct iommu_table *tbl,
> }
> EXPORT_SYMBOL_GPL(iommu_tce_put_param_check);
>
> -unsigned long iommu_clear_tce(struct iommu_table *tbl, unsigned long entry)
> +long iommu_tce_xchg(struct iommu_table *tbl, unsigned long entry,
> + unsigned long *hpa, enum dma_data_direction *direction)
> {
> - unsigned long oldtce;
> - struct iommu_pool *pool = get_pool(tbl, entry);
> + long ret;
>
> - spin_lock(&(pool->lock));
> + ret = tbl->it_ops->exchange(tbl, entry, hpa, direction);
>
> - oldtce = tbl->it_ops->get(tbl, entry);
> - if (oldtce & (TCE_PCI_WRITE | TCE_PCI_READ))
> - tbl->it_ops->clear(tbl, entry, 1);
> - else
> - oldtce = 0;
> -
> - spin_unlock(&(pool->lock));
> -
> - return oldtce;
> -}
> -EXPORT_SYMBOL_GPL(iommu_clear_tce);
> -
> -/*
> - * hwaddr is a kernel virtual address here (0xc... bazillion),
> - * tce_build converts it to a physical address.
> - */
> -int iommu_tce_build(struct iommu_table *tbl, unsigned long entry,
> - unsigned long hwaddr, enum dma_data_direction direction)
> -{
> - int ret = -EBUSY;
> - unsigned long oldtce;
> - struct iommu_pool *pool = get_pool(tbl, entry);
> -
> - spin_lock(&(pool->lock));
> -
> - oldtce = tbl->it_ops->get(tbl, entry);
> - /* Add new entry if it is not busy */
> - if (!(oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)))
> - ret = tbl->it_ops->set(tbl, entry, 1, hwaddr, direction, NULL);
> -
> - spin_unlock(&(pool->lock));
> + if (!ret && ((*direction == DMA_FROM_DEVICE) ||
> + (*direction == DMA_BIDIRECTIONAL)))
You could drop some of the parentheses:
if (!ret && (*direction == DMA_FROM_DEVICE ||
*direction == DMA_BIDIRECTIONAL))
> + SetPageDirty(pfn_to_page(*hpa >> PAGE_SHIFT));
>
> /* if (unlikely(ret))
> pr_err("iommu_tce: %s failed on hwaddr=%lx ioba=%lx kva=%lx
> ret=%d\n",
[...]
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c
> b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 2ead291..0724ec8 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -236,18 +236,11 @@ static void tce_iommu_release(void *iommu_data)
[...]
> @@ -405,19 +410,26 @@ static long tce_iommu_ioctl(void *iommu_data,
> return -EINVAL;
>
> /* iova is checked by the IOMMU API */
> - tce = param.vaddr;
> if (param.flags & VFIO_DMA_MAP_FLAG_READ)
> - tce |= TCE_PCI_READ;
> - if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
> - tce |= TCE_PCI_WRITE;
> + if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
> + direction = DMA_BIDIRECTIONAL;
> + else
> + direction = DMA_TO_DEVICE;
> + else
> + if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
> + direction = DMA_FROM_DEVICE;
> + else
> + return -EINVAL;
IMHO some curly braces for the outer if-statement would be really fine
here.
> - ret = iommu_tce_put_param_check(tbl, param.iova, tce);
> + ret = iommu_tce_put_param_check(tbl, param.iova, param.vaddr);
> if (ret)
> return ret;
>
> ret = tce_iommu_build(container, tbl,
> param.iova >> tbl->it_page_shift,
> - tce, param.size >> tbl->it_page_shift);
> + param.vaddr,
> + param.size >> tbl->it_page_shift,
> + direction);
>
> iommu_flush_tce(tbl);
>
Thomas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
