Message-ID: <31113797-29c5-1400-f7ac-bff79853b3fe@huawei.com>
Date:   Tue, 17 May 2022 16:09:45 +0800
From:   Yicong Yang <yangyicong@...wei.com>
To:     John Garry <john.garry@...wei.com>,
        Yicong Yang <yangyicong@...ilicon.com>,
        <gregkh@...uxfoundation.org>, <alexander.shishkin@...ux.intel.com>,
        <leo.yan@...aro.org>, <james.clark@....com>, <will@...nel.org>,
        <robin.murphy@....com>, <acme@...nel.org>,
        <jonathan.cameron@...wei.com>
CC:     <helgaas@...nel.org>, <lorenzo.pieralisi@....com>,
        <mathieu.poirier@...aro.org>, <suzuki.poulose@....com>,
        <mark.rutland@....com>, <joro@...tes.org>,
        <shameerali.kolothum.thodi@...wei.com>, <peterz@...radead.org>,
        <mingo@...hat.com>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-pci@...r.kernel.org>, <linux-perf-users@...r.kernel.org>,
        <iommu@...ts.linux-foundation.org>, <prime.zeng@...wei.com>,
        <liuqi115@...wei.com>, <zhangshaokun@...ilicon.com>,
        <linuxarm@...wei.com>
Subject: Re: [PATCH v8 2/8] hwtracing: hisi_ptt: Add trace function support
 for HiSilicon PCIe Tune and Trace device

On 2022/5/17 0:23, John Garry wrote:
> On 16/05/2022 13:52, 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 and trace the TLP headers.
>>
>> Add the driver for the device to enable the trace function. Register PMU
>> device of PTT trace, then users can use trace through perf command. The
>> driver makes use of perf AUX trace function and support the following
>> events to configure the trace:
>>
>> - filter: select Root port or Endpoint to trace
>> - type: select the type of traced TLP headers
>> - direction: select the direction of traced TLP headers
>> - format: select the data format of the traced TLP headers
>>
>> This patch initially add a basic driver of PTT trace.
> 
> Initially add basic trace support.
> 
>>
>> Signed-off-by: Yicong Yang <yangyicong@...ilicon.com>
> 
> Generally this looks ok, apart from nitpicking below, so, FWIW:
> Reviewed-by: John Garry <john.garry@...wei.com>
> >> ---
>>   drivers/Makefile                 |   1 +
>>   drivers/hwtracing/Kconfig        |   2 +
>>   drivers/hwtracing/ptt/Kconfig    |  12 +
>>   drivers/hwtracing/ptt/Makefile   |   2 +
>>   drivers/hwtracing/ptt/hisi_ptt.c | 964 +++++++++++++++++++++++++++++++
>>   drivers/hwtracing/ptt/hisi_ptt.h | 178 ++++++
>>   6 files changed, 1159 insertions(+)
>>   create mode 100644 drivers/hwtracing/ptt/Kconfig
>>   create mode 100644 drivers/hwtracing/ptt/Makefile
>>   create mode 100644 drivers/hwtracing/ptt/hisi_ptt.c
>>   create mode 100644 drivers/hwtracing/ptt/hisi_ptt.h
>>

[...]

>> +static int hisi_ptt_cpu_teardown(unsigned int cpu, struct hlist_node *node)
>> +{
>> +    struct hisi_ptt *hisi_ptt;
>> +    int target, src;
>> +
>> +    hisi_ptt = hlist_entry_safe(node, struct hisi_ptt, hotplug_node);
>> +    src = hisi_ptt->trace_ctrl.on_cpu;
>> +
>> +    if (!hisi_ptt->trace_ctrl.started || src != cpu)
>> +        return 0;
>> +
>> +    target = cpumask_any(cpumask_of_node(dev_to_node(&hisi_ptt->pdev->dev)));
>> +    if (target < nr_cpumask_bits) {
> 
> the comment for cpumask_any() hints to check against nr_cpu_ids - any specific reason to check against nr_cpumask_bits?
> 

here should be:
	if (target >= nr_cpumask_bits) {

will fix this up.

>> +        dev_err(hisi_ptt->hisi_ptt_pmu.dev, "no available cpu for perf context migration\n");
>> +        return 0;
>> +    }
>> +
>> +    perf_pmu_migrate_context(&hisi_ptt->hisi_ptt_pmu, src, target);
>> +    WARN_ON(irq_set_affinity(pci_irq_vector(hisi_ptt->pdev, HISI_PTT_TRACE_DMA_IRQ),
>> +                 cpumask_of(cpu)));
>> +    hisi_ptt->trace_ctrl.on_cpu = target;
>> +
>> +    return 0;
>> +}
>> +
>> +static int __init hisi_ptt_init(void)
>> +{
>> +    int ret;
>> +
>> +    ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, DRV_NAME, NULL,
>> +                      hisi_ptt_cpu_teardown);
>> +    if (ret < 0)
>> +        return ret;
>> +    hisi_ptt_pmu_online = ret;
>> +
>> +    ret = pci_register_driver(&hisi_ptt_driver);
>> +    if (ret)
>> +        cpuhp_remove_multi_state(hisi_ptt_pmu_online);
>> +
>> +    return ret;
>> +}
>> +module_init(hisi_ptt_init);
>> +
>> +static void __exit hisi_ptt_exit(void)
>> +{
>> +    pci_unregister_driver(&hisi_ptt_driver);
>> +    cpuhp_remove_multi_state(hisi_ptt_pmu_online);
>> +}
>> +module_exit(hisi_ptt_exit);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Yicong Yang <yangyicong@...ilicon.com>");
>> +MODULE_DESCRIPTION("Driver for HiSilicon PCIe tune and trace device");
>> diff --git a/drivers/hwtracing/ptt/hisi_ptt.h b/drivers/hwtracing/ptt/hisi_ptt.h
>> new file mode 100644
>> index 000000000000..2344e4195648
>> --- /dev/null
>> +++ b/drivers/hwtracing/ptt/hisi_ptt.h
>> @@ -0,0 +1,178 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Driver for HiSilicon PCIe tune and trace device
>> + *
>> + * Copyright (c) 2022 HiSilicon Technologies Co., Ltd.
>> + * Author: Yicong Yang <yangyicong@...ilicon.com>
>> + */
>> +
>> +#ifndef _HISI_PTT_H
>> +#define _HISI_PTT_H
>> +
>> +#include <linux/bits.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/list.h>
>> +#include <linux/pci.h>
>> +#include <linux/perf_event.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/types.h>
>> +
>> +#define DRV_NAME "hisi_ptt"
>> +
>> +/*
>> + * The definition of the device registers and register fields.
>> + */
>> +#define HISI_PTT_TRACE_ADDR_SIZE    0x0800
>> +#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 HISI_PTT_TRACE_CTRL        0x0850
>> +#define   HISI_PTT_TRACE_CTRL_EN    BIT(0)
>> +#define   HISI_PTT_TRACE_CTRL_RST    BIT(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_FORMAT    BIT(14)
>> +#define   HISI_PTT_TRACE_CTRL_FILTER_MODE    BIT(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
>> +#define HISI_PTT_TRACE_WR_STS        0x08a0
>> +#define   HISI_PTT_TRACE_WR_STS_WRITE    GENMASK(27, 0)
>> +#define   HISI_PTT_TRACE_WR_STS_BUFFER    GENMASK(29, 28)
>> +#define HISI_PTT_TRACE_STS        0x08b0
>> +#define   HISI_PTT_TRACE_IDLE        BIT(0)
>> +#define HISI_PTT_DEVICE_RANGE        0x0fe0
>> +#define   HISI_PTT_DEVICE_RANGE_UPPER    GENMASK(31, 16)
>> +#define   HISI_PTT_DEVICE_RANGE_LOWER    GENMASK(15, 0)
>> +#define HISI_PTT_LOCATION        0x0fe8
>> +#define   HISI_PTT_CORE_ID        GENMASK(15, 0)
>> +#define   HISI_PTT_SICL_ID        GENMASK(31, 16)
>> +
>> +/* Parameters of PTT trace DMA part. */
>> +#define HISI_PTT_TRACE_DMA_IRQ            0
>> +#define HISI_PTT_TRACE_BUF_CNT            4
>> +#define HISI_PTT_TRACE_BUF_SIZE            SZ_4M
>> +#define HISI_PTT_TRACE_TOTAL_BUF_SIZE        (HISI_PTT_TRACE_BUF_SIZE * \
>> +                         HISI_PTT_TRACE_BUF_CNT)
>> +/* Wait time for hardware DMA to reset */
>> +#define HISI_PTT_RESET_TIMEOUT_US    10UL
>> +#define HISI_PTT_RESET_POLL_INTERVAL_US    1UL
>> +/* Poll timeout and interval for waiting hardware work to finish */
>> +#define HISI_PTT_WAIT_TRACE_TIMEOUT_US    100UL
>> +#define HISI_PTT_WAIT_POLL_INTERVAL_US    10UL
>> +
>> +#define HISI_PCIE_CORE_PORT_ID(devfn)    (PCI_FUNC(devfn) << 1)
>> +
>> +/* Definition of the PMU configs */
>> +#define HISI_PTT_PMU_FILTER_IS_PORT    BIT(19)
>> +#define HISI_PTT_PMU_FILTER_VAL_MASK    GENMASK(15, 0)
>> +#define HISI_PTT_PMU_DIRECTION_MASK    GENMASK(23, 20)
>> +#define HISI_PTT_PMU_TYPE_MASK        GENMASK(31, 24)
>> +#define HISI_PTT_PMU_FORMAT_MASK    GENMASK(35, 32)
>> +
>> +/**
>> + * struct hisi_ptt_dma_buffer - Describe a single trace buffer of PTT trace.
>> + *                              The detail of the data format is described
>> + *                              in the documentation of PTT device.
>> + * @dma:   DMA address of this buffer visible to the device
>> + * @addr:  virtual address of this buffer visible to the cpu
>> + */
>> +struct hisi_ptt_dma_buffer {
>> +    dma_addr_t dma;
>> +    void *addr;
>> +};
>> +
>> +/**
>> + * struct hisi_ptt_trace_ctrl - Control and status of PTT trace
>> + * @trace_buf: array of the trace buffers for holding the trace data.
>> + *             the length will be HISI_PTT_TRACE_BUF_CNT.
>> + * @handle:    perf output handle of current trace session
>> + * @buf_index: the index of current using trace buffer
>> + * @on_cpu:    current tracing cpu
>> + * @started:   current trace status, true for started
>> + * @is_port:   whether we're tracing root port or not
>> + * @direction: direction of the TLP headers to trace
>> + * @filter:    filter value for tracing the TLP headers
>> + * @format:    format of the TLP headers to trace
>> + * @type:      type of the TLP headers to trace
>> + */
>> +struct hisi_ptt_trace_ctrl {
>> +    struct hisi_ptt_dma_buffer *trace_buf;
>> +    struct perf_output_handle handle;
>> +    u32 buf_index;
>> +    int on_cpu;
>> +    bool started;
>> +    bool is_port;
>> +    u32 direction:2;
>> +    u32 filter:16;
>> +    u32 format:1;
>> +    u32 type:4;
>> +};
>> +
>> +/**
>> + * struct hisi_ptt_filter_desc - Descriptor of the PTT trace filter
>> + * @list:    entry of this descriptor in the filter list
>> + * @is_port: the PCI device of the filter is a Root Port or not
>> + * @devid:   the PCI device's devid of the filter
>> + */
>> +struct hisi_ptt_filter_desc {
>> +    struct list_head list;
>> +    bool is_port;
>> +    u16 devid;
>> +};
>> +
>> +
>> +/**
>> + * struct hisi_ptt_pmu_buf - Descriptor of the AUX buffer of PTT trace
>> + * @length:   size of the AUX buffer
>> + * @nr_pages: number of pages of the AUX buffer
>> + * @base:     start address of AUX buffer
>> + * @pos:      position in the AUX buffer to commit traced data
>> + */
>> +struct hisi_ptt_pmu_buf {
>> +    size_t length;
>> +    int nr_pages;
>> +    void *base;
>> +    long pos;
>> +};
>> +
>> +/**
>> + * struct hisi_ptt - Per PTT device data
>> + * @trace_ctrl:   the control information of PTT trace
>> + * @hotplug_node: node for register cpu hotplug event
>> + * @hisi_ptt_pmu: the pum device of trace
>> + * @iobase:       base IO address of the device
>> + * @pdev:         pci_dev of this PTT device
>> + * @pmu_lock:     lock to serialize the perf process
>> + * @upper_bdf:    the upper BDF range of the PCI devices managed by this PTT device
>> + * @lower_bdf:    the lower BDF range of the PCI devices managed by this PTT device
>> + * @port_filters: the filter list of root ports
>> + * @req_filters:  the filter list of requester ID
>> + * @port_mask:    port mask of the managed root ports
>> + */
>> +struct hisi_ptt {
>> +    struct hisi_ptt_trace_ctrl trace_ctrl;
>> +    struct hlist_node hotplug_node;
>> +    struct pmu hisi_ptt_pmu;
>> +    void __iomem *iobase;
>> +    struct pci_dev *pdev;
>> +    spinlock_t pmu_lock;
>> +    u32 upper_bdf;
>> +    u32 lower_bdf;
>> +
>> +    /*
>> +     * The trace TLP headers can either be filtered by certain
>> +     * root port, or by the requester ID. Organize the filters
>> +     * by @port_filters and @req_filters here. The mask of all
>> +     * the valid ports is also cached for doing sanity check
>> +     * of user input.
>> +     */
>> +    struct list_head port_filters;
>> +    struct list_head req_filters;
>> +    u16 port_mask;
>> +};
>> +
>> +#define to_hisi_ptt(pmu) container_of(pmu, struct hisi_ptt, hisi_ptt_pmu)
>> +
>> +#endif /* _HISI_PTT_H */
> 
> .

Powered by blists - more mailing lists