Re: [PATCH] powerpc/hv-gpci: Fix the hcall return value checks in single_gpci_request function

From
Akanksha J N <hidden>
Date
2024-02-01 08:56:38
Reply to
Kajol Jain, 2024-01-31
On 31/01/24 4:56 pm, Kajol Jain wrote:
Running event hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/
in one of the system throws below error:

  ---Logs---
  # perf list | grep hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles
   hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=?/[Kernel PMU event]


  # perf stat -v -e hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/ sleep 2
Using CPUID 00800200
Control descriptor is not initialized
Warning:
hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/ event is not supported by the kernel.
failed to read counter hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/

  Performance counter stats for 'system wide':

    <not supported>      hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/

        2.000700771 seconds time elapsed

The above error is because of the hcall failure as required
permission "Enable Performance Information Collection" is not set.
Based on current code, single_gpci_request function did not check the
error type incase hcall fails and by default returns EINVAL. But we can
have other reasons for hcall failures like H_AUTHORITY/H_PARAMETER for which
we need to act accordingly.
Fix this issue by adding new checks in the single_gpci_request function.

Result after fix patch changes:

  # perf stat -e hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/ sleep 2
Error:
No permission to enable hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/ event.

Fixes: 220a0c609ad1 ("powerpc/perf: Add support for the hv gpci (get performance counter info) interface")
Reported-by: Akanksha J N <redacted>
Signed-off-by: Kajol Jain <redacted>
Tested the patch on Power10 system, and the fix works fine.

# ./perf stat -v -e hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=1/ sleep 2
Control descriptor is not initialized
hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=1/: 0 2001246376 2001246376
 Performance counter stats for 'system wide':
                 0      hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=1/
       2.001250313 seconds time elapsed

Tested-by: Akanksha J N <redacted>
quoted hunk ↗ jump to hunk
---
  arch/powerpc/perf/hv-gpci.c | 29 +++++++++++++++++++++++++----
  1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/perf/hv-gpci.c b/arch/powerpc/perf/hv-gpci.c
index 27f18119fda1..101060facd81 100644
--- a/arch/powerpc/perf/hv-gpci.c
+++ b/arch/powerpc/perf/hv-gpci.c
@@ -695,7 +695,17 @@ static unsigned long single_gpci_request(u32 req, u32 starting_index,
  
  	ret = plpar_hcall_norets(H_GET_PERF_COUNTER_INFO,
  			virt_to_phys(arg), HGPCI_REQ_BUFFER_SIZE);
-	if (ret) {
+
+	/*
+	 * ret value as 'H_PARAMETER' corresponds to 'GEN_BUF_TOO_SMALL',
+	 * which means that the current buffer size cannot accommodate
+	 * all the information and a partial buffer returned.
+	 * Since in this function we are only accessing data for a given starting index,
+	 * we don't need to accommodate whole data and can get required count by
+	 * accessing very first entry.
+	 * Hence hcall fails only incase the ret value is other than H_SUCCESS or H_PARAMETER.
+	 */
+	if (ret && (ret != H_PARAMETER)) {
  		pr_devel("hcall failed: 0x%lx\n", ret);
  		goto out;
  	}
@@ -724,7 +734,7 @@ static u64 h_gpci_get_value(struct perf_event *event)
  					event_get_offset(event),
  					event_get_length(event),
  					&count);
-	if (ret)
+	if (ret && (ret != H_PARAMETER))
  		return 0;
  	return count;
  }
@@ -759,6 +769,7 @@ static int h_gpci_event_init(struct perf_event *event)
  {
  	u64 count;
  	u8 length;
+	unsigned long ret;
  
  	/* Not our event */
  	if (event->attr.type != event->pmu->type)
@@ -789,13 +800,23 @@ static int h_gpci_event_init(struct perf_event *event)
  	}
  
  	/* check if the request works... */
-	if (single_gpci_request(event_get_request(event),
+	ret = single_gpci_request(event_get_request(event),
  				event_get_starting_index(event),
  				event_get_secondary_index(event),
  				event_get_counter_info_version(event),
  				event_get_offset(event),
  				length,
-				&count)) {
+				&count);
+
+	/*
+	 * ret value as H_AUTHORITY implies that partition is not permitted to retrieve
+	 * performance information, and required to set
+	 * "Enable Performance Information Collection" option.
+	 */
+	if (ret == H_AUTHORITY)
+		return -EPERM;
+
+	if (ret && (ret != H_PARAMETER)) {
  		pr_devel("gpci hcall failed\n");
  		return -EINVAL;
  	}
Thread (4 messages) 4 messages, 3 authors, last reply 2024-02-22
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help