| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <55541C55.4060807@ozlabs.ru>
Date: Thu, 14 May 2015 13:53:57 +1000
From: Alexey Kardashevskiy <aik@...abs.ru>
To: Thomas Huth <thuth@...hat.com>
CC: linuxppc-dev@...ts.ozlabs.org,
Wei Yang <weiyang@...ux.vnet.ibm.com>,
Gavin Shan <gwshan@...ux.vnet.ibm.com>,
linux-kernel@...r.kernel.org,
Alex Williamson <alex.williamson@...hat.com>,
Paul Mackerras <paulus@...ba.org>,
David Gibson <david@...son.dropbear.id.au>
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@...abs.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@...abs.ru>
>> [aw: for the vfio related changes]
>> Acked-by: Alex Williamson <alex.williamson@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists