linux-kernel - Re: [PATCH] driver/perf: Add PMU driver for the ARM DMC-620 memory controller.

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: <20200401111903.GC17163@C02TD0UTHF1T.local>
Date:   Wed, 1 Apr 2020 12:19:03 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Will Deacon <will@...nel.org>
Cc:     Tuan Phan <tuanphan@...eremail.onmicrosoft.com>,
        Tuan Phan <tuanphan@...amperecomputing.com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] driver/perf: Add PMU driver for the ARM DMC-620 memory
 controller.

On Wed, Apr 01, 2020 at 11:27:25AM +0100, Will Deacon wrote:
> On Wed, Apr 01, 2020 at 10:52:26AM +0100, Mark Rutland wrote:
> > On Tue, Mar 31, 2020 at 03:14:59PM -0700, Tuan Phan wrote:
> > > > On Mar 20, 2020, at 4:25 AM, Mark Rutland <mark.rutland@....com> wrote:
> > > > On Thu, Mar 19, 2020 at 12:03:43PM -0700, Tuan Phan wrote:
> > > >>> On Mar 19, 2020, at 8:16 AM, Mark Rutland <mark.rutland@....com> wrote:
> > > >>> On Tue, Mar 17, 2020 at 05:29:38PM -0700, Tuan Phan wrote:
> > > >>>> +static int arm_dmc620_pmu_dev_init(struct arm_dmc620_pmu *dmc620_pmu)
> > > >>>> +{
> > > >>>> +	struct platform_device *pdev = dmc620_pmu->pdev;
> > > >>>> +	int ret;
> > > >>>> +
> > > >>>> +	ret = devm_request_irq(&pdev->dev, dmc620_pmu->irq,
> > > >>>> +				arm_dmc620_pmu_handle_irq,
> > > >>>> +				IRQF_SHARED,
> > > >>>> +				dev_name(&pdev->dev), dmc620_pmu);
> > > >>> 
> > > >>> This should have IRQF_NOBALANCING | IRQF_NO_THREAD. I don't think we
> > > >>> should have IRQF_SHARED.
> > > >> => I agree on having IRQF_NOBALANCING and IRQF_NO_THREAD. But
> > > >> IRQF_SHARED is needed. In our platform all DMC620s share same IRQs and
> > > >> any cpus can access the pmu registers.
> > > > 
> > > > Linux needs to ensure that the same instance is concistently accessed
> > > > from the same CPU, and needs to migrate the IRQ to handle that. Given we
> > > > do that on a per-instance basis, we cannot share the IRQ with another
> > > > instance.
> > > > 
> > > > Please feed back to you HW designers that muxing IRQs like this causes
> > > > significant problems for software.
> > > 
> > > I looked at the SMMUv3 PMU driver and it also uses IRQF_SHARED. SMMUv3
> > > PMU and DMC620 PMU are very much similar in which counters can be
> > > accessed by any cores using memory map. Any special reasons
> > > IRQF_SHARED works with SMMUv3 PMU driver?
> > 
> > No; I believe that is a bug in the SMMUv3 PMU driver. If the IRQ were
> > shared, and another driver that held the IRQ changed the affinity,
> > things would go very wrong.
> 
> I *think* the idea is that the SMMUv3 PMU driver manages multiple PMCG
> devices, which may all share an irq line, rather than the irq line being
> shared by some other driver that might change the affinity. So I suspect
> dropping IRQF_SHARED will break things.

Ok. So long as each of the contexts are migrated before the IRQ is, I
think that's sound. Otherwise there's a small window where the IRQ
handler for an instance won't see the state expected (and could end up
treated as a screaming IRQ).

Otherwise, in that case I think that's not so bad.

> > Note that it's also missing IRQF_NOBALANCING, which is also necessary to
> > avoid such issues.
> 
>   unsigned long flags = IRQF_NOBALANCING | IRQF_SHARED | IRQF_NO_THREAD;
> 
> so it looks like IRQF_NOBALANCING is already taken care of.

Whoops; I'd misread thhe DMC-620 code above assuming it was the SMMUv3
PMU.

Thanks,
Mark.