From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by lists.ozlabs.org (Postfix) with ESMTP id 3yn5vt1tFqzDrLW for ; Thu, 30 Nov 2017 03:43:53 +1100 (AEDT) Subject: Re: [PATCH kernel] vfio/spapr: Add trace points for map/unmap To: Alex Williamson , Alexey Kardashevskiy Cc: kvm@vger.kernel.org, Shuah Khan , kvm-ppc@vger.kernel.org, iommu@lists.linux-foundation.org, linuxppc-dev@lists.ozlabs.org, David Gibson References: <20171027030008.17200-1-aik@ozlabs.ru> <8c58fcaa-dedc-e6be-44bf-2cb349ef9e38@ozlabs.ru> <20171116171301.6552869b@t450s.home> <52a19cfb-431f-522b-11de-c1c47f09a4c8@ozlabs.ru> <20171129093244.0849fd7c@t450s.home> From: Robin Murphy Message-ID: <24f7ea0f-8a18-752f-6841-7001f88d1e00@arm.com> Date: Wed, 29 Nov 2017 16:43:46 +0000 MIME-Version: 1.0 In-Reply-To: <20171129093244.0849fd7c@t450s.home> Content-Type: text/plain; charset=utf-8; format=flowed List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 29/11/17 16:32, Alex Williamson wrote: > On Thu, 23 Nov 2017 15:13:37 +1100 > Alexey Kardashevskiy 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 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 >>>> >>>> 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 >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> >>>>>> #include >>>>>> #include >>>>>> @@ -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@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu >