Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

From: Suravee Suthikulpanit
Date: Thu Feb 23 2017 - 12:43:53 EST


On 2/14/17 19:31, Peter Zijlstra wrote:
On Tue, Feb 07, 2017 at 08:57:52AM +0700, Suravee Suthikulpanit wrote:
But instead it looks like you get the counter form:

#define _GET_CNTR(ev) ((u8)(ev->hw.extra_reg.reg))

Which is absolutely insane.

So, the IOMMU counters are grouped into bank, and there could be
many banks. I use the extra_reg.reg to hold the bank and counter
indices. This will be used to program onto the counter configuration
register. This is handled in get_next_avail_iommu_bnk_cntr() and

But this is crazy. That's not what extra_regs are for.

Ok, I understand that we should not be using the extra_regs
since it is intended for other purposes. Please see more detail below.

Also, who cares about the banks, why is this exposed?

The bank and counter values are not exposed to the user-space.
The amd_iommu PMU only expose, csource, devid, domid, pasid, devid_mask,
domid_mask, and pasid_mask as event attributes.

That is, I would very much expect a linear range of counters. You can
always decompose this counter number if you really need to somewhere
down near the hardware accessors.

Actually, the counters are treated as linear range of counters. For example,
the IOMMU hardware has 2 banks with 4 counters/bank. So, we have total of 8
counters. The driver then assigns an index to each events when an event is added.
Here, the bank/counter are derived from the assigned index, and stored in
the perf_event as bank and counter values.

However, I have looked into reworking to not use the extra_regs, and I see
that the union in struct hw_perf_event currently contains various PMU-specific
structures (hardware, software, tracepoint, intel_cqm, itrace, amd_power,
and breakpoint).

For amd_iommu PMU, we need additional registers for holding amd_iommu-specific
parameters. So, it seems that we can just introduce amd_iommu-specific struct
instead of re-using the existing structure for hardware events.

I'm planning to add the following structure in the same union:

union {
struct { /* amd_iommu */
u8 iommu_csource;
u8 iommu_bank;
u8 iommu_cntr;
u16 iommu_devid;
u16 iommu_devid_msk;
u16 iommu_domid;
u16 iommu_domid_msk;
u32 iommu_pasid;
u32 iommu_pasid_msk;

Please let me know what you think, of if I am still missing your points.