Re: Re: [PATCH 4/7] drivers/perf: hisi_pcie: Check the target filter properly

[hejunhao <hejunhao3@huawei.com>]

On 2024/2/21 17:46, Yicong Yang wrote:
> On 2024/2/8 20:29, Jonathan Cameron wrote:
>> On Sun, 4 Feb 2024 15:45:24 +0800
>> Yicong Yang <yangyicong@huawei.com> wrote:
>>
>>> From: Junhao He <hejunhao3@huawei.com>
>>>
>>> The PMU can monitor traffic of certain target Root Port or downstream
>>> target Endpoint. User can specify the target filter by the "port" or
>>> "bdf" option respectively. The PMU can only monitor the Root Port or
>>> Endpoint on the same PCIe core so the value of "port" or "bdf" should
>>> be valid and will be checked by the driver.
>>>
>>> Currently at least and only one of "port" and "bdf" option must be set.
>>> If "port" filter is not set or is set explicitly to zero (default),
>>> driver will regard the user specifies a "bdf" option since "port" option
>>> is a bitmask of the target Root Ports and zero is not a valid
>>> value.
>>>
>>> If user not explicitly set "port" or "bdf" filter, the driver uses "bdf"
>>> default value (zero) to set target filter, but driver will skip the
>>> check of bdf=0, although it's a valid value (meaning 0000:000:00.0).
>>> Then the user just gets zero.
>>>
>>> Therefore, we need to check if both "port" and "bdf" are invalid, then
>>> return failure and report warning.
>>>
>>> Testing:
>>> before the patch:
>>>                     0      hisi_pcie0_core1/rx_mrd_flux/
>>>                     0      hisi_pcie0_core1/rx_mrd_flux,port=0/
>>>                24,124      hisi_pcie0_core1/rx_mrd_flux,port=1/
>>>                     0      hisi_pcie0_core1/rx_mrd_flux,bdf=0/
>>>       <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=1/
>> Nice to include an example that works for bdf
>> 			    hisi_pcie0_core1/rx_mrd_flux,bdf=1,port=0
>> or something like that?
>>> after the patch:
>>>       <not supported>      hisi_pcie0_core1/rx_mrd_flux/
>>>       <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0/
>>>                24,153      hisi_pcie0_core1/rx_mrd_flux,port=1/
>>>       <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=0/
>>>       <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=1/
>>>
>>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> Clearly the current situation is wrong, but perhaps we can
>> have a more intuitive scheme (could be added as a follow up patch)
>> and have the driver figure out which port the bdf lies below?
>>
>> Maybe that's a job for userspace tooling rather than the driver, but
>> the driver already has verification code and it wouldn't be hard
>> to not just check the rp is ours, but also set the filter to specify
>> that rp, or maybe just set the mask to include them all?
>>
> To do a check should be simple, we can decode the bdf and find the target
> endpoint and related root port for doing the check.

The function hisi_pcie_pmu_valid_requester_id() already does this.
It can get the RP of the bdf, then check whether the RP is within
the RP range of the PCIe PMU.

> Another example is what we've done in hisi_ptt that we maintian a list of
> supported root ports and endpoints, but that will be a bit more complex.
>
>> Jonathan
>>
>>
>>> ---
>>>   drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
>>> index 83be3390686c..b91f03c02c57 100644
>>> --- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
>>> +++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
>>> @@ -306,10 +306,10 @@ static bool hisi_pcie_pmu_valid_filter(struct perf_event *event,
>>>   	if (hisi_pcie_get_trig_len(event) > HISI_PCIE_TRIG_MAX_VAL)
>>>   		return false;
>>>   
>>> -	if (requester_id) {
>>> -		if (!hisi_pcie_pmu_valid_requester_id(pcie_pmu, requester_id))
>>> -			return false;
>>> -	}
>>> +	/* Need to explicitly set filter of "port" or "bdf" */
>>> +	if (!hisi_pcie_get_port(event) &&
>>> +	    !hisi_pcie_pmu_valid_requester_id(pcie_pmu, requester_id))
>>> +		return false;
>>>   
>>>   	return true;
>>>   }
>> .
>>