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

[Borislav Petkov]

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@xxxxxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Joerg Roedel <joro@xxxxxxxxxx>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> ---
>  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.