| 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
| ||
|
Message-ID: <8052c86d-0258-497b-a2b4-79b65d347a86@gmail.com>
Date: Fri, 17 Oct 2025 14:25:25 -0700
From: Bo Gan <ganboing@...il.com>
To: Charles Mirabile <cmirabil@...hat.com>, Bo Gan <ganboing@...il.com>
Cc: Lucas Zampieri <lzampier@...hat.com>, linux-kernel@...r.kernel.org,
Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>,
Samuel Holland <samuel.holland@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Alexandre Ghiti <alex@...ti.fr>, Vivian Wang <dramforever@...e.com>,
devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org,
Zhang Xincheng <zhangxincheng@...rarisc.com>
Subject: Re: [PATCH v5 3/3] irqchip/plic: add support for UltraRISC DP1000
PLIC
Hi Charles,
On 10/16/25 16:25, Charles Mirabile wrote:
> Hi Bo—
>
> On Thu, Oct 16, 2025 at 5:25 PM Bo Gan <ganboing@...il.com> wrote:
>>
>> Hi Lucas, Charles,
>>
>> I just realized your last reply and sorry about the messy formatting.
>> Please disregard the previous one from me and use this one.
>>
>> On 10/16/25 01:42, Lucas Zampieri wrote:
>>> From: Charles Mirabile <cmirabil@...hat.com>
>>>
>>> Add a new compatible for the plic found in UltraRISC DP1000 with a quirk to
>>> work around a known hardware bug with IRQ claiming in the UR-CP100 cores.
>>>
>>> When claiming an interrupt on UR-CP100 cores, all other interrupts must be
>>> disabled before the claim register is accessed to prevent incorrect
>>> handling of the interrupt. This is a hardware bug in the CP100 core
>>> implementation, not specific to the DP1000 SoC.
>>>
>>> When the PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM flag is present, a specialized
>>> handler (plic_handle_irq_cp100) saves the enable state of all interrupts,
>>> disables all interrupts except for the first pending one before reading the
>>> claim register, and then restores the interrupts before further processing of
>>> the claimed interrupt continues.
>>>
>>> The driver matches on "ultrarisc,cp100-plic" to apply the quirk to all
>>> SoCs using UR-CP100 cores, regardless of the specific SoC implementation.
>>> This has no impact on other platforms.
>>>
>>> Co-developed-by: Zhang Xincheng <zhangxincheng@...rarisc.com>
>>> Signed-off-by: Zhang Xincheng <zhangxincheng@...rarisc.com>
>>> Signed-off-by: Charles Mirabile <cmirabil@...hat.com>
>>> Acked-by: Samuel Holland <samuel.holland@...ive.com>
>>> Signed-off-by: Lucas Zampieri <lzampier@...hat.com>
>>> ---
>>> drivers/irqchip/irq-sifive-plic.c | 94 ++++++++++++++++++++++++++++++-
>>> 1 file changed, 93 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
>>> index bf69a4802b71..0428e9f3423d 100644
>>> --- a/drivers/irqchip/irq-sifive-plic.c
>>> +++ b/drivers/irqchip/irq-sifive-plic.c
>>> @@ -49,6 +49,8 @@
>>> #define CONTEXT_ENABLE_BASE 0x2000
>>> #define CONTEXT_ENABLE_SIZE 0x80
>>>
>>> +#define PENDING_BASE 0x1000
>>> +
>>> /*
>>> * Each hart context has a set of control registers associated with it. Right
>>> * now there's only two: a source priority threshold over which the hart will
>>> @@ -63,6 +65,7 @@
>>> #define PLIC_ENABLE_THRESHOLD 0
>>>
>>> #define PLIC_QUIRK_EDGE_INTERRUPT 0
>>> +#define PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM 1
>>>
>>> struct plic_priv {
>>> struct fwnode_handle *fwnode;
>>> @@ -394,6 +397,89 @@ static void plic_handle_irq(struct irq_desc *desc)
>>> chained_irq_exit(chip, desc);
>>> }
>>>
>>> +static bool cp100_isolate_pending_irq(int nr_irq_groups, u32 ie[],
>>> + void __iomem *pending,
>>> + void __iomem *enable)
>>> +{
>>> + u32 pending_irqs = 0;
>>> + int i, j;
>>> +
>>> + /* Look for first pending interrupt */
>>> + for (i = 0; i < nr_irq_groups; i++) {
>>> + pending_irqs = ie[i] & readl_relaxed(pending + i * sizeof(u32));
>>> + if (pending_irqs)
>>> + break;
>>
>> No need to start from group 0. Only readl on the group with ie[i] != 0
>
> You mean put something like `if (!ie[i]) continue;` to avoid the readl
> if the mask is going to obliterate it?
>
Yes.
> Sounds reasonable.
>>
>>> + }
>>> +
>>> + if (!pending_irqs)
>>> + return false;
>>> +
>>> + /* Disable all interrupts but the first pending one */
>>> + for (j = 0; j < nr_irq_groups; j++) {
>>> + u32 new_mask = 0;
>>> +
>>> + if (j == i) {
>>> + /* Extract mask with lowest set bit */
>>> + new_mask = (pending_irqs & -pending_irqs);
>>> + }
>>> +
>>> + writel_relaxed(new_mask, enable + j * sizeof(u32));
>>
>>
>> There's no need to write the register if the value isn't changing. You can
>> check new_mask with the value in ie[].
>
> Something similar like `if (!ie[j]) continue;` in this loop too? We
Better to have:
if (new_mask != ie[j])
writel_relaxed(...)
> know that this will not interact poorly with the i == j case because
> ie[i] is by definition nonzero if we hit this code path and so when i
> ==j ie[j] == ie[j] != 0 so we will hit the rest of the logic. Also
> sounds sane.
> >>
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler,
>>> + void __iomem *claim)
>>> +{
>>> + int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs, 32);
>>> + void __iomem *pending = handler->priv->regs + PENDING_BASE;
>>> + void __iomem *enable = handler->enable_base;
>>> + irq_hw_number_t hwirq = 0;
>>> + int i;
>>> +
>>> + guard(raw_spinlock)(&handler->enable_lock);
>>> +
>>> + /* Save current interrupt enable state */
>>> + for (i = 0; i < nr_irq_groups; i++)
>>> + handler->enable_save[i] = readl_relaxed(enable + i * sizeof(u32));
>>
>>
>> I see that you start to use handler->enable_save to track HW in the last reply.
>> I'm about to suggest that. Please send out a new patch, so people can properly
>> review it. There's change to common code path.
>
> Yes, a proper patch will come soon, just have to respin the whole
> series. Two separate commits, one for refactoring the common code,
> another for adding the quirk.
>
> The changes do not overlap - the first patch will be hunks 3, 4, & 5
> of the tentative diff I sent to Thomas, and patch two will be hunks 1,
> 2, 6, 7, 8.
>
> If you have any concerns about the changes to common code, do let us know.
>
> I will also pick up your feedback about avoiding the mmio by checking
> ie[] in the loops.
>
>>
>>> +
>>> + if (!cp100_isolate_pending_irq(nr_irq_groups, handler->enable_save, pending, enable))
>>> + return 0;
>>> +
>>> + hwirq = readl(claim);
>>
>> Possibly missing a io barrier. readl isn't going to enforce the ordering of
>> readl/writel_relaxed above and itself. There could be other barriers missing.
>> Please check.
>>
>>> +
>>> + /* Restore previous state */
>>> + for (i = 0; i < nr_irq_groups; i++)
>>> + writel_relaxed(handler->enable_save[i], enable + i * sizeof(u32));
You can also:
if (!handler->enable_save[i])
// enable_save[i] has never changed, it's 0,
// so we can't remove any more bits
continue;
if (i == hwirq / 32 && handler->enable_save[i] == (1UL << (hwirq %32)))
// enable_save[i] has never changed,
// because the enable bit of hwirq must have been enabled to
// be able to claim this hwirq, and there were no more bits to remove
continue;
writel_relaxed(...)
>>> +
>>> + return hwirq;
>>> +}
>>> +
>>> +static void plic_handle_irq_cp100(struct irq_desc *desc)
>>> +{
>>> + struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
>>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>>> + void __iomem *claim = handler->hart_base + CONTEXT_CLAIM;
>>> + irq_hw_number_t hwirq;
>>> +
>>> + WARN_ON_ONCE(!handler->present);
>>> +
>>> + chained_irq_enter(chip, desc);
>>> +
>>> + while ((hwirq = cp100_get_hwirq(handler, claim))) {
>>> + int err = generic_handle_domain_irq(handler->priv->irqdomain, hwirq);
>>> +
>>> + if (unlikely(err)) {
>>> + pr_warn_ratelimited("%pfwP: can't find mapping for hwirq %lu\n",
>>> + handler->priv->fwnode, hwirq);
>>> + }
>>> + }
>>> +
>>> + chained_irq_exit(chip, desc);
>>> +}
>>> +
>>> static void plic_set_threshold(struct plic_handler *handler, u32 threshold)
>>> {
>>> /* priority must be > threshold to trigger an interrupt */
>>> @@ -430,6 +516,8 @@ static const struct of_device_id plic_match[] = {
>>> .data = (const void *)BIT(PLIC_QUIRK_EDGE_INTERRUPT) },
>>> { .compatible = "thead,c900-plic",
>>> .data = (const void *)BIT(PLIC_QUIRK_EDGE_INTERRUPT) },
>>> + { .compatible = "ultrarisc,cp100-plic",
>>> + .data = (const void *)BIT(PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM) },
>>> {}
>>> };
>>>
>>> @@ -664,12 +752,16 @@ static int plic_probe(struct fwnode_handle *fwnode)
>>> }
>>>
>>> if (global_setup) {
>>> + void (*handler_fn)(struct irq_desc *) = plic_handle_irq;
>>> +
>>> + if (test_bit(PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM, &handler->priv->plic_quirks))
>>> + handler_fn = plic_handle_irq_cp100;
>>> +
>>> /* Find parent domain and register chained handler */
>>> domain = irq_find_matching_fwnode(riscv_get_intc_hwnode(), DOMAIN_BUS_ANY);
>>> if (domain)
>>> plic_parent_irq = irq_create_mapping(domain, RV_IRQ_EXT);
>>> if (plic_parent_irq)
>>> - irq_set_chained_handler(plic_parent_irq, plic_handle_irq);
>>> + irq_set_chained_handler(plic_parent_irq, handler_fn);
>>>
>>> cpuhp_setup_state(CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING,
>>> "irqchip/sifive/plic:starting",
>>
>> My rationale of the above comments is to achieve minimal overhead with this
>> "read pending[] -> disable IE[] -> claim -> enable IE[]" approach. In general,
>> the fewer interrupts enabled on a hart, the lower the overhead. If there's only
>> 1 interrupt enabled for a give hart, then there's zero reading/writing of IE[],
>> and you can further optimize away the reading of pending register.
>>
>> I'd imagine that if the user truly want to avoid the overhead of this quirk,
>> they can chose to spread out the irq groups onto different harts to alleviate
>> the slow down, or better isolate a single irq to a given hart, and we should
>> make it possible.
>>
>> Feel free to point out any of my misunderstandings.
>>
>> Bo
>>
>
> Best—Charlie
>
Bo
Powered by blists - more mailing lists