[PATCH v7 0/5] powerpc/hv-24x7: Expose chip/sockets info to add json file metric support for the hv_24x7 socket/chip level events

[PATCH v7 0/5] powerpc/hv-24x7: Expose chip/sockets info to add json file metric support for the hv_24x7 socket/chip level events

[Kajol Jain]

Patchset fixes the inconsistent results we are getting when
we run multiple 24x7 events.

"hv_24x7" pmu interface events needs system dependent parameter
like socket/chip/core. For example, hv_24x7 chip level events needs
specific chip-id to which the data is requested should be added as part
of pmu events.

So to enable JSON file support to "hv_24x7" interface, patchset expose
total number of sockets and chips per-socket details in sysfs
files (sockets, chips) under "/sys/devices/hv_24x7/interface/".

To get sockets and number of chips per sockets, patchset adds a rtas call
with token "PROCESSOR_MODULE_INFO" to get these details. Patchset also
handles partition migration case to re-init these system depended
parameters by adding proper calls in post_mobility_fixup() (mobility.c).

v6: http://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=164769
Changelog:
v6 -> v7
- Split patchset into two patch series, one with kernel changes
  and another with perf tool side changes. This pachset contain
  all kernel side changes.

Kajol Jain (5):
  powerpc/perf/hv-24x7: Fix inconsistent output values incase multiple
    hv-24x7 events run
  powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor
    details
  powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show
    processor details
  Documentation/ABI: Add ABI documentation for chips and sockets
  powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

 .../sysfs-bus-event_source-devices-hv_24x7    |  14 +++
 arch/powerpc/perf/hv-24x7.c                   | 104 ++++++++++++++++--
 arch/powerpc/platforms/pseries/mobility.c     |  12 ++
 arch/powerpc/platforms/pseries/pseries.h      |   3 +
 4 files changed, 123 insertions(+), 10 deletions(-)

-- 
2.18.1

[PATCH v7 1/5] powerpc/perf/hv-24x7: Fix inconsistent output values incase multiple hv-24x7 events run

[Kajol Jain]

Commit 2b206ee6b0df ("powerpc/perf/hv-24x7: Display change in counter
values")' added to print _change_ in the counter value rather then raw
value for 24x7 counters. Incase of transactions, the event count
is set to 0 at the beginning of the transaction. It also sets
the event's prev_count to the raw value at the time of initialization.
Because of setting event count to 0, we are seeing some weird behaviour,
whenever we run multiple 24x7 events at a time.

For example:

command#: ./perf stat -e "{hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/,
			   hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/}"
	  		   -C 0 -I 1000 sleep 100

     1.000121704                120 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     1.000121704                  5 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     2.000357733                  8 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     2.000357733                 10 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     3.000495215 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     3.000495215 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     4.000641884                 56 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     4.000641884 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     5.000791887 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/

Getting these large values in case we do -I.

As we are setting event_count to 0, for interval case, overall event_count is not
coming in incremental order. As we may can get new delta lesser then previous count.
Because of which when we print intervals, we are getting negative value which create
these large values.

This patch removes part where we set event_count to 0 in function
'h_24x7_event_read'. There won't be much impact as we do set event->hw.prev_count
to the raw value at the time of initialization to print change value.

With this patch
In power9 platform

command#: ./perf stat -e "{hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/,
		           hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/}"
			   -C 0 -I 1000 sleep 100

     1.000117685                 93 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     1.000117685                  1 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     2.000349331                 98 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     2.000349331                  2 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     3.000495900                131 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     3.000495900                  4 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     4.000645920                204 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
     4.000645920                 61 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
     4.284169997                 22 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Suggested-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 573e0b309c0c..48e8f4b17b91 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1400,16 +1400,6 @@ static void h_24x7_event_read(struct perf_event *event)
 			h24x7hw = &get_cpu_var(hv_24x7_hw);
 			h24x7hw->events[i] = event;
 			put_cpu_var(h24x7hw);
-			/*
-			 * Clear the event count so we can compute the _change_
-			 * in the 24x7 raw counter value at the end of the txn.
-			 *
-			 * Note that we could alternatively read the 24x7 value
-			 * now and save its value in event->hw.prev_count. But
-			 * that would require issuing a hcall, which would then
-			 * defeat the purpose of using the txn interface.
-			 */
-			local64_set(&event->count, 0);
 		}
 
 		put_cpu_var(hv_24x7_reqb);
-- 
2.18.1

[PATCH v7 2/5] powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor details

[Kajol Jain]

For hv_24x7 socket/chip level events, specific chip-id to which
the data requested should be added as part of pmu events.
But number of chips/socket in the system details are not exposed.

Patch implements read_sys_info_pseries() to get system
parameter values like number of sockets and chips per socket.
Rtas_call with token "PROCESSOR_MODULE_INFO"
is used to get these values.

Sub-sequent patch exports these values via sysfs.

Patch also make these parameters default to 1.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c              | 72 ++++++++++++++++++++++++
 arch/powerpc/platforms/pseries/pseries.h |  3 +
 2 files changed, 75 insertions(+)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 48e8f4b17b91..9ae00f29bd21 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -20,6 +20,11 @@
 #include <asm/io.h>
 #include <linux/byteorder/generic.h>
 
+#ifdef CONFIG_PPC_RTAS
+#include <asm/rtas.h>
+#include <../../platforms/pseries/pseries.h>
+#endif
+
 #include "hv-24x7.h"
 #include "hv-24x7-catalog.h"
 #include "hv-common.h"
@@ -57,6 +62,69 @@ static bool is_physical_domain(unsigned domain)
 	}
 }
 
+#ifdef CONFIG_PPC_RTAS
+#define PROCESSOR_MODULE_INFO   43
+#define PROCESSOR_MAX_LENGTH	(8 * 1024)
+
+static int strbe16toh(const char *buf, int offset)
+{
+	return (buf[offset] << 8) + buf[offset + 1];
+}
+
+static u32		physsockets;	/* Physical sockets */
+static u32		physchips;	/* Physical chips */
+
+/*
+ * Function read_sys_info_pseries() make a rtas_call which require
+ * data buffer of size 8K. As standard 'rtas_data_buf' is of size
+ * 4K, we are adding new local buffer 'rtas_local_data_buf'.
+ */
+char rtas_local_data_buf[PROCESSOR_MAX_LENGTH] __cacheline_aligned;
+
+/*
+ * read_sys_info_pseries()
+ * Retrieve the number of sockets and chips per socket details
+ * through the get-system-parameter rtas call.
+ */
+void read_sys_info_pseries(void)
+{
+	int call_status, len, ntypes;
+
+	/*
+	 * Making system parameter: chips and sockets default to 1.
+	 */
+	physsockets = 1;
+	physchips = 1;
+	memset(rtas_local_data_buf, 0, PROCESSOR_MAX_LENGTH);
+	spin_lock(&rtas_data_buf_lock);
+
+	call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
+				NULL,
+				PROCESSOR_MODULE_INFO,
+				__pa(rtas_local_data_buf),
+				PROCESSOR_MAX_LENGTH);
+
+	spin_unlock(&rtas_data_buf_lock);
+
+	if (call_status != 0) {
+		pr_info("%s %s Error calling get-system-parameter (0x%x)\n",
+			__FILE__, __func__, call_status);
+	} else {
+		rtas_local_data_buf[PROCESSOR_MAX_LENGTH - 1] = '\0';
+		len = strbe16toh(rtas_local_data_buf, 0);
+		if (len < 6)
+			return;
+
+		ntypes = strbe16toh(rtas_local_data_buf, 2);
+
+		if (!ntypes)
+			return;
+		physsockets = strbe16toh(rtas_local_data_buf, 4);
+		physchips = strbe16toh(rtas_local_data_buf, 6);
+	}
+}
+#endif /* CONFIG_PPC_RTAS */
+
 /* Domains for which more than one result element are returned for each event. */
 static bool domain_needs_aggregation(unsigned int domain)
 {
@@ -1605,6 +1673,10 @@ static int hv_24x7_init(void)
 	if (r)
 		return r;
 
+#ifdef CONFIG_PPC_RTAS
+	read_sys_info_pseries();
+#endif
+
 	return 0;
 }
 
diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
index 13fa370a87e4..1727559ce304 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h
@@ -19,6 +19,9 @@ extern void request_event_sources_irqs(struct device_node *np,
 struct pt_regs;
 
 extern int pSeries_system_reset_exception(struct pt_regs *regs);
+#ifdef CONFIG_PPC_RTAS
+extern void read_sys_info_pseries(void);
+#endif
 extern int pSeries_machine_check_exception(struct pt_regs *regs);
 extern long pseries_machine_check_realmode(struct pt_regs *regs);
 
-- 
2.18.1

[PATCH v7 3/5] powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show processor details

[Kajol Jain]

To expose the system dependent parameter like total number of
sockets and numbers of chips per socket, patch adds two sysfs files.
"sockets" and "chips" are added to /sys/devices/hv_24x7/interface/
of the "hv_24x7" pmu.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 9ae00f29bd21..a31bd5b88f7a 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -454,6 +454,20 @@ static ssize_t device_show_string(struct device *dev,
 	return sprintf(buf, "%s\n", (char *)d->var);
 }
 
+#ifdef CONFIG_PPC_RTAS
+static ssize_t sockets_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", physsockets);
+}
+
+static ssize_t chips_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	return sprintf(buf, "%d\n", physchips);
+}
+#endif
+
 static struct attribute *device_str_attr_create_(char *name, char *str)
 {
 	struct dev_ext_attribute *attr = kzalloc(sizeof(*attr), GFP_KERNEL);
@@ -1100,6 +1114,10 @@ PAGE_0_ATTR(catalog_len, "%lld\n",
 		(unsigned long long)be32_to_cpu(page_0->length) * 4096);
 static BIN_ATTR_RO(catalog, 0/* real length varies */);
 static DEVICE_ATTR_RO(domains);
+#ifdef CONFIG_PPC_RTAS
+static DEVICE_ATTR_RO(sockets);
+static DEVICE_ATTR_RO(chips);
+#endif
 
 static struct bin_attribute *if_bin_attrs[] = {
 	&bin_attr_catalog,
@@ -1110,6 +1128,10 @@ static struct attribute *if_attrs[] = {
 	&dev_attr_catalog_len.attr,
 	&dev_attr_catalog_version.attr,
 	&dev_attr_domains.attr,
+#ifdef CONFIG_PPC_RTAS
+	&dev_attr_sockets.attr,
+	&dev_attr_chips.attr,
+#endif
 	NULL,
 };
 
-- 
2.18.1

[PATCH v7 4/5] Documentation/ABI: Add ABI documentation for chips and sockets

[Kajol Jain]

Add documentation for the following sysfs files:
/sys/devices/hv_24x7/interface/chips,
/sys/devices/hv_24x7/interface/sockets

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 .../testing/sysfs-bus-event_source-devices-hv_24x7 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7 b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
index ec27c6c9e737..e17e5b444a1c 100644
--- a/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
+++ b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
@@ -22,6 +22,20 @@ Description:
 		Exposes the "version" field of the 24x7 catalog. This is also
 		extractable from the provided binary "catalog" sysfs entry.
 
+What:		/sys/devices/hv_24x7/interface/sockets
+Date:		March 2020
+Contact:	Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
+Description:	read only
+		This sysfs interface exposes the number of sockets present in the
+		system.
+
+What:		/sys/devices/hv_24x7/interface/chips
+Date:		March 2020
+Contact:	Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
+Description:	read only
+		This sysfs interface exposes the number of chips per socket
+		present in the system.
+
 What:		/sys/bus/event_source/devices/hv_24x7/event_descs/<event-name>
 Date:		February 2014
 Contact:	Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
-- 
2.18.1

[PATCH v7 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[Kajol Jain]

Function 'read_sys_info_pseries()' is added to get system parameter
values like number of sockets and chips per socket.
and it gets these details via rtas_call with token
"PROCESSOR_MODULE_INFO".

Incase lpar migrate from one system to another, system
parameter details like chips per sockets or number of sockets might
change. So, it needs to be re-initialized otherwise, these values
corresponds to previous system values.
This patch adds a call to 'read_sys_info_pseries()' from
'post-mobility_fixup()' to re-init the physsockets and physchips values.

Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/mobility.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index b571285f6c14..226accd6218b 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -371,6 +371,18 @@ void post_mobility_fixup(void)
 	/* Possibly switch to a new RFI flush type */
 	pseries_setup_rfi_flush();
 
+	/*
+	 * Incase lpar migrate from one system to another, system
+	 * parameter details like chips per sockets and number of sockets
+	 * might change. So, it needs to be re-initialized otherwise these
+	 * values corresponds to previous system.
+	 * Here, adding a call to read_sys_info_pseries() declared in
+	 * platforms/pseries/pseries.h to re-init the physsockets and
+	 * physchips value.
+	 */
+	if (IS_ENABLED(CONFIG_HV_PERF_CTRS) && IS_ENABLED(CONFIG_PPC_RTAS))
+		read_sys_info_pseries();
+
 	return;
 }
 
-- 
2.18.1

Re: [PATCH v7 2/5] powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor details

[Madhavan Srinivasan]

On 3/27/20 12:06 PM, Kajol Jain wrote:
> For hv_24x7 socket/chip level events, specific chip-id to which
> the data requested should be added as part of pmu events.
> But number of chips/socket in the system details are not exposed.
>
> Patch implements read_sys_info_pseries() to get system
> parameter values like number of sockets and chips per socket.
> Rtas_call with token "PROCESSOR_MODULE_INFO"
> is used to get these values.

Patch looks good to me.

Reviewed-by: Madhavan Srinivasan <maddy@linux.ibm.com>

> Sub-sequent patch exports these values via sysfs.
>
> Patch also make these parameters default to 1.
>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>   arch/powerpc/perf/hv-24x7.c              | 72 ++++++++++++++++++++++++
>   arch/powerpc/platforms/pseries/pseries.h |  3 +
>   2 files changed, 75 insertions(+)
>
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index 48e8f4b17b91..9ae00f29bd21 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -20,6 +20,11 @@
>   #include <asm/io.h>
>   #include <linux/byteorder/generic.h>
>
> +#ifdef CONFIG_PPC_RTAS
> +#include <asm/rtas.h>
> +#include <../../platforms/pseries/pseries.h>
> +#endif
> +
>   #include "hv-24x7.h"
>   #include "hv-24x7-catalog.h"
>   #include "hv-common.h"
> @@ -57,6 +62,69 @@ static bool is_physical_domain(unsigned domain)
>   	}
>   }
>
> +#ifdef CONFIG_PPC_RTAS
> +#define PROCESSOR_MODULE_INFO   43
> +#define PROCESSOR_MAX_LENGTH	(8 * 1024)
> +
> +static int strbe16toh(const char *buf, int offset)
> +{
> +	return (buf[offset] << 8) + buf[offset + 1];
> +}
> +
> +static u32		physsockets;	/* Physical sockets */
> +static u32		physchips;	/* Physical chips */
> +
> +/*
> + * Function read_sys_info_pseries() make a rtas_call which require
> + * data buffer of size 8K. As standard 'rtas_data_buf' is of size
> + * 4K, we are adding new local buffer 'rtas_local_data_buf'.
> + */
> +char rtas_local_data_buf[PROCESSOR_MAX_LENGTH] __cacheline_aligned;
> +
> +/*
> + * read_sys_info_pseries()
> + * Retrieve the number of sockets and chips per socket details
> + * through the get-system-parameter rtas call.
> + */
> +void read_sys_info_pseries(void)
> +{
> +	int call_status, len, ntypes;
> +
> +	/*
> +	 * Making system parameter: chips and sockets default to 1.
> +	 */
> +	physsockets = 1;
> +	physchips = 1;
> +	memset(rtas_local_data_buf, 0, PROCESSOR_MAX_LENGTH);
> +	spin_lock(&rtas_data_buf_lock);
> +
> +	call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
> +				NULL,
> +				PROCESSOR_MODULE_INFO,
> +				__pa(rtas_local_data_buf),
> +				PROCESSOR_MAX_LENGTH);
> +
> +	spin_unlock(&rtas_data_buf_lock);
> +
> +	if (call_status != 0) {
> +		pr_info("%s %s Error calling get-system-parameter (0x%x)\n",
> +			__FILE__, __func__, call_status);
> +	} else {
> +		rtas_local_data_buf[PROCESSOR_MAX_LENGTH - 1] = '\0';
> +		len = strbe16toh(rtas_local_data_buf, 0);
> +		if (len < 6)
> +			return;
> +
> +		ntypes = strbe16toh(rtas_local_data_buf, 2);
> +
> +		if (!ntypes)
> +			return;
> +		physsockets = strbe16toh(rtas_local_data_buf, 4);
> +		physchips = strbe16toh(rtas_local_data_buf, 6);
> +	}
> +}
> +#endif /* CONFIG_PPC_RTAS */
> +
>   /* Domains for which more than one result element are returned for each event. */
>   static bool domain_needs_aggregation(unsigned int domain)
>   {
> @@ -1605,6 +1673,10 @@ static int hv_24x7_init(void)
>   	if (r)
>   		return r;
>
> +#ifdef CONFIG_PPC_RTAS
> +	read_sys_info_pseries();
> +#endif
> +
>   	return 0;
>   }
>
> diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
> index 13fa370a87e4..1727559ce304 100644
> --- a/arch/powerpc/platforms/pseries/pseries.h
> +++ b/arch/powerpc/platforms/pseries/pseries.h
> @@ -19,6 +19,9 @@ extern void request_event_sources_irqs(struct device_node *np,
>   struct pt_regs;
>
>   extern int pSeries_system_reset_exception(struct pt_regs *regs);
> +#ifdef CONFIG_PPC_RTAS
> +extern void read_sys_info_pseries(void);
> +#endif
>   extern int pSeries_machine_check_exception(struct pt_regs *regs);
>   extern long pseries_machine_check_realmode(struct pt_regs *regs);
>

Re: [PATCH v7 1/5] powerpc/perf/hv-24x7: Fix inconsistent output values incase multiple hv-24x7 events run

[Madhavan Srinivasan]

On 3/27/20 12:06 PM, Kajol Jain wrote:
> Commit 2b206ee6b0df ("powerpc/perf/hv-24x7: Display change in counter
> values")' added to print _change_ in the counter value rather then raw
> value for 24x7 counters. Incase of transactions, the event count
> is set to 0 at the beginning of the transaction. It also sets
> the event's prev_count to the raw value at the time of initialization.
> Because of setting event count to 0, we are seeing some weird behaviour,
> whenever we run multiple 24x7 events at a time.
>
> For example:
>
> command#: ./perf stat -e "{hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/,
> 			   hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/}"
> 	  		   -C 0 -I 1000 sleep 100
>
>       1.000121704                120 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
>       1.000121704                  5 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
>       2.000357733                  8 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
>       2.000357733                 10 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
>       3.000495215 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
>       3.000495215 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
>       4.000641884                 56 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
>       4.000641884 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
>       5.000791887 18,446,744,073,709,551,616 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
>
> Getting these large values in case we do -I.
>
> As we are setting event_count to 0, for interval case, overall event_count is not
> coming in incremental order. As we may can get new delta lesser then previous count.
> Because of which when we print intervals, we are getting negative value which create
> these large values.
>
> This patch removes part where we set event_count to 0 in function
> 'h_24x7_event_read'. There won't be much impact as we do set event->hw.prev_count
> to the raw value at the time of initialization to print change value.
>
> With this patch
> In power9 platform
>
> command#: ./perf stat -e "{hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/,
> 		           hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/}"
> 			   -C 0 -I 1000 sleep 100
>
>       1.000117685                 93 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
>       1.000117685                  1 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
>       2.000349331                 98 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
>       2.000349331                  2 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
>       3.000495900                131 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
>       3.000495900                  4 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
>       4.000645920                204 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
>       4.000645920                 61 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=1/
>       4.284169997                 22 hv_24x7/PM_MCS01_128B_RD_DISP_PORT01,chip=0/
>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> Suggested-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>

Tested-by: Madhavan Srinivasan <maddy@linux.ibm.com>

> ---
>   arch/powerpc/perf/hv-24x7.c | 10 ----------
>   1 file changed, 10 deletions(-)
>
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index 573e0b309c0c..48e8f4b17b91 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -1400,16 +1400,6 @@ static void h_24x7_event_read(struct perf_event *event)
>   			h24x7hw = &get_cpu_var(hv_24x7_hw);
>   			h24x7hw->events[i] = event;
>   			put_cpu_var(h24x7hw);
> -			/*
> -			 * Clear the event count so we can compute the _change_
> -			 * in the 24x7 raw counter value at the end of the txn.
> -			 *
> -			 * Note that we could alternatively read the 24x7 value
> -			 * now and save its value in event->hw.prev_count. But
> -			 * that would require issuing a hcall, which would then
> -			 * defeat the purpose of using the txn interface.
> -			 */
> -			local64_set(&event->count, 0);
>   		}
>
>   		put_cpu_var(hv_24x7_reqb);

Re: [PATCH v7 3/5] powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show processor details

[Madhavan Srinivasan]

On 3/27/20 12:06 PM, Kajol Jain wrote:
> To expose the system dependent parameter like total number of
> sockets and numbers of chips per socket, patch adds two sysfs files.
> "sockets" and "chips" are added to /sys/devices/hv_24x7/interface/
> of the "hv_24x7" pmu.
>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>   arch/powerpc/perf/hv-24x7.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index 9ae00f29bd21..a31bd5b88f7a 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -454,6 +454,20 @@ static ssize_t device_show_string(struct device *dev,
>   	return sprintf(buf, "%s\n", (char *)d->var);
>   }
>
> +#ifdef CONFIG_PPC_RTAS
> +static ssize_t sockets_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", physsockets);
> +}
> +
> +static ssize_t chips_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	return sprintf(buf, "%d\n", physchips);
> +}
> +#endif
> +

rtas call gives you the cores per chip too. We can expose that
also with this patch?  I understand tool side patchset is using
only metrics added to socket/chip information, its better to
include that cores also here..

>   static struct attribute *device_str_attr_create_(char *name, char *str)
>   {
>   	struct dev_ext_attribute *attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> @@ -1100,6 +1114,10 @@ PAGE_0_ATTR(catalog_len, "%lld\n",
>   		(unsigned long long)be32_to_cpu(page_0->length) * 4096);
>   static BIN_ATTR_RO(catalog, 0/* real length varies */);
>   static DEVICE_ATTR_RO(domains);
> +#ifdef CONFIG_PPC_RTAS
> +static DEVICE_ATTR_RO(sockets);
> +static DEVICE_ATTR_RO(chips);
> +#endif
>
>   static struct bin_attribute *if_bin_attrs[] = {
>   	&bin_attr_catalog,
> @@ -1110,6 +1128,10 @@ static struct attribute *if_attrs[] = {
>   	&dev_attr_catalog_len.attr,
>   	&dev_attr_catalog_version.attr,
>   	&dev_attr_domains.attr,
> +#ifdef CONFIG_PPC_RTAS
> +	&dev_attr_sockets.attr,
> +	&dev_attr_chips.attr,
> +#endif
>   	NULL,
>   };
>

Re: [PATCH v7 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[Madhavan Srinivasan]

On 3/27/20 12:06 PM, Kajol Jain wrote:
> Function 'read_sys_info_pseries()' is added to get system parameter
> values like number of sockets and chips per socket.
> and it gets these details via rtas_call with token
> "PROCESSOR_MODULE_INFO".
>
> Incase lpar migrate from one system to another, system
> parameter details like chips per sockets or number of sockets might
> change. So, it needs to be re-initialized otherwise, these values
> corresponds to previous system values.
> This patch adds a call to 'read_sys_info_pseries()' from
> 'post-mobility_fixup()' to re-init the physsockets and physchips values.

Changes looks fine to me.
Reviewed-by: Madhavan Srinivasan <maddy@linux.ibm.com>

> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>   arch/powerpc/platforms/pseries/mobility.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> index b571285f6c14..226accd6218b 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -371,6 +371,18 @@ void post_mobility_fixup(void)
>   	/* Possibly switch to a new RFI flush type */
>   	pseries_setup_rfi_flush();
>
> +	/*
> +	 * Incase lpar migrate from one system to another, system
> +	 * parameter details like chips per sockets and number of sockets
> +	 * might change. So, it needs to be re-initialized otherwise these
> +	 * values corresponds to previous system.
> +	 * Here, adding a call to read_sys_info_pseries() declared in
> +	 * platforms/pseries/pseries.h to re-init the physsockets and
> +	 * physchips value.
> +	 */
> +	if (IS_ENABLED(CONFIG_HV_PERF_CTRS) && IS_ENABLED(CONFIG_PPC_RTAS))
> +		read_sys_info_pseries();
> +
>   	return;
>   }
>

Re: [PATCH v7 2/5] powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor details

[Michael Ellerman]

Hi Kajol,

Some comments inline ...

Kajol Jain <kjain@linux.ibm.com> writes:
> For hv_24x7 socket/chip level events, specific chip-id to which
> the data requested should be added as part of pmu events.
> But number of chips/socket in the system details are not exposed.
>
> Patch implements read_sys_info_pseries() to get system
> parameter values like number of sockets and chips per socket.
> Rtas_call with token "PROCESSOR_MODULE_INFO"
> is used to get these values.
>
> Sub-sequent patch exports these values via sysfs.
>
> Patch also make these parameters default to 1.
>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>  arch/powerpc/perf/hv-24x7.c              | 72 ++++++++++++++++++++++++
>  arch/powerpc/platforms/pseries/pseries.h |  3 +
>  2 files changed, 75 insertions(+)
>
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index 48e8f4b17b91..9ae00f29bd21 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -20,6 +20,11 @@
>  #include <asm/io.h>
>  #include <linux/byteorder/generic.h>
>  
> +#ifdef CONFIG_PPC_RTAS

This driver can only be build on pseries, and pseries always selects
RTAS. So the ifdef is unncessary.

> +#include <asm/rtas.h>
> +#include <../../platforms/pseries/pseries.h>
> +#endif

That's not really what the platform header is intended for.

You should put the extern in arch/powerpc/include/asm somewhere.

Maybe rtas.h

> @@ -57,6 +62,69 @@ static bool is_physical_domain(unsigned domain)
>  	}
>  }
>  
> +#ifdef CONFIG_PPC_RTAS

Not needed.

> +#define PROCESSOR_MODULE_INFO   43

Please document where these come from, presumably LoPAPR somewhere?

> +#define PROCESSOR_MAX_LENGTH	(8 * 1024)
> +
> +static int strbe16toh(const char *buf, int offset)
> +{
> +	return (buf[offset] << 8) + buf[offset + 1];
> +}

I'm confused by this. "str" implies string, a string is an array of
bytes and has no endian. But then be16 implies it's an array of __be16,
in which case buf should be a __be16 *.

> +
> +static u32		physsockets;	/* Physical sockets */
> +static u32		physchips;	/* Physical chips */

No tabs there please.

> +
> +/*
> + * Function read_sys_info_pseries() make a rtas_call which require
> + * data buffer of size 8K. As standard 'rtas_data_buf' is of size
> + * 4K, we are adding new local buffer 'rtas_local_data_buf'.
> + */
> +char rtas_local_data_buf[PROCESSOR_MAX_LENGTH] __cacheline_aligned;

static?

> +/*
> + * read_sys_info_pseries()
> + * Retrieve the number of sockets and chips per socket details
> + * through the get-system-parameter rtas call.
> + */
> +void read_sys_info_pseries(void)
> +{
> +	int call_status, len, ntypes;
> +
> +	/*
> +	 * Making system parameter: chips and sockets default to 1.
> +	 */
> +	physsockets = 1;
> +	physchips = 1;
> +	memset(rtas_local_data_buf, 0, PROCESSOR_MAX_LENGTH);
> +	spin_lock(&rtas_data_buf_lock);

You're not using the rtas_data_buf, so why are you taking the
rtas_data_buf_lock?

> +	call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
> +				NULL,
> +				PROCESSOR_MODULE_INFO,
> +				__pa(rtas_local_data_buf),
> +				PROCESSOR_MAX_LENGTH);
> +
> +	spin_unlock(&rtas_data_buf_lock);
> +
> +	if (call_status != 0) {
> +		pr_info("%s %s Error calling get-system-parameter (0x%x)\n",
> +			__FILE__, __func__, call_status);

pr_err(), don't use __FILE__, this file already uses pr_fmt(). Not sure
__func__ is really necessary either.

		return;

Then you can deindent the next block.

> +	} else {
> +		rtas_local_data_buf[PROCESSOR_MAX_LENGTH - 1] = '\0';
> +		len = strbe16toh(rtas_local_data_buf, 0);

Why isn't the buffer a __be16 array, and then you just use be16_to_cpu() ?

> +		if (len < 6)
> +			return;
> +
> +		ntypes = strbe16toh(rtas_local_data_buf, 2);
> +
> +		if (!ntypes)
> +			return;

What is ntypes?

> +		physsockets = strbe16toh(rtas_local_data_buf, 4);
> +		physchips = strbe16toh(rtas_local_data_buf, 6);
> +	}
> +}
> +#endif /* CONFIG_PPC_RTAS */
> +
>  /* Domains for which more than one result element are returned for each event. */
>  static bool domain_needs_aggregation(unsigned int domain)
>  {
> @@ -1605,6 +1673,10 @@ static int hv_24x7_init(void)
>  	if (r)
>  		return r;
>  
> +#ifdef CONFIG_PPC_RTAS
> +	read_sys_info_pseries();
> +#endif

> +
>  	return 0;
>  }
>  
> diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
> index 13fa370a87e4..1727559ce304 100644
> --- a/arch/powerpc/platforms/pseries/pseries.h
> +++ b/arch/powerpc/platforms/pseries/pseries.h
> @@ -19,6 +19,9 @@ extern void request_event_sources_irqs(struct device_node *np,
>  struct pt_regs;
>  
>  extern int pSeries_system_reset_exception(struct pt_regs *regs);
> +#ifdef CONFIG_PPC_RTAS
> +extern void read_sys_info_pseries(void);
> +#endif
>  extern int pSeries_machine_check_exception(struct pt_regs *regs);
>  extern long pseries_machine_check_realmode(struct pt_regs *regs);


cheers

Re: [PATCH v7 3/5] powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show processor details

[Michael Ellerman]

Kajol Jain <kjain@linux.ibm.com> writes:
> To expose the system dependent parameter like total number of
> sockets and numbers of chips per socket, patch adds two sysfs files.
> "sockets" and "chips" are added to /sys/devices/hv_24x7/interface/
> of the "hv_24x7" pmu.
>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>  arch/powerpc/perf/hv-24x7.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

This should also add documentation under Documentation/ABI.

cheers

> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index 9ae00f29bd21..a31bd5b88f7a 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -454,6 +454,20 @@ static ssize_t device_show_string(struct device *dev,
>  	return sprintf(buf, "%s\n", (char *)d->var);
>  }
>  
> +#ifdef CONFIG_PPC_RTAS
> +static ssize_t sockets_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", physsockets);
> +}
> +
> +static ssize_t chips_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	return sprintf(buf, "%d\n", physchips);
> +}
> +#endif
> +
>  static struct attribute *device_str_attr_create_(char *name, char *str)
>  {
>  	struct dev_ext_attribute *attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> @@ -1100,6 +1114,10 @@ PAGE_0_ATTR(catalog_len, "%lld\n",
>  		(unsigned long long)be32_to_cpu(page_0->length) * 4096);
>  static BIN_ATTR_RO(catalog, 0/* real length varies */);
>  static DEVICE_ATTR_RO(domains);
> +#ifdef CONFIG_PPC_RTAS
> +static DEVICE_ATTR_RO(sockets);
> +static DEVICE_ATTR_RO(chips);
> +#endif
>  
>  static struct bin_attribute *if_bin_attrs[] = {
>  	&bin_attr_catalog,
> @@ -1110,6 +1128,10 @@ static struct attribute *if_attrs[] = {
>  	&dev_attr_catalog_len.attr,
>  	&dev_attr_catalog_version.attr,
>  	&dev_attr_domains.attr,
> +#ifdef CONFIG_PPC_RTAS
> +	&dev_attr_sockets.attr,
> +	&dev_attr_chips.attr,
> +#endif
>  	NULL,
>  };
>  
> -- 
> 2.18.1

Re: [PATCH v7 3/5] powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show processor details

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Kajol Jain <kjain@linux.ibm.com> writes:
>> To expose the system dependent parameter like total number of
>> sockets and numbers of chips per socket, patch adds two sysfs files.
>> "sockets" and "chips" are added to /sys/devices/hv_24x7/interface/
>> of the "hv_24x7" pmu.
>>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>>  arch/powerpc/perf/hv-24x7.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>
> This should also add documentation under Documentation/ABI.

Ugh, sorry, you do that in the next patch :}

cheers

Re: [PATCH v7 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[Michael Ellerman]

Kajol Jain <kjain@linux.ibm.com> writes:
> Function 'read_sys_info_pseries()' is added to get system parameter
> values like number of sockets and chips per socket.
> and it gets these details via rtas_call with token
> "PROCESSOR_MODULE_INFO".
>
> Incase lpar migrate from one system to another, system
> parameter details like chips per sockets or number of sockets might
> change. So, it needs to be re-initialized otherwise, these values
> corresponds to previous system values.
> This patch adds a call to 'read_sys_info_pseries()' from
> 'post-mobility_fixup()' to re-init the physsockets and physchips values.
>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/mobility.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> index b571285f6c14..226accd6218b 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -371,6 +371,18 @@ void post_mobility_fixup(void)
>  	/* Possibly switch to a new RFI flush type */
>  	pseries_setup_rfi_flush();
>  
> +	/*
> +	 * Incase lpar migrate from one system to another, system

In case an LPAR migrates

> +	 * parameter details like chips per sockets and number of sockets
> +	 * might change. So, it needs to be re-initialized otherwise these
                             ^                                       ^
                             they need                               the
> +	 * values corresponds to previous system.
                  ^
                  will correspond to the

> +	 * Here, adding a call to read_sys_info_pseries() declared in

Adding is the wrong tense in a comment. When someone reads the comment
the code has already been added. Past tense would be right, but really
the comment shouldn't say what you did, it should say why.

> +	 * platforms/pseries/pseries.h to re-init the physsockets and
> +	 * physchips value.

Call read_sys_info_pseries() to reinitialise the values.

> +	 */
> +	if (IS_ENABLED(CONFIG_HV_PERF_CTRS) && IS_ENABLED(CONFIG_PPC_RTAS))
> +		read_sys_info_pseries();

The RTAS check is not needed. pseries always selects RTAS.

You shouldn't need the IS_ENABLED() check here though, do it with an
empty version in the header when CONFIG_HV_PERF_CTRS is not enabled.

cheers

Re: [PATCH v7 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[kajoljain]

On 4/29/20 5:07 PM, Michael Ellerman wrote:
> Kajol Jain <kjain@linux.ibm.com> writes:
>> Function 'read_sys_info_pseries()' is added to get system parameter
>> values like number of sockets and chips per socket.
>> and it gets these details via rtas_call with token
>> "PROCESSOR_MODULE_INFO".
>>
>> Incase lpar migrate from one system to another, system
>> parameter details like chips per sockets or number of sockets might
>> change. So, it needs to be re-initialized otherwise, these values
>> corresponds to previous system values.
>> This patch adds a call to 'read_sys_info_pseries()' from
>> 'post-mobility_fixup()' to re-init the physsockets and physchips values.
>>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/mobility.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
>> index b571285f6c14..226accd6218b 100644
>> --- a/arch/powerpc/platforms/pseries/mobility.c
>> +++ b/arch/powerpc/platforms/pseries/mobility.c
>> @@ -371,6 +371,18 @@ void post_mobility_fixup(void)
>>  	/* Possibly switch to a new RFI flush type */
>>  	pseries_setup_rfi_flush();
>>  
>> +	/*
>> +	 * Incase lpar migrate from one system to another, system
> 
> In case an LPAR migrates
> 
>> +	 * parameter details like chips per sockets and number of sockets
>> +	 * might change. So, it needs to be re-initialized otherwise these
>                              ^                                       ^
>                              they need                               the
>> +	 * values corresponds to previous system.
>                   ^
>                   will correspond to the
> 
>> +	 * Here, adding a call to read_sys_info_pseries() declared in
> 
> Adding is the wrong tense in a comment. When someone reads the comment
> the code has already been added. Past tense would be right, but really
> the comment shouldn't say what you did, it should say why.
> 
>> +	 * platforms/pseries/pseries.h to re-init the physsockets and
>> +	 * physchips value.
> 
> Call read_sys_info_pseries() to reinitialise the values.
> 
>> +	 */
>> +	if (IS_ENABLED(CONFIG_HV_PERF_CTRS) && IS_ENABLED(CONFIG_PPC_RTAS))
>> +		read_sys_info_pseries();
> 
> The RTAS check is not needed. pseries always selects RTAS.
> 
> You shouldn't need the IS_ENABLED() check here though, do it with an
> empty version in the header when CONFIG_HV_PERF_CTRS is not enabled.
> 

Hi Michael,
	Thanks for reviewing the patch. Is something like this you are suggesting. Please
let me know if my understanding is fine.

+#ifndef CONFIG_HV_PERF_CTRS
+#define read_sys_info_pseries() 
+#endif

Thanks,
Kajol Jain
> cheers
> 

Re: [PATCH v7 5/5] powerpc/hv-24x7: Update post_mobility_fixup() to handle migration

[Michael Ellerman]

kajoljain <kjain@linux.ibm.com> writes:
> On 4/29/20 5:07 PM, Michael Ellerman wrote:
>> Kajol Jain <kjain@linux.ibm.com> writes:
>>> Function 'read_sys_info_pseries()' is added to get system parameter
>>> values like number of sockets and chips per socket.
>>> and it gets these details via rtas_call with token
>>> "PROCESSOR_MODULE_INFO".
>>>
>>> Incase lpar migrate from one system to another, system
>>> parameter details like chips per sockets or number of sockets might
>>> change. So, it needs to be re-initialized otherwise, these values
>>> corresponds to previous system values.
>>> This patch adds a call to 'read_sys_info_pseries()' from
>>> 'post-mobility_fixup()' to re-init the physsockets and physchips values.
>>>
>>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>>> ---
>>>  arch/powerpc/platforms/pseries/mobility.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
>>> index b571285f6c14..226accd6218b 100644
>>> --- a/arch/powerpc/platforms/pseries/mobility.c
>>> +++ b/arch/powerpc/platforms/pseries/mobility.c
>>> @@ -371,6 +371,18 @@ void post_mobility_fixup(void)
>>>  	/* Possibly switch to a new RFI flush type */
>>>  	pseries_setup_rfi_flush();
>>>  
>>> +	/*
>>> +	 * Incase lpar migrate from one system to another, system
>> 
>> In case an LPAR migrates
>> 
>>> +	 * parameter details like chips per sockets and number of sockets
>>> +	 * might change. So, it needs to be re-initialized otherwise these
>>                              ^                                       ^
>>                              they need                               the
>>> +	 * values corresponds to previous system.
>>                   ^
>>                   will correspond to the
>> 
>>> +	 * Here, adding a call to read_sys_info_pseries() declared in
>> 
>> Adding is the wrong tense in a comment. When someone reads the comment
>> the code has already been added. Past tense would be right, but really
>> the comment shouldn't say what you did, it should say why.
>> 
>>> +	 * platforms/pseries/pseries.h to re-init the physsockets and
>>> +	 * physchips value.
>> 
>> Call read_sys_info_pseries() to reinitialise the values.
>> 
>>> +	 */
>>> +	if (IS_ENABLED(CONFIG_HV_PERF_CTRS) && IS_ENABLED(CONFIG_PPC_RTAS))
>>> +		read_sys_info_pseries();
>> 
>> The RTAS check is not needed. pseries always selects RTAS.
>> 
>> You shouldn't need the IS_ENABLED() check here though, do it with an
>> empty version in the header when CONFIG_HV_PERF_CTRS is not enabled.
>> 
>
> Hi Michael,
> 	Thanks for reviewing the patch. Is something like this you are suggesting. Please
> let me know if my understanding is fine.
>
> +#ifndef CONFIG_HV_PERF_CTRS
> +#define read_sys_info_pseries() 
> +#endif

It should be an empty static inline. So more like:

#ifdef CONFIG_HV_PERF_CTRS
void read_sys_info_pseries(void);
#else
static inline void read_sys_info_pseries(void) { }
#endif

cheers

Re: [PATCH v7 2/5] powerpc/hv-24x7: Add rtas call in hv-24x7 driver to get processor details

[kajoljain]

On 4/29/20 5:01 PM, Michael Ellerman wrote:
> Hi Kajol,
> 
> Some comments inline ...
> 
> Kajol Jain <kjain@linux.ibm.com> writes:
>> For hv_24x7 socket/chip level events, specific chip-id to which
>> the data requested should be added as part of pmu events.
>> But number of chips/socket in the system details are not exposed.
>>
>> Patch implements read_sys_info_pseries() to get system
>> parameter values like number of sockets and chips per socket.
>> Rtas_call with token "PROCESSOR_MODULE_INFO"
>> is used to get these values.
>>
>> Sub-sequent patch exports these values via sysfs.
>>
>> Patch also make these parameters default to 1.
>>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>>  arch/powerpc/perf/hv-24x7.c              | 72 ++++++++++++++++++++++++
>>  arch/powerpc/platforms/pseries/pseries.h |  3 +
>>  2 files changed, 75 insertions(+)
>>
>> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
>> index 48e8f4b17b91..9ae00f29bd21 100644
>> --- a/arch/powerpc/perf/hv-24x7.c
>> +++ b/arch/powerpc/perf/hv-24x7.c
>> @@ -20,6 +20,11 @@
>>  #include <asm/io.h>
>>  #include <linux/byteorder/generic.h>
>>  
>> +#ifdef CONFIG_PPC_RTAS
> 
> This driver can only be build on pseries, and pseries always selects
> RTAS. So the ifdef is unncessary.

Hi Michael,
	Thanks for review, I will remove this check.

> 
>> +#include <asm/rtas.h>
>> +#include <../../platforms/pseries/pseries.h>
>> +#endif
> 
> That's not really what the platform header is intended for.
> 
> You should put the extern in arch/powerpc/include/asm somewhere.
> 
> Maybe rtas.h
> 
>> @@ -57,6 +62,69 @@ static bool is_physical_domain(unsigned domain)
>>  	}
>>  }
>>  
>> +#ifdef CONFIG_PPC_RTAS
> 
> Not needed.
> 
>> +#define PROCESSOR_MODULE_INFO   43
> 
> Please document where these come from, presumably LoPAPR somewhere?
> 

Sure, will add the details.

>> +#define PROCESSOR_MAX_LENGTH	(8 * 1024)
>> +
>> +static int strbe16toh(const char *buf, int offset)
>> +{
>> +	return (buf[offset] << 8) + buf[offset + 1];
>> +}
> 
> I'm confused by this. "str" implies string, a string is an array of
> bytes and has no endian. But then be16 implies it's an array of __be16,
> in which case buf should be a __be16 *.
> 

Yes right, actually I was following implementation in util-linux. But what you
suggested make more sense, will update accordingly.

>> +
>> +static u32		physsockets;	/* Physical sockets */
>> +static u32		physchips;	/* Physical chips */
> 
> No tabs there please.

Sure will update.

> 
>> +
>> +/*
>> + * Function read_sys_info_pseries() make a rtas_call which require
>> + * data buffer of size 8K. As standard 'rtas_data_buf' is of size
>> + * 4K, we are adding new local buffer 'rtas_local_data_buf'.
>> + */
>> +char rtas_local_data_buf[PROCESSOR_MAX_LENGTH] __cacheline_aligned;
> 
> static?
> 
>> +/*
>> + * read_sys_info_pseries()
>> + * Retrieve the number of sockets and chips per socket details
>> + * through the get-system-parameter rtas call.
>> + */
>> +void read_sys_info_pseries(void)
>> +{
>> +	int call_status, len, ntypes;
>> +
>> +	/*
>> +	 * Making system parameter: chips and sockets default to 1.
>> +	 */
>> +	physsockets = 1;
>> +	physchips = 1;
>> +	memset(rtas_local_data_buf, 0, PROCESSOR_MAX_LENGTH);
>> +	spin_lock(&rtas_data_buf_lock);
> 
> You're not using the rtas_data_buf, so why are you taking the
> rtas_data_buf_lock?

Sure, I will add new lock specific for rtas_local_data_buf

> 
>> +	call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
>> +				NULL,
>> +				PROCESSOR_MODULE_INFO,
>> +				__pa(rtas_local_data_buf),
>> +				PROCESSOR_MAX_LENGTH);
>> +
>> +	spin_unlock(&rtas_data_buf_lock);
>> +
>> +	if (call_status != 0) {
>> +		pr_info("%s %s Error calling get-system-parameter (0x%x)\n",
>> +			__FILE__, __func__, call_status);
> 
> pr_err(), don't use __FILE__, this file already uses pr_fmt(). Not sure
> __func__ is really necessary either
> 
> 		return;
> 
> Then you can deindent the next block.
> 
>> +	} else {
>> +		rtas_local_data_buf[PROCESSOR_MAX_LENGTH - 1] = '\0';
>> +		len = strbe16toh(rtas_local_data_buf, 0);
> 
> Why isn't the buffer a __be16 array, and then you just use be16_to_cpu() ?
> 
>> +		if (len < 6)
>> +			return;
>> +
>> +		ntypes = strbe16toh(rtas_local_data_buf, 2);
>> +
>> +		if (!ntypes)
>> +			return;
> 
> What is ntype
ntype specify processor module type

> 
>> +		physsockets = strbe16toh(rtas_local_data_buf, 4);
>> +		physchips = strbe16toh(rtas_local_data_buf, 6);
>> +	}
>> +}
>> +#endif /* CONFIG_PPC_RTAS */
>> +
>>  /* Domains for which more than one result element are returned for each event. */
>>  static bool domain_needs_aggregation(unsigned int domain)
>>  {
>> @@ -1605,6 +1673,10 @@ static int hv_24x7_init(void)
>>  	if (r)
>>  		return r;
>>  
>> +#ifdef CONFIG_PPC_RTAS
>> +	read_sys_info_pseries();
>> +#endif
>

Will remove this config checks.

Thanks,
Kajol Jain

 
>> +
>>  	return 0;
>>  }
>>  
>> diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
>> index 13fa370a87e4..1727559ce304 100644
>> --- a/arch/powerpc/platforms/pseries/pseries.h
>> +++ b/arch/powerpc/platforms/pseries/pseries.h
>> @@ -19,6 +19,9 @@ extern void request_event_sources_irqs(struct device_node *np,
>>  struct pt_regs;
>>  
>>  extern int pSeries_system_reset_exception(struct pt_regs *regs);
>> +#ifdef CONFIG_PPC_RTAS
>> +extern void read_sys_info_pseries(void);
>> +#endif
>>  extern int pSeries_machine_check_exception(struct pt_regs *regs);
>>  extern long pseries_machine_check_realmode(struct pt_regs *regs);
> > 
> cheers
>