On Fri, 13 Jan 2017 17:03:07 +0000
Will Deacon [off-list ref] wrote:

On Fri, Jan 13, 2017 at 10:40:42AM -0600, Kim Phillips wrote:

quoted

On Fri, 13 Jan 2017 16:03:48 +0000
Will Deacon [off-list ref] wrote:

quoted

+#define DRVNAME				"arm_spe_pmu"

PMU is implied.  "arm_spe"?

As stated before, I'm going for consistency here.

me too, but apparently under the user-visible interface domain rather
than the driver source path domain.

Is it causing any
real issues on the tooling side?

Intel has a consistent "intel_pt", "intel_bts", and 'pmu' occurs
nowhere in their nomenclature.

Whether good or bad, we currently have "cs_etm".  This patch now gives
us "arm_spe_pmu".  I'm just trying to save the suffix consistency for
now, esp. since IDK how amenable "cs_etm" is to change, and 'perf list'
calls things "PMU event"s anyway.

I think the root cause might be the device tree node's
"arm,arm-spe-pmu-v1" compatiblity string, which I also find
a bit self-redundant ("arm,arm-"), but I'm not familiar with what's
being denoted there either (e.g., if the latter 'arm-' is an arch
reference, then SPE's might be 'armv8'?).  The device tree node isn't
exposed to the user, however.

quoted

quoted

+	if (is_kernel_in_hyp_mode()) {
+		if (attr->exclude_kernel != attr->exclude_hv)
+			return -EOPNOTSUPP;
+	} else if (!attr->exclude_hv) {
+		return -EOPNOTSUPP;
+	}
+
+	reg = arm_spe_event_to_pmsfcr(event);
+	if ((reg & BIT(PMSFCR_EL1_FE_SHIFT)) &&
+	    !(spe_pmu->features & SPE_PMU_FEAT_FILT_EVT))
+		return -EOPNOTSUPP;
+
+	if ((reg & BIT(PMSFCR_EL1_FT_SHIFT)) &&
+	    !(spe_pmu->features & SPE_PMU_FEAT_FILT_TYP))
+		return -EOPNOTSUPP;
+
+	if ((reg & BIT(PMSFCR_EL1_FL_SHIFT)) &&
+	    !(spe_pmu->features & SPE_PMU_FEAT_FILT_LAT))
+		return -EOPNOTSUPP;
+
+	return 0;
+}

Please insert pr_* statements before blindly returning errors before a
better facility becomes available.

That was discussed in the thread I linked to last time:

https://lkml.org/lkml/2015/8/26/661

ok, thanks for pinpointing the exact message this time.

and there are good reasons not to add those prints.

Processing that message (indentations are now quoting Peter Zijlstra):

Not really. That is something that's limited to root. Whereas the
problem is very much wider than that.

For the purposes of the SPE driver discussion, I'm ok limiting the
context of using the SPE as root.

If you set one bit wrong in the pretty large perf_event_attr you've got
a fair chance of getting -EINVAL on trying to create the event. Good
luck finding what you did wrong.

yes, this is the problem, and the SPE introduces a whole new set of
validity requirements that should be being communicated clearly, e.g.,
its restrictive event frequency specification.

Any user can create events (for their own tasks), this does not require
root.

I don't think this is relevant to our discussion.

Allowing users to flip your @debugging flag would be an insta DoS.

I think this is a reference to the non-root case, and might be mitigated
by either using dynamic or ratelimited pr_ versions if it were.

Furthermore, its very unfriendly in that you have to (manually) go
correlate random dmesg output with some program action.

Andrew Morton addresses this, and I did read all other follow-ups and
still conclude that adding pr_ messages is 1000x better than not, for
the user, and at least for the time being.

Kim