On Mon, Jan 09, 2017 at 09:33:47PM -0600, Suravee Suthikulpanit wrote:
> This patch adds multi-IOMMU support for perf by exposing
> an AMD IOMMU PMU for each IOMMU found in the system via:
>
> /sys/device/amd_iommu_x /* where x is the IOMMU index. */
Straight into the top-level devices hierarchy?
Why not add a proper hierarchy to
/sys/devices/system/iommu/ ...
?
Jörg, don't you have a sane iommu hierarchy already in sysfs?
> This allows users to specify different events to be programed
> onto performance counters of each IOMMU.
>
> Cc: Peter Zijlstra
> Cc: Borislav Petkov
> Signed-off-by: Suravee Suthikulpanit
> ---
> arch/x86/events/amd/iommu.c | 119
>
> 1 file changed, 64 insertions(+), 55 deletions(-)
>
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index 2403c78..5fd97b5 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
> @@ -35,10 +35,13 @@
> #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
>
> struct perf_amd_iommu {
> + struct list_head list;
> struct pmu pmu;
> + uint idx;
This uint thing has crept in:
$ git grep "uint " arch/x86/
arch/x86/crypto/crc32-pclmul_asm.S:101: * uint crc32_pclmul_le_16(unsigned
char const *buffer,
arch/x86/crypto/crc32-pclmul_asm.S:102: *size_t
len, uint crc32)
arch/x86/events/amd/iommu.h:32:extern u8 amd_iommu_pc_get_max_banks(uint idx);
arch/x86/events/amd/iommu.h:34:extern u8 amd_iommu_pc_get_max_counters(uint
idx);
arch/x86/events/amd/iommu.h:36:extern int amd_iommu_pc_set_reg(uint idx, u16
devid, u8 bank, u8 cntr,
arch/x86/events/amd/iommu.h:39:extern int amd_iommu_pc_set_counter(uint idx, u8
bank, u8 cntr, u64 *value);
arch/x86/events/amd/iommu.h:41:extern int amd_iommu_pc_get_counter(uint idx, u8
bank, u8 cntr, u64 *value);
"unsigned int" please.
> + char name[PERF_AMD_IOMMU_NAME_SZ];
> u8 max_banks;
> u8 max_counters;
> u64 cntr_assign_mask;
> @@ -46,6 +49,8 @@ struct perf_amd_iommu {
> const struct attribute_group *attr_groups[4];
> };
>
> +LIST_HEAD(perf_amd_iommu_list);
static
> +
> #define format_group attr_groups[0]
> #define cpumask_groupattr_groups[1]
> #define events_group attr_groups[2]
> @@ -204,8 +209,7 @@ static int clear_avail_iommu_bnk_cntr(struct
> perf_amd_iommu *perf_iommu,
> static int perf_iommu_event_init(struct perf_event *event)
> {
> struct hw_perf_event *hwc = &event->hw;
> - struct perf_amd_iommu *perf_iommu;
> - u64 config, config1;
> + struct perf_amd_iommu *pi = container_of(event->pmu, struct
> perf_amd_iommu, pmu);
Do this deref when you actually need it:
struct perf_amd_iommu *pi;
because you might exit earlier and end up doing it for nothing.
>
> /* test the event attr type check for PMU enumeration */
> if (event->attr.type != event->pmu->type)
> @@ -227,27 +231,17 @@ static int perf_iommu_event_init(struct perf_event
> *event)
> if (event->cpu < 0)
> return -EINVAL;
>
> - perf_iommu = &__perf_iommu;
> -
> - if (event->pmu != &perf_iommu->pmu)
> - return -ENOENT;
> -
> - if (perf_iommu) {
> - config = event->attr.config;
> - config1 = event->attr.config1;
> - } else {
> - return -EINVAL;
> - }
> -
> /* update the hw_perf_event struct with the iommu config data */
> - hwc->config = config;
> - hwc->extra_reg.config = config1;
---> here:
pi = container_of(event->pmu, struct perf_amd_iommu, pmu);
> + hwc->idx = pi->idx;
> + hwc->config = event->attr.config;
> + hwc->extra_reg.config = event->attr.config1;
Align vertically on the = sign.
>
> return 0;
> }
>
> static void perf_iommu_enable_event(struct perf_event *ev)
> {
> + struct hw_perf_event *hwc = &ev->hw;
> u8 csource = _GET_CSOURCE(ev);
> u16 devid = _GET_DEVID(ev);
> u8 bank = _GET_BANK(ev);
...
> static void perf_iommu_disable_event(struct perf_event *event)
> {
> + struct hw_perf_event *hwc = &event->hw;
> u64 reg = 0ULL;
>
> - amd_iommu_pc_set_reg(0, _GET_DEVID(event), _GET_BANK(event),
> + amd_iommu_pc_set_reg(hwc->idx, _GET_DEVID(event), _GET_BANK(event),
>_GET_CNTR(event), IOMMU_PC_COUNTER_SRC_REG, ®);
> }
>
> @@ -301,7 +296,7 @@ static void perf_iommu_start(struct perf_event *event,
> int flags)
>
> val = local64_read(&hwc->prev_count);
>
> - amd_iommu_pc_set_counter(0, _GET_BANK(event), _GET_CNTR(event), &val);
> + amd_iommu_pc_set_counter(hwc->idx, _GET_BANK(event), _GET_CNTR(event),
> &val);
> enable:
> perf_iommu_enable_event(event);
> perf_event_update_userpage(
