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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170223181116.GJ6515@twins.programming.kicks-ass.net>
Date:   Thu, 23 Feb 2017 19:11:16 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
Cc:     linux-kernel@...r.kernel.org, iommu@...ts.linux-foundation.org,
        joro@...tes.org, bp@...en8.de, mingo@...hat.com
Subject: Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

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.