Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs
On Mon, Jan 16, 2017 at 01:23:36AM -0600, Suravee Suthikulpanit wrote: > From: Suravee Suthikulpanit> > Add multi-IOMMU support for perf by exposing an AMD IOMMU PMU > for each IOMMU found in the system via: > > /bus/event_source/devices/amd_iommu_x > > where x is the IOMMU index. This allows users to specify > different events to be programed onto performance counters "programmed" Please introduce a spellchecker into your patch creation workflow. > of each IOMMU. > > Cc: Peter Zijlstra > Cc: Borislav Petkov > Signed-off-by: Suravee Suthikulpanit > --- > arch/x86/events/amd/iommu.c | 114 > ++-- > 1 file changed, 67 insertions(+), 47 deletions(-) > > diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c > index 223c01d..38eafbf 100644 > --- a/arch/x86/events/amd/iommu.c > +++ b/arch/x86/events/amd/iommu.c > @@ -35,16 +35,21 @@ > #define _GET_PASID_MASK(ev) ((ev->hw.extra_reg.config >> 16) & 0xULL) > #define _GET_DOMID_MASK(ev) ((ev->hw.extra_reg.config >> 32) & 0xULL) > > -static struct perf_amd_iommu __perf_iommu; > +#define PERF_AMD_IOMMU_NAME_SZ 16 AMD_IOMMU_PMU_NAME_SIZE sounds more to the point to me. > struct perf_amd_iommu { > + struct list_head list; > struct pmu pmu; > + unsigned int idx; > + char name[PERF_AMD_IOMMU_NAME_SZ]; > u8 max_banks; > u8 max_counters; > u64 cntr_assign_mask; > raw_spinlock_t lock; > }; ... > @@ -253,30 +248,34 @@ static void perf_iommu_enable_event(struct perf_event > *ev) > u64 reg = 0ULL; > > reg = csource; > - amd_iommu_pc_set_reg(0, bank, cntr, > + amd_iommu_pc_set_reg(hwc->idx, bank, cntr, >IOMMU_PC_COUNTER_SRC_REG, ); > > reg = devid | (_GET_DEVID_MASK(ev) << 32); > if (reg) > reg |= BIT(31); > - amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_DEVID_MATCH_REG, ); > + amd_iommu_pc_set_reg(hwc->idx, bank, cntr, > + IOMMU_PC_DEVID_MATCH_REG, ); > > reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32); > if (reg) > reg |= BIT(31); > - amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_PASID_MATCH_REG, ); > + amd_iommu_pc_set_reg(hwc->idx, bank, cntr, > + IOMMU_PC_PASID_MATCH_REG, ); > > reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32); > if (reg) > reg |= BIT(31); > - amd_iommu_pc_set_reg(0, bank, cntr, IOMMU_PC_DOMID_MATCH_REG, ); > + amd_iommu_pc_set_reg(hwc->idx, bank, cntr, > + IOMMU_PC_DOMID_MATCH_REG, ); You can let those stick out - the 80 cols rule is not a strict one: reg = csource; amd_iommu_pc_set_reg(hwc->idx, bank, cntr, IOMMU_PC_COUNTER_SRC_REG, ); reg = devid | (_GET_DEVID_MASK(ev) << 32); if (reg) reg |= BIT(31); amd_iommu_pc_set_reg(hwc->idx, bank, cntr, IOMMU_PC_DEVID_MATCH_REG, ); reg = _GET_PASID(ev) | (_GET_PASID_MASK(ev) << 32); if (reg) reg |= BIT(31); amd_iommu_pc_set_reg(hwc->idx, bank, cntr, IOMMU_PC_PASID_MATCH_REG, ); reg = _GET_DOMID(ev) | (_GET_DOMID_MASK(ev) << 32); if (reg) reg |= BIT(31); amd_iommu_pc_set_reg(hwc->idx, bank, cntr, IOMMU_PC_DOMID_MATCH_REG, ); } > static void perf_iommu_disable_event(struct perf_event *event) > { > + struct hw_perf_event *hwc = >hw; > u64 reg = 0ULL; > > - amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event), > + amd_iommu_pc_set_reg(hwc->idx, _GET_BANK(event), _GET_CNTR(event), >IOMMU_PC_COUNTER_SRC_REG, ); > } > > @@ -295,7 +294,7 @@ static void perf_iommu_start(struct perf_event *event, > int flags) > return; > > val = local64_read(>prev_count) & GENMASK_ULL(48, 0); > - if (amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event), > + if (amd_iommu_pc_set_reg(hwc->idx, _GET_BANK(event), _GET_CNTR(event), > IOMMU_PC_COUNTER_REG, )) > return; > > @@ -309,7 +308,7 @@ static void perf_iommu_read(struct perf_event *event) > s64 delta; > struct hw_perf_event *hwc = >hw; > > - if (amd_iommu_pc_get_reg(0, _GET_BANK(event), _GET_CNTR(event), > + if (amd_iommu_pc_get_reg(hwc->idx, _GET_BANK(event), _GET_CNTR(event), >IOMMU_PC_COUNTER_REG, )) > return; > > @@ -407,6 +406,13 @@ static __init int _init_events_attrs(void) > > static __init void amd_iommu_pc_exit(void) > { > + struct perf_amd_iommu *pi, *next; > + > + list_for_each_entry_safe(pi, next, _amd_iommu_list, list) { > + list_del(>list); > + kfree(pi); > + } > + > if (amd_iommu_events_group.attrs) { >
Re: [PATCH v8 7/9] perf/amd/iommu: Check return value when set and get counter value
On Mon, Jan 16, 2017 at 01:23:34AM -0600, Suravee Suthikulpanit wrote: > From: Suravee Suthikulpanit> > In, perf_iommu_start(), we need to check the return value from > amd_iommu_set_reg(). In case of failure, we should not enable the PMU. > > Also, in perf_iommu_read(), we need to check the return value from > amd_iommu_get_reg() before using the value. So basically you wanna say: "Add amd_iommu_{g,s}et_reg() error handling." > > Cc: Peter Zijlstra > Cc: Borislav Petkov > Cc: Joerg Roedel > Signed-off-by: Suravee Suthikulpanit > --- > arch/x86/events/amd/iommu.c | 19 +++ > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c > index 200d2e8..cc7bea4 100644 > --- a/arch/x86/events/amd/iommu.c > +++ b/arch/x86/events/amd/iommu.c > @@ -284,6 +284,7 @@ static void perf_iommu_disable_event(struct perf_event > *event) > > static void perf_iommu_start(struct perf_event *event, int flags) > { > + u64 val; > struct hw_perf_event *hwc = >hw; > > if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED))) > @@ -292,15 +293,16 @@ static void perf_iommu_start(struct perf_event *event, > int flags) > WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE)); > hwc->state = 0; > > - if (flags & PERF_EF_RELOAD) { > - u64 prev_raw_count = local64_read(>prev_count); > - amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event), > - IOMMU_PC_COUNTER_REG, _raw_count); > - } > + if (!(flags & PERF_EF_RELOAD)) > + return; This looks wrong - you still need to do perf_iommu_enable_event(event); perf_event_update_userpage(event); even if !PERF_EF_RELOAD. > + val = local64_read(>prev_count) & GENMASK_ULL(48, 0); > + if (amd_iommu_pc_set_reg(0, _GET_BANK(event), _GET_CNTR(event), > +IOMMU_PC_COUNTER_REG, )) > + return; > > perf_iommu_enable_event(event); > perf_event_update_userpage(event); > - > } -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 6/9] perf/amd/iommu: Modify amd_iommu_pc_get_set_reg_val() API to allow specifying IOMMU index
On Mon, Jan 16, 2017 at 01:23:33AM -0600, Suravee Suthikulpanit wrote: > From: Suravee Suthikulpanit> > The current amd_iommu_pc_get_set_reg_val() cannot support multiple IOMMUs > It is also confusing since it is trying to support set and get in > one function. > > So break it down to amd_iommu_pc_[get|set]_reg(), > and modifies them to allow callers to specify IOMMU index. This prepares and modify > the driver for supporting multi-IOMMU in subsequent patch. > > Also remove unnecessary function declarations in amd_iommu_proto.h. > > Cc: Peter Zijlstra > Cc: Borislav Petkov > Cc: Joerg Roedel > Signed-off-by: Suravee Suthikulpanit > --- > arch/x86/events/amd/iommu.c | 34 ++ > arch/x86/events/amd/iommu.h | 7 -- > drivers/iommu/amd_iommu_init.c | 53 > ++--- > drivers/iommu/amd_iommu_proto.h | 5 > 4 files changed, 52 insertions(+), 47 deletions(-) ... > diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c > index ed21307d..5b7fb6c 100644 > --- a/drivers/iommu/amd_iommu_init.c > +++ b/drivers/iommu/amd_iommu_init.c > @@ -253,10 +253,6 @@ enum iommu_init_state { > static int __init iommu_go_to_state(enum iommu_init_state state); > static void init_device_table_dma(void); > > -static int iommu_pc_get_set_reg_val(struct amd_iommu *iommu, > - u8 bank, u8 cntr, u8 fxn, > - u64 *value, bool is_write); > - > static inline void update_last_devid(u16 devid) > { > if (devid > amd_iommu_last_bdf) > @@ -1481,6 +1477,14 @@ static int __init init_iommu_all(struct > acpi_table_header *table) > return 0; > } > > +#define iommu_pc_get_reg(i, b, c, f, v) \ > + iommu_pc_get_set_reg(i, b, c, f, v, false) > + > +#define iommu_pc_set_reg(i, b, c, f, v) \ > + iommu_pc_get_set_reg(i, b, c, f, v, true) Those are completely useless indirection. Just call iommu_pc_get_set_reg() directly. > + > +static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr, > + u8 fxn, u64 *value, bool is_write); > > static void init_iommu_perf_ctr(struct amd_iommu *iommu) > { -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
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> Cc: Borislav Petkov > Cc: Joerg Roedel > Signed-off-by: Suravee Suthikulpanit > --- > 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 (), 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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu