Re: [PATCH v1 2/3] drivers/perf: add DesignWare PCIe PMU driver

From: Robin Murphy <robin.murphy@arm.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Shuai Xue <xueshuai@linux.alibaba.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	will@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, rdunlap@infradead.org,
	mark.rutland@arm.com, baolin.wang@linux.alibaba.com,
	zhuo.song@linux.alibaba.com, linux-pci@vger.kernel.org
Subject: Re: [PATCH v1 2/3] drivers/perf: add DesignWare PCIe PMU driver
Date: Tue, 27 Sep 2022 11:14:07 +0100	[thread overview]
Message-ID: <5372edb4-5717-42a0-142e-91657a9b18c3@arm.com> (raw)
In-Reply-To: <20220927110435.00005b4d@huawei.com>

On 2022-09-27 11:04, Jonathan Cameron wrote:
> On Tue, 27 Sep 2022 13:13:29 +0800
> Shuai Xue <xueshuai@linux.alibaba.com> wrote:
> 
>> 在 2022/9/27 AM1:18, Bjorn Helgaas 写道:
>>> On Mon, Sep 26, 2022 at 09:31:34PM +0800, Shuai Xue wrote:
>>>> 在 2022/9/23 PM11:54, Jonathan Cameron 写道:
>>>>>> I found a similar definition in arch/ia64/pci/pci.c .
>>>>>>
>>>>>> 	#define PCI_SAL_ADDRESS(seg, bus, devfn, reg)		\
>>>>>> 	(((u64) seg << 24) | (bus << 16) | (devfn << 8) | (reg))
>>>>>>
>>>>>> Should we move it into a common header first?
>>>>>
>>>>> Maybe. The bus, devfn, reg part is standard bdf, but I don't think
>>>>> the PCI 6.0 spec defined a version with the seg in the upper bits.
>>>>> I'm not sure if we want to adopt that in LInux.
>>>>
>>>> I found lots of code use seg,bus,devfn,reg with format "%04x:%02x:%02x.%x",
>>>> I am not quite familiar with PCIe spec. What do you think about it, Bjorn?
>>>
>>> The PCIe spec defines an address encoding for bus/device/function/reg
>>> for the purposes of ECAM (PCIe r6.0, sec 7.2.2), but as far as I know,
>>> it doesn't define anything similar that includes the segment.  The
>>> segment is really outside the scope of PCIe because each segment is a
>>> completely separate PCIe hierarchy.
>>
>> Thank you for your explanation.
>>
>>>
>>> So I probably wouldn't make this a generic definition.  But if/when
>>> you print things like this out, please do use the format spec you
>>> mentioned above so it matches the style used elsewhere.
>>>    
>>
>> Agree. The print format of bus/device/function/reg is "%04x:%02x:%02x.%x",
>> so I named the PMU as the same format. Then the usage flow would be:
>>
>> - lspci to get the device root port in format seg/bus/device/function/reg.
>> 	10:00.0 PCI bridge: Device 1ded:8000 (rev 01)
>> - select its PMU name pcie_bdf_100000.
>> - monitor with perf:
>> 	perf stat -a -e pcie_bdf_200/Rx_PCIe_TLP_Data_Payload/
> 
> I think you probably want something in there to indicate it's an RP
> and the bdf part may be redundant...

Indeed that seems horribly unclear; personally I reckon something like 
"dw_pcie_200" would be more appropriate. The address is just a 
disambiguator between multiple instances so doesn't need any further 
emphasis, but what is crucial to the user is exactly what kind of PMU it 
is (especially if there's potential for other unrelated PCIe functions 
to start exposing their own different PMUs).

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel