[PATCH V2 0/4] ARM: perf: Support i.MX53

[PATCH V2 0/4] ARM: perf: Support i.MX53

[Martin Fuzzey]

This series enables hardware performance counters on the i.MX53 SoC

This requires setting registers at both the ARM V7 core level
and the i.MX53 SoC level.

V2: Rework according to suggestions from Will Deacon:
* Make SDER access conditional on DT property
* Use existing runtime PM hooks rather than inventing new ones

[PATCH V2 1/4] ARM: perf: Set suniden bit.

[Martin Fuzzey]

Counters other than the CPU cycle counter only work if the security module
SUNIDEN bit is set.

Since access to this register is only possible in secure mode it will
only be done if the device tree property "secure-reg-access" is set.

Without this:
# perf stat -e cycles,instructions sleep 1

 Performance counter stats for 'sleep 1':

          14606094 cycles                    #    0.000 GHz
                 0 instructions              #    0.00  insns per cycle

Some platforms (eg i.MX53) may also need additional platform specific setup.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 Documentation/devicetree/bindings/arm/pmu.txt |    7 ++++
 arch/arm/include/asm/pmu.h                    |   10 ++++++
 arch/arm/kernel/perf_event_cpu.c              |    3 ++
 arch/arm/kernel/perf_event_v7.c               |   44 +++++++++++++++++++++++++
 4 files changed, 64 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
index 75ef91d..7dab77e 100644
--- a/Documentation/devicetree/bindings/arm/pmu.txt
+++ b/Documentation/devicetree/bindings/arm/pmu.txt
@@ -27,6 +27,13 @@ Optional properties:
 - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd
                      events.
 
+- secure-reg-access : Indicates that secure mode access is available.
+		      This will cause the driver to do any setup required that
+		      is only possible in secure mode.
+		      If not present the secure registers will not be touched,
+		      which means the PMU may fail to operate unless external
+		      code (bootloader or security monitor) has performed the
+		      appropriate initialisation.
 Example:
 
 pmu {
diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
index ae1919b..dfb654f 100644
--- a/arch/arm/include/asm/pmu.h
+++ b/arch/arm/include/asm/pmu.h
@@ -89,6 +89,16 @@ struct arm_pmu {
 	u64		max_period;
 	struct platform_device	*plat_device;
 	struct pmu_hw_events	*(*get_hw_events)(void);
+
+	/*
+	 * Bits indicating any CPU or platform specific activations that have
+	 * been done so we can undo them when stopping
+	 */
+	struct {
+		unsigned int secure_regs_available : 1;
+		unsigned int secure_debug_requested : 1;
+		unsigned int platform_enabled : 1;
+	} activated_flags;
 };
 
 #define to_arm_pmu(p) (container_of(p, struct arm_pmu, pmu))
diff --git a/arch/arm/kernel/perf_event_cpu.c b/arch/arm/kernel/perf_event_cpu.c
index af9e35e..c09e18e 100644
--- a/arch/arm/kernel/perf_event_cpu.c
+++ b/arch/arm/kernel/perf_event_cpu.c
@@ -313,6 +313,9 @@ static int cpu_pmu_device_probe(struct platform_device *pdev)
 	cpu_pmu->plat_device = pdev;
 
 	if (node && (of_id = of_match_node(cpu_pmu_of_device_ids, pdev->dev.of_node))) {
+		pmu->activated_flags.secure_regs_available =
+			of_property_read_bool(pdev->dev.of_node,
+						"secure-reg-access");
 		init_fn = of_id->data;
 		ret = init_fn(pmu);
 	} else {
diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index 1d37568..5ad9626 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -1377,12 +1377,47 @@ static irqreturn_t armv7pmu_handle_irq(int irq_num, void *dev)
 	return IRQ_HANDLED;
 }
 
+#define SDER_SUNIDEN (1 << 1)
+
+static inline u32 armv2pmu_read_sder(void)
+{
+	u32 sder;
+
+	asm volatile("mrc p15, 0, %0, c1, c1, 1" : "=r" (sder));
+
+	return sder;
+}
+
+static inline void armv2pmu_write_sder(u32 sder)
+{
+	asm volatile("mcr p15, 0, %0, c1, c1, 1" : : "r" (sder));
+}
+
 static void armv7pmu_start(struct arm_pmu *cpu_pmu)
 {
 	unsigned long flags;
 	struct pmu_hw_events *events = cpu_pmu->get_hw_events();
 
 	raw_spin_lock_irqsave(&events->pmu_lock, flags);
+
+	/*
+	 * Counters other than cycle counter require SUNIDEN bit set
+	 * Set the bit if the DT configuration allows it (secure mode)
+	 * Otherwise do nothing and hope bootloader / secure monitor did the
+	 * setup.
+	 */
+	if (cpu_pmu->activated_flags.secure_regs_available) {
+		u32 sder = armv2pmu_read_sder();
+
+		if (sder & SDER_SUNIDEN) {
+			cpu_pmu->activated_flags.secure_debug_requested = 0;
+		} else {
+			sder |= SDER_SUNIDEN;
+			armv2pmu_write_sder(sder);
+			cpu_pmu->activated_flags.secure_debug_requested = 1;
+		}
+	}
+
 	/* Enable all counters */
 	armv7_pmnc_write(armv7_pmnc_read() | ARMV7_PMNC_E);
 	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
@@ -1394,6 +1429,15 @@ static void armv7pmu_stop(struct arm_pmu *cpu_pmu)
 	struct pmu_hw_events *events = cpu_pmu->get_hw_events();
 
 	raw_spin_lock_irqsave(&events->pmu_lock, flags);
+
+	if (cpu_pmu->activated_flags.secure_debug_requested) {
+		u32 sder = armv2pmu_read_sder();
+
+		sder &= ~SDER_SUNIDEN;
+		armv2pmu_write_sder(sder);
+		cpu_pmu->activated_flags.secure_debug_requested = 0;
+	}
+
 	/* Disable all counters */
 	armv7_pmnc_write(armv7_pmnc_read() & ~ARMV7_PMNC_E);
 	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);

[PATCH V2 2/4] ARM: perf: Associate PMU data with driver.

[Martin Fuzzey]

In order to use the PM hooks for platform specific control we sometimes
need access to the PMU driver data.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 arch/arm/kernel/perf_event_cpu.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/perf_event_cpu.c b/arch/arm/kernel/perf_event_cpu.c
index c09e18e..951a542 100644
--- a/arch/arm/kernel/perf_event_cpu.c
+++ b/arch/arm/kernel/perf_event_cpu.c
@@ -311,6 +311,7 @@ static int cpu_pmu_device_probe(struct platform_device *pdev)
 
 	cpu_pmu = pmu;
 	cpu_pmu->plat_device = pdev;
+	dev_set_drvdata(&pdev->dev, pmu);
 
 	if (node && (of_id = of_match_node(cpu_pmu_of_device_ids, pdev->dev.of_node))) {
 		pmu->activated_flags.secure_regs_available =

[PATCH V2 3/4] ARM: i.MX53: Add Soc specific PMU setup.

[Martin Fuzzey]

On i.MX53 it is necessary to set the DBG_EN bit in the
platform GPC register to enable access to PMU counters other
than the cycle counter.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 arch/arm/mach-imx/mach-imx53.c |   50 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c
index 2bad387..74f6bf3 100644
--- a/arch/arm/mach-imx/mach-imx53.c
+++ b/arch/arm/mach-imx/mach-imx53.c
@@ -19,16 +19,64 @@
 #include <linux/of_platform.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/time.h>
+#include <asm/pmu.h>
 
 #include "common.h"
+#include "crm-regs-imx5.h"
 #include "hardware.h"
 #include "mx53.h"
 
+#define GPC_DBG_EN (1 << 16)
+
+static int imx53_pmu_resume(struct device *dev)
+{
+	struct arm_pmu *arm_pmu = dev_get_drvdata(dev);
+	u32 gpc;
+
+	gpc = __raw_readl(MXC_CORTEXA8_PLAT_GPC);
+	if (gpc & GPC_DBG_EN) {
+		arm_pmu->activated_flags.platform_enabled = 0;
+	} else {
+		gpc |= GPC_DBG_EN;
+		__raw_writel(gpc, MXC_CORTEXA8_PLAT_GPC);
+		arm_pmu->activated_flags.platform_enabled = 1;
+	}
+
+	return 0;
+}
+
+static int imx53_pmu_suspend(struct device *dev)
+{
+	struct arm_pmu *arm_pmu = dev_get_drvdata(dev);
+	u32 gpc;
+
+	if (arm_pmu->activated_flags.platform_enabled) {
+		gpc = __raw_readl(MXC_CORTEXA8_PLAT_GPC);
+		gpc &= ~GPC_DBG_EN;
+		__raw_writel(gpc, MXC_CORTEXA8_PLAT_GPC);
+		arm_pmu->activated_flags.platform_enabled = 0;
+	}
+
+	return 0;
+}
+
+
+static struct arm_pmu_platdata imx53_pmu_platdata = {
+	.runtime_resume = imx53_pmu_resume,
+	.runtime_suspend = imx53_pmu_suspend,
+};
+
+static struct of_dev_auxdata imx53_auxdata_lookup[] __initdata = {
+	OF_DEV_AUXDATA("arm,cortex-a8-pmu", 0, "arm-pmu", &imx53_pmu_platdata),
+	{}
+};
+
 static void __init imx53_dt_init(void)
 {
 	mxc_arch_reset_init_dt();
 
-	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+	of_platform_populate(NULL, of_default_bus_match_table,
+					imx53_auxdata_lookup, NULL);
 }
 
 static const char *imx53_dt_board_compat[] __initconst = {

[PATCH V2 4/4] ARM: dts: i.MX53: Add PMU DT entry

[Martin Fuzzey]

Add device tree node for PMU on i.MX53

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 arch/arm/boot/dts/imx53.dtsi |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
index 6456a00..ab3afe0 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -101,6 +101,11 @@
 		interrupt-parent = <&tzic>;
 		ranges;
 
+		pmu {
+			compatible = "arm,cortex-a8-pmu";
+			interrupts = <77>;
+		};
+
 		sata: sata at 10000000 {
 			compatible = "fsl,imx53-ahci";
 			reg = <0x10000000 0x1000>;

[PATCH V2 1/4] ARM: perf: Set suniden bit.

[Will Deacon]

Hi Martin,

On Tue, Aug 05, 2014 at 03:48:33PM +0100, Martin Fuzzey wrote:
> Counters other than the CPU cycle counter only work if the security module
> SUNIDEN bit is set.
> 
> Since access to this register is only possible in secure mode it will
> only be done if the device tree property "secure-reg-access" is set.
> 
> Without this:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>           14606094 cycles                    #    0.000 GHz
>                  0 instructions              #    0.00  insns per cycle
> 
> Some platforms (eg i.MX53) may also need additional platform specific setup.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  Documentation/devicetree/bindings/arm/pmu.txt |    7 ++++
>  arch/arm/include/asm/pmu.h                    |   10 ++++++
>  arch/arm/kernel/perf_event_cpu.c              |    3 ++
>  arch/arm/kernel/perf_event_v7.c               |   44 +++++++++++++++++++++++++
>  4 files changed, 64 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
> index 75ef91d..7dab77e 100644
> --- a/Documentation/devicetree/bindings/arm/pmu.txt
> +++ b/Documentation/devicetree/bindings/arm/pmu.txt
> @@ -27,6 +27,13 @@ Optional properties:
>  - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd
>                       events.
>  
> +- secure-reg-access : Indicates that secure mode access is available.
> +		      This will cause the driver to do any setup required that
> +		      is only possible in secure mode.
> +		      If not present the secure registers will not be touched,
> +		      which means the PMU may fail to operate unless external
> +		      code (bootloader or security monitor) has performed the
> +		      appropriate initialisation.
>  Example:
>  
>  pmu {
> diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
> index ae1919b..dfb654f 100644
> --- a/arch/arm/include/asm/pmu.h
> +++ b/arch/arm/include/asm/pmu.h
> @@ -89,6 +89,16 @@ struct arm_pmu {
>  	u64		max_period;
>  	struct platform_device	*plat_device;
>  	struct pmu_hw_events	*(*get_hw_events)(void);
> +
> +	/*
> +	 * Bits indicating any CPU or platform specific activations that have
> +	 * been done so we can undo them when stopping
> +	 */
> +	struct {
> +		unsigned int secure_regs_available : 1;
> +		unsigned int secure_debug_requested : 1;
> +		unsigned int platform_enabled : 1;
> +	} activated_flags;

This should be a single bool secure_access; field.

>  };
>  
>  #define to_arm_pmu(p) (container_of(p, struct arm_pmu, pmu))
> diff --git a/arch/arm/kernel/perf_event_cpu.c b/arch/arm/kernel/perf_event_cpu.c
> index af9e35e..c09e18e 100644
> --- a/arch/arm/kernel/perf_event_cpu.c
> +++ b/arch/arm/kernel/perf_event_cpu.c
> @@ -313,6 +313,9 @@ static int cpu_pmu_device_probe(struct platform_device *pdev)
>  	cpu_pmu->plat_device = pdev;
>  
>  	if (node && (of_id = of_match_node(cpu_pmu_of_device_ids, pdev->dev.of_node))) {
> +		pmu->activated_flags.secure_regs_available =
> +			of_property_read_bool(pdev->dev.of_node,
> +						"secure-reg-access");
>  		init_fn = of_id->data;
>  		ret = init_fn(pmu);
>  	} else {
> diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
> index 1d37568..5ad9626 100644
> --- a/arch/arm/kernel/perf_event_v7.c
> +++ b/arch/arm/kernel/perf_event_v7.c
> @@ -1377,12 +1377,47 @@ static irqreturn_t armv7pmu_handle_irq(int irq_num, void *dev)
>  	return IRQ_HANDLED;
>  }
>  
> +#define SDER_SUNIDEN (1 << 1)
> +
> +static inline u32 armv2pmu_read_sder(void)
> +{
> +	u32 sder;
> +
> +	asm volatile("mrc p15, 0, %0, c1, c1, 1" : "=r" (sder));
> +
> +	return sder;
> +}
> +
> +static inline void armv2pmu_write_sder(u32 sder)
> +{
> +	asm volatile("mcr p15, 0, %0, c1, c1, 1" : : "r" (sder));
> +}

Please stick with the naming convention of the file (armv7pmu_*). You can
also combine these functions into armv7pmu_enable_secure_access...

>  static void armv7pmu_start(struct arm_pmu *cpu_pmu)
>  {
>  	unsigned long flags;
>  	struct pmu_hw_events *events = cpu_pmu->get_hw_events();
>  
>  	raw_spin_lock_irqsave(&events->pmu_lock, flags);
> +
> +	/*
> +	 * Counters other than cycle counter require SUNIDEN bit set
> +	 * Set the bit if the DT configuration allows it (secure mode)
> +	 * Otherwise do nothing and hope bootloader / secure monitor did the
> +	 * setup.
> +	 */
> +	if (cpu_pmu->activated_flags.secure_regs_available) {
> +		u32 sder = armv2pmu_read_sder();
> +
> +		if (sder & SDER_SUNIDEN) {
> +			cpu_pmu->activated_flags.secure_debug_requested = 0;
> +		} else {
> +			sder |= SDER_SUNIDEN;
> +			armv2pmu_write_sder(sder);
> +			cpu_pmu->activated_flags.secure_debug_requested = 1;
> +		}

... then just call that here.

> +	}
> +
>  	/* Enable all counters */
>  	armv7_pmnc_write(armv7_pmnc_read() | ARMV7_PMNC_E);
>  	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
> @@ -1394,6 +1429,15 @@ static void armv7pmu_stop(struct arm_pmu *cpu_pmu)
>  	struct pmu_hw_events *events = cpu_pmu->get_hw_events();
>  
>  	raw_spin_lock_irqsave(&events->pmu_lock, flags);
> +
> +	if (cpu_pmu->activated_flags.secure_debug_requested) {
> +		u32 sder = armv2pmu_read_sder();
> +
> +		sder &= ~SDER_SUNIDEN;
> +		armv2pmu_write_sder(sder);
> +		cpu_pmu->activated_flags.secure_debug_requested = 0;
> +	}

Why do we need to disable this? Couldn't we just set this once during probe
and be done with it?

Will

[PATCH V2 2/4] ARM: perf: Associate PMU data with driver.

[Will Deacon]

On Tue, Aug 05, 2014 at 03:48:35PM +0100, Martin Fuzzey wrote:
> In order to use the PM hooks for platform specific control we sometimes
> need access to the PMU driver data.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  arch/arm/kernel/perf_event_cpu.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/kernel/perf_event_cpu.c b/arch/arm/kernel/perf_event_cpu.c
> index c09e18e..951a542 100644
> --- a/arch/arm/kernel/perf_event_cpu.c
> +++ b/arch/arm/kernel/perf_event_cpu.c
> @@ -311,6 +311,7 @@ static int cpu_pmu_device_probe(struct platform_device *pdev)
>  
>  	cpu_pmu = pmu;
>  	cpu_pmu->plat_device = pdev;
> +	dev_set_drvdata(&pdev->dev, pmu);

I'd rather the platform-specific code used its own structures to keep track
of what it's doing. Exposing the PMU like this is almost certainly going to
cause us problems later on.

Will

[PATCH V2 4/4] ARM: dts: i.MX53: Add PMU DT entry

[Will Deacon]

On Tue, Aug 05, 2014 at 03:48:40PM +0100, Martin Fuzzey wrote:
> Add device tree node for PMU on i.MX53
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  arch/arm/boot/dts/imx53.dtsi |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
> index 6456a00..ab3afe0 100644
> --- a/arch/arm/boot/dts/imx53.dtsi
> +++ b/arch/arm/boot/dts/imx53.dtsi
> @@ -101,6 +101,11 @@
>  		interrupt-parent = <&tzic>;
>  		ranges;
>  
> +		pmu {
> +			compatible = "arm,cortex-a8-pmu";
> +			interrupts = <77>;
> +		};

You're missing the new property.

Will

[PATCH V2 2/4] ARM: perf: Associate PMU data with driver.

[Russell King - ARM Linux]

On Wed, Aug 06, 2014 at 11:50:29AM +0100, Will Deacon wrote:
> On Tue, Aug 05, 2014 at 03:48:35PM +0100, Martin Fuzzey wrote:
> > In order to use the PM hooks for platform specific control we sometimes
> > need access to the PMU driver data.
> > 
> > Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> > ---
> >  arch/arm/kernel/perf_event_cpu.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/kernel/perf_event_cpu.c b/arch/arm/kernel/perf_event_cpu.c
> > index c09e18e..951a542 100644
> > --- a/arch/arm/kernel/perf_event_cpu.c
> > +++ b/arch/arm/kernel/perf_event_cpu.c
> > @@ -311,6 +311,7 @@ static int cpu_pmu_device_probe(struct platform_device *pdev)
> >  
> >  	cpu_pmu = pmu;
> >  	cpu_pmu->plat_device = pdev;
> > +	dev_set_drvdata(&pdev->dev, pmu);
> 
> I'd rather the platform-specific code used its own structures to keep track
> of what it's doing. Exposing the PMU like this is almost certainly going to
> cause us problems later on.

+1 - it's fine for the driver itself to access its private data in the
device struct, but this is not supposed to be a mechanism for passing
stuff between drivers, or indeed code outside of the driver.

A better solution would be a proper API to obtain a reference to this
data - and that must have some side effects to protect against the driver
being unbound at /any/ moment, and therefore must also have an interface
to tell the driver when it can proceed with being unbound.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH V2 1/4] ARM: perf: Set suniden bit.

[Martin Fuzzey]

Hi Will,
thanks for the review.

On 06/08/14 12:49, Will Deacon wrote:
> +	/*
> +	 * Bits indicating any CPU or platform specific activations that have
> +	 * been done so we can undo them when stopping
> +	 */
> +	struct {
> +		unsigned int secure_regs_available : 1;
> +		unsigned int secure_debug_requested : 1;
> +		unsigned int platform_enabled : 1;
> +	} activated_flags;
> This should be a single bool secure_access; field.

The idea was to only actually modify the security settings while someone 
actually uses perf rather than just anytime the driver is loaded.
But I didn't want to disable the bit when exiting perf mode if it had 
already been enabled elsewhere (eg by the bootloader).
Hence the first two bits.

I'm not sure how important this really is but it seemed sensible and not 
too hard.

The third one is no longer necessary if the platform code handles all 
that itself as you suggest in comments on subsequent patch.
>>   
>> +#define SDER_SUNIDEN (1 << 1)
>> +
>> +static inline u32 armv2pmu_read_sder(void)
>> +{
>> +	u32 sder;
>> +
>> +	asm volatile("mrc p15, 0, %0, c1, c1, 1" : "=r" (sder));
>> +
>> +	return sder;
>> +}
>> +
>> +static inline void armv2pmu_write_sder(u32 sder)
>> +{
>> +	asm volatile("mcr p15, 0, %0, c1, c1, 1" : : "r" (sder));
>> +}
> Please stick with the naming convention of the file (armv7pmu_*). You can
> also combine these functions into armv7pmu_enable_secure_access...
Argh typo, sorry

> .
>> +	}
>> +
>>   	/* Enable all counters */
>>   	armv7_pmnc_write(armv7_pmnc_read() | ARMV7_PMNC_E);
>>   	raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
>> @@ -1394,6 +1429,15 @@ static void armv7pmu_stop(struct arm_pmu *cpu_pmu)
>>   	struct pmu_hw_events *events = cpu_pmu->get_hw_events();
>>   
>>   	raw_spin_lock_irqsave(&events->pmu_lock, flags);
>> +
>> +	if (cpu_pmu->activated_flags.secure_debug_requested) {
>> +		u32 sder = armv2pmu_read_sder();
>> +
>> +		sder &= ~SDER_SUNIDEN;
>> +		armv2pmu_write_sder(sder);
>> +		cpu_pmu->activated_flags.secure_debug_requested = 0;
>> +	}
> Why do we need to disable this? Couldn't we just set this once during probe
> and be done with it?

Same as my remark above

Martin

[PATCH V2 4/4] ARM: dts: i.MX53: Add PMU DT entry

[Martin Fuzzey]

On 06/08/14 12:50, Will Deacon wrote:
> On Tue, Aug 05, 2014 at 03:48:40PM +0100, Martin Fuzzey wrote:
>> Add device tree node for PMU on i.MX53
>>
>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
>> ---
>>   arch/arm/boot/dts/imx53.dtsi |    5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
>> index 6456a00..ab3afe0 100644
>> --- a/arch/arm/boot/dts/imx53.dtsi
>> +++ b/arch/arm/boot/dts/imx53.dtsi
>> @@ -101,6 +101,11 @@
>>   		interrupt-parent = <&tzic>;
>>   		ranges;
>>   
>> +		pmu {
>> +			compatible = "arm,cortex-a8-pmu";
>> +			interrupts = <77>;
>> +		};
> You're missing the new property.

Actually this was deliberate.

Since the property should only be used if booting in secure mode this 
generic dtsi file can't know if it is ok or not.
I presume a (probably out of tree) board / application dts file will add 
the property.

Maybe I should put a comment here about that though?

Martin

[PATCH V2 4/4] ARM: dts: i.MX53: Add PMU DT entry

[Shawn Guo]

On Wed, Aug 06, 2014 at 03:35:49PM +0200, Martin Fuzzey wrote:
> On 06/08/14 12:50, Will Deacon wrote:
> >On Tue, Aug 05, 2014 at 03:48:40PM +0100, Martin Fuzzey wrote:
> >>Add device tree node for PMU on i.MX53
> >>
> >>Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> >>---
> >>  arch/arm/boot/dts/imx53.dtsi |    5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >>diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
> >>index 6456a00..ab3afe0 100644
> >>--- a/arch/arm/boot/dts/imx53.dtsi
> >>+++ b/arch/arm/boot/dts/imx53.dtsi
> >>@@ -101,6 +101,11 @@
> >>  		interrupt-parent = <&tzic>;
> >>  		ranges;
> >>+		pmu {
> >>+			compatible = "arm,cortex-a8-pmu";
> >>+			interrupts = <77>;
> >>+		};
> >You're missing the new property.
> 
> Actually this was deliberate.
> 
> Since the property should only be used if booting in secure mode
> this generic dtsi file can't know if it is ok or not.

As far as I know, most of the i.MX53 systems boot Linux kernel in secure
mode.  In that case, we should probably have the new property by
default?

Shawn

> I presume a (probably out of tree) board / application dts file will
> add the property.
> 
> Maybe I should put a comment here about that though?
> 
> Martin

[PATCH V2 4/4] ARM: dts: i.MX53: Add PMU DT entry

[Martin Fuzzey]

On 07/08/14 07:53, Shawn Guo wrote:
> On Wed, Aug 06, 2014 at 03:35:49PM +0200, Martin Fuzzey wrote:
>> On 06/08/14 12:50, Will Deacon wrote:
>>>
>>> You're missing the new property.
>> Actually this was deliberate.
>>
>> Since the property should only be used if booting in secure mode
>> this generic dtsi file can't know if it is ok or not.
> As far as I know, most of the i.MX53 systems boot Linux kernel in secure
> mode.  In that case, we should probably have the new property by
> default?

Is it possible to override a boolean property to remove it (set to 
false) in a board specific dts?

If it can be done then yes I agree it should be the default.

However seeing as the dts syntax doesn't give a value for boolean 
properties I don't see how.

But I don't think people booting non secure should have to modify imx53.dtsi


Martin

[PATCH V2 4/4] ARM: dts: i.MX53: Add PMU DT entry

[Shawn Guo]

On Thu, Aug 07, 2014 at 10:00:08AM +0200, Martin Fuzzey wrote:
> On 07/08/14 07:53, Shawn Guo wrote:
> >On Wed, Aug 06, 2014 at 03:35:49PM +0200, Martin Fuzzey wrote:
> >>On 06/08/14 12:50, Will Deacon wrote:
> >>>
> >>>You're missing the new property.
> >>Actually this was deliberate.
> >>
> >>Since the property should only be used if booting in secure mode
> >>this generic dtsi file can't know if it is ok or not.
> >As far as I know, most of the i.MX53 systems boot Linux kernel in secure
> >mode.  In that case, we should probably have the new property by
> >default?
> 
> Is it possible to override a boolean property to remove it (set to
> false) in a board specific dts?
> 
> If it can be done then yes I agree it should be the default.
> 
> However seeing as the dts syntax doesn't give a value for boolean
> properties I don't see how.
> 
> But I don't think people booting non secure should have to modify imx53.dtsi

Ah, yes.  I missed the fact that the property is a boolean.

Shawn

[PATCH V2 1/4] ARM: perf: Set suniden bit.

[Will Deacon]

On Wed, Aug 06, 2014 at 02:30:51PM +0100, Martin Fuzzey wrote:
> On 06/08/14 12:49, Will Deacon wrote:
> > +	/*
> > +	 * Bits indicating any CPU or platform specific activations that have
> > +	 * been done so we can undo them when stopping
> > +	 */
> > +	struct {
> > +		unsigned int secure_regs_available : 1;
> > +		unsigned int secure_debug_requested : 1;
> > +		unsigned int platform_enabled : 1;
> > +	} activated_flags;
> > This should be a single bool secure_access; field.
> 
> The idea was to only actually modify the security settings while someone 
> actually uses perf rather than just anytime the driver is loaded.
> But I didn't want to disable the bit when exiting perf mode if it had 
> already been enabled elsewhere (eg by the bootloader).
> Hence the first two bits.
> 
> I'm not sure how important this really is but it seemed sensible and not 
> too hard.

I think it's unnecessary complication. We've booted secure, we're not
spawning a non-secure OS and we're not doing any strange
soft-virtualisation. This means that we own the PMU and can configure it
once during probe.

Will

[PATCH V3 1/1] ARM: perf: Set suniden bit

[George G. Davis]

From: Martin Fuzzey <mfuzzey@parkeon.com>

Counters other than the CPU cycle counter only work if the security module
SUNIDEN bit is set.

Since access to this register is only possible in secure mode it will
only be done if the device tree property "secure-reg-access" is set.

Without this:
# perf stat -e cycles,instructions sleep 1

 Performance counter stats for 'sleep 1':

          14606094 cycles                    #    0.000 GHz
                 0 instructions              #    0.00  insns per cycle

After applying:
# perf stat -e cycles,instructions sleep 1

 Performance counter stats for 'sleep 1':

           5843809      cycles                   
           2566484      instructions              #    0.44  insns per cycle        

       1.020144000 seconds time elapsed


Some platforms (eg i.MX53) may also need additional platform specific setup.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com>
Signed-off-by: George G. Davis <george_davis@mentor.com>
---

Changes in v3:
- Pooya Keshavarzi:
  * v2 review comment fixups
  * Use on_each_cpu() to set SUNIDEN on all CPUs
  * Move armv7pmu_enable_secure_access() call from armv7pmu_start() to
    armv7_a8_map_event() such that is called only once instead of each
    time `perf` is executed
- George G. Davis:
  * Fixup to apply after file renames due to commit fa8ad78 (arm: perf:
    factor arm_pmu core out to drivers)
  * Fix checkpatch 'CHECK: Prefer using the BIT macro' issue

 Documentation/devicetree/bindings/arm/pmu.txt |  8 ++++++++
 arch/arm/kernel/perf_event_v7.c               | 17 +++++++++++++++++
 drivers/perf/arm_pmu.c                        |  3 +++
 include/linux/perf/arm_pmu.h                  |  1 +
 4 files changed, 29 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
index 97ba45a..8d4b831 100644
--- a/Documentation/devicetree/bindings/arm/pmu.txt
+++ b/Documentation/devicetree/bindings/arm/pmu.txt
@@ -45,6 +45,14 @@ Optional properties:
 - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd
                      events.
 
+- secure-reg-access : Indicates that secure mode access is available.
+		This will cause the driver to do any setup required that
+		is only possible in secure mode.
+		If not present the secure registers will not be touched,
+		which means the PMU may fail to operate unless external
+		code (bootloader or security monitor) has performed the
+		appropriate initialisation.
+
 Example:
 
 pmu {
diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index 126dc67..8dc5582 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -600,6 +600,11 @@ static const unsigned scorpion_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]
 #define	ARMV7_EXCLUDE_USER	(1 << 30)
 #define	ARMV7_INCLUDE_HYP	(1 << 27)
 
+/*
+ * Secure debug enable reg
+ */
+#define ARMV7_SDER_SUNIDEN	BIT(1) /* Permit non-invasive debug */
+
 static inline u32 armv7_pmnc_read(void)
 {
 	u32 val;
@@ -994,6 +999,15 @@ static void armv7pmu_reset(void *info)
 	armv7_pmnc_write(ARMV7_PMNC_P | ARMV7_PMNC_C);
 }
 
+static void armv7pmu_enable_secure_access(void *data)
+{
+	u32 val;
+
+	asm volatile("mrc p15, 0, %0, c1, c1, 1" : "=r" (val));
+	val |= ARMV7_SDER_SUNIDEN;
+	asm volatile("mcr p15, 0, %0, c1, c1, 1" : : "r" (val));
+}
+
 static int armv7_a8_map_event(struct perf_event *event)
 {
 	return armpmu_map_event(event, &armv7_a8_perf_map,
@@ -1060,6 +1074,9 @@ static void armv7pmu_init(struct arm_pmu *cpu_pmu)
 	cpu_pmu->stop		= armv7pmu_stop;
 	cpu_pmu->reset		= armv7pmu_reset;
 	cpu_pmu->max_period	= (1LLU << 32) - 1;
+
+	if (cpu_pmu->secure_access)
+		on_each_cpu(armv7pmu_enable_secure_access, NULL, 1);
 };
 
 static void armv7_read_num_pmnc_events(void *info)
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index be3755c..972e6b7 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -895,6 +895,9 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 	if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
 		init_fn = of_id->data;
 
+		pmu->secure_access = of_property_read_bool(pdev->dev.of_node,
+							   "secure-reg-access");
+
 		ret = of_pmu_irq_cfg(pmu);
 		if (!ret)
 			ret = init_fn(pmu);
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index bfa673b..4b8dc13 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -104,6 +104,7 @@ struct arm_pmu {
 	atomic_t	active_events;
 	struct mutex	reserve_mutex;
 	u64		max_period;
+	bool		secure_access;
 	struct platform_device	*plat_device;
 	struct pmu_hw_events	__percpu *hw_events;
 	struct notifier_block	hotplug_nb;
-- 
1.9.3

[PATCH V3 1/1] ARM: perf: Set suniden bit

[Will Deacon]

On Wed, Jan 06, 2016 at 09:55:00AM -0500, George G. Davis wrote:
> From: Martin Fuzzey <mfuzzey@parkeon.com>
> 
> Counters other than the CPU cycle counter only work if the security module
> SUNIDEN bit is set.
> 
> Since access to this register is only possible in secure mode it will
> only be done if the device tree property "secure-reg-access" is set.
> 
> Without this:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>           14606094 cycles                    #    0.000 GHz
>                  0 instructions              #    0.00  insns per cycle
> 
> After applying:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>            5843809      cycles                   
>            2566484      instructions              #    0.44  insns per cycle        
> 
>        1.020144000 seconds time elapsed
> 
> 
> Some platforms (eg i.MX53) may also need additional platform specific setup.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> ---
> 
> Changes in v3:
> - Pooya Keshavarzi:
>   * v2 review comment fixups
>   * Use on_each_cpu() to set SUNIDEN on all CPUs
>   * Move armv7pmu_enable_secure_access() call from armv7pmu_start() to
>     armv7_a8_map_event() such that is called only once instead of each
>     time `perf` is executed
> - George G. Davis:
>   * Fixup to apply after file renames due to commit fa8ad78 (arm: perf:
>     factor arm_pmu core out to drivers)
>   * Fix checkpatch 'CHECK: Prefer using the BIT macro' issue
> 
>  Documentation/devicetree/bindings/arm/pmu.txt |  8 ++++++++
>  arch/arm/kernel/perf_event_v7.c               | 17 +++++++++++++++++
>  drivers/perf/arm_pmu.c                        |  3 +++
>  include/linux/perf/arm_pmu.h                  |  1 +
>  4 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
> index 97ba45a..8d4b831 100644
> --- a/Documentation/devicetree/bindings/arm/pmu.txt
> +++ b/Documentation/devicetree/bindings/arm/pmu.txt
> @@ -45,6 +45,14 @@ Optional properties:
>  - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd
>                       events.
>  
> +- secure-reg-access : Indicates that secure mode access is available.
> +		This will cause the driver to do any setup required that
> +		is only possible in secure mode.
> +		If not present the secure registers will not be touched,
> +		which means the PMU may fail to operate unless external
> +		code (bootloader or security monitor) has performed the
> +		appropriate initialisation.

You should make this clear that it's for ARMv7 CPUs only and is not
supported on anything else (in particular, the arm64 port requires you
to boot in non-secure mode).

You should also run this binding change by a devicetree maintainer.

> +
>  Example:
>  
>  pmu {
> diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
> index 126dc67..8dc5582 100644
> --- a/arch/arm/kernel/perf_event_v7.c
> +++ b/arch/arm/kernel/perf_event_v7.c
> @@ -600,6 +600,11 @@ static const unsigned scorpion_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]
>  #define	ARMV7_EXCLUDE_USER	(1 << 30)
>  #define	ARMV7_INCLUDE_HYP	(1 << 27)
>  
> +/*
> + * Secure debug enable reg
> + */
> +#define ARMV7_SDER_SUNIDEN	BIT(1) /* Permit non-invasive debug */
> +
>  static inline u32 armv7_pmnc_read(void)
>  {
>  	u32 val;
> @@ -994,6 +999,15 @@ static void armv7pmu_reset(void *info)
>  	armv7_pmnc_write(ARMV7_PMNC_P | ARMV7_PMNC_C);
>  }
>  
> +static void armv7pmu_enable_secure_access(void *data)
> +{
> +	u32 val;
> +
> +	asm volatile("mrc p15, 0, %0, c1, c1, 1" : "=r" (val));
> +	val |= ARMV7_SDER_SUNIDEN;
> +	asm volatile("mcr p15, 0, %0, c1, c1, 1" : : "r" (val));
> +}

This is probably better off in the ->reset callback, since that it
called off the back of a CPU hotplug notifier when the PMU may need to
be reinitialised.

>  static int armv7_a8_map_event(struct perf_event *event)
>  {
>  	return armpmu_map_event(event, &armv7_a8_perf_map,
> @@ -1060,6 +1074,9 @@ static void armv7pmu_init(struct arm_pmu *cpu_pmu)
>  	cpu_pmu->stop		= armv7pmu_stop;
>  	cpu_pmu->reset		= armv7pmu_reset;
>  	cpu_pmu->max_period	= (1LLU << 32) - 1;
> +
> +	if (cpu_pmu->secure_access)
> +		on_each_cpu(armv7pmu_enable_secure_access, NULL, 1);

Then you can remove this too, since ->reset is called on the relevant
cores already.

Will

[PATCH v4 1/1] ARM: perf: Set ARMv7 SDER SUNIDEN bit

[George G. Davis]

From: Martin Fuzzey <mfuzzey-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org>

ARMv7 counters other than the CPU cycle counter only work if the Secure
Debug Enable Register (SDER) SUNIDEN bit is set.

Since access to the SDER is only possible in secure state, it will
only be done if the device tree property "secure-reg-access" is set.

Without this:
# perf stat -e cycles,instructions sleep 1

 Performance counter stats for 'sleep 1':

          14606094 cycles                    #    0.000 GHz
                 0 instructions              #    0.00  insns per cycle

After applying:
# perf stat -e cycles,instructions sleep 1

 Performance counter stats for 'sleep 1':

           5843809 cycles
           2566484 instructions              #    0.44  insns per cycle

       1.020144000 seconds time elapsed

Some platforms (eg i.MX53) may also need additional platform specific
setup.

Signed-off-by: Martin Fuzzey <mfuzzey-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi-V5te9oGctAVWk0Htik3J/w@public.gmane.org>
Signed-off-by: George G. Davis <george_davis-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
Changes in v4:
- Reword commit message to clarify that this change is ARMv7 specific.
- Clarify that secure-reg-access property is only valid for ARMv7 CPUs
  and is not supported on anything else (in particular, the arm64
  port requires you to boot in non-secure mode).
- Convert on_each_cpu(armv7pmu_enable_secure_access, NULL, 1) call in
  armv7pmu_init() to in-lined code in ->reset callback, since that is
  called off the back of a CPU hotplug notifier when the PMU may need
  to be reinitialised.
Changes in v3:
- Pooya Keshavarzi:
  * v2 review comment fixups
  * Use on_each_cpu() to set SUNIDEN on all CPUs
  * Move armv7pmu_enable_secure_access() call from armv7pmu_start() to
    armv7_a8_map_event() such that is called only once instead of each
    time `perf` is executed
- George G. Davis:
  * Fixup to apply after file renames due to commit fa8ad78 (arm: perf:
    factor arm_pmu core out to drivers)
  * Fix checkpatch 'CHECK: Prefer using the BIT macro' issue
---
 Documentation/devicetree/bindings/arm/pmu.txt | 10 ++++++++++
 arch/arm/kernel/perf_event_v7.c               | 13 ++++++++++++-
 drivers/perf/arm_pmu.c                        |  3 +++
 include/linux/perf/arm_pmu.h                  |  1 +
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
index 97ba45a..27f5873 100644
--- a/Documentation/devicetree/bindings/arm/pmu.txt
+++ b/Documentation/devicetree/bindings/arm/pmu.txt
@@ -45,6 +45,16 @@ Optional properties:
 - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd
                      events.
 
+- secure-reg-access : Indicates that the ARMv7 Secure Debug Enable Register
+		      (SDER) is accessible. This will cause the driver to do
+		      any setup required that is only possible in ARMv7 secure
+		      state. If not present the ARMv7 SDER will not be touched,
+		      which means the PMU may fail to operate unless external
+		      code (bootloader or security monitor) has performed the
+		      appropriate initialisation. Note that this property is
+		      not valid for non-ARMv7 CPUs or ARMv7 CPUs booting Linux
+		      in Non-secure state.
+
 Example:
 
 pmu {
diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index 126dc67..1c3551c 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -600,6 +600,11 @@ static const unsigned scorpion_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]
 #define	ARMV7_EXCLUDE_USER	(1 << 30)
 #define	ARMV7_INCLUDE_HYP	(1 << 27)
 
+/*
+ * Secure debug enable reg
+ */
+#define ARMV7_SDER_SUNIDEN	BIT(1) /* Permit non-invasive debug */
+
 static inline u32 armv7_pmnc_read(void)
 {
 	u32 val;
@@ -982,7 +987,13 @@ static int armv7pmu_set_event_filter(struct hw_perf_event *event,
 static void armv7pmu_reset(void *info)
 {
 	struct arm_pmu *cpu_pmu = (struct arm_pmu *)info;
-	u32 idx, nb_cnt = cpu_pmu->num_events;
+	u32 idx, nb_cnt = cpu_pmu->num_events, val;
+
+	if (cpu_pmu->secure_access) {
+		asm volatile("mrc p15, 0, %0, c1, c1, 1" : "=r" (val));
+		val |= ARMV7_SDER_SUNIDEN;
+		asm volatile("mcr p15, 0, %0, c1, c1, 1" : : "r" (val));
+	}
 
 	/* The counter and interrupt enable registers are unknown at reset. */
 	for (idx = ARMV7_IDX_CYCLE_COUNTER; idx < nb_cnt; ++idx) {
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index be3755c..972e6b7 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -895,6 +895,9 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 	if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
 		init_fn = of_id->data;
 
+		pmu->secure_access = of_property_read_bool(pdev->dev.of_node,
+							   "secure-reg-access");
+
 		ret = of_pmu_irq_cfg(pmu);
 		if (!ret)
 			ret = init_fn(pmu);
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index bfa673b..4b8dc13 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -104,6 +104,7 @@ struct arm_pmu {
 	atomic_t	active_events;
 	struct mutex	reserve_mutex;
 	u64		max_period;
+	bool		secure_access;
 	struct platform_device	*plat_device;
 	struct pmu_hw_events	__percpu *hw_events;
 	struct notifier_block	hotplug_nb;
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/1] ARM: perf: Set ARMv7 SDER SUNIDEN bit

[George G. Davis]

From: Martin Fuzzey <mfuzzey@parkeon.com>

ARMv7 counters other than the CPU cycle counter only work if the Secure
Debug Enable Register (SDER) SUNIDEN bit is set.

Since access to the SDER is only possible in secure state, it will
only be done if the device tree property "secure-reg-access" is set.

Without this:
# perf stat -e cycles,instructions sleep 1

 Performance counter stats for 'sleep 1':

          14606094 cycles                    #    0.000 GHz
                 0 instructions              #    0.00  insns per cycle

After applying:
# perf stat -e cycles,instructions sleep 1

 Performance counter stats for 'sleep 1':

           5843809 cycles
           2566484 instructions              #    0.44  insns per cycle

       1.020144000 seconds time elapsed

Some platforms (eg i.MX53) may also need additional platform specific
setup.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com>
Signed-off-by: George G. Davis <george_davis@mentor.com>
---
Changes in v4:
- Reword commit message to clarify that this change is ARMv7 specific.
- Clarify that secure-reg-access property is only valid for ARMv7 CPUs
  and is not supported on anything else (in particular, the arm64
  port requires you to boot in non-secure mode).
- Convert on_each_cpu(armv7pmu_enable_secure_access, NULL, 1) call in
  armv7pmu_init() to in-lined code in ->reset callback, since that is
  called off the back of a CPU hotplug notifier when the PMU may need
  to be reinitialised.
Changes in v3:
- Pooya Keshavarzi:
  * v2 review comment fixups
  * Use on_each_cpu() to set SUNIDEN on all CPUs
  * Move armv7pmu_enable_secure_access() call from armv7pmu_start() to
    armv7_a8_map_event() such that is called only once instead of each
    time `perf` is executed
- George G. Davis:
  * Fixup to apply after file renames due to commit fa8ad78 (arm: perf:
    factor arm_pmu core out to drivers)
  * Fix checkpatch 'CHECK: Prefer using the BIT macro' issue
---
 Documentation/devicetree/bindings/arm/pmu.txt | 10 ++++++++++
 arch/arm/kernel/perf_event_v7.c               | 13 ++++++++++++-
 drivers/perf/arm_pmu.c                        |  3 +++
 include/linux/perf/arm_pmu.h                  |  1 +
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
index 97ba45a..27f5873 100644
--- a/Documentation/devicetree/bindings/arm/pmu.txt
+++ b/Documentation/devicetree/bindings/arm/pmu.txt
@@ -45,6 +45,16 @@ Optional properties:
 - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd
                      events.
 
+- secure-reg-access : Indicates that the ARMv7 Secure Debug Enable Register
+		      (SDER) is accessible. This will cause the driver to do
+		      any setup required that is only possible in ARMv7 secure
+		      state. If not present the ARMv7 SDER will not be touched,
+		      which means the PMU may fail to operate unless external
+		      code (bootloader or security monitor) has performed the
+		      appropriate initialisation. Note that this property is
+		      not valid for non-ARMv7 CPUs or ARMv7 CPUs booting Linux
+		      in Non-secure state.
+
 Example:
 
 pmu {
diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index 126dc67..1c3551c 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -600,6 +600,11 @@ static const unsigned scorpion_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]
 #define	ARMV7_EXCLUDE_USER	(1 << 30)
 #define	ARMV7_INCLUDE_HYP	(1 << 27)
 
+/*
+ * Secure debug enable reg
+ */
+#define ARMV7_SDER_SUNIDEN	BIT(1) /* Permit non-invasive debug */
+
 static inline u32 armv7_pmnc_read(void)
 {
 	u32 val;
@@ -982,7 +987,13 @@ static int armv7pmu_set_event_filter(struct hw_perf_event *event,
 static void armv7pmu_reset(void *info)
 {
 	struct arm_pmu *cpu_pmu = (struct arm_pmu *)info;
-	u32 idx, nb_cnt = cpu_pmu->num_events;
+	u32 idx, nb_cnt = cpu_pmu->num_events, val;
+
+	if (cpu_pmu->secure_access) {
+		asm volatile("mrc p15, 0, %0, c1, c1, 1" : "=r" (val));
+		val |= ARMV7_SDER_SUNIDEN;
+		asm volatile("mcr p15, 0, %0, c1, c1, 1" : : "r" (val));
+	}
 
 	/* The counter and interrupt enable registers are unknown at reset. */
 	for (idx = ARMV7_IDX_CYCLE_COUNTER; idx < nb_cnt; ++idx) {
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index be3755c..972e6b7 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -895,6 +895,9 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 	if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
 		init_fn = of_id->data;
 
+		pmu->secure_access = of_property_read_bool(pdev->dev.of_node,
+							   "secure-reg-access");
+
 		ret = of_pmu_irq_cfg(pmu);
 		if (!ret)
 			ret = init_fn(pmu);
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index bfa673b..4b8dc13 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -104,6 +104,7 @@ struct arm_pmu {
 	atomic_t	active_events;
 	struct mutex	reserve_mutex;
 	u64		max_period;
+	bool		secure_access;
 	struct platform_device	*plat_device;
 	struct pmu_hw_events	__percpu *hw_events;
 	struct notifier_block	hotplug_nb;
-- 
1.9.3

Re: [PATCH v4 1/1] ARM: perf: Set ARMv7 SDER SUNIDEN bit

[Rob Herring]

On Wed, Jan 13, 2016 at 11:36:26PM -0500, George G. Davis wrote:
> From: Martin Fuzzey <mfuzzey-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org>
> 
> ARMv7 counters other than the CPU cycle counter only work if the Secure
> Debug Enable Register (SDER) SUNIDEN bit is set.
> 
> Since access to the SDER is only possible in secure state, it will
> only be done if the device tree property "secure-reg-access" is set.
> 
> Without this:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>           14606094 cycles                    #    0.000 GHz
>                  0 instructions              #    0.00  insns per cycle
> 
> After applying:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>            5843809 cycles
>            2566484 instructions              #    0.44  insns per cycle
> 
>        1.020144000 seconds time elapsed
> 
> Some platforms (eg i.MX53) may also need additional platform specific
> setup.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi-V5te9oGctAVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: George G. Davis <george_davis-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v4:
> - Reword commit message to clarify that this change is ARMv7 specific.
> - Clarify that secure-reg-access property is only valid for ARMv7 CPUs
>   and is not supported on anything else (in particular, the arm64
>   port requires you to boot in non-secure mode).
> - Convert on_each_cpu(armv7pmu_enable_secure_access, NULL, 1) call in
>   armv7pmu_init() to in-lined code in ->reset callback, since that is
>   called off the back of a CPU hotplug notifier when the PMU may need
>   to be reinitialised.
> Changes in v3:
> - Pooya Keshavarzi:
>   * v2 review comment fixups
>   * Use on_each_cpu() to set SUNIDEN on all CPUs
>   * Move armv7pmu_enable_secure_access() call from armv7pmu_start() to
>     armv7_a8_map_event() such that is called only once instead of each
>     time `perf` is executed
> - George G. Davis:
>   * Fixup to apply after file renames due to commit fa8ad78 (arm: perf:
>     factor arm_pmu core out to drivers)
>   * Fix checkpatch 'CHECK: Prefer using the BIT macro' issue
> ---
>  Documentation/devicetree/bindings/arm/pmu.txt | 10 ++++++++++

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/1] ARM: perf: Set ARMv7 SDER SUNIDEN bit

[Rob Herring]

On Wed, Jan 13, 2016 at 11:36:26PM -0500, George G. Davis wrote:
> From: Martin Fuzzey <mfuzzey@parkeon.com>
> 
> ARMv7 counters other than the CPU cycle counter only work if the Secure
> Debug Enable Register (SDER) SUNIDEN bit is set.
> 
> Since access to the SDER is only possible in secure state, it will
> only be done if the device tree property "secure-reg-access" is set.
> 
> Without this:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>           14606094 cycles                    #    0.000 GHz
>                  0 instructions              #    0.00  insns per cycle
> 
> After applying:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>            5843809 cycles
>            2566484 instructions              #    0.44  insns per cycle
> 
>        1.020144000 seconds time elapsed
> 
> Some platforms (eg i.MX53) may also need additional platform specific
> setup.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> ---
> Changes in v4:
> - Reword commit message to clarify that this change is ARMv7 specific.
> - Clarify that secure-reg-access property is only valid for ARMv7 CPUs
>   and is not supported on anything else (in particular, the arm64
>   port requires you to boot in non-secure mode).
> - Convert on_each_cpu(armv7pmu_enable_secure_access, NULL, 1) call in
>   armv7pmu_init() to in-lined code in ->reset callback, since that is
>   called off the back of a CPU hotplug notifier when the PMU may need
>   to be reinitialised.
> Changes in v3:
> - Pooya Keshavarzi:
>   * v2 review comment fixups
>   * Use on_each_cpu() to set SUNIDEN on all CPUs
>   * Move armv7pmu_enable_secure_access() call from armv7pmu_start() to
>     armv7_a8_map_event() such that is called only once instead of each
>     time `perf` is executed
> - George G. Davis:
>   * Fixup to apply after file renames due to commit fa8ad78 (arm: perf:
>     factor arm_pmu core out to drivers)
>   * Fix checkpatch 'CHECK: Prefer using the BIT macro' issue
> ---
>  Documentation/devicetree/bindings/arm/pmu.txt | 10 ++++++++++

Acked-by: Rob Herring <robh@kernel.org>

Rob

Re: [PATCH v4 1/1] ARM: perf: Set ARMv7 SDER SUNIDEN bit

[Will Deacon]

On Wed, Jan 13, 2016 at 11:36:26PM -0500, George G. Davis wrote:
> From: Martin Fuzzey <mfuzzey-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org>
> 
> ARMv7 counters other than the CPU cycle counter only work if the Secure
> Debug Enable Register (SDER) SUNIDEN bit is set.
> 
> Since access to the SDER is only possible in secure state, it will
> only be done if the device tree property "secure-reg-access" is set.
> 
> Without this:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>           14606094 cycles                    #    0.000 GHz
>                  0 instructions              #    0.00  insns per cycle
> 
> After applying:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>            5843809 cycles
>            2566484 instructions              #    0.44  insns per cycle
> 
>        1.020144000 seconds time elapsed
> 
> Some platforms (eg i.MX53) may also need additional platform specific
> setup.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey-mB3Nsq4MPf1BDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi-V5te9oGctAVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: George G. Davis <george_davis-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v4:
> - Reword commit message to clarify that this change is ARMv7 specific.
> - Clarify that secure-reg-access property is only valid for ARMv7 CPUs
>   and is not supported on anything else (in particular, the arm64
>   port requires you to boot in non-secure mode).
> - Convert on_each_cpu(armv7pmu_enable_secure_access, NULL, 1) call in
>   armv7pmu_init() to in-lined code in ->reset callback, since that is
>   called off the back of a CPU hotplug notifier when the PMU may need
>   to be reinitialised.

Thanks. I'll queue this for 4.6 along with Rob's ack.

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/1] ARM: perf: Set ARMv7 SDER SUNIDEN bit

[Will Deacon]

On Wed, Jan 13, 2016 at 11:36:26PM -0500, George G. Davis wrote:
> From: Martin Fuzzey <mfuzzey@parkeon.com>
> 
> ARMv7 counters other than the CPU cycle counter only work if the Secure
> Debug Enable Register (SDER) SUNIDEN bit is set.
> 
> Since access to the SDER is only possible in secure state, it will
> only be done if the device tree property "secure-reg-access" is set.
> 
> Without this:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>           14606094 cycles                    #    0.000 GHz
>                  0 instructions              #    0.00  insns per cycle
> 
> After applying:
> # perf stat -e cycles,instructions sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>            5843809 cycles
>            2566484 instructions              #    0.44  insns per cycle
> 
>        1.020144000 seconds time elapsed
> 
> Some platforms (eg i.MX53) may also need additional platform specific
> setup.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> ---
> Changes in v4:
> - Reword commit message to clarify that this change is ARMv7 specific.
> - Clarify that secure-reg-access property is only valid for ARMv7 CPUs
>   and is not supported on anything else (in particular, the arm64
>   port requires you to boot in non-secure mode).
> - Convert on_each_cpu(armv7pmu_enable_secure_access, NULL, 1) call in
>   armv7pmu_init() to in-lined code in ->reset callback, since that is
>   called off the back of a CPU hotplug notifier when the PMU may need
>   to be reinitialised.

Thanks. I'll queue this for 4.6 along with Rob's ack.

Will