* [PATCH kernel] vfio/spapr: Add trace points for map/unmap @ 2017-10-27 3:00 Alexey Kardashevskiy 2017-11-13 23:47 ` Alexey Kardashevskiy 0 siblings, 1 reply; 8+ messages in thread From: Alexey Kardashevskiy @ 2017-10-27 3:00 UTC (permalink / raw) To: linuxppc-dev Cc: Alexey Kardashevskiy, Alex Williamson, David Gibson, kvm-ppc, kvm 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@ozlabs.ru> --- 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; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH kernel] vfio/spapr: Add trace points for map/unmap 2017-10-27 3:00 [PATCH kernel] vfio/spapr: Add trace points for map/unmap Alexey Kardashevskiy @ 2017-11-13 23:47 ` Alexey Kardashevskiy 2017-11-17 0:13 ` Alex Williamson 0 siblings, 1 reply; 8+ messages in thread From: Alexey Kardashevskiy @ 2017-11-13 23:47 UTC (permalink / raw) To: linuxppc-dev; +Cc: Alex Williamson, David Gibson, kvm-ppc, kvm 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@ozlabs.ru> Ping? > --- > > 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; > } > -- Alexey ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH kernel] vfio/spapr: Add trace points for map/unmap 2017-11-13 23:47 ` Alexey Kardashevskiy @ 2017-11-17 0:13 ` Alex Williamson 2017-11-17 6:58 ` Alexey Kardashevskiy 0 siblings, 1 reply; 8+ messages in thread From: Alex Williamson @ 2017-11-17 0:13 UTC (permalink / raw) To: Alexey Kardashevskiy Cc: linuxppc-dev, David Gibson, kvm-ppc, kvm, Shuah Khan, iommu On Tue, 14 Nov 2017 10:47:12 +1100 Alexey Kardashevskiy <aik@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@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. 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? 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; > > } > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH kernel] vfio/spapr: Add trace points for map/unmap 2017-11-17 0:13 ` Alex Williamson @ 2017-11-17 6:58 ` Alexey Kardashevskiy 2017-11-23 4:13 ` Alexey Kardashevskiy 0 siblings, 1 reply; 8+ messages in thread From: Alexey Kardashevskiy @ 2017-11-17 6:58 UTC (permalink / raw) To: Alex Williamson Cc: linuxppc-dev, David Gibson, kvm-ppc, kvm, Shuah Khan, iommu, Joerg Roedel On 17/11/17 11:13, Alex Williamson wrote: > On Tue, 14 Nov 2017 10:47:12 +1100 > Alexey Kardashevskiy <aik@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@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. Judges? :) > 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; >>> } >>> >> >> > -- Alexey ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH kernel] vfio/spapr: Add trace points for map/unmap 2017-11-17 6:58 ` Alexey Kardashevskiy @ 2017-11-23 4:13 ` Alexey Kardashevskiy 2017-11-29 16:32 ` Alex Williamson 0 siblings, 1 reply; 8+ messages in thread From: Alexey Kardashevskiy @ 2017-11-23 4:13 UTC (permalink / raw) To: Alex Williamson Cc: linuxppc-dev, David Gibson, kvm-ppc, kvm, Shuah Khan, iommu, Joerg Roedel 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@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@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. > > Judges? :) Still nak? I discussed this locally, the conclusion was it is a matter of taste and this proposal is not that disgusting. Thanks. > > > >> 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; >>>> } >>>> >>> >>> >> > > -- Alexey ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH kernel] vfio/spapr: Add trace points for map/unmap 2017-11-23 4:13 ` Alexey Kardashevskiy @ 2017-11-29 16:32 ` Alex Williamson 2017-11-29 16:43 ` Robin Murphy 2017-11-29 16:44 ` Shuah Khan 0 siblings, 2 replies; 8+ messages in thread From: Alex Williamson @ 2017-11-29 16:32 UTC (permalink / raw) To: Alexey Kardashevskiy Cc: linuxppc-dev, David Gibson, kvm-ppc, kvm, Shuah Khan, iommu, Joerg Roedel On Thu, 23 Nov 2017 15:13:37 +1100 Alexey Kardashevskiy <aik@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@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@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. > > 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; > >>>> } > >>>> > >>> > >>> > >> > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH kernel] vfio/spapr: Add trace points for map/unmap 2017-11-29 16:32 ` Alex Williamson @ 2017-11-29 16:43 ` Robin Murphy 2017-11-29 16:44 ` Shuah Khan 1 sibling, 0 replies; 8+ messages in thread From: Robin Murphy @ 2017-11-29 16:43 UTC (permalink / raw) To: Alex Williamson, Alexey Kardashevskiy Cc: kvm, Shuah Khan, kvm-ppc, iommu, linuxppc-dev, David Gibson On 29/11/17 16:32, Alex Williamson wrote: > On Thu, 23 Nov 2017 15:13:37 +1100 > Alexey Kardashevskiy <aik@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@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@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@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH kernel] vfio/spapr: Add trace points for map/unmap 2017-11-29 16:32 ` Alex Williamson 2017-11-29 16:43 ` Robin Murphy @ 2017-11-29 16:44 ` Shuah Khan 1 sibling, 0 replies; 8+ messages in thread From: Shuah Khan @ 2017-11-29 16:44 UTC (permalink / raw) To: Alex Williamson, Alexey Kardashevskiy Cc: linuxppc-dev, David Gibson, kvm-ppc, kvm, iommu, Joerg Roedel, Shuah Khan On 11/29/2017 09:32 AM, Alex Williamson wrote: > On Thu, 23 Nov 2017 15:13:37 +1100 > Alexey Kardashevskiy <aik@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@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@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); Tracepoints need to be exported as per the tracepoint sub-system requirements. >>> >>> 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. These tracepoints are intended to be called after a iommo/map/unmap calls from IOMMU API. It doesn't make any sense to call them from random places. It is totally useless to do so and makes it difficult to debug problems. As an author of these tracepoints - that is my opinion. thanks, -- Shuah ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-11-29 16:44 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-27 3:00 [PATCH kernel] vfio/spapr: Add trace points for map/unmap Alexey Kardashevskiy 2017-11-13 23:47 ` Alexey Kardashevskiy 2017-11-17 0:13 ` Alex Williamson 2017-11-17 6:58 ` Alexey Kardashevskiy 2017-11-23 4:13 ` Alexey Kardashevskiy 2017-11-29 16:32 ` Alex Williamson 2017-11-29 16:43 ` Robin Murphy 2017-11-29 16:44 ` Shuah Khan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).