Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On 2021/12/8 0:31, Robin Murphy wrote:
> On 2021-11-29 08:22, Yicong Yang via iommu wrote:
>> On 2021/11/25 23:49, Robin Murphy wrote:
>>> On 2021-11-18 09:01, Yicong Yang via iommu wrote:
Hi Robin,
On 2021/11/16 19:37, Yicong Yang wrote:
> On 2021/11/16 18:56, Robin Murphy wrote:
>> On 2021-11-16 09:06, Yicong Yang via iommu wrote:
>> [...]
>>> +/*
>>> + * Get RMR address if provided by the firmware.
>>> + * Return 0 if the IOMMU doesn't present or the policy of the
>>> + * IOMMU domain is passthrough or we get a usable RMR region.
>>> + * Otherwise a negative value is returned.
>>> + */
>>> +static int hisi_ptt_get_rmr(struct hisi_ptt *hisi_ptt)
>>> +{
>>> + struct pci_dev *pdev = hisi_ptt->pdev;
>>> + struct iommu_domain *iommu_domain;
>>> + struct iommu_resv_region *region;
>>> + LIST_HEAD(list);
>>> +
>>> + /*
>>> + * Use direct DMA if IOMMU does not present or the policy of the
>>> + * IOMMU domain is passthrough.
>>> + */
>>> + iommu_domain = iommu_get_domain_for_dev(>dev);
>>> + if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY)
>>> + return 0;
>>> +
>>> + iommu_get_resv_regions(>dev, );
>>> + list_for_each_entry(region, , list)
>>> + if (region->type == IOMMU_RESV_DIRECT &&
>>> + region->length >= HISI_PTT_TRACE_BUFFER_SIZE) {
>>> + hisi_ptt->trace_ctrl.has_rmr = true;
>>> + hisi_ptt->trace_ctrl.rmr_addr = region->start;
>>> + hisi_ptt->trace_ctrl.rmr_length = region->length;
>>> + break;
>>> + }
>>> +
>>> + iommu_put_resv_regions(>dev, );
>>> + return hisi_ptt->trace_ctrl.has_rmr ? 0 : -ENOMEM;
>>> +}
>>
>> No.
>>
>> The whole point of RMRs is for devices that are already configured to
>> access the given address range in a manner beyond the kernel's control.
>> If you can do this, it proves that you should not have an RMR in the
>> first place.
>>
>> The notion of a kernel driver explicitly configuring its device to DMA
>> into any random RMR that looks big enough is so egregiously wrong that
>> I'm almost lost for words...
>>
>
> our bios will reserve such a region and reported it through iort. the
> device will write to the region and in the driver we need to access the
> region
> to get the traced data. the region is reserved exclusively and will not
> be accessed by kernel or other devices.
>
> is it ok to let bios configure the address to the device and from CPU
> side we just read it?
>
Any suggestion? Is this still an issue you concern if we move the
configuration of the device address to BIOS and just read from the CPU
side?
>>>
>>> If the firmware configures the device so that it's actively tracing and
>>> writing out to memory while the kernel boots, then that is a valid reason
>>> to have an RMR. However what you're doing in the driver is still complete
>>> nonsense. As far as I can follow, the way it's working is this:
>>>
>>> - At probe time, the initial state of the hardware is entirely ignored. If
>>> it *is* already active, there appears to be a fun chance of crashing if
>>> TRACE_INT_MASK is clear and an interrupt happens to fire before anyone has
>>> got round to calling perf_aux_output_begin() to make trace_ctrl.handle.rb
>>> non-NULL.
>>>
>>> - Later, once the user starts a tracing session, a buffer is set up
>>> *either* as a completely normal DMA allocation, or by memremap()ing some
>>> random IOVA carveout which may or may not be whatever memory the firmware
>>> was tracing to.
>>>
>>> - The hardware is then reset and completely reprogrammed to use the new
>>> buffer, again without any consideration of its previous state (other than
>>> possibly timing out and failing if it's already running and that means it
>>> never goes idle).
>>>
>>> Therefore the driver does not seem to respect any prior configuration of
>>> the device by firmware, does not seem to expect it to be running at boot
>>> time, does not seem to have any way to preserve and export any trace data
>>> captured in an RMR if it *was* running at boot time, and thus without loss
>>> of generality could simply use the dma_alloc_coherent() path all the time.
>>> Am I missing anything?
>>>
>>
>> Thanks for the further explanation and I think I understand your concerns
>> more clearer.
>>
>> The trace is not supposed to begin by the firmware at boot time. Due to some
>> hardware restriction, the device cannot trace with non-identical mapping.
>> So we'd like to use RMR to make the device work when the dma mapping is
>> non-identical. Thus we check here to decide whether to use RMR or not: if
>> the iommu
>> is not presented or in the passthrough mode, we can use direct DMA by
>>
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On 2021-11-29 08:22, Yicong Yang via iommu wrote:
On 2021/11/25 23:49, Robin Murphy wrote:
On 2021-11-18 09:01, Yicong Yang via iommu wrote:
Hi Robin,
On 2021/11/16 19:37, Yicong Yang wrote:
On 2021/11/16 18:56, Robin Murphy wrote:
On 2021-11-16 09:06, Yicong Yang via iommu wrote:
[...]
+/*
+ * Get RMR address if provided by the firmware.
+ * Return 0 if the IOMMU doesn't present or the policy of the
+ * IOMMU domain is passthrough or we get a usable RMR region.
+ * Otherwise a negative value is returned.
+ */
+static int hisi_ptt_get_rmr(struct hisi_ptt *hisi_ptt)
+{
+ struct pci_dev *pdev = hisi_ptt->pdev;
+ struct iommu_domain *iommu_domain;
+ struct iommu_resv_region *region;
+ LIST_HEAD(list);
+
+ /*
+ * Use direct DMA if IOMMU does not present or the policy of the
+ * IOMMU domain is passthrough.
+ */
+ iommu_domain = iommu_get_domain_for_dev(>dev);
+ if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY)
+ return 0;
+
+ iommu_get_resv_regions(>dev, );
+ list_for_each_entry(region, , list)
+ if (region->type == IOMMU_RESV_DIRECT &&
+ region->length >= HISI_PTT_TRACE_BUFFER_SIZE) {
+ hisi_ptt->trace_ctrl.has_rmr = true;
+ hisi_ptt->trace_ctrl.rmr_addr = region->start;
+ hisi_ptt->trace_ctrl.rmr_length = region->length;
+ break;
+ }
+
+ iommu_put_resv_regions(>dev, );
+ return hisi_ptt->trace_ctrl.has_rmr ? 0 : -ENOMEM;
+}
No.
The whole point of RMRs is for devices that are already configured to access
the given address range in a manner beyond the kernel's control. If you can do
this, it proves that you should not have an RMR in the first place.
The notion of a kernel driver explicitly configuring its device to DMA into any
random RMR that looks big enough is so egregiously wrong that I'm almost lost
for words...
our bios will reserve such a region and reported it through iort. the device
will write to the region and in the driver we need to access the region
to get the traced data. the region is reserved exclusively and will not be
accessed by kernel or other devices.
is it ok to let bios configure the address to the device and from CPU side we
just read it?
Any suggestion? Is this still an issue you concern if we move the
configuration of the device address to BIOS and just read from the CPU side?
If the firmware configures the device so that it's actively tracing and writing
out to memory while the kernel boots, then that is a valid reason to have an
RMR. However what you're doing in the driver is still complete nonsense. As far
as I can follow, the way it's working is this:
- At probe time, the initial state of the hardware is entirely ignored. If it
*is* already active, there appears to be a fun chance of crashing if
TRACE_INT_MASK is clear and an interrupt happens to fire before anyone has got
round to calling perf_aux_output_begin() to make trace_ctrl.handle.rb non-NULL.
- Later, once the user starts a tracing session, a buffer is set up *either* as
a completely normal DMA allocation, or by memremap()ing some random IOVA
carveout which may or may not be whatever memory the firmware was tracing to.
- The hardware is then reset and completely reprogrammed to use the new buffer,
again without any consideration of its previous state (other than possibly
timing out and failing if it's already running and that means it never goes
idle).
Therefore the driver does not seem to respect any prior configuration of the
device by firmware, does not seem to expect it to be running at boot time, does
not seem to have any way to preserve and export any trace data captured in an
RMR if it *was* running at boot time, and thus without loss of generality could
simply use the dma_alloc_coherent() path all the time. Am I missing anything?
Thanks for the further explanation and I think I understand your concerns more
clearer.
The trace is not supposed to begin by the firmware at boot time. Due to some
hardware restriction, the device cannot trace with non-identical mapping.
So we'd like to use RMR to make the device work when the dma mapping is
non-identical. Thus we check here to decide whether to use RMR or not: if the
iommu
is not presented or in the passthrough mode, we can use direct DMA by
dma_alloc_coherent(); if the iommu is present and the mode is not passthrough,
we try
to retrieve RMR or we fail the probe. The firmware is expected to reserve a
range of memory and reports it to the driver and is not expected to configure
the trace and do boot time tracing.
As things stand, RMRs are not yet supported upstream (FYI we're still working
on fixing the spec...), so the code above is at best dead, and at worst
actively wrong. Furthermore, if the expected usage model *is* that the kernel
driver completely resets and reprograms the hardware, then even if there is an
RMR for boot-time tracing I would rather expect it to be
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On 2021/11/27 1:10, Mathieu Poirier wrote:
>
> [...]
>
>>
> +
> +#define HISI_PTT_TRACE_DMA_IRQ 0
> +#define HISI_PTT_TRACE_BUFLETS_CNT 4
> +#define HISI_PTT_TRACE_BUFLET_SIZE SZ_4M
> +#define HISI_PTT_TRACE_BUFFER_SIZE
> (HISI_PTT_TRACE_BUFLET_SIZE * \
> + HISI_PTT_TRACE_BUFLETS_CNT)
> +#define HISI_PTT_FILTER_UPDATE_FIFO_SIZE 16
> +
> +/* Delay time for filter updating work */
> +#define HISI_PTT_WORK_DELAY_MS 100UL
> +/* Wait time for DMA hardware to reset */
> +#define HISI_PTT_RESET_WAIT_MS 1000UL
> +/* Poll timeout and interval for waiting hardware work to finish */
> +#define HISI_PTT_WAIT_TIMEOUT_US 100UL
> +#define HISI_PTT_WAIT_POLL_INTERVAL_US 100UL
> +
> +#define HISI_PCIE_CORE_PORT_ID(devfn)(PCI_FUNC(devfn) << 1)
> +
> +enum hisi_ptt_trace_status {
> + HISI_PTT_TRACE_STATUS_OFF = 0,
> + HISI_PTT_TRACE_STATUS_ON,
> +};
> +
> +struct hisi_ptt_dma_buflet {
I'm not sure what a "buflet" is... Probably best to just call this
hisi_ptt_dma_buffer" if it pertains to a buffer. On that note it is hard
to
know due to the lack of proper structure documentation. Please have a
look at
how "coresight_device" is documented. The same applies to the rest of the
structures declared below.
>>>
>>> It's mentioned in section 5 of hisi_ptt.rst as "Driver will allocate each
>>> DMA buffer
>>> (we call it buflet) of 4MiB ...".
>>
>> I just noticed the documentation in patch 5 - I will read it before
>> continuing
>> with this set.
>>
>
> As promised I read the documentation and it didn't do much to alliviate my
> concern about the terminology. If a "buflet" is a DMA buffer than simply call
> it a DMA buffer, regardless of the size.
>
ok. If it's hard for the readers to understand, I'll use DMA buffer instead.
> The size of this patchset makes it hard enough to review as it is... On top
> of things reviewers have to remember that PTT means "PCIe tune and trace
> device", tune means "PCIe link's events" and trace means "TLP headers".
>
ok. I used to make the complete function into one patch, will see how to split
it to make review easier.
Trace and tune denote the two function of the device. Trace denote the function
of tracing the TLP headers and tune denotes the function of read and modify
the PCIe link's events. It's unavoidable of knowing the acronyms for the user
of the device. :)
> But we also have to keep in mind that a buflet is really just a DMA buffer...
>
> I will not continue reviewing this set. Please fix the terminology and spin
> another revision. Doing so I suggest you trim down the submission to the
> absolute minimum feature set this driver should provide - enhancements can be
> made later when a foundation has been laid out.
>
> Robin has pointed out what seems to be serious problems with the
> implemenation. That will also have to be sorted out for the next iteration.
>
ok. Will discuss with robin with his concerns and with the comments fixed in
the next version.
Thanks,
Yicong
> Regards,
> Mathieu
>
>
>>>
>>> The total DMA buffer of PTT device is divided into 4 parts, and each part
>>> is called
>>> a 'buflet', which means a small buffer (there exists other words with
>>> similiar
>>> format: droplet, wavelet, ...).
>>>
>>> The device is designed like this to make sure we won't lose any data when
>>> we have to change
>>> the buffer address, the device can continue to write to the next buflet and
>>> don't
>>> neet to pause.
>>>
> + struct list_head list;
> + unsigned int size;
> + dma_addr_t dma;
> +
> + /*
> + * The address of the buflet holding the trace data.
> + * See Documentation/trace/hisi-ptt.rst for the details
As of this writing, hisi-ptt.rst doesn't exist.
>>>
>>> The comment intends to direct the user to the documentation if the user
>>> want to know details about the data format. The data format is decribed
>>> in the section 4 of the doc.
>>>
> + * of the data format.
> + */
> + void *addr;
> + int index;
> +};
> +
> +struct hisi_ptt_trace_ctrl {
> + enum hisi_ptt_trace_status status;
> + struct perf_output_handle handle;
> + struct list_head trace_buf;
> + /*
> + * The index of the buffer which trace data
> + * currently is writing to.
> + */
> + u32 buf_index;
> +
> + int default_cpu;
> + u32 buflet_size;
> + bool is_port;
> + u32 format:1; /* Format of the traced TLP headers */
> + u32 type:4; /* Type of the TLP headers to trace */
> + u32 direction:2;/* Direction of the TLP headers to trace */
> + u32 filter:16; /* Root port or Requester to filter the TLP
> headers */
> +
> +
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On 2021/11/26 2:50, Mathieu Poirier wrote: > On Thu, Nov 25, 2021 at 04:39:46PM +0800, Yicong Yang wrote: >> Hi Mathieu, >> >> Thanks for the comments! Replies inline. >> >> On 2021/11/25 2:51, Mathieu Poirier wrote: >>> Hi Yicong, >>> >>> On Tue, Nov 16, 2021 at 05:06:21PM +0800, Yicong Yang wrote: HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex integrated Endpoint(RCiEP) device, providing the capability to dynamically monitor and tune the PCIe traffic(tune), and trace the TLP headers(trace). >>> >>> Is there a reason to put "tune" and "trace" are whithin parentheses? >>> >> >> yeah. I want to use these single word to denote the related function, and >> place them in the parantheses near the function's description to make sure >> readers can match them. >> > > Probably a better idea to simply write "TLP headers" and "PCIe traffic" when > referring to them. Otherwise it is very confusing because the reader doesn't > know if you mean the acronym or the real definition of "tune" and "trace". > ok I reread the commit and seems reader will know the acronym as it's mentioned as "*tune* the PCIe traffic" and "*trace* the TLP headers". I think the contents in the parantheses can be simply dropped. Add the driver for the device to enable the trace function. The driver will create PMU device for each PTT device, and users can start trace through perf command. Signed-off-by: Yicong Yang --- drivers/Makefile |1 + drivers/hwtracing/Kconfig |2 + drivers/hwtracing/hisilicon/Kconfig|8 + drivers/hwtracing/hisilicon/Makefile |2 + drivers/hwtracing/hisilicon/hisi_ptt.c | 1146 >>> >>> Not sure about the "hisilicon" directory. Right now we have "coresight", >>> "intel_th" and "stm" - they concentrate on the technology rather than the >>> vendor. I think "ptt" would be just fine: >>> >>> drivers/hwtracing/ptt/Kconfig >>> drivers/hwtracing/ptt/Makefile >>> drivers/hwtracing/ptt/hisi_ptt.c >>> >>> That way if other vendors want to introduce support for the same kind of >>> technology we can simply put them under "drivers/hwtracing/ptt/" and things >>> are >>> still accurate. >>> >> >> sure. sounds sensible. will rename the directory to "ptt" if no objection. >> >>> 5 files changed, 1159 insertions(+) create mode 100644 drivers/hwtracing/hisilicon/Kconfig create mode 100644 drivers/hwtracing/hisilicon/Makefile create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c diff --git a/drivers/Makefile b/drivers/Makefile index be5d40ae1488..bb0dc9b55ea2 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -176,6 +176,7 @@ obj-$(CONFIG_USB4) += thunderbolt/ obj-$(CONFIG_CORESIGHT) += hwtracing/coresight/ obj-y += hwtracing/intel_th/ obj-$(CONFIG_STM) += hwtracing/stm/ +obj-$(CONFIG_HISI_PTT) += hwtracing/hisilicon/ obj-$(CONFIG_ANDROID) += android/ obj-$(CONFIG_NVMEM) += nvmem/ obj-$(CONFIG_FPGA)+= fpga/ diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig index 13085835a636..e3796b17541a 100644 --- a/drivers/hwtracing/Kconfig +++ b/drivers/hwtracing/Kconfig @@ -5,4 +5,6 @@ source "drivers/hwtracing/stm/Kconfig" source "drivers/hwtracing/intel_th/Kconfig" +source "drivers/hwtracing/hisilicon/Kconfig" + endmenu diff --git a/drivers/hwtracing/hisilicon/Kconfig b/drivers/hwtracing/hisilicon/Kconfig new file mode 100644 index ..9c3ab80d99fe --- /dev/null +++ b/drivers/hwtracing/hisilicon/Kconfig @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: GPL-2.0-only +config HISI_PTT + tristate "HiSilicon PCIe Tune and Trace Device" + depends on PCI && HAS_DMA && HAS_IOMEM >>> >>> What Arm architecture are you aiming this for? Can it be found on both >>> armv7 >>> and armv8? >>> >> >> currently the device will appear on Kunpeng 930, which is armv8. > > Ok, then please make it dependent on armv8. > sure. >> + help +HiSilicon PCIe Tune and Trace Device exist as a PCIe RCiEP +device, provides support for PCIe traffic tuning and +tracing TLP headers to the memory. >>> >>> Please indicate what the name of the driver will be when compiled as a >>> module. >>> >> >> sure. will add the module's name here. >> diff --git a/drivers/hwtracing/hisilicon/Makefile b/drivers/hwtracing/hisilicon/Makefile new file mode 100644 index ..908c09a98161 --- /dev/null +++ b/drivers/hwtracing/hisilicon/Makefile @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o diff --git
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On 2021/11/25 23:49, Robin Murphy wrote:
> On 2021-11-18 09:01, Yicong Yang via iommu wrote:
>> Hi Robin,
>>
>> On 2021/11/16 19:37, Yicong Yang wrote:
>>> On 2021/11/16 18:56, Robin Murphy wrote:
On 2021-11-16 09:06, Yicong Yang via iommu wrote:
[...]
> +/*
> + * Get RMR address if provided by the firmware.
> + * Return 0 if the IOMMU doesn't present or the policy of the
> + * IOMMU domain is passthrough or we get a usable RMR region.
> + * Otherwise a negative value is returned.
> + */
> +static int hisi_ptt_get_rmr(struct hisi_ptt *hisi_ptt)
> +{
> + struct pci_dev *pdev = hisi_ptt->pdev;
> + struct iommu_domain *iommu_domain;
> + struct iommu_resv_region *region;
> + LIST_HEAD(list);
> +
> + /*
> + * Use direct DMA if IOMMU does not present or the policy of the
> + * IOMMU domain is passthrough.
> + */
> + iommu_domain = iommu_get_domain_for_dev(>dev);
> + if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY)
> + return 0;
> +
> + iommu_get_resv_regions(>dev, );
> + list_for_each_entry(region, , list)
> + if (region->type == IOMMU_RESV_DIRECT &&
> + region->length >= HISI_PTT_TRACE_BUFFER_SIZE) {
> + hisi_ptt->trace_ctrl.has_rmr = true;
> + hisi_ptt->trace_ctrl.rmr_addr = region->start;
> + hisi_ptt->trace_ctrl.rmr_length = region->length;
> + break;
> + }
> +
> + iommu_put_resv_regions(>dev, );
> + return hisi_ptt->trace_ctrl.has_rmr ? 0 : -ENOMEM;
> +}
No.
The whole point of RMRs is for devices that are already configured to
access the given address range in a manner beyond the kernel's control. If
you can do this, it proves that you should not have an RMR in the first
place.
The notion of a kernel driver explicitly configuring its device to DMA
into any random RMR that looks big enough is so egregiously wrong that I'm
almost lost for words...
>>>
>>> our bios will reserve such a region and reported it through iort. the
>>> device will write to the region and in the driver we need to access the
>>> region
>>> to get the traced data. the region is reserved exclusively and will not be
>>> accessed by kernel or other devices.
>>>
>>> is it ok to let bios configure the address to the device and from CPU side
>>> we just read it?
>>>
>>
>> Any suggestion? Is this still an issue you concern if we move the
>> configuration of the device address to BIOS and just read from the CPU side?
>
> If the firmware configures the device so that it's actively tracing and
> writing out to memory while the kernel boots, then that is a valid reason to
> have an RMR. However what you're doing in the driver is still complete
> nonsense. As far as I can follow, the way it's working is this:
>
> - At probe time, the initial state of the hardware is entirely ignored. If it
> *is* already active, there appears to be a fun chance of crashing if
> TRACE_INT_MASK is clear and an interrupt happens to fire before anyone has
> got round to calling perf_aux_output_begin() to make trace_ctrl.handle.rb
> non-NULL.
>
> - Later, once the user starts a tracing session, a buffer is set up *either*
> as a completely normal DMA allocation, or by memremap()ing some random IOVA
> carveout which may or may not be whatever memory the firmware was tracing to.
>
> - The hardware is then reset and completely reprogrammed to use the new
> buffer, again without any consideration of its previous state (other than
> possibly timing out and failing if it's already running and that means it
> never goes idle).
>
> Therefore the driver does not seem to respect any prior configuration of the
> device by firmware, does not seem to expect it to be running at boot time,
> does not seem to have any way to preserve and export any trace data captured
> in an RMR if it *was* running at boot time, and thus without loss of
> generality could simply use the dma_alloc_coherent() path all the time. Am I
> missing anything?
>
Thanks for the further explanation and I think I understand your concerns more
clearer.
The trace is not supposed to begin by the firmware at boot time. Due to some
hardware restriction, the device cannot trace with non-identical mapping.
So we'd like to use RMR to make the device work when the dma mapping is
non-identical. Thus we check here to decide whether to use RMR or not: if the
iommu
is not presented or in the passthrough mode, we can use direct DMA by
dma_alloc_coherent(); if the iommu is present and the mode is not passthrough,
we try
to retrieve RMR or we fail the probe. The firmware is expected to reserve a
range of memory and reports it to the driver and is not expected to configure
the trace and do boot time tracing.
> As things stand, RMRs are not yet
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
[...]
>
> > >> +
> > >> +#define HISI_PTT_TRACE_DMA_IRQ 0
> > >> +#define HISI_PTT_TRACE_BUFLETS_CNT 4
> > >> +#define HISI_PTT_TRACE_BUFLET_SIZE SZ_4M
> > >> +#define HISI_PTT_TRACE_BUFFER_SIZE
> > >> (HISI_PTT_TRACE_BUFLET_SIZE * \
> > >> +
> > >> HISI_PTT_TRACE_BUFLETS_CNT)
> > >> +#define HISI_PTT_FILTER_UPDATE_FIFO_SIZE16
> > >> +
> > >> +/* Delay time for filter updating work */
> > >> +#define HISI_PTT_WORK_DELAY_MS 100UL
> > >> +/* Wait time for DMA hardware to reset */
> > >> +#define HISI_PTT_RESET_WAIT_MS 1000UL
> > >> +/* Poll timeout and interval for waiting hardware work to finish */
> > >> +#define HISI_PTT_WAIT_TIMEOUT_US100UL
> > >> +#define HISI_PTT_WAIT_POLL_INTERVAL_US 100UL
> > >> +
> > >> +#define HISI_PCIE_CORE_PORT_ID(devfn) (PCI_FUNC(devfn) << 1)
> > >> +
> > >> +enum hisi_ptt_trace_status {
> > >> +HISI_PTT_TRACE_STATUS_OFF = 0,
> > >> +HISI_PTT_TRACE_STATUS_ON,
> > >> +};
> > >> +
> > >> +struct hisi_ptt_dma_buflet {
> > >
> > > I'm not sure what a "buflet" is... Probably best to just call this
> > > hisi_ptt_dma_buffer" if it pertains to a buffer. On that note it is hard
> > > to
> > > know due to the lack of proper structure documentation. Please have a
> > > look at
> > > how "coresight_device" is documented. The same applies to the rest of the
> > > structures declared below.
> > >
> >
> > It's mentioned in section 5 of hisi_ptt.rst as "Driver will allocate each
> > DMA buffer
> > (we call it buflet) of 4MiB ...".
>
> I just noticed the documentation in patch 5 - I will read it before continuing
> with this set.
>
As promised I read the documentation and it didn't do much to alliviate my
concern about the terminology. If a "buflet" is a DMA buffer than simply call
it a DMA buffer, regardless of the size.
The size of this patchset makes it hard enough to review as it is... On top
of things reviewers have to remember that PTT means "PCIe tune and trace
device", tune means "PCIe link's events" and trace means "TLP headers".
But we also have to keep in mind that a buflet is really just a DMA buffer...
I will not continue reviewing this set. Please fix the terminology and spin
another revision. Doing so I suggest you trim down the submission to the
absolute minimum feature set this driver should provide - enhancements can be
made later when a foundation has been laid out.
Robin has pointed out what seems to be serious problems with the
implemenation. That will also have to be sorted out for the next iteration.
Regards,
Mathieu
> >
> > The total DMA buffer of PTT device is divided into 4 parts, and each part
> > is called
> > a 'buflet', which means a small buffer (there exists other words with
> > similiar
> > format: droplet, wavelet, ...).
> >
> > The device is designed like this to make sure we won't lose any data when
> > we have to change
> > the buffer address, the device can continue to write to the next buflet and
> > don't
> > neet to pause.
> >
> > >> +struct list_head list;
> > >> +unsigned int size;
> > >> +dma_addr_t dma;
> > >> +
> > >> +/*
> > >> + * The address of the buflet holding the trace data.
> > >> + * See Documentation/trace/hisi-ptt.rst for the details
> > >
> > > As of this writing, hisi-ptt.rst doesn't exist.
> > >
> >
> > The comment intends to direct the user to the documentation if the user
> > want to know details about the data format. The data format is decribed
> > in the section 4 of the doc.
> >
> > >> + * of the data format.
> > >> + */
> > >> +void *addr;
> > >> +int index;
> > >> +};
> > >> +
> > >> +struct hisi_ptt_trace_ctrl {
> > >> +enum hisi_ptt_trace_status status;
> > >> +struct perf_output_handle handle;
> > >> +struct list_head trace_buf;
> > >> +/*
> > >> + * The index of the buffer which trace data
> > >> + * currently is writing to.
> > >> + */
> > >> +u32 buf_index;
> > >> +
> > >> +int default_cpu;
> > >> +u32 buflet_size;
> > >> +bool is_port;
> > >> +u32 format:1; /* Format of the traced TLP headers */
> > >> +u32 type:4; /* Type of the TLP headers to trace */
> > >> +u32 direction:2;/* Direction of the TLP headers to
> > >> trace */
> > >> +u32 filter:16; /* Root port or Requester to filter the
> > >> TLP headers */
> > >> +
> > >> +phys_addr_t rmr_addr;
> > >> +size_t rmr_length;
> > >> +bool has_rmr;
> > >> +};
> > >> +
> > >> +struct hisi_ptt_filter_desc {
> > >> +struct list_head list;
> > >> +struct pci_dev *pdev;
> > >> +u16 val;
> > >> +};
> > >> +
> > >> +struct hisi_ptt_pmu_buf {
> > >> +size_t length;
> > >> +int
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On Thu, Nov 25, 2021 at 04:39:46PM +0800, Yicong Yang wrote: > Hi Mathieu, > > Thanks for the comments! Replies inline. > > On 2021/11/25 2:51, Mathieu Poirier wrote: > > Hi Yicong, > > > > On Tue, Nov 16, 2021 at 05:06:21PM +0800, Yicong Yang wrote: > >> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex > >> integrated Endpoint(RCiEP) device, providing the capability > >> to dynamically monitor and tune the PCIe traffic(tune), > >> and trace the TLP headers(trace). > >> > > > > Is there a reason to put "tune" and "trace" are whithin parentheses? > > > > yeah. I want to use these single word to denote the related function, and > place them in the parantheses near the function's description to make sure > readers can match them. > Probably a better idea to simply write "TLP headers" and "PCIe traffic" when referring to them. Otherwise it is very confusing because the reader doesn't know if you mean the acronym or the real definition of "tune" and "trace". > >> Add the driver for the device to enable the trace function. The driver > >> will create PMU device for each PTT device, and users can start trace > >> through perf command. > >> > >> Signed-off-by: Yicong Yang > >> --- > >> drivers/Makefile |1 + > >> drivers/hwtracing/Kconfig |2 + > >> drivers/hwtracing/hisilicon/Kconfig|8 + > >> drivers/hwtracing/hisilicon/Makefile |2 + > >> drivers/hwtracing/hisilicon/hisi_ptt.c | 1146 > > > > Not sure about the "hisilicon" directory. Right now we have "coresight", > > "intel_th" and "stm" - they concentrate on the technology rather than the > > vendor. I think "ptt" would be just fine: > > > > drivers/hwtracing/ptt/Kconfig > > drivers/hwtracing/ptt/Makefile > > drivers/hwtracing/ptt/hisi_ptt.c > > > > That way if other vendors want to introduce support for the same kind of > > technology we can simply put them under "drivers/hwtracing/ptt/" and things > > are > > still accurate. > > > > sure. sounds sensible. will rename the directory to "ptt" if no objection. > > > > >> 5 files changed, 1159 insertions(+) > >> create mode 100644 drivers/hwtracing/hisilicon/Kconfig > >> create mode 100644 drivers/hwtracing/hisilicon/Makefile > >> create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c > >> > >> diff --git a/drivers/Makefile b/drivers/Makefile > >> index be5d40ae1488..bb0dc9b55ea2 100644 > >> --- a/drivers/Makefile > >> +++ b/drivers/Makefile > >> @@ -176,6 +176,7 @@ obj-$(CONFIG_USB4) += thunderbolt/ > >> obj-$(CONFIG_CORESIGHT) += hwtracing/coresight/ > >> obj-y += hwtracing/intel_th/ > >> obj-$(CONFIG_STM) += hwtracing/stm/ > >> +obj-$(CONFIG_HISI_PTT) += hwtracing/hisilicon/ > >> obj-$(CONFIG_ANDROID) += android/ > >> obj-$(CONFIG_NVMEM) += nvmem/ > >> obj-$(CONFIG_FPGA)+= fpga/ > >> diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig > >> index 13085835a636..e3796b17541a 100644 > >> --- a/drivers/hwtracing/Kconfig > >> +++ b/drivers/hwtracing/Kconfig > >> @@ -5,4 +5,6 @@ source "drivers/hwtracing/stm/Kconfig" > >> > >> source "drivers/hwtracing/intel_th/Kconfig" > >> > >> +source "drivers/hwtracing/hisilicon/Kconfig" > >> + > >> endmenu > >> diff --git a/drivers/hwtracing/hisilicon/Kconfig > >> b/drivers/hwtracing/hisilicon/Kconfig > >> new file mode 100644 > >> index ..9c3ab80d99fe > >> --- /dev/null > >> +++ b/drivers/hwtracing/hisilicon/Kconfig > >> @@ -0,0 +1,8 @@ > >> +# SPDX-License-Identifier: GPL-2.0-only > >> +config HISI_PTT > >> + tristate "HiSilicon PCIe Tune and Trace Device" > >> + depends on PCI && HAS_DMA && HAS_IOMEM > > > > What Arm architecture are you aiming this for? Can it be found on both > > armv7 > > and armv8? > > > > currently the device will appear on Kunpeng 930, which is armv8. Ok, then please make it dependent on armv8. > > >> + help > >> +HiSilicon PCIe Tune and Trace Device exist as a PCIe RCiEP > >> +device, provides support for PCIe traffic tuning and > >> +tracing TLP headers to the memory. > > > > Please indicate what the name of the driver will be when compiled as a > > module. > > > > sure. will add the module's name here. > > >> diff --git a/drivers/hwtracing/hisilicon/Makefile > >> b/drivers/hwtracing/hisilicon/Makefile > >> new file mode 100644 > >> index ..908c09a98161 > >> --- /dev/null > >> +++ b/drivers/hwtracing/hisilicon/Makefile > >> @@ -0,0 +1,2 @@ > >> +# SPDX-License-Identifier: GPL-2.0 > >> +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o > >> diff --git a/drivers/hwtracing/hisilicon/hisi_ptt.c > >> b/drivers/hwtracing/hisilicon/hisi_ptt.c > >> new file mode 100644 > >> index ..e11e9b6cc2a8 > >> --- /dev/null > >> +++ b/drivers/hwtracing/hisilicon/hisi_ptt.c > >> @@ -0,0 +1,1146 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + *
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On 2021-11-18 09:01, Yicong Yang via iommu wrote:
Hi Robin,
On 2021/11/16 19:37, Yicong Yang wrote:
On 2021/11/16 18:56, Robin Murphy wrote:
On 2021-11-16 09:06, Yicong Yang via iommu wrote:
[...]
+/*
+ * Get RMR address if provided by the firmware.
+ * Return 0 if the IOMMU doesn't present or the policy of the
+ * IOMMU domain is passthrough or we get a usable RMR region.
+ * Otherwise a negative value is returned.
+ */
+static int hisi_ptt_get_rmr(struct hisi_ptt *hisi_ptt)
+{
+ struct pci_dev *pdev = hisi_ptt->pdev;
+ struct iommu_domain *iommu_domain;
+ struct iommu_resv_region *region;
+ LIST_HEAD(list);
+
+ /*
+ * Use direct DMA if IOMMU does not present or the policy of the
+ * IOMMU domain is passthrough.
+ */
+ iommu_domain = iommu_get_domain_for_dev(>dev);
+ if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY)
+ return 0;
+
+ iommu_get_resv_regions(>dev, );
+ list_for_each_entry(region, , list)
+ if (region->type == IOMMU_RESV_DIRECT &&
+ region->length >= HISI_PTT_TRACE_BUFFER_SIZE) {
+ hisi_ptt->trace_ctrl.has_rmr = true;
+ hisi_ptt->trace_ctrl.rmr_addr = region->start;
+ hisi_ptt->trace_ctrl.rmr_length = region->length;
+ break;
+ }
+
+ iommu_put_resv_regions(>dev, );
+ return hisi_ptt->trace_ctrl.has_rmr ? 0 : -ENOMEM;
+}
No.
The whole point of RMRs is for devices that are already configured to access
the given address range in a manner beyond the kernel's control. If you can do
this, it proves that you should not have an RMR in the first place.
The notion of a kernel driver explicitly configuring its device to DMA into any
random RMR that looks big enough is so egregiously wrong that I'm almost lost
for words...
our bios will reserve such a region and reported it through iort. the device
will write to the region and in the driver we need to access the region
to get the traced data. the region is reserved exclusively and will not be
accessed by kernel or other devices.
is it ok to let bios configure the address to the device and from CPU side we
just read it?
Any suggestion? Is this still an issue you concern if we move the
configuration of the device address to BIOS and just read from the CPU side?
If the firmware configures the device so that it's actively tracing and
writing out to memory while the kernel boots, then that is a valid
reason to have an RMR. However what you're doing in the driver is still
complete nonsense. As far as I can follow, the way it's working is this:
- At probe time, the initial state of the hardware is entirely ignored.
If it *is* already active, there appears to be a fun chance of crashing
if TRACE_INT_MASK is clear and an interrupt happens to fire before
anyone has got round to calling perf_aux_output_begin() to make
trace_ctrl.handle.rb non-NULL.
- Later, once the user starts a tracing session, a buffer is set up
*either* as a completely normal DMA allocation, or by memremap()ing some
random IOVA carveout which may or may not be whatever memory the
firmware was tracing to.
- The hardware is then reset and completely reprogrammed to use the new
buffer, again without any consideration of its previous state (other
than possibly timing out and failing if it's already running and that
means it never goes idle).
Therefore the driver does not seem to respect any prior configuration of
the device by firmware, does not seem to expect it to be running at boot
time, does not seem to have any way to preserve and export any trace
data captured in an RMR if it *was* running at boot time, and thus
without loss of generality could simply use the dma_alloc_coherent()
path all the time. Am I missing anything?
As things stand, RMRs are not yet supported upstream (FYI we're still
working on fixing the spec...), so the code above is at best dead, and
at worst actively wrong. Furthermore, if the expected usage model *is*
that the kernel driver completely resets and reprograms the hardware,
then even if there is an RMR for boot-time tracing I would rather expect
it to be flagged as remappable, and thus potentially end up as an
IOMMU_RESV_DIRECT_RELAXABLE reservation which you wouldn't match anyway.
And after all that, if you really do have a genuine need to respect and
preserve prior firmware configuration of the device, then I would surely
expect to see the driver actually doing exactly that. Presumably: at
probe time, look at TRACE_CTRL; if the device is already configured,
read out that configuration - especially including TRACE_ADDR_* - and
make sure to reuse it. Not go off on a tangent blindly poking into
internal IOMMU API abstractions in the vain hope that the first thing
you find happens to be sort-of-related to the information that you
actually care about.
Thanks,
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
Hi Mathieu, Thanks for the comments! Replies inline. On 2021/11/25 2:51, Mathieu Poirier wrote: > Hi Yicong, > > On Tue, Nov 16, 2021 at 05:06:21PM +0800, Yicong Yang wrote: >> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex >> integrated Endpoint(RCiEP) device, providing the capability >> to dynamically monitor and tune the PCIe traffic(tune), >> and trace the TLP headers(trace). >> > > Is there a reason to put "tune" and "trace" are whithin parentheses? > yeah. I want to use these single word to denote the related function, and place them in the parantheses near the function's description to make sure readers can match them. >> Add the driver for the device to enable the trace function. The driver >> will create PMU device for each PTT device, and users can start trace >> through perf command. >> >> Signed-off-by: Yicong Yang >> --- >> drivers/Makefile |1 + >> drivers/hwtracing/Kconfig |2 + >> drivers/hwtracing/hisilicon/Kconfig|8 + >> drivers/hwtracing/hisilicon/Makefile |2 + >> drivers/hwtracing/hisilicon/hisi_ptt.c | 1146 > > Not sure about the "hisilicon" directory. Right now we have "coresight", > "intel_th" and "stm" - they concentrate on the technology rather than the > vendor. I think "ptt" would be just fine: > > drivers/hwtracing/ptt/Kconfig > drivers/hwtracing/ptt/Makefile > drivers/hwtracing/ptt/hisi_ptt.c > > That way if other vendors want to introduce support for the same kind of > technology we can simply put them under "drivers/hwtracing/ptt/" and things > are > still accurate. > sure. sounds sensible. will rename the directory to "ptt" if no objection. > >> 5 files changed, 1159 insertions(+) >> create mode 100644 drivers/hwtracing/hisilicon/Kconfig >> create mode 100644 drivers/hwtracing/hisilicon/Makefile >> create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c >> >> diff --git a/drivers/Makefile b/drivers/Makefile >> index be5d40ae1488..bb0dc9b55ea2 100644 >> --- a/drivers/Makefile >> +++ b/drivers/Makefile >> @@ -176,6 +176,7 @@ obj-$(CONFIG_USB4) += thunderbolt/ >> obj-$(CONFIG_CORESIGHT) += hwtracing/coresight/ >> obj-y += hwtracing/intel_th/ >> obj-$(CONFIG_STM) += hwtracing/stm/ >> +obj-$(CONFIG_HISI_PTT) += hwtracing/hisilicon/ >> obj-$(CONFIG_ANDROID) += android/ >> obj-$(CONFIG_NVMEM) += nvmem/ >> obj-$(CONFIG_FPGA) += fpga/ >> diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig >> index 13085835a636..e3796b17541a 100644 >> --- a/drivers/hwtracing/Kconfig >> +++ b/drivers/hwtracing/Kconfig >> @@ -5,4 +5,6 @@ source "drivers/hwtracing/stm/Kconfig" >> >> source "drivers/hwtracing/intel_th/Kconfig" >> >> +source "drivers/hwtracing/hisilicon/Kconfig" >> + >> endmenu >> diff --git a/drivers/hwtracing/hisilicon/Kconfig >> b/drivers/hwtracing/hisilicon/Kconfig >> new file mode 100644 >> index ..9c3ab80d99fe >> --- /dev/null >> +++ b/drivers/hwtracing/hisilicon/Kconfig >> @@ -0,0 +1,8 @@ >> +# SPDX-License-Identifier: GPL-2.0-only >> +config HISI_PTT >> +tristate "HiSilicon PCIe Tune and Trace Device" >> +depends on PCI && HAS_DMA && HAS_IOMEM > > What Arm architecture are you aiming this for? Can it be found on both armv7 > and armv8? > currently the device will appear on Kunpeng 930, which is armv8. >> +help >> + HiSilicon PCIe Tune and Trace Device exist as a PCIe RCiEP >> + device, provides support for PCIe traffic tuning and >> + tracing TLP headers to the memory. > > Please indicate what the name of the driver will be when compiled as a module. > sure. will add the module's name here. >> diff --git a/drivers/hwtracing/hisilicon/Makefile >> b/drivers/hwtracing/hisilicon/Makefile >> new file mode 100644 >> index ..908c09a98161 >> --- /dev/null >> +++ b/drivers/hwtracing/hisilicon/Makefile >> @@ -0,0 +1,2 @@ >> +# SPDX-License-Identifier: GPL-2.0 >> +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o >> diff --git a/drivers/hwtracing/hisilicon/hisi_ptt.c >> b/drivers/hwtracing/hisilicon/hisi_ptt.c >> new file mode 100644 >> index ..e11e9b6cc2a8 >> --- /dev/null >> +++ b/drivers/hwtracing/hisilicon/hisi_ptt.c >> @@ -0,0 +1,1146 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Driver for HiSilicon PCIe tune and trace device >> + * >> + * Copyright (c) 2021 HiSilicon Limited. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define HISI_PTT_TRACE_ADDR_SIZE0x0800 >> +#define HISI_PTT_TRACE_ADDR_BASE_LO_0 0x0810 >> +#define HISI_PTT_TRACE_ADDR_BASE_HI_0 0x0814 >> +#define HISI_PTT_TRACE_ADDR_STRIDE 0x8 >> +#define
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
Hi Yicong, On Tue, Nov 16, 2021 at 05:06:21PM +0800, Yicong Yang wrote: > HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex > integrated Endpoint(RCiEP) device, providing the capability > to dynamically monitor and tune the PCIe traffic(tune), > and trace the TLP headers(trace). > Is there a reason to put "tune" and "trace" are whithin parentheses? > Add the driver for the device to enable the trace function. The driver > will create PMU device for each PTT device, and users can start trace > through perf command. > > Signed-off-by: Yicong Yang > --- > drivers/Makefile |1 + > drivers/hwtracing/Kconfig |2 + > drivers/hwtracing/hisilicon/Kconfig|8 + > drivers/hwtracing/hisilicon/Makefile |2 + > drivers/hwtracing/hisilicon/hisi_ptt.c | 1146 Not sure about the "hisilicon" directory. Right now we have "coresight", "intel_th" and "stm" - they concentrate on the technology rather than the vendor. I think "ptt" would be just fine: drivers/hwtracing/ptt/Kconfig drivers/hwtracing/ptt/Makefile drivers/hwtracing/ptt/hisi_ptt.c That way if other vendors want to introduce support for the same kind of technology we can simply put them under "drivers/hwtracing/ptt/" and things are still accurate. > 5 files changed, 1159 insertions(+) > create mode 100644 drivers/hwtracing/hisilicon/Kconfig > create mode 100644 drivers/hwtracing/hisilicon/Makefile > create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c > > diff --git a/drivers/Makefile b/drivers/Makefile > index be5d40ae1488..bb0dc9b55ea2 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -176,6 +176,7 @@ obj-$(CONFIG_USB4)+= thunderbolt/ > obj-$(CONFIG_CORESIGHT) += hwtracing/coresight/ > obj-y+= hwtracing/intel_th/ > obj-$(CONFIG_STM)+= hwtracing/stm/ > +obj-$(CONFIG_HISI_PTT) += hwtracing/hisilicon/ > obj-$(CONFIG_ANDROID)+= android/ > obj-$(CONFIG_NVMEM) += nvmem/ > obj-$(CONFIG_FPGA) += fpga/ > diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig > index 13085835a636..e3796b17541a 100644 > --- a/drivers/hwtracing/Kconfig > +++ b/drivers/hwtracing/Kconfig > @@ -5,4 +5,6 @@ source "drivers/hwtracing/stm/Kconfig" > > source "drivers/hwtracing/intel_th/Kconfig" > > +source "drivers/hwtracing/hisilicon/Kconfig" > + > endmenu > diff --git a/drivers/hwtracing/hisilicon/Kconfig > b/drivers/hwtracing/hisilicon/Kconfig > new file mode 100644 > index ..9c3ab80d99fe > --- /dev/null > +++ b/drivers/hwtracing/hisilicon/Kconfig > @@ -0,0 +1,8 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +config HISI_PTT > + tristate "HiSilicon PCIe Tune and Trace Device" > + depends on PCI && HAS_DMA && HAS_IOMEM What Arm architecture are you aiming this for? Can it be found on both armv7 and armv8? > + help > + HiSilicon PCIe Tune and Trace Device exist as a PCIe RCiEP > + device, provides support for PCIe traffic tuning and > + tracing TLP headers to the memory. Please indicate what the name of the driver will be when compiled as a module. > diff --git a/drivers/hwtracing/hisilicon/Makefile > b/drivers/hwtracing/hisilicon/Makefile > new file mode 100644 > index ..908c09a98161 > --- /dev/null > +++ b/drivers/hwtracing/hisilicon/Makefile > @@ -0,0 +1,2 @@ > +# SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o > diff --git a/drivers/hwtracing/hisilicon/hisi_ptt.c > b/drivers/hwtracing/hisilicon/hisi_ptt.c > new file mode 100644 > index ..e11e9b6cc2a8 > --- /dev/null > +++ b/drivers/hwtracing/hisilicon/hisi_ptt.c > @@ -0,0 +1,1146 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for HiSilicon PCIe tune and trace device > + * > + * Copyright (c) 2021 HiSilicon Limited. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define HISI_PTT_TRACE_ADDR_SIZE 0x0800 > +#define HISI_PTT_TRACE_ADDR_BASE_LO_00x0810 > +#define HISI_PTT_TRACE_ADDR_BASE_HI_00x0814 > +#define HISI_PTT_TRACE_ADDR_STRIDE 0x8 > +#define HISI_PTT_TRACE_CTRL 0x0850 > +#define HISI_PTT_TRACE_CTRL_EN BIT(0) > +#define HISI_PTT_TRACE_CTRL_RSTBIT(1) > +#define HISI_PTT_TRACE_CTRL_RXTX_SEL GENMASK(3, 2) > +#define HISI_PTT_TRACE_CTRL_TYPE_SEL GENMASK(7, 4) > +#define HISI_PTT_TRACE_CTRL_DATA_FORMATBIT(14) > +#define HISI_PTT_TRACE_CTRL_FILTER_MODEBIT(15) > +#define HISI_PTT_TRACE_CTRL_TARGET_SEL GENMASK(31, 16) > +#define HISI_PTT_TRACE_INT_STAT 0x0890 > +#define HISI_PTT_TRACE_INT_STAT_MASK GENMASK(3, 0) > +#define HISI_PTT_TRACE_INT_MASK 0x0894 >
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
Hi Robin,
On 2021/11/16 19:37, Yicong Yang wrote:
> On 2021/11/16 18:56, Robin Murphy wrote:
>> On 2021-11-16 09:06, Yicong Yang via iommu wrote:
>> [...]
>>> +/*
>>> + * Get RMR address if provided by the firmware.
>>> + * Return 0 if the IOMMU doesn't present or the policy of the
>>> + * IOMMU domain is passthrough or we get a usable RMR region.
>>> + * Otherwise a negative value is returned.
>>> + */
>>> +static int hisi_ptt_get_rmr(struct hisi_ptt *hisi_ptt)
>>> +{
>>> + struct pci_dev *pdev = hisi_ptt->pdev;
>>> + struct iommu_domain *iommu_domain;
>>> + struct iommu_resv_region *region;
>>> + LIST_HEAD(list);
>>> +
>>> + /*
>>> + * Use direct DMA if IOMMU does not present or the policy of the
>>> + * IOMMU domain is passthrough.
>>> + */
>>> + iommu_domain = iommu_get_domain_for_dev(>dev);
>>> + if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY)
>>> + return 0;
>>> +
>>> + iommu_get_resv_regions(>dev, );
>>> + list_for_each_entry(region, , list)
>>> + if (region->type == IOMMU_RESV_DIRECT &&
>>> + region->length >= HISI_PTT_TRACE_BUFFER_SIZE) {
>>> + hisi_ptt->trace_ctrl.has_rmr = true;
>>> + hisi_ptt->trace_ctrl.rmr_addr = region->start;
>>> + hisi_ptt->trace_ctrl.rmr_length = region->length;
>>> + break;
>>> + }
>>> +
>>> + iommu_put_resv_regions(>dev, );
>>> + return hisi_ptt->trace_ctrl.has_rmr ? 0 : -ENOMEM;
>>> +}
>>
>> No.
>>
>> The whole point of RMRs is for devices that are already configured to access
>> the given address range in a manner beyond the kernel's control. If you can
>> do this, it proves that you should not have an RMR in the first place.
>>
>> The notion of a kernel driver explicitly configuring its device to DMA into
>> any random RMR that looks big enough is so egregiously wrong that I'm almost
>> lost for words...
>>
>
> our bios will reserve such a region and reported it through iort. the device
> will write to the region and in the driver we need to access the region
> to get the traced data. the region is reserved exclusively and will not be
> accessed by kernel or other devices.
>
> is it ok to let bios configure the address to the device and from CPU side we
> just read it?
>
Any suggestion? Is this still an issue you concern if we move the
configuration of the device address to BIOS and just read from the CPU side?
>
>
> .
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On 2021/11/16 18:56, Robin Murphy wrote:
> On 2021-11-16 09:06, Yicong Yang via iommu wrote:
> [...]
>> +/*
>> + * Get RMR address if provided by the firmware.
>> + * Return 0 if the IOMMU doesn't present or the policy of the
>> + * IOMMU domain is passthrough or we get a usable RMR region.
>> + * Otherwise a negative value is returned.
>> + */
>> +static int hisi_ptt_get_rmr(struct hisi_ptt *hisi_ptt)
>> +{
>> + struct pci_dev *pdev = hisi_ptt->pdev;
>> + struct iommu_domain *iommu_domain;
>> + struct iommu_resv_region *region;
>> + LIST_HEAD(list);
>> +
>> + /*
>> + * Use direct DMA if IOMMU does not present or the policy of the
>> + * IOMMU domain is passthrough.
>> + */
>> + iommu_domain = iommu_get_domain_for_dev(>dev);
>> + if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY)
>> + return 0;
>> +
>> + iommu_get_resv_regions(>dev, );
>> + list_for_each_entry(region, , list)
>> + if (region->type == IOMMU_RESV_DIRECT &&
>> + region->length >= HISI_PTT_TRACE_BUFFER_SIZE) {
>> + hisi_ptt->trace_ctrl.has_rmr = true;
>> + hisi_ptt->trace_ctrl.rmr_addr = region->start;
>> + hisi_ptt->trace_ctrl.rmr_length = region->length;
>> + break;
>> + }
>> +
>> + iommu_put_resv_regions(>dev, );
>> + return hisi_ptt->trace_ctrl.has_rmr ? 0 : -ENOMEM;
>> +}
>
> No.
>
> The whole point of RMRs is for devices that are already configured to access
> the given address range in a manner beyond the kernel's control. If you can
> do this, it proves that you should not have an RMR in the first place.
>
> The notion of a kernel driver explicitly configuring its device to DMA into
> any random RMR that looks big enough is so egregiously wrong that I'm almost
> lost for words...
>
our bios will reserve such a region and reported it through iort. the device
will write to the region and in the driver we need to access the region
to get the traced data. the region is reserved exclusively and will not be
accessed by kernel or other devices.
is it ok to let bios configure the address to the device and from CPU side we
just read it?
Thanks,
Yicong
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On 2021-11-16 09:06, Yicong Yang via iommu wrote:
[...]
+/*
+ * Get RMR address if provided by the firmware.
+ * Return 0 if the IOMMU doesn't present or the policy of the
+ * IOMMU domain is passthrough or we get a usable RMR region.
+ * Otherwise a negative value is returned.
+ */
+static int hisi_ptt_get_rmr(struct hisi_ptt *hisi_ptt)
+{
+ struct pci_dev *pdev = hisi_ptt->pdev;
+ struct iommu_domain *iommu_domain;
+ struct iommu_resv_region *region;
+ LIST_HEAD(list);
+
+ /*
+* Use direct DMA if IOMMU does not present or the policy of the
+* IOMMU domain is passthrough.
+*/
+ iommu_domain = iommu_get_domain_for_dev(>dev);
+ if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY)
+ return 0;
+
+ iommu_get_resv_regions(>dev, );
+ list_for_each_entry(region, , list)
+ if (region->type == IOMMU_RESV_DIRECT &&
+ region->length >= HISI_PTT_TRACE_BUFFER_SIZE) {
+ hisi_ptt->trace_ctrl.has_rmr = true;
+ hisi_ptt->trace_ctrl.rmr_addr = region->start;
+ hisi_ptt->trace_ctrl.rmr_length = region->length;
+ break;
+ }
+
+ iommu_put_resv_regions(>dev, );
+ return hisi_ptt->trace_ctrl.has_rmr ? 0 : -ENOMEM;
+}
No.
The whole point of RMRs is for devices that are already configured to
access the given address range in a manner beyond the kernel's control.
If you can do this, it proves that you should not have an RMR in the
first place.
The notion of a kernel driver explicitly configuring its device to DMA
into any random RMR that looks big enough is so egregiously wrong that
I'm almost lost for words...
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
