Re: [PATCH kernel v9 26/32] powerpc/iommu: Add userspace view of TCE table

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 05/01/2015 02:23 PM, David Gibson wrote:
> On Fri, May 01, 2015 at 02:01:17PM +1000, Alexey Kardashevskiy wrote:
>> On 04/29/2015 04:31 PM, David Gibson wrote:
>>> On Sat, Apr 25, 2015 at 10:14:50PM +1000, Alexey Kardashevskiy wrote:
>>>> In order to support memory pre-registration, we need a way to track
>>>> the use of every registered memory region and only allow unregistration
>>>> if a region is not in use anymore. So we need a way to tell from what
>>>> region the just cleared TCE was from.
>>>>
>>>> This adds a userspace view of the TCE table into iommu_table struct.
>>>> It contains userspace address, one per TCE entry. The table is only
>>>> allocated when the ownership over an IOMMU group is taken which means
>>>> it is only used from outside of the powernv code (such as VFIO).
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v9:
>>>> * fixed code flow in error cases added in v8
>>>>
>>>> v8:
>>>> * added ENOMEM on failed vzalloc()
>>>> ---
>>>>   arch/powerpc/include/asm/iommu.h          |  6 ++++++
>>>>   arch/powerpc/kernel/iommu.c               | 18 ++++++++++++++++++
>>>>   arch/powerpc/platforms/powernv/pci-ioda.c | 22 ++++++++++++++++++++--
>>>>   3 files changed, 44 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
>>>> index 7694546..1472de3 100644
>>>> --- a/arch/powerpc/include/asm/iommu.h
>>>> +++ b/arch/powerpc/include/asm/iommu.h
>>>> @@ -111,9 +111,15 @@ struct iommu_table {
>>>>   	unsigned long *it_map;       /* A simple allocation bitmap for now */
>>>>   	unsigned long  it_page_shift;/* table iommu page size */
>>>>   	struct iommu_table_group *it_table_group;
>>>> +	unsigned long *it_userspace; /* userspace view of the table */
>>>
>>> A single unsigned long doesn't seem like enough.
>>
>> Why single? This is an array.
>
> As in single per page.


Sorry, I am not following you here.
It is per IOMMU page. MAP/UNMAP work with IOMMU pages which are fully 
backed with either system page or a huge page.


>
>>> How do you know
>>> which process's address space this address refers to?
>>
>> It is a current task. Multiple userspaces cannot use the same container/tables.
>
> Where is that enforced?


It is accessed from VFIO DMA map/unmap which are ioctls() to a container's 
fd which is per a process. Same for KVM - when it registers IOMMU groups in 
KVM, fd's of opened IOMMU groups are passed there. Or I did not understand 
the question...


> More to the point, that's a VFIO constraint, but it's here affecting
> the design of a structure owned by the platform code.

Right. But keeping in mind KVM, I cannot think of any better design here.


> [snip]
>>>>   static void pnv_pci_ioda_setup_opal_tce_kill(struct pnv_phb *phb,
>>>> @@ -2062,12 +2071,21 @@ static long pnv_pci_ioda2_create_table(struct iommu_table_group *table_group,
>>>>   	int nid = pe->phb->hose->node;
>>>>   	__u64 bus_offset = num ? pe->tce_bypass_base : 0;
>>>>   	long ret;
>>>> +	unsigned long *uas, uas_cb = sizeof(*uas) * (window_size >> page_shift);
>>>> +
>>>> +	uas = vzalloc(uas_cb);
>>>> +	if (!uas)
>>>> +		return -ENOMEM;
>>>
>>> I don't see why this is allocated both here as well as in
>>> take_ownership.
>>
>> Where else? The only alternative is vfio_iommu_spapr_tce but I really do not
>> want to touch iommu_table fields there.
>
> Well to put it another way, why isn't take_ownership calling create
> itself (or at least a common helper).

I am trying to keep DDW stuff away from platform-oriented 
arch/powerpc/kernel/iommu.c which main purpose is to implement 
iommu_alloc()&co. It already has

I'd rather move it_userspace allocation completely to vfio_iommu_spapr_tce 
(should have done earlier, actually), would this be ok?


> Clearly the it_userspace table needs to have lifetime which matches
> the TCE table itself, so there should be a single function that marks
> the beginning of that joint lifetime.


No. it_userspace lives as long as the platform code does not control the 
table. For IODA2 it is equal for the lifetime of the table, for 
IODA1/P5IOC2 it is not.



>>> Isn't this function used for core-kernel users of the
>>> iommu as well, in which case it shouldn't need the it_userspace.
>>
>>
>> No. This is an iommu_table_group_ops callback which calls what the platform
>> code calls (pnv_pci_create_table()) plus allocates this it_userspace thing.
>> The callback is only called from VFIO.
>
> Ok.
>
> As touched on above it seems more like this should be owned by VFIO
> code than the platform code.

Agree now :) I'll move the allocation to VFIO. Thanks!


-- 
Alexey