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

[Suravee Suthikulpanit]

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