linux-kernel - Re: [PATCH] irqchip/sifive-plic: Avoid clearing the per-hart enable bits

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: <86zg3ttvsi.wl-maz@kernel.org>
Date:   Tue, 18 Jul 2023 10:26:37 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Samuel Holland <samuel.holland@...ive.com>
Cc:     Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Samuel Holland <samuel@...lland.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH] irqchip/sifive-plic: Avoid clearing the per-hart enable bits

On Mon, 17 Jul 2023 19:58:40 +0100,
Samuel Holland <samuel.holland@...ive.com> wrote:
> 
> Writes to the PLIC completion register are ignored if the enable bit for
> that (interrupt, hart) combination is cleared. This leaves the interrupt
> in a claimed state, preventing it from being triggered again.
> 
> Originally, the enable bit was cleared in the .irq_mask operation, and
> commit 69ea463021be ("irqchip/sifive-plic: Fixup EOI failed when masked")
> added a workaround for this issue. Later, commit a1706a1c5062
> ("irqchip/sifive-plic: Separate the enable and mask operations") moved
> toggling the enable bit to the .irq_enable/.irq_disable operations and
> removed the workaround.
> 
> However, there are still places where .irq_disable can be called from
> inside the hard IRQ handler, for example in irq_pm_check_wakeup(). As a
> result, this issue causes an interrupt to get stuck in a claimed state
> after being used to wake the system from s2idle.
> 
> There is no real benefit to implementing the .irq_enable/.irq_disable
> operations using the enable bits. In fact, the existing mask/unmask
> implementation using the threshold register is already more efficient,
> as it requires no read/modify/write cycles. So let's leave the enable
> bits set for the lifetime of the IRQ, using them only to control its
> affinity.

Side question, which doesn't affect this patch: what happens with
interrupts that are firing while the interrupt is in a disabled state?
It's fine for levels, but what of edge interrupts?

My reading of the spec is that it is the role of the "gateway" to hold
the signal, and that this is upstream of the PLIC itself, so it
*should* be fine, but I'd like confirmation on that.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.