Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965407AbbEMPBA (ORCPT ); Wed, 13 May 2015 11:01:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55407 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934578AbbEMPAz (ORCPT ); Wed, 13 May 2015 11:00:55 -0400 Date: Wed, 13 May 2015 17:00:48 +0200 From: Thomas Huth To: Alexey Kardashevskiy Cc: linuxppc-dev@lists.ozlabs.org, Wei Yang , Gavin Shan , linux-kernel@vger.kernel.org, Alex Williamson , Paul Mackerras , David Gibson Subject: Re: [PATCH kernel v10 23/34] powerpc/iommu/powernv: Release replaced TCE Message-ID: <20150513170048.2c6115c6@thh440s> In-Reply-To: <1431358763-24371-24-git-send-email-aik@ozlabs.ru> References: <1431358763-24371-1-git-send-email-aik@ozlabs.ru> <1431358763-24371-24-git-send-email-aik@ozlabs.ru> Organization: Red Hat MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5750 Lines: 161 On Tue, 12 May 2015 01:39:12 +1000 Alexey Kardashevskiy 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 > [aw: for the vfio related changes] > Acked-by: Alex Williamson [...] > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/