| 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: <CABe3_aF2OYXUWdQHa5ZR5cohB6m5uPpAj3Yg5GDFtK60NBoXSg@mail.gmail.com>
Date: Tue, 14 Oct 2025 12:51:23 -0400
From: Charles Mirabile <cmirabil@...hat.com>
To: Samuel Holland <samuel.holland@...ive.com>
Cc: Lucas Zampieri <lzampier@...hat.com>, 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>, 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>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] irqchip/plic: add support for UltraRISC DP1000 PLIC
On Tue, Oct 14, 2025 at 12:45 PM Samuel Holland
<samuel.holland@...ive.com> wrote:
>
> Hi Charles,
>
> On 2025-10-14 11:28 AM, Charles Mirabile wrote:
> > On Tue, Oct 14, 2025 at 12:09 PM Samuel Holland
> > <samuel.holland@...ive.com> wrote:
> >>
> >> On 2025-10-14 10:40 AM, 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_CLAIM_REGISTER is present, a specialized handler
> >>
> >> You may want to name this something a bit more specific. Every PLIC has a claim
> >> register, so it seems a bit weird saying that this is a quirk :)
> >
> > Something like `PLIC_QUIRK_CLAIM_REGISTER_ERRATA`? Or
> > `PLIC_QUIRK_CP100_CLAIM_REGISTER`? Or
> > `PLIC_QUIRK_MULTIPLE_PENDING_CLAIM`? I guess the trouble is that they
> > get pretty long pretty quick. Hard to summarize the issue concisely.
>
> If I had to suggest something, I'd say PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM,
> but it's really up to you. The function name indicates it is specific to CP100,
> so the flag probably should too. I don't think being long is a problem if it
> helps the reader guess the value/effect of the flag at a glance.
>
> Regards,
> Samuel
Great, I agree that that makes sense. If we have to send a v4, we will
fold that change in, and keep your ack. I guess if we don't need a v4,
we could make a separate commit or the maintainer could make the
change while pulling it in.
>
> >>
> >> Anyway, the code looks good, so:
> >>
> >> Acked-by: Samuel Holland <samuel.holland@...ive.com>
> >
> > Thanks!
> >
> >>
> >>> (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>
> >>> 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..1d528904b353 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_CLAIM_REGISTER 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;
> >>> + }
> >>> +
> >>> + 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));
> >>> + }
> >>> +
> >>> + return true;
> >>> +}
> >>> +
> >>> +static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler,
> >>> + void __iomem *claim)
> >>> +{
> >>> + void __iomem *enable = handler->enable_base;
> >>> + void __iomem *pending = handler->priv->regs + PENDING_BASE;
> >>> + int nr_irqs = handler->priv->nr_irqs;
> >>> + int nr_irq_groups = DIV_ROUND_UP(nr_irqs, 32);
> >>> + int i;
> >>> + irq_hw_number_t hwirq = 0;
> >>> +
> >>> + raw_spin_lock(&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));
> >>> +
> >>> + if (!cp100_isolate_pending_irq(nr_irq_groups, handler->enable_save, pending, enable))
> >>> + goto out;
> >>> +
> >>> + hwirq = readl(claim);
> >>> +
> >>> + /* Restore previous state */
> >>> + for (i = 0; i < nr_irq_groups; i++)
> >>> + writel_relaxed(handler->enable_save[i], enable + i * sizeof(u32));
> >>> +out:
> >>> + raw_spin_unlock(&handler->enable_lock);
> >>> + 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_CLAIM_REGISTER) },
> >>> {}
> >>> };
> >>>
> >>> @@ -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_CLAIM_REGISTER, &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",
> >>
> >
>
Powered by blists - more mailing lists