linux-kernel - Re: [PATCH kernel v10 11/34] vfio: powerpc/spapr: Moving pinning/unpinning to helpers

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5552FD8A.6030900@ozlabs.ru>
Date:	Wed, 13 May 2015 17:30:18 +1000
From:	Alexey Kardashevskiy <aik@...abs.ru>
To:	Gavin Shan <gwshan@...ux.vnet.ibm.com>
CC:	linuxppc-dev@...ts.ozlabs.org,
	David Gibson <david@...son.dropbear.id.au>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Paul Mackerras <paulus@...ba.org>,
	Alex Williamson <alex.williamson@...hat.com>,
	Wei Yang <weiyang@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH kernel v10 11/34] vfio: powerpc/spapr: Moving pinning/unpinning
 to helpers

On 05/13/2015 04:32 PM, Gavin Shan wrote:
> On Tue, May 12, 2015 at 01:39:00AM +1000, Alexey Kardashevskiy wrote:
>> 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 <aik@...abs.ru>
>> [aw: for the vfio related changes]
>> Acked-by: Alex Williamson <alex.williamson@...hat.com>
>> Reviewed-by: David Gibson <david@...son.dropbear.id.au>
>
> Reviewed-by: Gavin Shan <gwshan@...ux.vnet.ibm.com>
>
>> ---
>> 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
--
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/