Re: [PULL 15/96] ppc/pnv: Implement POWER9 LPC PSI serirq outputs and auto-clear function

[Peter Maydell]

On Mon, 29 Jul 2024 at 11:11, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 7/26/24 01:52, Nicholas Piggin wrote:
> > The POWER8 LPC ISA device irqs all get combined and reported to the line
> > connected the PSI LPCHC irq. POWER9 changed this so only internal LPC
> > host controller irqs use that line, and the device irqs get routed to
> > 4 new lines connected to PSI SERIRQ0-3.

Ping! It looks like these issues are still floating around...

> > -static void pnv_lpc_eval_irqs(PnvLpcController *lpc)
> > +/* Program the POWER9 LPC irq to PSI serirq routing table */
> > +static void pnv_lpc_eval_serirq_routes(PnvLpcController *lpc)
> >   {
> > -    bool lpc_to_opb_irq = false;
> > +    int irq;
> >
> > -    /* Update LPC controller to OPB line */
> > -    if (lpc->lpc_hc_irqser_ctrl & LPC_HC_IRQSER_EN) {
> > -        uint32_t irqs;
> > +    if (!lpc->psi_has_serirq) {
> > +        if ((lpc->opb_irq_route0 & PPC_BITMASK(8, 13)) ||
> > +            (lpc->opb_irq_route1 & PPC_BITMASK(4, 31))) {
>
>
> Coverity reports an issue :
>
>      CID 1558829:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
>      "lpc->opb_irq_route0 & (70931694131085312ULL /* (0x8000000000000000ULL 
> >> 8) - (0x8000000000000000ULL >> 13) | (0x8000000000000000ULL >> 8) */)" is 
> always 0 regardless of the values of its operands. This occurs as the logical 
> first operand of "||".
>
> Should the above if use PPC_BITMASK32 instead ?

Seems likely to me.

> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                "OPB: setting serirq routing on POWER8 system, 
> > ignoring.\n");
> > +        }
> > +        return;
> > +    }
> >
> > -        irqs = lpc->lpc_hc_irqstat & lpc->lpc_hc_irqmask;
> > -        lpc_to_opb_irq = (irqs != 0);
> > +    for (irq = 0; irq <= 13; irq++) {
> > +        int serirq = (lpc->opb_irq_route1 >> (31 - 5 - (irq * 2))) & 0x3;
> > +        lpc->irq_to_serirq_route[irq] = serirq;
> >       }
> >
> > -    /* We don't honor the polarity register, it's pointless and unused
> > -     * anyway
> > -     */
> > -    if (lpc_to_opb_irq) {
> > -        lpc->opb_irq_input |= OPB_MASTER_IRQ_LPC;
> > -    } else {
> > -        lpc->opb_irq_input &= ~OPB_MASTER_IRQ_LPC;
> > +    for (irq = 14; irq < ISA_NUM_IRQS; irq++) {
> > +        int serirq = (lpc->opb_irq_route0 >> (31 - 9 - (irq * 2))) & 0x3;
>
>
> Coverity reports an issue :
>
>      CID 1558828:    (BAD_SHIFT)
>      In expression "lpc->opb_irq_route0 >> 22 - irq * 2", shifting by a 
> negative amount has undefined behavior.  The shift amount, "22 - irq * 2", is 
> -8.
>
> The shift value (irq * 2) seems incorrect. should it be ((irq - 14) * 2) ?

You can also detect this one by doing a clang undefined-sanitizer
build: we try to do a negative shift on startup:

$ ./build/clang/qemu-system-ppc64 -M powernv9
../../hw/ppc/pnv_lpc.c:444:43: runtime error: shift exponent -6 is negative
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../../hw/ppc/pnv_lpc.c:444:43 in

thanks
-- PMM