lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 13 Mar 2020 13:59:40 +0000 From: Mark Rutland <mark.rutland@....com> To: Robin Murphy <robin.murphy@....com> Cc: Qi Liu <liuqi115@...wei.com>, will@...nel.org, bhelgaas@...gle.com, linux-pci@...r.kernel.org, linuxarm@...wei.com, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH RFC] perf:Add driver for HiSilicon PCIe PMU On Fri, Mar 13, 2020 at 01:23:53PM +0000, Robin Murphy wrote: > On 2020-03-12 12:06 pm, Qi Liu wrote: > > From: Qi liu <liuqi115@...wei.com> [...] > > +#define HISI_PCIE_EVENT_SHIFT_M GENMASK(15, 0) > > +#define HISI_PCIE_SUBEVENT_SHIFT_M GENMASK(31, 16) > > +#define HISI_PCIE_SUBEVENT_SHIFT_S 16 > > +#define HISI_PCIE_PORT_SHIFT_M GENMASK(7, 0) > > +#define HISI_PCIE_FUNC_SHIFT_M GENMASK(15, 8) > > +#define HISI_PCIE_FUNC_SHIFT_S 8 > > So "SHIFT_S" means "shift" and "SHIFT_M" actually means "mask"? That's > unnecessarily confusing. Furthermore it might be helpful if there was a more > obvious distinction between hardware register fields and config fields. Also, If you use the FIELD_GET() and FIELD_PREP() helpers, you only need to define the mask. See <linux/bitfield.h>. > > +int hisi_pcie_pmu_event_init(struct perf_event *event) > > +{ > > + struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu); > > + struct hw_perf_event *hwc = &event->hw; > > + u32 subevent_id, event_id, func_id, port_id; > > + > > + if (event->attr.type != event->pmu->type) > > + return -ENOENT; > > + > > + /* > > + * We do not support sampling as the counters are all shared by all > > + * CPU cores in a CPU die(SCCL). Also we do not support attach to a > > Do the PCIe counters have anything to do with CPU clusters at all? > > > + * task(per-process mode) > > + */ > > + if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK) > > + return -EOPNOTSUPP; > > + > > + /* > > + * The uncore counters not specific to any CPU, so cannot > > + * support per-task > > + */ > > + if (event->cpu < 0) > > + return -EINVAL; > > + > > + /* > > + * Validate if the events in group does not exceed the > > + * available counters in hardware. > > + */ > > + if (!hisi_validate_event_group(event)) > > + return -EINVAL; > > + > > + event_id = event->attr.config && HISI_PCIE_EVENT_SHIFT_M; > > Really? Are you sure you've tested this properly? If you had: #define HISI_PCI_EVENT_ID GENMASK(15, 0) #define HISI_PCI_SUBEVENT_ID GENMASK(31, 16) ... here you could do: event_id = FIELD_GET(HISI_PCI_EVENT_ID, event->attr.config); > > > + subevent_id = (event->attr.config && HISI_PCIE_SUBEVENT_SHIFT_M) > > + >> HISI_PCIE_SUBEVENT_SHIFT_S; ... and: subevent_id = FIELD_GET(HISI_PCI_SUBEVENT_ID, event->attr.config); ... and so on for other fields. Thanks, Mark.
Powered by blists - more mailing lists