| Message ID | 1455182127-17551-3-git-send-email-Suravee.Suthikulpanit@amd.com |
|---|---|
| State | New |
| Headers | show |
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
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( 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)) + return -EINVAL; + /* Init null attributes */ perf_iommu->null_group = NULL; perf_iommu->pmu.attr_groups = perf_iommu->attr_groups; @@ -460,8 +457,8 @@ static __init int _init_perf_amd_iommu( amd_iommu_pc_exit(); } else { pr_info("perf: amd_iommu: Detected. (%d banks, %d counters/bank)\n", - amd_iommu_pc_get_max_banks(IOMMU_BASE_DEVID), - amd_iommu_pc_get_max_counters(IOMMU_BASE_DEVID)); + amd_iommu_pc_get_max_banks(), + amd_iommu_pc_get_max_counters()); } return ret; diff --git a/arch/x86/include/asm/perf/amd/iommu.h b/arch/x86/include/asm/perf/amd/iommu.h index 72f64b7..97e1be5 100644 --- a/arch/x86/include/asm/perf/amd/iommu.h +++ b/arch/x86/include/asm/perf/amd/iommu.h @@ -24,15 +24,12 @@ #define PC_MAX_SPEC_BNKS 64 #define PC_MAX_SPEC_CNTRS 16 -/* iommu pc reg masks*/ -#define IOMMU_BASE_DEVID 0x0000 - /* amd_iommu_init.c external support functions */ bool amd_iommu_pc_supported(void); -u8 amd_iommu_pc_get_max_banks(u16 devid); +u8 amd_iommu_pc_get_max_banks(void); -u8 amd_iommu_pc_get_max_counters(u16 devid); +u8 amd_iommu_pc_get_max_counters(void); int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value); 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))) + return 0; ret = iommu->max_banks; + } return ret; } @@ -2271,15 +2273,17 @@ bool amd_iommu_pc_supported(void) } EXPORT_SYMBOL(amd_iommu_pc_supported); -u8 amd_iommu_pc_get_max_counters(u16 devid) +u8 amd_iommu_pc_get_max_counters(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_counters || + (ret && (iommu->max_counters != ret))) + return 0; ret = iommu->max_counters; + } return ret; }