linux-kernel - Re: [PATCH v8 5/9] perf/amd/iommu: Modify functions to query max banks and counters

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: <20170122195326.y5stvlexrobetfeq@pd.tnic>
Date:   Sun, 22 Jan 2017 20:53:26 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
Cc:     linux-kernel@...r.kernel.org, iommu@...ts.linux-foundation.org,
        joro@...tes.org, peterz@...radead.org, mingo@...hat.com
Subject: Re: [PATCH v8 5/9] perf/amd/iommu: Modify functions to query max
 banks and counters

On Mon, Jan 16, 2017 at 01:23:32AM -0600, Suravee Suthikulpanit wrote:
> Currently, amd_iommu_pc_get_max_[banks|counters]() use end-point
> device ID to locate an IOMMU and check the reported max banks/counters.
> The logic assumes that the IOMMU_BASE_DEVID belongs to the first IOMMU,
> and uses it to acquire a reference to the first IOMMU, which does not work
> on certain systems. Instead, we modify the function to take IOMMU index,
							     ^
s/we //							    an

> and use it to query the corresponded AMD IOMMU instance.

corresponding

> Note that we currently hard-code the IOMMU index to 0, since the current
> AMD IOMMU perf implementation only supports single IOMMU. Subsequent patch
					     ^		   ^
					     a		   A subsequent ...

> will add support for multi-IOMMU, and will use proper IOMMU index.
						^
s/multi-IOMMU/multiple IOMMUs/			a/the 

> 
> This patch also removes unnecessary function declaration in
> amd_iommu_proto.h.

This sentence can go away: never write "what" the patch is doing -
always "why" it is doing it.

> 
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: Joerg Roedel <joro@...tes.org>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
> ---
>  arch/x86/events/amd/iommu.c     | 17 +++++++----------
>  arch/x86/events/amd/iommu.h     |  7 ++-----
>  drivers/iommu/amd_iommu_init.c  | 36 ++++++++++++++++++++++--------------
>  drivers/iommu/amd_iommu_proto.h |  2 --
>  4 files changed, 31 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 3f1c18a..ec7e873 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -239,14 +239,6 @@ static int perf_iommu_event_init(struct perf_event *event)
>  		return -EINVAL;
>  	}
>  
> -	/* integrate with iommu base devid (0000), assume one iommu */
> -	perf_iommu->max_banks =
> -		amd_iommu_pc_get_max_banks(IOMMU_BASE_DEVID);
> -	perf_iommu->max_counters =
> -		amd_iommu_pc_get_max_counters(IOMMU_BASE_DEVID);
> -	if ((perf_iommu->max_banks == 0) || (perf_iommu->max_counters == 0))
> -		return -EINVAL;
> -
>  	/* update the hw_perf_event struct with the iommu config data */
>  	hwc->config = config;
>  	hwc->extra_reg.config = config1;
> @@ -453,6 +445,11 @@ static __init int _init_perf_amd_iommu(
>  		return ret;
>  	}
>  
> +	perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
> +	perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);

Align vertically on the "=".

> +	if (!perf_iommu->max_banks || !perf_iommu->max_counters)
> +		return -EINVAL;
> +
>  	perf_iommu->null_group = NULL;
>  	perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
>  
-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.