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]

Date:   Fri, 24 Feb 2017 00:43:19 +0700
From:   Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
To:     Peter Zijlstra <peterz@...radead.org>
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

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