Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

Suravee Suthikulpanit
2017-02-07T01:43:04Z

Boris,

On 1/23/17 02:55, Borislav Petkov wrote:
>> @@ -421,46 +427,46 @@ static __init void amd_iommu_pc_exit(void)
>> };
>>
>> static __init int
>> -_init_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, char *name)
>> +init_one_perf_amd_iommu(struct perf_amd_iommu *perf_iommu, unsigned int idx)
>> {
>> int ret;
>>
>> raw_spin_lock_init(&perf_iommu->lock);
>>
>> - /* Init cpumask attributes to only core 0 */
>> - cpumask_set_cpu(0, &iommu_cpumask);
>> -
>> - perf_iommu->max_banks = amd_iommu_pc_get_max_banks(0);
>> - perf_iommu->max_counters = amd_iommu_pc_get_max_counters(0);
>> + perf_iommu->idx = idx;
>> + perf_iommu->max_banks = amd_iommu_pc_get_max_banks(idx);
>> + perf_iommu->max_counters = amd_iommu_pc_get_max_counters(idx);
>> if (!perf_iommu->max_banks || !perf_iommu->max_counters)
>> return -EINVAL;
>>
>> + snprintf(perf_iommu->name, PERF_AMD_IOMMU_NAME_SZ, "amd_iommu_%u", idx);
>> +
>> + perf_iommu->pmu.event_init = perf_iommu_event_init,
>> + perf_iommu->pmu.add = perf_iommu_add,
>> + perf_iommu->pmu.del = perf_iommu_del,
>> + perf_iommu->pmu.start = perf_iommu_start,
>> + perf_iommu->pmu.stop = perf_iommu_stop,
>> + perf_iommu->pmu.read = perf_iommu_read,
> This compiles but it is yucky.
>
> You should do that instead:
>
> static struct pmu amd_iommu_pmu = {
> .event_init = perf_iommu_event_init,
> .add = perf_iommu_add,
> .del = perf_iommu_del,
> .start = perf_iommu_start,
> .stop = perf_iommu_stop,
> .read = perf_iommu_read,
> .task_ctx_nr = perf_invalid_context,
> .attr_groups = amd_iommu_attr_groups,
> };
>
> ...
>
> ret = perf_pmu_register(&amd_iommu_pmu, perf_iommu->name, -1);
>
> Because otherwise you're carrying a struct pmu in each struct
> perf_amd_iommu which has identical contents.

Actually, only the callbacks above will be identical on each pmu, but
there are other parts of the structure which are different
(e.g. pmu->name, pmu->type, etc.) Also, we need one pmu instance per
IOMMU since each pmu reference will get assigned to perf_event, and
also used to reference back to struct perf_amd_iommu. Note that each
pmu can also have different events.

> Now, you need to access the struct perf_amd_iommu pointer for each
> IOMMU PMU in some of the functions like perf_iommu_event_init(), for
> example. But for that you only need the index and to iterate the
> perf_amd_iommu_list.

I think replacing the index w/ pointer to amd_iommu is a good idea.
I'm changing this in V9.

Thanks,
Suravee
>
> I wasn't able to find a good way to do that from a quick stare but
> PeterZ might have a better idea...
>