[PATCH kernel] vfio/spapr: Add trace points for map/unmap

[Robin Murphy]

On 29/11/17 16:32, Alex Williamson wrote:
> On Thu, 23 Nov 2017 15:13:37 +1100
> Alexey Kardashevskiy <aik at ozlabs.ru> wrote:
> 
>> On 17/11/17 17:58, Alexey Kardashevskiy wrote:
>>> On 17/11/17 11:13, Alex Williamson wrote:
>>>> On Tue, 14 Nov 2017 10:47:12 +1100
>>>> Alexey Kardashevskiy <aik at ozlabs.ru> wrote:
>>>>   
>>>>> On 27/10/17 14:00, Alexey Kardashevskiy wrote:
>>>>>> This adds trace_map/trace_unmap tracepoints to spapr driver. Type1 already
>>>>>> uses these via the IOMMU API (iommu_map/__iommu_unmap).
>>>>>>
>>>>>> Signed-off-by: Alexey Kardashevskiy <aik at ozlabs.ru>
>>>>
>>>> Is this really legitimate to include tracepoints from a different
>>>> subsystem?>  The vfio type1 backend gets these trace points by virtue of
>>>> it actually using the IOMMU API, it doesn't call them itself.  I'm kind
>>>> of surprised these are actually available to be called from a module.
>>>
>>> They are explicitly exported:
>>>
>>> EXPORT_TRACEPOINT_SYMBOL_GPL(map);
>>> EXPORT_TRACEPOINT_SYMBOL_GPL(unmap);
>>>
>>> I would think this is for things like drivers/vfio/vfio_iommu_spapr_tce.c ,
>>> why else?...
>>>
>>>    
>>>> I suspect the way to do this is probably to define our own tracepoints
>>>> in the vfio/spapr backend or insert tracepoints into the IOMMU layers
>>>> that that code calls into rather than masquerading as tracepoints from
>>>> a different subsystem.  Right?
>>>
>>> This makes sense too. But it is going to be just cut-n-paste and some
>>> confusion -
>>> /sys/kernel/debug/tracing/events/iommu/map will still be present but
>>> won't work and
>>> /sys/kernel/debug/tracing/events/vfio/vfio_iommu_spapr_tce/map will.
> 
> But iommu/map does work, it's just not called by the vfio spapr tce
> backend, which is absolutely correct.  In fact, that's part of my
> reservation about this approach is that it could be interpreted as
> implying a call path that doesn't exist on this arch.

Come to think of it, the inverse also applies - if users are led to 
expect x86-like behaviour where the iommu/map tracepoint just happens to 
reflect VFIO calls, they'll likely also get a nasty surprise on 
platforms like arm64 where regular DMA API calls by drivers may be 
hammering iommu_map() in the background.

Robin.

>>> Judges? :)
>>
>>
>> Still nak? I discussed this locally, the conclusion was it is a matter of
>> taste and this proposal is not that disgusting. Thanks.
> 
> Is that our goal now, proposals that aren't too disgusting?  I think
> it's an opportunity to introduce our own tracing infrastructure and it
> feels a lot more correct to do that than to generalize existing trace
> points from other subsystems.  Thanks,
> 
> Alex
> 
> 
>>>>>> ---
>>>>>>
>>>>>> Example:
>>>>>>   qemu-system-ppc-8655  [096]   724.662740: unmap:                IOMMU: iova=0x000000003ffff000 size=4096 unmapped_size=4096
>>>>>>   qemu-system-ppc-8656  [104]   724.970912: map:                  IOMMU: iova=0x0800000000000000 paddr=0x00007ffef7ff0000 size=65536
>>>>>> ---
>>>>>>   drivers/vfio/vfio_iommu_spapr_tce.c | 12 ++++++++++--
>>>>>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>>>>>> index 63112c36ab2d..4531486c77c6 100644
>>>>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>>>>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>>>>>> @@ -22,6 +22,7 @@
>>>>>>   #include <linux/vmalloc.h>
>>>>>>   #include <linux/sched/mm.h>
>>>>>>   #include <linux/sched/signal.h>
>>>>>> +#include <trace/events/iommu.h>
>>>>>>   
>>>>>>   #include <asm/iommu.h>
>>>>>>   #include <asm/tce.h>
>>>>>> @@ -502,17 +503,19 @@ static int tce_iommu_clear(struct tce_container *container,
>>>>>>   		struct iommu_table *tbl,
>>>>>>   		unsigned long entry, unsigned long pages)
>>>>>>   {
>>>>>> -	unsigned long oldhpa;
>>>>>> +	unsigned long oldhpa, unmapped, firstentry = entry, totalpages = pages;
>>>>>>   	long ret;
>>>>>>   	enum dma_data_direction direction;
>>>>>>   
>>>>>> -	for ( ; pages; --pages, ++entry) {
>>>>>> +	for (unmapped = 0; pages; --pages, ++entry) {
>>>>>>   		direction = DMA_NONE;
>>>>>>   		oldhpa = 0;
>>>>>>   		ret = iommu_tce_xchg(tbl, entry, &oldhpa, &direction);
>>>>>>   		if (ret)
>>>>>>   			continue;
>>>>>>   
>>>>>> +		++unmapped;
>>>>>> +
>>>>>>   		if (direction == DMA_NONE)
>>>>>>   			continue;
>>>>>>   
>>>>>> @@ -523,6 +526,9 @@ static int tce_iommu_clear(struct tce_container *container,
>>>>>>   
>>>>>>   		tce_iommu_unuse_page(container, oldhpa);
>>>>>>   	}
>>>>>> +	trace_unmap(firstentry << tbl->it_page_shift,
>>>>>> +			totalpages << tbl->it_page_shift,
>>>>>> +			unmapped << tbl->it_page_shift);
>>>>>>   
>>>>>>   	return 0;
>>>>>>   }
>>>>>> @@ -965,6 +971,8 @@ static long tce_iommu_ioctl(void *iommu_data,
>>>>>>   					direction);
>>>>>>   
>>>>>>   		iommu_flush_tce(tbl);
>>>>>> +		if (!ret)
>>>>>> +			trace_map(param.iova, param.vaddr, param.size);
>>>>>>   
>>>>>>   		return ret;
>>>>>>   	}
>>>>>>      
>>>>>
>>>>>   
>>>>   
>>>
>>>    
>>
>>
> 
> _______________________________________________
> iommu mailing list
> iommu at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>