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

2024-02-22 Thread kajoljain 



On 2/20/24 18:08, Michael Ellerman wrote:
> Kajol Jain  writes:
>> 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':
>>
>>  
>> 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 
>> Signed-off-by: Kajol Jain 
>> ---
>>  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',
> 
> Don't we expect H_PARAMETER if any parameter value is incorrect?
> 
>> + * which means that the current buffer size cannot accommodate
>> + * all the information and a partial buffer returned.
> 
> I don't see how we can infer that H_PARAMETER means the buffer is too
> small and accessing the first entry is OK?

Hi Michael,
  Based on getCounterInfo documentation and the name convention it uses,
we actually used H_PARAMETER to specify the buffer issue incase buffer
cannot accommodate all the data.
Hence we are using that return value in the check.

Since based on hv-gpci event counter we only want data for specific
starting index and the hv-gpci hcall actually store data starting from
given starting index in the result buffer. We can ensure that accessing
first entry will be enough.

Thanks,
Kajol Jain


> 
> cheers
> 
>> + * 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;
>>  }

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

2024-02-20 Thread Michael Ellerman 
Kajol Jain  writes:
> 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':
>
>  
> 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 
> Signed-off-by: Kajol Jain 
> ---
>  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',

Don't we expect H_PARAMETER if any parameter value is incorrect?

> +  * which means that the current buffer size cannot accommodate
> +  * all the information and a partial buffer returned.

I don't see how we can infer that H_PARAMETER means the buffer is too
small and accessing the first entry is OK?

cheers

> +  * 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;
>   }

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

2024-02-01 Thread Akanksha J N 

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':

  
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 
Signed-off-by: Kajol Jain 


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 


---
  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),
);
-   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,
-   )) {
+   );
+
+   /*
+* ret value as

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

2024-01-31 Thread Kajol Jain 
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':

 
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 
Signed-off-by: Kajol Jain 
---
 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),
);
-   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,
-   )) {
+   );
+
+   /*
+* 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;
}
-- 
2.43.0