On Fri, May 13, 2022 at 2:37 AM Peter Maydell wrote:
>
> On Fri, 22 Apr 2022 at 01:40, Alistair Francis
> wrote:
> >
> > From: Wilfred Mallawa
> >
> > 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(>tx_fifo)) {
> > +/* Assert Stall */
> > +s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXSTALL_MASK;
> > +break;
> > +} else if (fifo8_is_full(>rx_fifo)) {
> > +/* Assert Stall */
> > +s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXSTALL_MASK;
> > +break;
> > +} else {
> > +tx = fifo8_pop(>tx_fifo);
> > +}
> > +
> > +rx = ssi_transfer(s->ssi, tx);
> > +
> > +trace_ibex_spi_host_transfer(tx, rx);
> > +
> > +if (!fifo8_is_full(>rx_fifo)) {
> > +fifo8_push(>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(>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
