diff mbox series

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

Message ID 20240204074527.47110-5-yangyicong@huawei.com
State New
Headers
Series None |

Commit Message

Yicong Yang Feb. 4, 2024, 7:45 a.m. UTC 
  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/

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>
---
 drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Jonathan Cameron Feb. 8, 2024, 12:29 p.m. UTC | #1 
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?

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;
>  }
Yicong Yang Feb. 21, 2024, 9:46 a.m. UTC | #2 
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.

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;
>>  }
> 
> .
>
hejunhao Feb. 22, 2024, 1:21 a.m. UTC | #3 
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;
>>>   }
>> .
>>
hejunhao Feb. 22, 2024, 1:29 a.m. UTC | #4 
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?

Yes, I will do that.
These combined parameter test cases have been validated.

>> 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?
>
> 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;
>>   }
>
diff mbox series

Patch

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