* [PATCH] powerpc/hv-gpci: Fix the hcall return value checks in single_gpci_request function
@ 2024-01-31 11:26 Kajol Jain
2024-02-01 6:47 ` Akanksha J N
2024-02-20 12:38 ` Michael Ellerman
0 siblings, 2 replies; 4+ messages in thread
From: Kajol Jain @ 2024-01-31 11:26 UTC (permalink / raw)
To: mpe; +Cc: atrajeev, kjain, akanksha, maddy, disgoel, linuxppc-dev
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 <akanksha@linux.ibm.com>
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
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;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/hv-gpci: Fix the hcall return value checks in single_gpci_request function
2024-01-31 11:26 [PATCH] powerpc/hv-gpci: Fix the hcall return value checks in single_gpci_request function Kajol Jain
@ 2024-02-01 6:47 ` Akanksha J N
2024-02-20 12:38 ` Michael Ellerman
1 sibling, 0 replies; 4+ messages in thread
From: Akanksha J N @ 2024-02-01 6:47 UTC (permalink / raw)
To: Kajol Jain, mpe; +Cc: atrajeev, maddy, linuxppc-dev, disgoel
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 <akanksha@linux.ibm.com>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
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 <akanksha@linux.ibm.com>
> ---
> 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;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/hv-gpci: Fix the hcall return value checks in single_gpci_request function
2024-01-31 11:26 [PATCH] powerpc/hv-gpci: Fix the hcall return value checks in single_gpci_request function Kajol Jain
2024-02-01 6:47 ` Akanksha J N
@ 2024-02-20 12:38 ` Michael Ellerman
2024-02-22 8:43 ` kajoljain
1 sibling, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2024-02-20 12:38 UTC (permalink / raw)
To: Kajol Jain; +Cc: atrajeev, kjain, akanksha, maddy, disgoel, linuxppc-dev
Kajol Jain <kjain@linux.ibm.com> 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':
>
> <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 <akanksha@linux.ibm.com>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
> 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;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/hv-gpci: Fix the hcall return value checks in single_gpci_request function
2024-02-20 12:38 ` Michael Ellerman
@ 2024-02-22 8:43 ` kajoljain
0 siblings, 0 replies; 4+ messages in thread
From: kajoljain @ 2024-02-22 8:43 UTC (permalink / raw)
To: Michael Ellerman; +Cc: atrajeev, linuxppc-dev, maddy, disgoel, akanksha
On 2/20/24 18:08, Michael Ellerman wrote:
> Kajol Jain <kjain@linux.ibm.com> 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':
>>
>> <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 <akanksha@linux.ibm.com>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>> 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;
>> }
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-02-22 8:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-31 11:26 [PATCH] powerpc/hv-gpci: Fix the hcall return value checks in single_gpci_request function Kajol Jain
2024-02-01 6:47 ` Akanksha J N
2024-02-20 12:38 ` Michael Ellerman
2024-02-22 8:43 ` kajoljain
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).