linux-kernel - Re: [PATCH 5/5] irqchip/sifive-plic: Separate the enable and mask operations

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] [day] [month] [year] [list]

Message-ID: <2b063917-17c8-0add-fadf-5aa42532fbbf@sholland.org>
Date:   Wed, 11 May 2022 05:00:51 -0500
From:   Samuel Holland <samuel@...lland.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Florian Fainelli <f.fainelli@...il.com>,
        Guo Ren <guoren@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        Wei Xu <xuwei5@...ilicon.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-riscv@...ts.infradead.org
Subject: Re: [PATCH 5/5] irqchip/sifive-plic: Separate the enable and mask
 operations

On 5/10/22 5:27 AM, Marc Zyngier wrote:
> On Mon, 09 May 2022 04:43:33 +0100,
> Samuel Holland <samuel@...lland.org> wrote:
>>
>> The PLIC has two per-IRQ checks before sending an IRQ to a hart context.
>> First, it checks that the IRQ's priority is nonzero. Then, it checks
>> that the enable bit is set for that combination of IRQ and context.
>>
>> Currently, the PLIC driver sets both the priority value and the enable
>> bit in its (un)mask operations. However, modifying the enable bit is
>> problematic for two reasons:
>>   1) The enable bits are packed, so changes are not atomic and require
>>      taking a spinlock.
>>   2) The following requirememnt from the PLIC spec, which explains the
>>      racy (un)mask operations in plic_irq_eoi():
>>
>>        If the completion ID does not match an interrupt source
>>        that is currently enabled for the target, the completion
>>        is silently ignored.
>>
>> Both of these problems are solved by using the priority value to mask
>> IRQs. Each IRQ has a separate priority register, so writing the priority
>> value is atomic. And since the enable bit remains set while an IRQ is
>> masked, the EOI operation works normally. The enable bits are still used
>> to control the IRQ's affinity.
> 
> This is pretty neat.
> 
> My only concern is around whether implementations do when changing
> priority of enabled interrupts. The PLIC architecture is conveniently
> silent on the subject, but that's certainly something that can result
> in total crap with the ARM GICs, for example, because an
> implementation is free to apply this priority change on an already
> pending interrupt, or not. But the kernel really wants the interrupt
> to be masked once it tells the HW to do so.

I think this can be expected based on this comment from the PLIC spec, which is
the only place using the word "mask":

"The PLIC will mask all PLIC interrupts of a priority less than or equal to
threshold. For example, a`threshold` value of zero permits all interrupts with
non-zero priority."

> Could anyone please check the RTL for some common implementations?

The C9xx PLIC explicitly checks for priority 0 in each gateway:

https://github.com/T-head-Semi/openc906/blob/main/C906_RTL_FACTORY/gen_rtl/plic/rtl/plic_int_kid.v#L152

and also continuously updates the IRQ outputs based on comparing the arbitrated
highest priority interrupt with the target's threshold:

https://github.com/T-head-Semi/openc906/blob/main/C906_RTL_FACTORY/gen_rtl/plic/rtl/plic_arb_ctrl.v#L295


Other implementations appear to do something similar, continuously computing the
highest priority interrupt, and comparing that to the target threshold:

https://github.com/chipsalliance/rocket-chip/blob/master/src/main/scala/devices/tilelink/Plic.scala#L187
https://github.com/lowRISC/rv_plic/blob/master/rtl/rv_plic_target.sv#L92
https://github.com/lowRISC/opentitan/blob/master/hw/ip_templates/rv_plic/rtl/rv_plic_target.sv.tpl#L58
https://github.com/pulp-platform/rv_plic/blob/master/rtl/rv_plic_target.sv#L92
https://github.com/RoaLogic/plic/blob/master/rtl/verilog/core/plic_target.sv#L126
https://github.com/qemu/qemu/blob/master/hw/intc/sifive_plic.c#L96

Those are the implementations I could find. Others may know about non-public
implementations.

Regards,
Samuel

> A way to avoid the above trouble (should it exist) would be to
> disable the interrupt when changing the priority, and then reenable
> it. You'd still get the simpler EOI, which is what you really want.
> 
> Thanks,
> 
> 	M.
>