Messages in this thread | | | Date | Thu, 14 May 2015 13:53:57 +1000 | From | Alexey Kardashevskiy <> | Subject | Re: [PATCH kernel v10 23/34] powerpc/iommu/powernv: Release replaced TCE |
| |
On 05/14/2015 01:00 AM, Thomas Huth wrote: > On Tue, 12 May 2015 01:39:12 +1000 > Alexey Kardashevskiy <aik@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 <aik@ozlabs.ru> >> [aw: for the vfio related changes] >> Acked-by: Alex Williamson <alex.williamson@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))
I really (really) like braces. Is there any kernel code design rule against it?
> >> + 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.
I believe checkpatch.pl won't like it. There is a check against single lines having braces after "if" statements.
> >> - 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 >
-- Alexey
| |