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

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 <redacted>
---

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

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 <redacted>

Ping?

-- 
Alexey

On Tue, 14 Nov 2017 10:47:12 +1100
Alexey Kardashevskiy [off-list ref] wrote:

On 27/10/17 14:00, Alexey Kardashevskiy wrote:

quoted

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 <redacted>  

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

quoted

---

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;
 	}
  

On 17/11/17 11:13, Alex Williamson wrote:

On Tue, 14 Nov 2017 10:47:12 +1100
Alexey Kardashevskiy [off-list ref] wrote:

quoted

On 27/10/17 14:00, Alexey Kardashevskiy wrote:

quoted

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 <redacted>  

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

quoted

quoted

---

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

On 17/11/17 17:58, Alexey Kardashevskiy wrote:

On 17/11/17 11:13, Alex Williamson wrote:

quoted

On Tue, 14 Nov 2017 10:47:12 +1100
Alexey Kardashevskiy [off-list ref] wrote:

quoted

On 27/10/17 14:00, Alexey Kardashevskiy wrote:

quoted

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 <redacted>  

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?...

quoted

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.

quoted

 Thanks,

Alex

quoted

quoted

---

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

On Thu, 23 Nov 2017 15:13:37 +1100
Alexey Kardashevskiy [off-list ref] wrote:

On 17/11/17 17:58, Alexey Kardashevskiy wrote:

quoted

On 17/11/17 11:13, Alex Williamson wrote:  

quoted

On Tue, 14 Nov 2017 10:47:12 +1100
Alexey Kardashevskiy [off-list ref] wrote:
 

quoted

On 27/10/17 14:00, Alexey Kardashevskiy wrote:  

quoted

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 <redacted>    

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?...

  

quoted

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.

quoted

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

quoted

quoted

quoted

quoted

---

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;
 	}
    

On 29/11/17 16:32, Alex Williamson wrote:

On Thu, 23 Nov 2017 15:13:37 +1100
Alexey Kardashevskiy [off-list ref] wrote:

quoted

On 17/11/17 17:58, Alexey Kardashevskiy wrote:

quoted

On 17/11/17 11:13, Alex Williamson wrote:

quoted

On Tue, 14 Nov 2017 10:47:12 +1100
Alexey Kardashevskiy [off-list ref] wrote:
  

quoted

On 27/10/17 14:00, Alexey Kardashevskiy wrote:

quoted

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 <redacted>

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?...

   

quoted

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.

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

---

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

On 11/29/2017 09:32 AM, Alex Williamson wrote:

On Thu, 23 Nov 2017 15:13:37 +1100
Alexey Kardashevskiy [off-list ref] wrote:

quoted

On 17/11/17 17:58, Alexey Kardashevskiy wrote:

quoted

On 17/11/17 11:13, Alex Williamson wrote:  

quoted

On Tue, 14 Nov 2017 10:47:12 +1100
Alexey Kardashevskiy [off-list ref] wrote:
 

quoted

On 27/10/17 14:00, Alexey Kardashevskiy wrote:  

quoted

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 <redacted>    

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.

quoted

quoted

I would think this is for things like drivers/vfio/vfio_iommu_spapr_tce.c ,
why else?...

  

quoted

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