From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751315AbdBWSoY (ORCPT ); Thu, 23 Feb 2017 13:44:24 -0500 Received: from merlin.infradead.org ([205.233.59.134]:49388 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751224AbdBWSoV (ORCPT ); Thu, 23 Feb 2017 13:44:21 -0500 Date: Thu, 23 Feb 2017 19:11:16 +0100 From: Peter Zijlstra To: Suravee Suthikulpanit Cc: linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, joro@8bytes.org, bp@alien8.de, mingo@redhat.com Subject: Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs Message-ID: <20170223181116.GJ6515@twins.programming.kicks-ass.net> References: <1484551416-5440-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1484551416-5440-10-git-send-email-Suravee.Suthikulpanit@amd.com> <20170125094653.GO6515@twins.programming.kicks-ass.net> <20170214123149.GV6515@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.