linux-kernel - Re: [PATCH v5 3/3] irqchip/plic: add support for UltraRISC DP1000 PLIC

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: <87plan0yvd.ffs@tglx>
Date: Thu, 16 Oct 2025 15:17:42 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Lucas Zampieri <lzampier@...hat.com>, linux-kernel@...r.kernel.org
Cc: Charles Mirabile <cmirabil@...hat.com>, 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>, Lucas Zampieri <lzampier@...hat.com>
Subject: Re: [PATCH v5 3/3] irqchip/plic: add support for UltraRISC DP1000 PLIC

On Thu, Oct 16 2025 at 09:42, Lucas Zampieri wrote:

After fixing the corrupted patch up I had a closer look and decided not
to merge it. See comments below.
  
> +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)
> +{
> +	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));

This is truly the most inefficient way to solve that problem. The enable
registers are modified with enabled_lock held, so you can just cache the
value in plic_handler::enabled_save and avoid this read loop completely.
After claiming the interrupt you restore from that cache, no?

Now for the search and disable mechanism. Of course you need to search
for th pending interrupt first, but then you can make that masking loop
very simple by having a plic_handler::enabled_clear[] array which is
zeroed on initialization:

        unsigned long pending = 0;
        
	for (group = 0; !pending && group < nr_irq_groups; group++) {
		pending = handler->enabled_save[i];
                pending =& readl_relaxed(pending + group * sizeof(u32));
	}
        if (!pending)
        	return false;

        bit = ffs(pending) - 1;
        handler->enabled_clear[group] |= BIT(bit);
        for (int i = 0; i < nr_irq_groups; i++)
		writel_relaxed(handler->enabled_clear[i], enable + i * sizeof(u32));
        handler->enabled_clear[group] = 0;

No?

But looking at this makes me wonder about the functional correctness of all
this. What happens in this case:

Device A raises an interrupt

    handler()
        ....
        disable_groups();

Device B raises a now disabled interrupt

        restore_groups();

Is the device B interrupt preserved in the interrupt chip and actually
raised when the interrupt enable bit is restored or is it lost?

Thanks,

        tglx