ARM: test for PMU feature on v7.

PATCH New Rusty Russell 5 participants 14 messages Mar 23, 2012 – Mar 30, 2012 1 message not in the archive

Subsystems (4)

Arm (32-bit) KVM perf events Arm PMU profiling and debugging

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Rusty Russell

author

2012-03-23 00:32 UTC #1 lore raw

PATCH 1/1 ARM: test for PMU feature on v7.

The v7 ARM (B4.1.82) specifies that bits 27-24 of the ID_DFR0 reg show what performance monitoring (if any) is available. We should test this before assuming (useful for inside virtualized environments, for example).

Signed-off-by Rusty Russell <[email protected]>

arch/arm/kernel/perf_event_v7.c +14 -0

index 6933244..303d1e1 100644
@@ -1108,6 +1108,12 @@ static struct arm_pmu armv7pmu = {
 	.max_period		= (1LLU << 32) - 1,
 };
 
+static bool armv7_pmu_avail(void)
+{
+	/* 0 means unknown (maybe v1), 1 means v2, 2 means v2, 15 means none */
+	return ((read_cpuid_ext(CPUID_EXT_DFR0) >> 24) & 0xF) != 0xF;
+}
+
 static u32 __init armv7_read_num_pmnc_events(void)
 {
 	u32 nb_cnt;

@@ -1121,6 +1127,8 @@ static u32 __init armv7_read_num_pmnc_events(void)
 
 static struct arm_pmu *__init armv7_a8_pmu_init(void)
 {
+	if (!armv7_pmu_avail())
+		return NULL;
 	armv7pmu.id		= ARM_PERF_PMU_ID_CA8;
 	armv7pmu.name		= "ARMv7 Cortex-A8";
 	armv7pmu.map_event	= armv7_a8_map_event;

@@ -1130,6 +1138,8 @@ static struct arm_pmu *__init armv7_a8_pmu_init(void)
 
 static struct arm_pmu *__init armv7_a9_pmu_init(void)
 {
+	if (!armv7_pmu_avail())
+		return NULL;
 	armv7pmu.id		= ARM_PERF_PMU_ID_CA9;
 	armv7pmu.name		= "ARMv7 Cortex-A9";
 	armv7pmu.map_event	= armv7_a9_map_event;

@@ -1139,6 +1149,8 @@ static struct arm_pmu *__init armv7_a9_pmu_init(void)
 
 static struct arm_pmu *__init armv7_a5_pmu_init(void)
 {
+	if (!armv7_pmu_avail())
+		return NULL;
 	armv7pmu.id		= ARM_PERF_PMU_ID_CA5;
 	armv7pmu.name		= "ARMv7 Cortex-A5";
 	armv7pmu.map_event	= armv7_a5_map_event;

@@ -1148,6 +1160,8 @@ static struct arm_pmu *__init armv7_a5_pmu_init(void)
 
 static struct arm_pmu *__init armv7_a15_pmu_init(void)
 {
+	if (!armv7_pmu_avail())
+		return NULL;
 	armv7pmu.id		= ARM_PERF_PMU_ID_CA15;
 	armv7pmu.name		= "ARMv7 Cortex-A15";
 	armv7pmu.map_event	= armv7_a15_map_event;

Rusty Russell

author

2012-03-23 00:34 UTC #2 lore raw

PATCH 1/1 ARM: test for PMU feature on v7 (v2 with typo fix)

Signed-off-by Rusty Russell <[email protected]>

arch/arm/kernel/perf_event_v7.c +14 -0

index 6933244..303d1e1 100644
@@ -1108,6 +1108,12 @@ static struct arm_pmu armv7pmu = {
 	.max_period		= (1LLU << 32) - 1,
 };
 
+static bool armv7_pmu_avail(void)
+{
+	/* 0 means unknown (maybe v1), 1 means v1, 2 means v2, 15 means none */
+	return ((read_cpuid_ext(CPUID_EXT_DFR0) >> 24) & 0xF) != 0xF;
+}
+
 static u32 __init armv7_read_num_pmnc_events(void)
 {
 	u32 nb_cnt;

@@ -1121,6 +1127,8 @@ static u32 __init armv7_read_num_pmnc_events(void)
 
 static struct arm_pmu *__init armv7_a8_pmu_init(void)
 {
+	if (!armv7_pmu_avail())
+		return NULL;
 	armv7pmu.id		= ARM_PERF_PMU_ID_CA8;
 	armv7pmu.name		= "ARMv7 Cortex-A8";
 	armv7pmu.map_event	= armv7_a8_map_event;

@@ -1130,6 +1138,8 @@ static struct arm_pmu *__init armv7_a8_pmu_init(void)
 
 static struct arm_pmu *__init armv7_a9_pmu_init(void)
 {
+	if (!armv7_pmu_avail())
+		return NULL;
 	armv7pmu.id		= ARM_PERF_PMU_ID_CA9;
 	armv7pmu.name		= "ARMv7 Cortex-A9";
 	armv7pmu.map_event	= armv7_a9_map_event;

@@ -1139,6 +1149,8 @@ static struct arm_pmu *__init armv7_a9_pmu_init(void)
 
 static struct arm_pmu *__init armv7_a5_pmu_init(void)
 {
+	if (!armv7_pmu_avail())
+		return NULL;
 	armv7pmu.id		= ARM_PERF_PMU_ID_CA5;
 	armv7pmu.name		= "ARMv7 Cortex-A5";
 	armv7pmu.map_event	= armv7_a5_map_event;

@@ -1148,6 +1160,8 @@ static struct arm_pmu *__init armv7_a5_pmu_init(void)
 
 static struct arm_pmu *__init armv7_a15_pmu_init(void)
 {
+	if (!armv7_pmu_avail())
+		return NULL;
 	armv7pmu.id		= ARM_PERF_PMU_ID_CA15;
 	armv7pmu.name		= "ARMv7 Cortex-A15";
 	armv7pmu.map_event	= armv7_a15_map_event;

Will Deacon

2012-03-23 09:53 UTC #3 lore raw

PATCH 1/1 ARM: test for PMU feature on v7 (v2 with typo fix)

Hi Rusty,

Quoted 3 lines · Rusty Russell

The v7 ARM (B4.1.82) specifies that bits 27-24 of the ID_DFR0 reg show what performance monitoring (if any) is available. We should test this before assuming (useful for inside virtualized environments, for example).

Ok, I can see why you don't want to emulate a PMU but I don't agree with the patch :)

Quoted 18 lines

arch/arm/kernel/perf_event_v7.c +6 -0

index 6933244..303d1e1 100644
@@ -1108,6 +1108,12 @@ static struct arm_pmu armv7pmu = {
 	.max_period		= (1LLU << 32) - 1,
 };
 
+static bool armv7_pmu_avail(void)
+{
+	/* 0 means unknown (maybe v1), 1 means v1, 2 means v2, 15 means none */
+	return ((read_cpuid_ext(CPUID_EXT_DFR0) >> 24) & 0xF) != 0xF;
+}
+
 static u32 __init armv7_read_num_pmnc_events(void)
 {
 	u32 nb_cnt;

arch/arm/kernel/perf_event_v7.c static u32 __init armv7_read_num_pmnc_events(void) · line 1131

Quoted 5 lines

arch/arm/kernel/perf_event_v7.c +2 -0

@@ -1121,6 +1127,8 @@ static u32 __init armv7_read_num_pmnc_events(void)
 
 static struct arm_pmu *__init armv7_a8_pmu_init(void)
 {
+	if (!armv7_pmu_avail())
+		return NULL;

This code is only executed if we know for sure that we are running on a Cortex-A8. Cortex-A8 implementations have a PMU, so I don't like having the probe here in case we are running on a virtualised CPU that doesn't quite match the hardware.

Would you be able to advertise 0 event counters instead and treat the PMU largely as RAZ/WI? The only tricky bit I can see is the cycle counter, which is mandated by the presence of a PMU, however this could also just RAZ initially.

arch/arm/kernel/perf_event_v7.c static struct arm_pmu *__init armv7_a8_pmu_init(void) · line 1142

Quoted 9 lines

arch/arm/kernel/perf_event_v7.c +2 -0

 	armv7pmu.id		= ARM_PERF_PMU_ID_CA8;
 	armv7pmu.name		= "ARMv7 Cortex-A8";
 	armv7pmu.map_event	= armv7_a8_map_event;

@@ -1130,6 +1138,8 @@ static struct arm_pmu *__init armv7_a8_pmu_init(void)
 
 static struct arm_pmu *__init armv7_a9_pmu_init(void)
 {
+	if (!armv7_pmu_avail())
+		return NULL;

Similarly for A5, A9, A7 and A15...

Cheers,

Will

Rusty Russell

author

2012-03-23 23:17 UTC #4 lore raw

PATCH 1/1 ARM: test for PMU feature on v7 (v2 with typo fix)

Quoted 9 lines · Will Deacon

Hi Rusty,
Quoted 3 lines · Rusty Russell
The v7 ARM (B4.1.82) specifies that bits 27-24 of the ID_DFR0 reg show what performance monitoring (if any) is available. We should test this before assuming (useful for inside virtualized environments, for example).
Ok, I can see why you don't want to emulate a PMU but I don't agree with the patch :)

...

Quoted 9 lines

Review
Quoted 4 lines
static struct arm_pmu *__init armv7_a8_pmu_init(void) {
(quoted diff) +2 -0
+	if (!armv7_pmu_avail())
+		return NULL;
This code is only executed if we know for sure that we are running on a Cortex-A8. Cortex-A8 implementations have a PMU, so I don't like having the probe here in case we are running on a virtualised CPU that doesn't quite match the hardware.

OK, I see that argument, but I don't like it. eg. the Cortex A-15 is defined to have a PMUv2 with 6 counters, and below you're arguing we should implement zero of them.

I had assumed that as we trend towards a common cross-platform kernel, we should be relying on feature registers where available, not the model.

Quoted 4 lines

Would you be able to advertise 0 event counters instead and treat the PMU largely as RAZ/WI? The only tricky bit I can see is the cycle counter, which is mandated by the presence of a PMU, however this could also just RAZ initially.

Yes, that's the effect of the current emulation hack. Far better not to lie to the guest, however, than get weird results from profiling.

Cheers,
Rusty.

Signature · 1 line

  How could I marry someone with more hair than me?  http://baldalex.org

Will Deacon

2012-03-26 09:40 UTC #5 lore raw

PATCH 1/1 ARM: test for PMU feature on v7 (v2 with typo fix)

[removed some dead email addresses from CC]

Quoted 13 lines · Rusty Russell

Quoted 1 line · Will Deacon
On Fri, Mar 23, 2012 at 12:34:57AM +0000, Rusty Russell wrote:
...
Quoted 9 lines
Review
Quoted 4 lines
static struct arm_pmu *__init armv7_a8_pmu_init(void) {
(quoted diff) +2 -0
+	if (!armv7_pmu_avail())
+		return NULL;
This code is only executed if we know for sure that we are running on a Cortex-A8. Cortex-A8 implementations have a PMU, so I don't like having the probe here in case we are running on a virtualised CPU that doesn't quite match the hardware.

Quoted 3 lines

OK, I see that argument, but I don't like it. eg. the Cortex A-15 is defined to have a PMUv2 with 6 counters, and below you're arguing we should implement zero of them.

Well, I think you should actually implement all 6 of them if that's the target which you're emulating. However, there are bigger peripherals to fry than the PMU so if you want to get something working quickly then a dummy PMU makes sense as a stopgap.

Quoted 3 lines

I had assumed that as we trend towards a common cross-platform kernel, we should be relying on feature registers where available, not the model.

The PMU is very tightly coupled with the CPU and it is necessary to know exactly which CPU you have so that you can map microarchitectural events to their identifiers. This is not discoverable in the architecture [PMUv2 provides some support for discovering the status of architected events, but that is still not enough and is only present on A7 and A15 so far] so that's why we probe the CPU instead.

Now, if everything was device-tree based then we could simply use a different binding for each CPU but since we support perf on non-DT platforms, probing the CPU type is the best solution. I would like to avoid the probing code if we are initialised from DT, but I've not got round to it yet (this would be useful for big.LITTLE).

Quoted 7 lines

Quoted 4 lines
Would you be able to advertise 0 event counters instead and treat the PMU largely as RAZ/WI? The only tricky bit I can see is the cycle counter, which is mandated by the presence of a PMU, however this could also just RAZ initially.
Yes, that's the effect of the current emulation hack. Far better not to lie to the guest, however, than get weird results from profiling.

I'd rather lie to the guest than modify it just to avoid emulating the device correctly. Even then, the profiling results wouldn't be especially weird -- everything would be 0, which is a lot better than random numbers.

Will

Peter Maydell

2012-03-26 09:50 UTC #6 lore raw

PATCH 1/1 ARM: test for PMU feature on v7 (v2 with typo fix)

Quoted 9 lines · Will Deacon

[removed some dead email addresses from CC]
Quoted 6 lines · Rusty Russell
Quoted 4 lines · Will Deacon
This code is only executed if we know for sure that we are running on a Cortex-A8. Cortex-A8 implementations have a PMU, so I don't like having the probe here in case we are running on a virtualised CPU that doesn't quite match the hardware.

Quoted 6 lines

Quoted 3 lines
OK, I see that argument, but I don't like it. ?eg. the Cortex A-15 is defined to have a PMUv2 with 6 counters, and below you're arguing we should implement zero of them.
Well, I think you should actually implement all 6 of them if that's the target which you're emulating.

The trouble with this line of thought is that it leads to the requirement to support nested virtualization and virtualization of TrustZone, neither of which are actually particularly useful or simple to do. At some point we're going to end up saying that the "A15" we expose to the guest is not an exact match to the real hardware, that it's missing certain features (and hopefully that the lack of those features is advertised via the feature registers). So the question is where you draw the line...

-- PMM

Will Deacon

2012-03-26 15:15 UTC #7 lore raw

PATCH 1/1 ARM: test for PMU feature on v7 (v2 with typo fix)

Quoted 10 lines · Peter Maydell

Quoted 8 lines · Will Deacon
Quoted 4 lines · Rusty Russell
OK, I see that argument, but I don't like it. ?eg. the Cortex A-15 is defined to have a PMUv2 with 6 counters, and below you're arguing we should implement zero of them.
Well, I think you should actually implement all 6 of them if that's the target which you're emulating.

Quoted 7 lines

The trouble with this line of thought is that it leads to the requirement to support nested virtualization and virtualization of TrustZone, neither of which are actually particularly useful or simple to do. At some point we're going to end up saying that the "A15" we expose to the guest is not an exact match to the real hardware, that it's missing certain features (and hopefully that the lack of those features is advertised via the feature registers). So the question is where you draw the line...

I'd be inclined to say that, at least for kvm, you should draw the line at the point where the guest doesn't need modifications to run in the virtual environment. That is, everything used by Linux is emulated enough for the kernel to run happily. Clearly you can improve on that in the areas which are considered to be most critical.

Aside: given that guests have to run non-secure, how would you emulate a secure platform without emulating the corresponding TrustZone API? Or is that one of the reasons why you're currently targetting Versatile Express?

Will

Rusty Russell

author

2012-03-28 06:09 UTC #8 lore raw

PATCH 1/1 ARM: test for PMU feature on v7 (v2 with typo fix)

In reply to Will Deacon

Quoted 1 line · Will Deacon

[removed some dead email addresses from CC]

Thanks!

Quoted 11 lines · Rusty Russell

Quoted 3 lines
I had assumed that as we trend towards a common cross-platform kernel, we should be relying on feature registers where available, not the model.
The PMU is very tightly coupled with the CPU and it is necessary to know exactly which CPU you have so that you can map microarchitectural events to their identifiers. This is not discoverable in the architecture [PMUv2 provides some support for discovering the status of architected events, but that is still not enough and is only present on A7 and A15 so far] so that's why we probe the CPU instead.

OK.

Quoted 5 lines

Now, if everything was device-tree based then we could simply use a different binding for each CPU but since we support perf on non-DT platforms, probing the CPU type is the best solution. I would like to avoid the probing code if we are initialised from DT, but I've not got round to it yet (this would be useful for big.LITTLE).

I'll be interested to see how you encode the mapping of microarchitectural events to their identifiers here.

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
Would you be able to advertise 0 event counters instead and treat the PMU largely as RAZ/WI? The only tricky bit I can see is the cycle counter, which is mandated by the presence of a PMU, however this could also just RAZ initially.

Quoted 7 lines

Quoted 2 lines
Yes, that's the effect of the current emulation hack. Far better not to lie to the guest, however, than get weird results from profiling.
I'd rather lie to the guest than modify it just to avoid emulating the device correctly. Even then, the profiling results wouldn't be especially weird -- everything would be 0, which is a lot better than random numbers.

Sure, but nowhere as neat an experience as having "no hardware support available" printed, and nicely defined failure mode :(

When I get back in May, I'll look at how hard it will to be implement this properly, if noone else does.

Thanks,
Rusty.

Signature · 1 line

  How could I marry someone with more hair than me?  http://baldalex.org

Nicolas Pitre

2012-03-28 14:17 UTC #9 lore raw

PATCH 1/1 ARM: test for PMU feature on v7 (v2 with typo fix)

In reply to Will Deacon

Quoted 5 lines · Will Deacon

Now, if everything was device-tree based then we could simply use a different binding for each CPU but since we support perf on non-DT platforms, probing the CPU type is the best solution. I would like to avoid the probing code if we are initialised from DT, but I've not got round to it yet (this would be useful for big.LITTLE).

Still... my opinion is that we should try to autodetect as much as possible and avoid overstuffing the DT with content that can otherwise be run-time probed. OK to use DT to override the probe for corner cases, but IMHO the probe should be the default method of initialization. The rational is that we want to spread knowledge about part of the system and have it confined into respective drivers and subsystems for easier maintenance. If the guy who has to maintain the dts has to know all the details for everything then that won't scale and the risk for discrepancies is increased.

Nicolas

Will Deacon

2012-03-30 17:04 UTC #10 lore raw

PATCH 1/1 ARM: test for PMU feature on v7 (v2 with typo fix)

Hi Nicolas,

Quoted 8 lines · Nicolas Pitre

Quoted 5 lines · Will Deacon
Now, if everything was device-tree based then we could simply use a different binding for each CPU but since we support perf on non-DT platforms, probing the CPU type is the best solution. I would like to avoid the probing code if we are initialised from DT, but I've not got round to it yet (this would be useful for big.LITTLE).

Quoted 9 lines

Still... my opinion is that we should try to autodetect as much as possible and avoid overstuffing the DT with content that can otherwise be run-time probed. OK to use DT to override the probe for corner cases, but IMHO the probe should be the default method of initialization. The rational is that we want to spread knowledge about part of the system and have it confined into respective drivers and subsystems for easier maintenance. If the guy who has to maintain the dts has to know all the details for everything then that won't scale and the risk for discrepancies is increased.

I agree that probing is preferable where possible but, since the PMUs are banked, we cannot reliably probe them on big.LITTLE platforms. I guess if there were some infrastructure for probing on the remote cluster, I could use that, but it seems like DT would be easier.

Will

Nicolas Pitre

2012-03-30 17:40 UTC #11 lore raw

PATCH 1/1 ARM: test for PMU feature on v7 (v2 with typo fix)

Quoted 21 lines · Will Deacon

Hi Nicolas,
Quoted 8 lines · Nicolas Pitre
Quoted 5 lines · Will Deacon
Now, if everything was device-tree based then we could simply use a different binding for each CPU but since we support perf on non-DT platforms, probing the CPU type is the best solution. I would like to avoid the probing code if we are initialised from DT, but I've not got round to it yet (this would be useful for big.LITTLE).
Quoted 9 lines
Still... my opinion is that we should try to autodetect as much as possible and avoid overstuffing the DT with content that can otherwise be run-time probed. OK to use DT to override the probe for corner cases, but IMHO the probe should be the default method of initialization. The rational is that we want to spread knowledge about part of the system and have it confined into respective drivers and subsystems for easier maintenance. If the guy who has to maintain the dts has to know all the details for everything then that won't scale and the risk for discrepancies is increased.

Quoted 2 lines

I agree that probing is preferable where possible but, since the PMUs are banked, we cannot reliably probe them on big.LITTLE platforms.

Are they tied to each cluster power domains? If so you'll have to preserve/restore their state when clusters are suspended even if the interface tries to present a consistent view to users. The probe can be done there, and differences in capabilities adjusted dynamically.

Nicolas

Rusty Russell

2012-03-23 00:44 UTC #12 lore raw

PATCH 1/1 ARM: KVM: Emulate ID_DFR0 to say we don't support anything.

In reply to Rusty Russell

The guest should be checking this before trying to use performance monitors, for example.

Signed-off-by Rusty Russell <[email protected]>

arch/arm/kvm/emulate.c +12 -0

index e356d1c..c07eb2b 100644
@@ -238,6 +238,15 @@ static bool read_l2ctlr(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool read_id_dfr0(struct kvm_vcpu *vcpu,
+			 const struct coproc_params *p,
+			 unsigned long arg)
+{
+	/* We support no performance monitors (27-24 = b1111) and no debug */
+	*vcpu_reg(vcpu, p->Rt1) = 0x0F000000;
+	return true;
+}
+
 static bool access_cp15_reg(struct kvm_vcpu *vcpu,
 			    const struct coproc_params *p,
 			    unsigned long cp15_reg)

@@ -277,6 +286,9 @@ struct coproc_emulate {
 #define RW		.is_w  = DF
 
 static const struct coproc_emulate coproc_emulate[] = {
+	/* ID_DFR0 (guest asks what PMU/debug we support) */
+	{ CRn( 0), CRm( 1), Op1( 0), Op2( 2), is32,  READ,  read_id_dfr0},
+
 	/*
 	 * L2CTLR access:
 	 *

Peter Maydell

2012-03-23 09:04 UTC #13 lore raw

Re: [Android-virt] [PATCH] ARM: KVM: Emulate ID_DFR0 to say we don't support anything.

Quoted 4 lines · Rusty Russell

The guest should be checking this before trying to use performance monitors, for example.
Signed-off-by Rusty Russell <[email protected]>

Does this code get used currently? I thought we hadn't enabled trapping for feature register reads because of that use of one of the MMFR registers in a hot path for page allocation...

-- PMM

Rusty Russell

author

2012-03-23 23:28 UTC #14 lore raw

Re: [Android-virt] [PATCH] ARM: KVM: Emulate ID_DFR0 to say we don't support anything.

Quoted 9 lines · Peter Maydell

Quoted 4 lines · Rusty Russell
The guest should be checking this before trying to use performance monitors, for example.
Signed-off-by Rusty Russell <[email protected]>
Does this code get used currently? I thought we hadn't enabled trapping for feature register reads because of that use of one of the MMFR registers in a hot path for page allocation...

True (I mis-labelled the patches, they should have been RFC, since this is tied to the host-side probing patch).

So maybe not today, but we're going to end up doing this eventually. Everyone expects their virtual machines to work fine with the next model of ARM CPUs, which means we need to hide more and more host features over time.

Cheers,
Rusty.

Signature · 1 line

  How could I marry someone with more hair than me?  http://baldalex.org