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

[Samuel Holland <samuel@sholland.org>]

On 5/10/22 5:27 AM, Marc Zyngier wrote:
> On Mon, 09 May 2022 04:43:33 +0100,
> Samuel Holland <samuel@sholland.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.
>