lkml.org 
[lkml]   [2016]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH V4 2/6] perf/amd/iommu: Modify functions to query max banks and counters
From
Date
Hi,

On 02/18/2016 06:11 PM, Borislav Petkov wrote:
> On Thu, Feb 11, 2016 at 04:15:23PM +0700, Suravee Suthikulpanit wrote:
>> Currently, amd_iommu_pc_get_max_[banks|counters]() require devid,
>> which should not be the case.
>
> Why?
>
> Commit message could use an explanation.
>
>> Also, these don't properly support
>> multi-IOMMU system.
>>
>> Current and future AMD systems with IOMMU that support perf counter
>
> "with an IOMMU that supports performance counters"
>
>> would likely contain homogeneous IOMMUs where multiple IOMMUs are
>
> What are homogeneous IOMMUs?

I intended to mean the same IOMMU version/capability for all IOMMUs in
the system.

>
>> availalbe. So, this patch modifies these function to iterate all IOMMU
>
> Please integrate a spellchecker in your patch creation workflow:
>
> s/availalbe/available/
>

Thanks. I have now rephrased and spell check the new commit message for
the V5.

>>
>> Reviewed-by: Joerg Roedel <jroedel@suse.de>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> ---
>> arch/x86/events/amd/iommu.c | 17 +++++++----------
>> arch/x86/include/asm/perf/amd/iommu.h | 7 ++-----
>> drivers/iommu/amd_iommu_init.c | 20 ++++++++++++--------
>> 3 files changed, 21 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
>> index 2f96072..debf22d 100644
>> --- a/arch/x86/events/amd/iommu.c
>> +++ b/arch/x86/events/amd/iommu.c
>> @@ -232,14 +232,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;
>> @@ -450,6 +442,11 @@ static __init int _init_perf_amd_iommu(
>
> Btw, that _init_perf_amd_iommu is unnecessarily split from
> amd_iommu_pc_init(). You should merge those two. But that's another
> patch. In that same cleanup patch you could do
>
> s/perf_iommu/pi/g
>
> or so because that perf_iommu local var is unnecesarily long and impairs
> readability.
>

Sure, I'll clean up both of these.

>> if (_init_events_attrs(perf_iommu) != 0)
>> pr_err("perf: amd_iommu: Only support raw events.\n");
>>
>> + perf_iommu->max_banks = amd_iommu_pc_get_max_banks();
>> + perf_iommu->max_counters = amd_iommu_pc_get_max_counters();
>> + if ((perf_iommu->max_banks == 0) || (perf_iommu->max_counters == 0))
>
> Simplify:
>
> if (!perf_iommu->max_banks ||
> !perf_iommu->max_counters)

Ok

[....]
>> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
>> index d30f4b2..a62b5ce 100644
>> --- a/drivers/iommu/amd_iommu_init.c
>> +++ b/drivers/iommu/amd_iommu_init.c
>> @@ -2251,15 +2251,17 @@ EXPORT_SYMBOL(amd_iommu_v2_supported);
>> *
>> ****************************************************************************/
>>
>> -u8 amd_iommu_pc_get_max_banks(u16 devid)
>> +u8 amd_iommu_pc_get_max_banks(void)
>> {
>> struct amd_iommu *iommu;
>> u8 ret = 0;
>>
>> - /* locate the iommu governing the devid */
>> - iommu = amd_iommu_rlookup_table[devid];
>> - if (iommu)
>> + for_each_iommu(iommu) {
>> + if (!iommu->max_banks ||
>> + (ret && (iommu->max_banks != ret)))
>
> What is that supposed to do?
>
> Check that the max_banks of a previous IOMMU are == to the max_banks of
> the current IOMMU?
>
> Why? Could definitely use a comment.

Current AMD IOMMU perf implementation assumes that all IOMMUs must have
the same value of max_counters. Therefore, this logic iterates through
all IOMMUs to check this. I'll add the comment here.

Thanks,
Suravee

\
 
 \ /
  Last update: 2016-02-22 06:21    [W:0.194 / U:1.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site