Re: [PULL v2 01/31] hw/ssi: Add Ibex SPI device model

[Alistair Francis]

On Fri, May 13, 2022 at 2:37 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 22 Apr 2022 at 01:40, Alistair Francis
> <alistair.francis@opensource.wdc.com> wrote:
> >
> > From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> >
> > Adds the SPI_HOST device model for ibex. The device specification is as per
> > [1]. The model has been tested on opentitan with spi_host unit tests
> > written for TockOS.
> >
> > [1] https://docs.opentitan.org/hw/ip/spi_host/doc/
>
>
> Hi; Coverity points out a bug in this code (CID 1488107):
>
> > +REG32(STATUS, 0x14)
> > +    FIELD(STATUS, TXQD, 0, 8)
> > +    FIELD(STATUS, RXQD, 18, 8)
>
> RXQD isn't at the bottom of this register, so the R_STATUS_RXQD_MASK
> define is a shifted-up mask...
>
> > +static void ibex_spi_host_transfer(IbexSPIHostState *s)
> > +{
> > +    uint32_t rx, tx;
> > +    /* Get num of one byte transfers */
> > +    uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] & 
> > R_COMMAND_LEN_MASK)
> > +                          >> R_COMMAND_LEN_SHIFT);
> > +    while (segment_len > 0) {
> > +        if (fifo8_is_empty(&s->tx_fifo)) {
> > +            /* Assert Stall */
> > +            s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXSTALL_MASK;
> > +            break;
> > +        } else if (fifo8_is_full(&s->rx_fifo)) {
> > +            /* Assert Stall */
> > +            s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXSTALL_MASK;
> > +            break;
> > +        } else {
> > +            tx = fifo8_pop(&s->tx_fifo);
> > +        }
> > +
> > +        rx = ssi_transfer(s->ssi, tx);
> > +
> > +        trace_ibex_spi_host_transfer(tx, rx);
> > +
> > +        if (!fifo8_is_full(&s->rx_fifo)) {
> > +            fifo8_push(&s->rx_fifo, rx);
> > +        } else {
> > +            /* Assert RXFULL */
> > +            s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXFULL_MASK;
> > +        }
> > +        --segment_len;
> > +    }
> > +
> > +    /* Assert Ready */
> > +    s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK;
> > +    /* Set RXQD */
> > +    s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXQD_MASK;
> > +    s->regs[IBEX_SPI_HOST_STATUS] |= (R_STATUS_RXQD_MASK
> > +                                    & div4_round_up(segment_len));
>
> ...but here we don't shift div4_round_up(segment_len) up to the
> right place before ORing it with the MASK, so Coverity points
> out that the result is always zero.
>
> > +    /* Set TXQD */
> > +    s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK;
> > +    s->regs[IBEX_SPI_HOST_STATUS] |= (fifo8_num_used(&s->tx_fifo) / 4)
> > +                                    & R_STATUS_TXQD_MASK;
>
> This has the same issue, but avoids it by luck because TXQD
> does start at bit 0.
>
> Since we're using the FIELD() macros, it would be clearer to
> write all this to use FIELD_DP32() rather than manual
> bit operations to clear the bits and then OR in the new ones.
> (True here and also in what looks like several other places
> through out the file, for deposit and extract operations.)

Thanks Peter,

Wilfred is looking into it and should be sending patches soon.

Alistair

>
> thanks
> -- PMM