Re: [PATCH 6/7] iommu: change intel-iommu to use IOVA frame numbers

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

On 12/04/16 16:12, Adam Morrison wrote:
> On Tue, Apr 12, 2016 at 1:55 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
>
>>> Make intel-iommu map/unmap/invalidate work with IOVA pfns instead of
>>> pointers to "struct iova". This avoids using the iova struct from the IOVA
>>> red-black tree and the resulting explicit find_iova() on unmap.
>
> [...]
>
>>> diff --git a/include/linux/iova.h b/include/linux/iova.h
>>> index 92f7177..efecee0 100644
>>> --- a/include/linux/iova.h
>>> +++ b/include/linux/iova.h
>>> @@ -75,7 +75,7 @@ struct iova *alloc_iova_mem(void);
>>>    void free_iova_mem(struct iova *iova);
>>>    void free_iova(struct iova_domain *iovad, unsigned long pfn);
>>>    void __free_iova(struct iova_domain *iovad, struct iova *iova);
>>> -struct iova *alloc_iova(struct iova_domain *iovad, unsigned long size,
>>> +unsigned long alloc_iova(struct iova_domain *iovad, unsigned long size,
>>>          unsigned long limit_pfn,
>>>          bool size_aligned);
>>>    struct iova *reserve_iova(struct iova_domain *iovad, unsigned long
>>> pfn_lo,
>>
>>
>> Making the internals more efficient is no bad thing, but changing the
>> external interface to the IOVA library like this breaks at least
>> dma-iommu.c, thus stops arm64 from building.
>
> Good point.  I will add new versions of alloc_iova() and free_iova()
> for this functionality and make intel-iommu uses them, so as to not
> break other users.

I wonder if it might be worth going for the intermediate compromise of 
splitting up the structure thus:

struct iova {
	unsigned long	pfn_hi;
	unsigned long	pfn_lo;
};

/* internal to iova.c */
struct iova_private {
	struct rb_node	node;
	struct iova	iova;
};
	
then externally passing the pfn-only struct iovas by value everywhere we 
currently pass a struct iova *. AFAICS, that ought to require minimal 
changes in the users, and they shouldn't need to know or care whether 
any given iova is backed by the tree or the rcache, because they're 
simple to fake up:

	unsigned long iova_pfn =
		iova_rcache_get(&iovad->rcaches[log_size]);
	if (iova_pfn)
		return { iova_pfn + log_size, iova_pfn };

What do you reckon?

Robin.