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

[Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>]

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