Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580511 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs Peter Zijlstra <peterz@infradead.org> - 2017-02-14 13:40 +0100
    Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs Peter Zijlstra <peterz@infradead.org> - 2017-02-23 19:50 +0100

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

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. Also, who cares
about the banks, why is this exposed?

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.

[toc] | [next] | [standalone]

#1587082

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web