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

[Peter Zijlstra]

On Fri, Feb 24, 2017 at 12:43:19AM +0700, Suravee Suthikulpanit wrote:

> >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.

Ah good, for a little while I was worried the BANK stuff came from
userspace; I misread extra_reg.config and extra_reg.reg, the former
being perf_event_attr::config1 and the latter holding the bank thing.

> >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.

Yes, adding a struct to that union is fine and clarifies things. And
just because I'm weird like that, there's a u8 hole after iommu_cntr.