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

[Charles Mirabile <cmirabil@redhat.com>]

Hi Thomas—

On Thu, Oct 16, 2025 at 12:12 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Thu, Oct 16 2025 at 11:54, Charles Mirabile wrote:
> > On Thu, Oct 16, 2025 at 9:17 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> > +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?
> >
> > You mean touch the other functions where the enable bits are modified
> > to keep the cache in sync so that we don't need to do this read loop
> > and can have a proper set of values cached?
> >
> > My concern is that this obviously has an impact on other platforms
> > which do not have this quirk since keeping the cache in sync would get
> > pushed all throughout the driver.
>
> The irq_enable()/disable() callbacks are not really hotpath and caching
> the bit in plic_toggle() or such is just not measurable overhead
> compared to the register access.

Fair enough, if you insist. This will probably require a second patch
refactoring a decent amount of code (since the suspend / resume path
for which this enable array was added becomes simpler).

>
> >> 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?
> >
> > Sure that would also work, but why are we using ffs (slow) only to
> > shift the result back to make a new mask when (x & -x) is faster and
> > skips the intermediate step delivering immediately the mask of the
> > lowest bit.
>
> Because I did not spend time thinking about it.

Sorry, did you mean "because I had not considered the original
approach carefully enough" or "because this other approach, while
slower, is more self evidently correct."

If you meant the latter, I still want to argue for the bit twiddling
approach. I verified that clang and gcc are both not smart enough to
recognize the pattern of ffs followed by shift and optimize to x & -x
(even on -O3 [1]), so using ffs is definitely slower and relies on a
bunch of machinery (alternatives because risc-v does not include a
dedicated count trailing zeros instruction in the base isa). To me
anyways, the logic of x & -x is pretty obvious, and even more so with
a comment, and this is actually in the hot path.

>
> > As for making another caching array, I guess, but again that is just a
> > time vs space trade off with its own invariants to maintain that would
> > also impact other platforms.
>
> It's a pointer in struct plic_handler (or whatever it's named) and you
> can allocate it when the quirk is required. The pointer is definitely
> not a burden for anyone else.

This I still don't understand how this is particuarly helpful. Since
we are doing mmio, this is going to be an explicit loop and not a
memcpy. The code is branchless in either case (set equal for the check
of i against j negate and and with mask before loading into the mmio).


>
> >> Is the device B interrupt preserved in the interrupt chip and actually
> >> raised when the interrupt enable bit is restored or is it lost?
> >
> > I am not sure how to verify this other than to tell you that without
> > this quirk (i.e. trying to use normal plic behavior) the device does
> > not work, but with this quirk I can boot to a desktop with a pcie
> > graphics card and storage, use networking etc that all obviously
> > depend on the correct functioning of the interrupt controller.
> >
> > My reading of the spec for PLIC also suggests (but does not explicitly
> > confirm) that the pending bits function irrespective of the state of
> > the corresponding enable bit: "A pending bit in the PLIC core can be
> > cleared by setting the associated enable bit then performing a claim."
> > (page 14 plic spec 1.0.0 [1]).
> >
> > This sentence implies to me that it is possible for a pending bit to
> > be set even though the corresponding enable bit is not, which lends
> > credence to the idea that the pending bits operate independently.
>
> Looks like that. Please add a comment to that effect then.

Will do.

>
> Thanks,
>
>         tglx
>

[1] https://godbolt.org/z/eofozYjPo