1 files changed, 66 insertions, 83 deletions

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 36b29fd9295d..59012e750abe 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -27,20 +27,6 @@
 #include "hv-24x7-catalog.h"
 #include "hv-common.h"
 
-static const char *event_domain_suffix(unsigned domain)
-{
-	switch (domain) {
-#define DOMAIN(n, v, x, c)		\
-	case HV_PERF_DOMAIN_##n:	\
-		return "__" #n;
-#include "hv-24x7-domains.h"
-#undef DOMAIN
-	default:
-		WARN(1, "unknown domain %d\n", domain);
-		return "__UNKNOWN_DOMAIN_SUFFIX";
-	}
-}
-
 static bool domain_is_valid(unsigned domain)
 {
 	switch (domain) {
@@ -294,38 +280,70 @@ static unsigned long h_get_24x7_catalog_page(char page[],
 					version, index);
 }
 
-static unsigned core_domains[] = {
-	HV_PERF_DOMAIN_PHYS_CORE,
-	HV_PERF_DOMAIN_VCPU_HOME_CORE,
-	HV_PERF_DOMAIN_VCPU_HOME_CHIP,
-	HV_PERF_DOMAIN_VCPU_HOME_NODE,
-	HV_PERF_DOMAIN_VCPU_REMOTE_NODE,
-};
-/* chip event data always yeilds a single event, core yeilds multiple */
-#define MAX_EVENTS_PER_EVENT_DATA ARRAY_SIZE(core_domains)
-
+/*
+ * Each event we find in the catalog, will have a sysfs entry. Format the
+ * data for this sysfs entry based on the event's domain.
+ *
+ * Events belonging to the Chip domain can only be monitored in that domain.
+ * i.e the domain for these events is a fixed/knwon value.
+ *
+ * Events belonging to the Core domain can be monitored either in the physical
+ * core or in one of the virtual CPU domains. So the domain value for these
+ * events must be specified by the user (i.e is a required parameter). Format
+ * the Core events with 'domain=?' so the perf-tool can error check required
+ * parameters.
+ *
+ * NOTE: For the Core domain events, rather than making domain a required
+ *	 parameter we could default it to PHYS_CORE and allowe users to
+ *	 override the domain to one of the VCPU domains.
+ *
+ *	 However, this can make the interface a little inconsistent.
+ *
+ *	 If we set domain=2 (PHYS_CHIP) and allow user to override this field
+ *	 the user may be tempted to also modify the "offset=x" field in which
+ *	 can lead to confusing usage. Consider the HPM_PCYC (offset=0x18) and
+ *	 HPM_INST (offset=0x20) events. With:
+ *
+ *		perf stat -e hv_24x7/HPM_PCYC,offset=0x20/
+ *
+ *	we end up monitoring HPM_INST, while the command line has HPM_PCYC.
+ *
+ *	By not assigning a default value to the domain for the Core events,
+ *	we can have simple guidelines:
+ *
+ *		- Specifying values for parameters with "=?" is required.
+ *
+ *		- Specifying (i.e overriding) values for other parameters
+ *		  is undefined.
+ */
 static char *event_fmt(struct hv_24x7_event_data *event, unsigned domain)
 {
 	const char *sindex;
 	const char *lpar;
+	const char *domain_str;
+	char buf[8];
 
 	switch (domain) {
 	case HV_PERF_DOMAIN_PHYS_CHIP:
+		snprintf(buf, sizeof(buf), "%d", domain);
+		domain_str = buf;
 		lpar = "0x0";
 		sindex = "chip";
 		break;
 	case HV_PERF_DOMAIN_PHYS_CORE:
+		domain_str = "?";
 		lpar = "0x0";
 		sindex = "core";
 		break;
 	default:
+		domain_str = "?";
 		lpar = "?";
 		sindex = "vcpu";
 	}
 
 	return kasprintf(GFP_KERNEL,
-			"domain=0x%x,offset=0x%x,%s=?,lpar=%s",
-			domain,
+			"domain=%s,offset=0x%x,%s=?,lpar=%s",
+			domain_str,
 			be16_to_cpu(event->event_counter_offs) +
 				be16_to_cpu(event->event_group_record_offs),
 			sindex,
@@ -365,6 +383,15 @@ static struct attribute *device_str_attr_create_(char *name, char *str)
 	return &attr->attr.attr;
 }
 
+/*
+ * Allocate and initialize strings representing event attributes.
+ *
+ * NOTE: The strings allocated here are never destroyed and continue to
+ *	 exist till shutdown. This is to allow us to create as many events
+ *	 from the catalog as possible, even if we encounter errors with some.
+ *	 In case of changes to error paths in future, these may need to be
+ *	 freed by the caller.
+ */
 static struct attribute *device_str_attr_create(char *name, int name_max,
 						int name_nonce,
 						char *str, size_t str_max)
@@ -396,16 +423,6 @@ out_s:
 	return NULL;
 }
 
-static void device_str_attr_destroy(struct attribute *attr)
-{
-	struct dev_ext_attribute *d;
-
-	d = container_of(attr, struct dev_ext_attribute, attr.attr);
-	kfree(d->var);
-	kfree(d->attr.attr.name);
-	kfree(d);
-}
-
 static struct attribute *event_to_attr(unsigned ix,
 				       struct hv_24x7_event_data *event,
 				       unsigned domain,
@@ -413,7 +430,6 @@ static struct attribute *event_to_attr(unsigned ix,
 {
 	int event_name_len;
 	char *ev_name, *a_ev_name, *val;
-	const char *ev_suffix;
 	struct attribute *attr;
 
 	if (!domain_is_valid(domain)) {
@@ -426,14 +442,13 @@ static struct attribute *event_to_attr(unsigned ix,
 	if (!val)
 		return NULL;
 
-	ev_suffix = event_domain_suffix(domain);
 	ev_name = event_name(event, &event_name_len);
 	if (!nonce)
-		a_ev_name = kasprintf(GFP_KERNEL, "%.*s%s",
-				(int)event_name_len, ev_name, ev_suffix);
+		a_ev_name = kasprintf(GFP_KERNEL, "%.*s",
+				(int)event_name_len, ev_name);
 	else
-		a_ev_name = kasprintf(GFP_KERNEL, "%.*s%s__%d",
-				(int)event_name_len, ev_name, ev_suffix, nonce);
+		a_ev_name = kasprintf(GFP_KERNEL, "%.*s__%d",
+				(int)event_name_len, ev_name, nonce);
 
 	if (!a_ev_name)
 		goto out_val;
@@ -478,45 +493,14 @@ event_to_long_desc_attr(struct hv_24x7_event_data *event, int nonce)
 	return device_str_attr_create(name, nl, nonce, desc, dl);
 }
 
-static ssize_t event_data_to_attrs(unsigned ix, struct attribute **attrs,
+static int event_data_to_attrs(unsigned ix, struct attribute **attrs,
 				   struct hv_24x7_event_data *event, int nonce)
 {
-	unsigned i;
-
-	switch (event->domain) {
-	case HV_PERF_DOMAIN_PHYS_CHIP:
-		*attrs = event_to_attr(ix, event, event->domain, nonce);
-		return 1;
-	case HV_PERF_DOMAIN_PHYS_CORE:
-		for (i = 0; i < ARRAY_SIZE(core_domains); i++) {
-			attrs[i] = event_to_attr(ix, event, core_domains[i],
-						nonce);
-			if (!attrs[i]) {
-				pr_warn("catalog event %u: individual attr %u "
-					"creation failure\n", ix, i);
-				for (; i; i--)
-					device_str_attr_destroy(attrs[i - 1]);
-				return -1;
-			}
-		}
-		return i;
-	default:
-		pr_warn("catalog event %u: domain %u is not allowed in the "
-				"catalog\n", ix, event->domain);
+	*attrs = event_to_attr(ix, event, event->domain, nonce);
+	if (!*attrs)
 		return -1;
-	}
-}
 
-static size_t event_to_attr_ct(struct hv_24x7_event_data *event)
-{
-	switch (event->domain) {
-	case HV_PERF_DOMAIN_PHYS_CHIP:
-		return 1;
-	case HV_PERF_DOMAIN_PHYS_CORE:
-		return ARRAY_SIZE(core_domains);
-	default:
-		return 0;
-	}
+	return 0;
 }
 
 static unsigned long vmalloc_to_phys(void *v)
@@ -752,9 +736,8 @@ static int create_events_from_catalog(struct attribute ***events_,
 		goto e_free;
 	}
 
-	if (SIZE_MAX / MAX_EVENTS_PER_EVENT_DATA - 1 < event_entry_count) {
-		pr_err("event_entry_count %zu is invalid\n",
-				event_entry_count);
+	if (SIZE_MAX - 1 < event_entry_count) {
+		pr_err("event_entry_count %zu is invalid\n", event_entry_count);
 		ret = -EIO;
 		goto e_free;
 	}
@@ -827,7 +810,7 @@ static int create_events_from_catalog(struct attribute ***events_,
 			continue;
 		}
 
-		attr_max += event_to_attr_ct(event);
+		attr_max++;
 	}
 
 	event_idx_last = event_idx;
@@ -877,12 +860,12 @@ static int create_events_from_catalog(struct attribute ***events_,
 		nonce = event_uniq_add(&ev_uniq, name, nl, event->domain);
 		ct    = event_data_to_attrs(event_idx, events + event_attr_ct,
 					    event, nonce);
-		if (ct <= 0) {
+		if (ct < 0) {
 			pr_warn("event %zu (%.*s) creation failure, skipping\n",
 				event_idx, nl, name);
 			junk_events++;
 		} else {
-			event_attr_ct += ct;
+			event_attr_ct++;
 			event_descs[desc_ct] = event_to_desc_attr(event, nonce);
 			if (event_descs[desc_ct])
 				desc_ct++;