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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d1e38417-286a-015f-b7c3-6f78b0929363@amd.com>
Date:   Tue, 7 Feb 2017 08:42:42 +0700
From:   Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
To:     Borislav Petkov <bp@...en8.de>
CC:     <linux-kernel@...r.kernel.org>, <iommu@...ts.linux-foundation.org>,
        <joro@...tes.org>, <peterz@...radead.org>, <mingo@...hat.com>
Subject: Re: [PATCH v8 9/9] perf/amd/iommu: Enable support for multiple IOMMUs

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...
>