Messages in this thread | | | Subject | Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs | From | Suravee Suthikulpanit <> | Date | Fri, 24 Feb 2017 00:43:19 +0700 |
| |
Peter,
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 >> clear_avail_iommu_bnk_cntr(). > > 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.
Thanks, Suravee
| |