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

[kajoljain <kjain@linux.ibm.com>]

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