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

[Peter Maydell]

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
-- PMM