[PATCH V2 0/6] ARM CoreSight: Enhance ETM tracing control

[PATCH V2 0/6] ARM CoreSight: Enhance ETM tracing control

[Adrien Vergé]

Hi,

Different ARM users have shown their interest in this patch, so I made
a second version that corrects the first mistakes.

Mainly, it extends current support of CoreSight ETM, that is currently
very limited. ETM provides hardware-assisted program tracing, a
cycle-precise and low-overhead solution that software tools such as
'perf record' cannot provide.

Usage of ETM tracing facility is presently limited to start and stop
tracing. This set of patches enables management of address combinations
and PIDs that trigger tracing, thus allowing to trace specific
functions and processes.

ETM management was done via sysfs entries (trace_info,
trace_running...), this code adds trace_addrrange and trace_pid to
let the user read/write custom values.

Changes in V2:
- Use device attributes rather than raw kobjects
- Make PID_IN_CONTEXTIDR incompatible with PID_NS in Kconfig
- Use int for pid type (not long)
- During trace fetching, call vmalloc() only if really needed

This series of patches applies to v3.13.

Adrien Verg? (6):
  ARM CoreSight: ETM: Use device attributes
  ARM CoreSight: ETM: Rename 'comparator' to 'address comparator'
  ARM CoreSight: ETM: Add address control support
  ARM: Make PID_IN_CONTEXTIDR incompatible with PID_NS
  ARM CoreSight: ETM: Add PID control support
  ARM CoreSight: ETM: Allocate a trace buffer only when necessary

 arch/arm/Kconfig.debug                    |   2 +-
 arch/arm/include/asm/hardware/coresight.h |   9 +-
 arch/arm/kernel/etm.c                     | 208 +++++++++++++++++++++++-------
 arch/arm64/Kconfig.debug                  |   1 +
 4 files changed, 169 insertions(+), 51 deletions(-)

-- 
1.8.5.2

[PATCH V2 1/6] ARM CoreSight: ETM: Use device attributes

[Adrien Vergé]

Replace all kobjects attributes with device attributes.
User experience isn't changed since the same files are created in sysfs.

Signed-off-by: Adrien Verg? <adrienverge@gmail.com>
---
 arch/arm/kernel/etm.c | 48 +++++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/arch/arm/kernel/etm.c b/arch/arm/kernel/etm.c
index 8ff0ecd..2b1a307 100644
--- a/arch/arm/kernel/etm.c
+++ b/arch/arm/kernel/etm.c
@@ -430,15 +430,14 @@ static struct amba_driver etb_driver = {
 };
 
 /* use a sysfs file "trace_running" to start/stop tracing */
-static ssize_t trace_running_show(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  char *buf)
+static ssize_t trace_running_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
 {
 	return sprintf(buf, "%x\n", trace_isrunning(&tracer));
 }
 
-static ssize_t trace_running_store(struct kobject *kobj,
-				   struct kobj_attribute *attr,
+static ssize_t trace_running_store(struct device *dev,
+				   struct device_attribute *attr,
 				   const char *buf, size_t n)
 {
 	unsigned int value;
@@ -454,12 +453,11 @@ static ssize_t trace_running_store(struct kobject *kobj,
 	return ret ? : n;
 }
 
-static struct kobj_attribute trace_running_attr =
-	__ATTR(trace_running, 0644, trace_running_show, trace_running_store);
+DEVICE_ATTR(trace_running, S_IRUGO|S_IWUSR,
+	    trace_running_show, trace_running_store);
 
-static ssize_t trace_info_show(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  char *buf)
+static ssize_t trace_info_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
 {
 	u32 etb_wa, etb_ra, etb_st, etb_fc, etm_ctrl, etm_st;
 	int datalen;
@@ -495,21 +493,19 @@ static ssize_t trace_info_show(struct kobject *kobj,
 			);
 }
 
-static struct kobj_attribute trace_info_attr =
-	__ATTR(trace_info, 0444, trace_info_show, NULL);
+DEVICE_ATTR(trace_info, S_IRUGO, trace_info_show, NULL);
 
-static ssize_t trace_mode_show(struct kobject *kobj,
-				  struct kobj_attribute *attr,
-				  char *buf)
+static ssize_t trace_mode_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
 {
 	return sprintf(buf, "%d %d\n",
 			!!(tracer.flags & TRACER_CYCLE_ACC),
 			tracer.etm_portsz);
 }
 
-static ssize_t trace_mode_store(struct kobject *kobj,
-				   struct kobj_attribute *attr,
-				   const char *buf, size_t n)
+static ssize_t trace_mode_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t n)
 {
 	unsigned int cycacc, portsz;
 
@@ -528,8 +524,7 @@ static ssize_t trace_mode_store(struct kobject *kobj,
 	return n;
 }
 
-static struct kobj_attribute trace_mode_attr =
-	__ATTR(trace_mode, 0644, trace_mode_show, trace_mode_store);
+DEVICE_ATTR(trace_mode, S_IRUGO|S_IWUSR, trace_mode_show, trace_mode_store);
 
 static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 {
@@ -568,17 +563,16 @@ static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 	etm_writel(t, 0x440, ETMR_CTRL);
 	etm_lock(t);
 
-	ret = sysfs_create_file(&dev->dev.kobj,
-			&trace_running_attr.attr);
+	ret = device_create_file(&dev->dev, &dev_attr_trace_running);
 	if (ret)
 		goto out_unmap;
 
 	/* failing to create any of these two is not fatal */
-	ret = sysfs_create_file(&dev->dev.kobj, &trace_info_attr.attr);
+	ret = device_create_file(&dev->dev, &dev_attr_trace_info);
 	if (ret)
 		dev_dbg(&dev->dev, "Failed to create trace_info in sysfs\n");
 
-	ret = sysfs_create_file(&dev->dev.kobj, &trace_mode_attr.attr);
+	ret = device_create_file(&dev->dev, &dev_attr_trace_mode);
 	if (ret)
 		dev_dbg(&dev->dev, "Failed to create trace_mode in sysfs\n");
 
@@ -608,9 +602,9 @@ static int etm_remove(struct amba_device *dev)
 
 	amba_release_regions(dev);
 
-	sysfs_remove_file(&dev->dev.kobj, &trace_running_attr.attr);
-	sysfs_remove_file(&dev->dev.kobj, &trace_info_attr.attr);
-	sysfs_remove_file(&dev->dev.kobj, &trace_mode_attr.attr);
+	device_remove_file(&dev->dev, &dev_attr_trace_running);
+	device_remove_file(&dev->dev, &dev_attr_trace_info);
+	device_remove_file(&dev->dev, &dev_attr_trace_mode);
 
 	return 0;
 }
-- 
1.8.5.2

[PATCH V2 2/6] ARM CoreSight: ETM: Rename 'comparator' to 'address comparator'

[Adrien Vergé]

Since there are different types of comparators, and other kinds will
be used (such as Context ID comparators), rename them properly.

Signed-off-by: Adrien Verg? <adrienverge@gmail.com>
---
 arch/arm/include/asm/hardware/coresight.h |  4 ++--
 arch/arm/kernel/etm.c                     | 19 ++++++++++---------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/arm/include/asm/hardware/coresight.h b/arch/arm/include/asm/hardware/coresight.h
index ad774f3..8c50cf6 100644
--- a/arch/arm/include/asm/hardware/coresight.h
+++ b/arch/arm/include/asm/hardware/coresight.h
@@ -95,8 +95,8 @@
 #define ETMAAT_NSONLY		(1 << 10)
 #define ETMAAT_SONLY		(2 << 10)
 
-#define ETMR_COMP_VAL(x)	(0x40 + (x) * 4)
-#define ETMR_COMP_ACC_TYPE(x)	(0x80 + (x) * 4)
+#define ETMR_ADDRCOMP_VAL(x)	(0x40 + (x) * 4)
+#define ETMR_ADDRCOMP_ACC_TYPE(x)	(0x80 + (x) * 4)
 
 /* ETM status register, "ETM Architecture", 3.3.2 */
 #define ETMR_STATUS		(0x10)
diff --git a/arch/arm/kernel/etm.c b/arch/arm/kernel/etm.c
index 2b1a307..7f7a0ee 100644
--- a/arch/arm/kernel/etm.c
+++ b/arch/arm/kernel/etm.c
@@ -39,7 +39,7 @@ struct tracectx {
 	void __iomem	*etb_regs;
 	void __iomem	*etm_regs;
 	unsigned long	flags;
-	int		ncmppairs;
+	int		naddrcmppairs;
 	int		etm_portsz;
 	struct device	*dev;
 	struct clk	*emu_clk;
@@ -59,7 +59,7 @@ static int etm_setup_address_range(struct tracectx *t, int n,
 	u32 flags = ETMAAT_ARM | ETMAAT_IGNCONTEXTID | ETMAAT_NSONLY | \
 		    ETMAAT_NOVALCMP;
 
-	if (n < 1 || n > t->ncmppairs)
+	if (n < 1 || n > t->naddrcmppairs)
 		return -EINVAL;
 
 	/* comparators and ranges are numbered starting with 1 as opposed
@@ -72,12 +72,12 @@ static int etm_setup_address_range(struct tracectx *t, int n,
 		flags |= ETMAAT_IEXEC;
 
 	/* first comparator for the range */
-	etm_writel(t, flags, ETMR_COMP_ACC_TYPE(n * 2));
-	etm_writel(t, start, ETMR_COMP_VAL(n * 2));
+	etm_writel(t, flags, ETMR_ADDRCOMP_ACC_TYPE(n * 2));
+	etm_writel(t, start, ETMR_ADDRCOMP_VAL(n * 2));
 
 	/* second comparator is right next to it */
-	etm_writel(t, flags, ETMR_COMP_ACC_TYPE(n * 2 + 1));
-	etm_writel(t, end, ETMR_COMP_VAL(n * 2 + 1));
+	etm_writel(t, flags, ETMR_ADDRCOMP_ACC_TYPE(n * 2 + 1));
+	etm_writel(t, end, ETMR_ADDRCOMP_VAL(n * 2 + 1));
 
 	flags = exclude ? ETMTE_INCLEXCL : 0;
 	etm_writel(t, flags | (1 << n), ETMR_TRACEENCTRL);
@@ -475,7 +475,8 @@ static ssize_t trace_info_show(struct device *dev,
 	etm_st = etm_readl(&tracer, ETMR_STATUS);
 	etm_lock(&tracer);
 
-	return sprintf(buf, "Trace buffer len: %d\nComparator pairs: %d\n"
+	return sprintf(buf, "Trace buffer len: %d\n"
+			"Addr comparator pairs: %d\n"
 			"ETBR_WRITEADDR:\t%08x\n"
 			"ETBR_READADDR:\t%08x\n"
 			"ETBR_STATUS:\t%08x\n"
@@ -483,7 +484,7 @@ static ssize_t trace_info_show(struct device *dev,
 			"ETMR_CTRL:\t%08x\n"
 			"ETMR_STATUS:\t%08x\n",
 			datalen,
-			tracer.ncmppairs,
+			tracer.naddrcmppairs,
 			etb_wa,
 			etb_ra,
 			etb_st,
@@ -559,7 +560,7 @@ static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 	/* dummy first read */
 	(void)etm_readl(&tracer, ETMMR_OSSRR);
 
-	t->ncmppairs = etm_readl(t, ETMR_CONFCODE) & 0xf;
+	t->naddrcmppairs = etm_readl(t, ETMR_CONFCODE) & 0xf;
 	etm_writel(t, 0x440, ETMR_CTRL);
 	etm_lock(t);
 
-- 
1.8.5.2

[PATCH V2 3/6] ARM CoreSight: ETM: Add address control support

[Adrien Vergé]

In the same manner as for enabling tracing, an entry is created
in sysfs to set the address range that triggers tracing.

Signed-off-by: Adrien Verg? <adrienverge@gmail.com>
---
 arch/arm/kernel/etm.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/etm.c b/arch/arm/kernel/etm.c
index 7f7a0ee..4442e5c 100644
--- a/arch/arm/kernel/etm.c
+++ b/arch/arm/kernel/etm.c
@@ -44,6 +44,8 @@ struct tracectx {
 	struct device	*dev;
 	struct clk	*emu_clk;
 	struct mutex	mutex;
+	unsigned long	addrrange_start;
+	unsigned long	addrrange_end;
 };
 
 static struct tracectx tracer;
@@ -53,6 +55,13 @@ static inline bool trace_isrunning(struct tracectx *t)
 	return !!(t->flags & TRACER_RUNNING);
 }
 
+/*
+ * Setups ETM to trace only when:
+ *   - address between start and end
+ *     or address not between start and end (if exclude)
+ *   - trace executed instructions
+ *     or trace loads and stores (if data)
+ */
 static int etm_setup_address_range(struct tracectx *t, int n,
 		unsigned long start, unsigned long end, int exclude, int data)
 {
@@ -115,8 +124,8 @@ static int trace_start(struct tracectx *t)
 		return -EFAULT;
 	}
 
-	etm_setup_address_range(t, 1, (unsigned long)_stext,
-			(unsigned long)_etext, 0, 0);
+	etm_setup_address_range(t, 1, t->addrrange_start, t->addrrange_end,
+				0, 0);
 	etm_writel(t, 0, ETMR_TRACEENCTRL2);
 	etm_writel(t, 0, ETMR_TRACESSCTRL);
 	etm_writel(t, 0x6f, ETMR_TRACEENEVT);
@@ -527,6 +536,36 @@ static ssize_t trace_mode_store(struct device *dev,
 
 DEVICE_ATTR(trace_mode, S_IRUGO|S_IWUSR, trace_mode_show, trace_mode_store);
 
+static ssize_t trace_addrrange_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%08lx - %08lx\n", tracer.addrrange_start,
+		       tracer.addrrange_end);
+}
+
+static ssize_t trace_addrrange_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t n)
+{
+	unsigned long start, end;
+
+	if (tracer.flags & TRACER_RUNNING)
+		return -EBUSY;
+
+	if (sscanf(buf, "%08lx - %08lx", &start, &end) != 2)
+		return -EINVAL;
+
+	mutex_lock(&tracer.mutex);
+	tracer.addrrange_start = start;
+	tracer.addrrange_end = end;
+	mutex_unlock(&tracer.mutex);
+
+	return n;
+}
+
+DEVICE_ATTR(trace_addrrange, S_IRUGO|S_IWUSR,
+	    trace_addrrange_show, trace_addrrange_store);
+
 static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 {
 	struct tracectx *t = &tracer;
@@ -554,6 +593,8 @@ static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 	t->dev = &dev->dev;
 	t->flags = TRACER_CYCLE_ACC;
 	t->etm_portsz = 1;
+	t->addrrange_start = (unsigned long) _stext;
+	t->addrrange_end = (unsigned long) _etext;
 
 	etm_unlock(t);
 	(void)etm_readl(t, ETMMR_PDSR);
@@ -568,7 +609,7 @@ static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 	if (ret)
 		goto out_unmap;
 
-	/* failing to create any of these two is not fatal */
+	/* failing to create any of these three is not fatal */
 	ret = device_create_file(&dev->dev, &dev_attr_trace_info);
 	if (ret)
 		dev_dbg(&dev->dev, "Failed to create trace_info in sysfs\n");
@@ -577,6 +618,10 @@ static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 	if (ret)
 		dev_dbg(&dev->dev, "Failed to create trace_mode in sysfs\n");
 
+	ret = device_create_file(&dev->dev, &dev_attr_trace_addrrange);
+	if (ret)
+		dev_dbg(&dev->dev, "Failed to create trace_addrrange in sysfs\n");
+
 	dev_dbg(t->dev, "ETM AMBA driver initialized.\n");
 
 out:
@@ -606,6 +651,7 @@ static int etm_remove(struct amba_device *dev)
 	device_remove_file(&dev->dev, &dev_attr_trace_running);
 	device_remove_file(&dev->dev, &dev_attr_trace_info);
 	device_remove_file(&dev->dev, &dev_attr_trace_mode);
+	device_remove_file(&dev->dev, &dev_attr_trace_addrrange);
 
 	return 0;
 }
-- 
1.8.5.2

[PATCH V2 4/6] ARM: Make PID_IN_CONTEXTIDR incompatible with PID_NS

[Adrien Vergé]

When using namespaces, different processes can have the same PID.
It makes no sense to store a PID value in the Context ID register
to track a specific process, when others share the same value.

Consequently, PID_IN_CONTEXTIDR (which is used for tracing and
debugging processes) should not be compatible with PID_NS.

Signed-off-by: Adrien Verg? <adrienverge@gmail.com>
---
 arch/arm/Kconfig.debug   | 2 +-
 arch/arm64/Kconfig.debug | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 5765abf..ed46748 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -1143,7 +1143,7 @@ config ARM_KPROBES_TEST
 
 config PID_IN_CONTEXTIDR
 	bool "Write the current PID to the CONTEXTIDR register"
-	depends on CPU_COPY_V6
+	depends on CPU_COPY_V6 && !PID_NS
 	help
 	  Enabling this option causes the kernel to write the current PID to
 	  the PROCID field of the CONTEXTIDR register, at the expense of some
diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index 835c559..06b2633b 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -17,6 +17,7 @@ config EARLY_PRINTK
 
 config PID_IN_CONTEXTIDR
 	bool "Write the current PID to the CONTEXTIDR register"
+	depends on !PID_NS
 	help
 	  Enabling this option causes the kernel to write the current PID to
 	  the CONTEXTIDR register, at the expense of some additional
-- 
1.8.5.2

[PATCH V2 4/6] ARM: Make PID_IN_CONTEXTIDR incompatible with PID_NS

[Will Deacon]

On Fri, Jan 24, 2014 at 04:40:54PM +0000, Adrien Verg? wrote:
> When using namespaces, different processes can have the same PID.
> It makes no sense to store a PID value in the Context ID register
> to track a specific process, when others share the same value.
> 
> Consequently, PID_IN_CONTEXTIDR (which is used for tracing and
> debugging processes) should not be compatible with PID_NS.
> 
> Signed-off-by: Adrien Verg? <adrienverge@gmail.com>
> ---
>  arch/arm/Kconfig.debug   | 2 +-
>  arch/arm64/Kconfig.debug | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index 5765abf..ed46748 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -1143,7 +1143,7 @@ config ARM_KPROBES_TEST
>  
>  config PID_IN_CONTEXTIDR
>  	bool "Write the current PID to the CONTEXTIDR register"
> -	depends on CPU_COPY_V6
> +	depends on CPU_COPY_V6 && !PID_NS
>  	help
>  	  Enabling this option causes the kernel to write the current PID to
>  	  the PROCID field of the CONTEXTIDR register, at the expense of some
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index 835c559..06b2633b 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -17,6 +17,7 @@ config EARLY_PRINTK
>  
>  config PID_IN_CONTEXTIDR
>  	bool "Write the current PID to the CONTEXTIDR register"
> +	depends on !PID_NS
>  	help
>  	  Enabling this option causes the kernel to write the current PID to
>  	  the CONTEXTIDR register, at the expense of some additional

Are you sure about this? The value we write is actually task_pid_nr, which I
believe to be globally unique.

Will

[PATCH V2 4/6] ARM: Make PID_IN_CONTEXTIDR incompatible with PID_NS

[Adrien Vergé]

2014/1/24 Will Deacon <will.deacon@arm.com>:
> Are you sure about this? The value we write is actually task_pid_nr, which I
> believe to be globally unique.

You are right: the task_pid_nr is unique in the system. However when
using namespaces, the so called "PID" is the virtual number that
processes in different namespaces can share.

This PID is the one visible by user-space tasks, in particular
user-space tracers and debuggers. These programs would expect to find
the PID of the traced process in the Context ID reg, while it is not.
I think it is better to remove confusion by making PID_IN_CONTEXTIDR
and PID_NS incompatible.

What do you think?

[PATCH V2 4/6] ARM: Make PID_IN_CONTEXTIDR incompatible with PID_NS

[Will Deacon]

On Fri, Jan 24, 2014 at 05:16:28PM +0000, Adrien Verg? wrote:
> 2014/1/24 Will Deacon <will.deacon@arm.com>:
> > Are you sure about this? The value we write is actually task_pid_nr, which I
> > believe to be globally unique.
> 
> You are right: the task_pid_nr is unique in the system. However when
> using namespaces, the so called "PID" is the virtual number that
> processes in different namespaces can share.
> 
> This PID is the one visible by user-space tasks, in particular
> user-space tracers and debuggers. These programs would expect to find
> the PID of the traced process in the Context ID reg, while it is not.
> I think it is better to remove confusion by making PID_IN_CONTEXTIDR
> and PID_NS incompatible.
> 
> What do you think?

I think I'd rather have the global ID than disable a potentially useful
feature, especially since this is likely to be consumed by external trace
tools as opposed to user-space tasks.

Will

[PATCH V2 4/6] ARM: Make PID_IN_CONTEXTIDR incompatible with PID_NS

[Christopher Covington]

On 01/24/2014 12:17 PM, Will Deacon wrote:
> On Fri, Jan 24, 2014 at 05:16:28PM +0000, Adrien Verg? wrote:
>> 2014/1/24 Will Deacon <will.deacon@arm.com>:
>>> Are you sure about this? The value we write is actually task_pid_nr, which I
>>> believe to be globally unique.
>>
>> You are right: the task_pid_nr is unique in the system. However when
>> using namespaces, the so called "PID" is the virtual number that
>> processes in different namespaces can share.
>>
>> This PID is the one visible by user-space tasks, in particular
>> user-space tracers and debuggers. These programs would expect to find
>> the PID of the traced process in the Context ID reg, while it is not.
>> I think it is better to remove confusion by making PID_IN_CONTEXTIDR
>> and PID_NS incompatible.
>>
>> What do you think?
> 
> I think I'd rather have the global ID than disable a potentially useful
> feature, especially since this is likely to be consumed by external trace
> tools as opposed to user-space tasks.

We've discussed before that the ARM architecture doesn't say what should be
written to the CONTEXTIDR, so it's up to us to decide. Will has a use case
where the global PID is useful. Adrien's patches present a use case where I
think the virtual PID would be useful. I've done work in the past where
writing the process group ID was useful. Would it be reasonable to make what's
written to the CONTEXTIDR run-time configurable? If so, what would be the best
interface for configuring it?

Thanks,
Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

[PATCH V2 4/6] ARM: Make PID_IN_CONTEXTIDR incompatible with PID_NS

[Christopher Covington]

On 01/24/2014 12:52 PM, Christopher Covington wrote:
> On 01/24/2014 12:17 PM, Will Deacon wrote:
>> On Fri, Jan 24, 2014 at 05:16:28PM +0000, Adrien Verg? wrote:
>>> 2014/1/24 Will Deacon <will.deacon@arm.com>:
>>>> Are you sure about this? The value we write is actually task_pid_nr, which I
>>>> believe to be globally unique.
>>>
>>> You are right: the task_pid_nr is unique in the system. However when
>>> using namespaces, the so called "PID" is the virtual number that
>>> processes in different namespaces can share.
>>>
>>> This PID is the one visible by user-space tasks, in particular
>>> user-space tracers and debuggers. These programs would expect to find
>>> the PID of the traced process in the Context ID reg, while it is not.
>>> I think it is better to remove confusion by making PID_IN_CONTEXTIDR
>>> and PID_NS incompatible.
>>>
>>> What do you think?
>>
>> I think I'd rather have the global ID than disable a potentially useful
>> feature, especially since this is likely to be consumed by external trace
>> tools as opposed to user-space tasks.
> 
> We've discussed before that the ARM architecture doesn't say what should be
> written to the CONTEXTIDR, so it's up to us to decide. Will has a use case
> where the global PID is useful. Adrien's patches present a use case where I
> think the virtual PID would be useful. I've done work in the past where
> writing the process group ID was useful. Would it be reasonable to make what's
> written to the CONTEXTIDR run-time configurable? If so, what would be the best
> interface for configuring it?

D'oh, I mixed things up. For ETM to work it can only use global PID's in the
CONTEXTIDR.

Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

[PATCH V2 4/6] ARM: Make PID_IN_CONTEXTIDR incompatible with PID_NS

[Adrien Vergé]

2014/1/24 Will Deacon <will.deacon@arm.com>:
> I think I'd rather have the global ID than disable a potentially useful
> feature, especially since this is likely to be consumed by external trace
> tools as opposed to user-space tasks.

I understand.

2014/1/24 Christopher Covington <cov@codeaurora.org>:
> Would it be reasonable to make what's
> written to the CONTEXTIDR run-time configurable? If so, what would be the best
> interface for configuring it?

This is an interesting option.

An other option would be to keep the global PID in the Context ID
register, and rely on kernel support to translate virtual PID to
global PID when needed. Then, it would be possible to select a task to
trace via its PID, by asking the kernel to write its global ID to the
Context ID comparator.

[PATCH V2 5/6] ARM CoreSight: ETM: Add PID control support

[Adrien Vergé]

In the same manner as for enabling tracing, an entry is created in
sysfs to set the PID that triggers tracing. This change is effective
only if CONFIG_PID_IN_CONTEXTIDR is set, and thus is not compatible
with PID namespaces.

Signed-off-by: Adrien Verg? <adrienverge@gmail.com>
---
 arch/arm/include/asm/hardware/coresight.h |  5 ++
 arch/arm/kernel/etm.c                     | 82 ++++++++++++++++++++++++++++---
 2 files changed, 80 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/hardware/coresight.h b/arch/arm/include/asm/hardware/coresight.h
index 8c50cf6..2051af0 100644
--- a/arch/arm/include/asm/hardware/coresight.h
+++ b/arch/arm/include/asm/hardware/coresight.h
@@ -98,6 +98,11 @@
 #define ETMR_ADDRCOMP_VAL(x)	(0x40 + (x) * 4)
 #define ETMR_ADDRCOMP_ACC_TYPE(x)	(0x80 + (x) * 4)
 
+#ifdef CONFIG_PID_IN_CONTEXTIDR
+#define ETMR_CTXIDCOMP_VAL(x)	(0x1b0 + (x) * 4)
+#define ETMR_CTXIDCOMP_MASK	(0x1bc)
+#endif
+
 /* ETM status register, "ETM Architecture", 3.3.2 */
 #define ETMR_STATUS		(0x10)
 #define ETMST_OVERFLOW		BIT(0)
diff --git a/arch/arm/kernel/etm.c b/arch/arm/kernel/etm.c
index 4442e5c..54b5128 100644
--- a/arch/arm/kernel/etm.c
+++ b/arch/arm/kernel/etm.c
@@ -40,12 +40,14 @@ struct tracectx {
 	void __iomem	*etm_regs;
 	unsigned long	flags;
 	int		naddrcmppairs;
+	int		nctxidcmp;
 	int		etm_portsz;
 	struct device	*dev;
 	struct clk	*emu_clk;
 	struct mutex	mutex;
 	unsigned long	addrrange_start;
 	unsigned long	addrrange_end;
+	int		pid;
 };
 
 static struct tracectx tracer;
@@ -59,14 +61,18 @@ static inline bool trace_isrunning(struct tracectx *t)
  * Setups ETM to trace only when:
  *   - address between start and end
  *     or address not between start and end (if exclude)
+ *   - in user-space when process id equals pid,
+ *     in kernel-space (if pid == 0),
+ *     always (if pid == -1)
  *   - trace executed instructions
  *     or trace loads and stores (if data)
  */
-static int etm_setup_address_range(struct tracectx *t, int n,
-		unsigned long start, unsigned long end, int exclude, int data)
+static int etm_setup(struct tracectx *t, int n,
+		     unsigned long start, unsigned long end, int exclude,
+		     int pid,
+		     int data)
 {
-	u32 flags = ETMAAT_ARM | ETMAAT_IGNCONTEXTID | ETMAAT_NSONLY | \
-		    ETMAAT_NOVALCMP;
+	u32 flags = ETMAAT_ARM | ETMAAT_NSONLY | ETMAAT_NOVALCMP;
 
 	if (n < 1 || n > t->naddrcmppairs)
 		return -EINVAL;
@@ -75,6 +81,23 @@ static int etm_setup_address_range(struct tracectx *t, int n,
 	 * to bits in a word */
 	n--;
 
+#ifdef CONFIG_PID_IN_CONTEXTIDR
+	if (pid < 0) {
+		/* ignore Context ID */
+		flags |= ETMAAT_IGNCONTEXTID;
+	} else {
+		flags |= ETMAAT_VALUE1;
+
+		/* Set up the first Context ID comparator.
+		   Process ID is found in the 24 first bits of Context ID
+		   (provided by CONFIG_PID_IN_CONTEXTIDR) */
+		etm_writel(t, pid << 8, ETMR_CTXIDCOMP_VAL(0));
+		etm_writel(t, 0xff, ETMR_CTXIDCOMP_MASK);
+	}
+#else
+	flags |= ETMAAT_IGNCONTEXTID;
+#endif
+
 	if (data)
 		flags |= ETMAAT_DLOADSTORE;
 	else
@@ -124,8 +147,10 @@ static int trace_start(struct tracectx *t)
 		return -EFAULT;
 	}
 
-	etm_setup_address_range(t, 1, t->addrrange_start, t->addrrange_end,
-				0, 0);
+	etm_setup(t, 1,
+		  t->addrrange_start, t->addrrange_end, 0,
+		  t->pid,
+		  0);
 	etm_writel(t, 0, ETMR_TRACEENCTRL2);
 	etm_writel(t, 0, ETMR_TRACESSCTRL);
 	etm_writel(t, 0x6f, ETMR_TRACEENEVT);
@@ -486,6 +511,7 @@ static ssize_t trace_info_show(struct device *dev,
 
 	return sprintf(buf, "Trace buffer len: %d\n"
 			"Addr comparator pairs: %d\n"
+			"Ctx ID comparators: %d\n"
 			"ETBR_WRITEADDR:\t%08x\n"
 			"ETBR_READADDR:\t%08x\n"
 			"ETBR_STATUS:\t%08x\n"
@@ -494,6 +520,7 @@ static ssize_t trace_info_show(struct device *dev,
 			"ETMR_STATUS:\t%08x\n",
 			datalen,
 			tracer.naddrcmppairs,
+			tracer.nctxidcmp,
 			etb_wa,
 			etb_ra,
 			etb_st,
@@ -566,6 +593,36 @@ static ssize_t trace_addrrange_store(struct device *dev,
 DEVICE_ATTR(trace_addrrange, S_IRUGO|S_IWUSR,
 	    trace_addrrange_show, trace_addrrange_store);
 
+#ifdef CONFIG_PID_IN_CONTEXTIDR
+static ssize_t trace_pid_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	return sprintf(buf, "%d\n", tracer.pid);
+}
+
+static ssize_t trace_pid_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t n)
+{
+	int pid;
+
+	if (tracer.flags & TRACER_RUNNING)
+		return -EBUSY;
+
+	if (sscanf(buf, "%i", &pid) != 1)
+		return -EINVAL;
+
+	mutex_lock(&tracer.mutex);
+	tracer.pid = pid;
+	mutex_unlock(&tracer.mutex);
+
+	return n;
+}
+
+DEVICE_ATTR(trace_pid, S_IRUGO|S_IWUSR, trace_pid_show, trace_pid_store);
+#endif
+
 static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 {
 	struct tracectx *t = &tracer;
@@ -595,6 +652,7 @@ static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 	t->etm_portsz = 1;
 	t->addrrange_start = (unsigned long) _stext;
 	t->addrrange_end = (unsigned long) _etext;
+	t->pid = -1; /* trace everything */
 
 	etm_unlock(t);
 	(void)etm_readl(t, ETMMR_PDSR);
@@ -602,6 +660,7 @@ static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 	(void)etm_readl(&tracer, ETMMR_OSSRR);
 
 	t->naddrcmppairs = etm_readl(t, ETMR_CONFCODE) & 0xf;
+	t->nctxidcmp = (etm_readl(t, ETMR_CONFCODE) >> 24) & 0x3;
 	etm_writel(t, 0x440, ETMR_CTRL);
 	etm_lock(t);
 
@@ -609,7 +668,7 @@ static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 	if (ret)
 		goto out_unmap;
 
-	/* failing to create any of these three is not fatal */
+	/* failing to create any of these four is not fatal */
 	ret = device_create_file(&dev->dev, &dev_attr_trace_info);
 	if (ret)
 		dev_dbg(&dev->dev, "Failed to create trace_info in sysfs\n");
@@ -622,6 +681,12 @@ static int etm_probe(struct amba_device *dev, const struct amba_id *id)
 	if (ret)
 		dev_dbg(&dev->dev, "Failed to create trace_addrrange in sysfs\n");
 
+#ifdef CONFIG_PID_IN_CONTEXTIDR
+	ret = device_create_file(&dev->dev, &dev_attr_trace_pid);
+	if (ret)
+		dev_dbg(&dev->dev, "Failed to create trace_pid in sysfs\n");
+#endif
+
 	dev_dbg(t->dev, "ETM AMBA driver initialized.\n");
 
 out:
@@ -652,6 +717,9 @@ static int etm_remove(struct amba_device *dev)
 	device_remove_file(&dev->dev, &dev_attr_trace_info);
 	device_remove_file(&dev->dev, &dev_attr_trace_mode);
 	device_remove_file(&dev->dev, &dev_attr_trace_addrrange);
+#ifdef CONFIG_PID_IN_CONTEXTIDR
+	device_remove_file(&dev->dev, &dev_attr_trace_pid);
+#endif
 
 	return 0;
 }
-- 
1.8.5.2

[PATCH V2 6/6] ARM CoreSight: ETM: Allocate a trace buffer only when necessary

[Adrien Vergé]

Some user applications try to retrieve trace data often, there
is no need to call vmalloc() when there is 0 byte to fetch.

Signed-off-by: Adrien Verg? <adrienverge@gmail.com>
---
 arch/arm/kernel/etm.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/etm.c b/arch/arm/kernel/etm.c
index 54b5128..bc97240 100644
--- a/arch/arm/kernel/etm.c
+++ b/arch/arm/kernel/etm.c
@@ -309,7 +309,7 @@ static ssize_t etb_read(struct file *file, char __user *data,
 	long length;
 	struct tracectx *t = file->private_data;
 	u32 first = 0;
-	u32 *buf;
+	u32 *buf = NULL;
 
 	mutex_lock(&t->mutex);
 
@@ -327,12 +327,14 @@ static ssize_t etb_read(struct file *file, char __user *data,
 	etb_writel(t, first, ETBR_READADDR);
 
 	length = min(total * 4, (int)len);
-	buf = vmalloc(length);
+	if (length != 0)
+		buf = vmalloc(length);
 
 	dev_dbg(t->dev, "ETB buffer length: %d\n", total);
 	dev_dbg(t->dev, "ETB status reg: %x\n", etb_readl(t, ETBR_STATUS));
-	for (i = 0; i < length / 4; i++)
-		buf[i] = etb_readl(t, ETBR_READMEM);
+	if (buf)
+		for (i = 0; i < length / 4; i++)
+			buf[i] = etb_readl(t, ETBR_READMEM);
 
 	/* the only way to deassert overflow bit in ETB status is this */
 	etb_writel(t, 1, ETBR_CTRL);
@@ -345,7 +347,8 @@ static ssize_t etb_read(struct file *file, char __user *data,
 	etb_lock(t);
 
 	length -= copy_to_user(data, buf, length);
-	vfree(buf);
+	if (buf)
+		vfree(buf);
 
 out:
 	mutex_unlock(&t->mutex);
-- 
1.8.5.2