On Mon, 2022-08-22 at 13:42 +1000, Alistair Francis wrote:
> On Mon, Aug 22, 2022 at 9:53 AM Wilfred Mallawa
> <wilfred.mall...@opensource.wdc.com> wrote:
> >
> > From: Wilfred Mallawa <wilfred.mall...@wdc.com>
> >
> > This patch addresses the coverity issues specified in [1],
> > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been
> > implemented to clean up the code.
> >
> > [1]
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html
> >
> > Fixes: Coverity CID 1488107
> >
> > Signed-off-by: Wilfred Mallawa <wilfred.mall...@wdc.com>
> > Reviewed-by: Andrew Jones <ajo...@ventanamicro.com>
> > ---
> > hw/ssi/ibex_spi_host.c | 130 +++++++++++++++++++++----------------
> > ----
> > 1 file changed, 66 insertions(+), 64 deletions(-)
> >
> > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c
> > index 601041d719..1298664d2b 100644
> > --- a/hw/ssi/ibex_spi_host.c
> > +++ b/hw/ssi/ibex_spi_host.c
> > @@ -108,18 +108,20 @@ static inline uint8_t div4_round_up(uint8_t
> > dividend)
> >
> > static void ibex_spi_rxfifo_reset(IbexSPIHostState *s)
> > {
> > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS];
> > /* Empty the RX FIFO and assert RXEMPTY */
> > fifo8_reset(&s->rx_fifo);
> > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK;
> > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK;
> > + data = FIELD_DP32(data, STATUS, RXEMPTY, 1);
>
> Doesn't this no longer clear the RXFULL bits?
>
> > + s->regs[IBEX_SPI_HOST_STATUS] = data;
> > }
> >
> > static void ibex_spi_txfifo_reset(IbexSPIHostState *s)
> > {
> > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS];
> > /* Empty the TX FIFO and assert TXEMPTY */
> > fifo8_reset(&s->tx_fifo);
> > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK;
> > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK;
> > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1);
>
> Same here about TXFULL
>
> Otherwise the patch looks good
>
> Alistair
>
Good catch! I'll fix these up.
Wilfred
> > + s->regs[IBEX_SPI_HOST_STATUS] = data;
> > }
> >
> > static void ibex_spi_host_reset(DeviceState *dev)
> > @@ -162,37 +164,38 @@ static void ibex_spi_host_reset(DeviceState
> > *dev)
> > */
> > static void ibex_spi_host_irq(IbexSPIHostState *s)
> > {
> > - bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE]
> > - & R_INTR_ENABLE_ERROR_MASK;
> > - bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE]
> > - & R_INTR_ENABLE_SPI_EVENT_MASK;
> > - bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE]
> > - & R_INTR_STATE_ERROR_MASK;
> > - bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE]
> > - & R_INTR_STATE_SPI_EVENT_MASK;
> > + uint32_t intr_test_reg = s->regs[IBEX_SPI_HOST_INTR_TEST];
> > + uint32_t intr_en_reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE];
> > + uint32_t intr_state_reg = s->regs[IBEX_SPI_HOST_INTR_STATE];
> > +
> > + uint32_t err_en_reg = s->regs[IBEX_SPI_HOST_ERROR_ENABLE];
> > + uint32_t event_en_reg = s->regs[IBEX_SPI_HOST_EVENT_ENABLE];
> > + uint32_t err_status_reg = s->regs[IBEX_SPI_HOST_ERROR_STATUS];
> > + uint32_t status_reg = s->regs[IBEX_SPI_HOST_STATUS];
> > +
> > +
> > + bool error_en = FIELD_EX32(intr_en_reg, INTR_ENABLE, ERROR);
> > + bool event_en = FIELD_EX32(intr_en_reg, INTR_ENABLE,
> > SPI_EVENT);
> > + bool err_pending = FIELD_EX32(intr_state_reg, INTR_STATE,
> > ERROR);
> > + bool status_pending = FIELD_EX32(intr_state_reg, INTR_STATE,
> > SPI_EVENT);
> > +
> > int err_irq = 0, event_irq = 0;
> >
> > /* Error IRQ enabled and Error IRQ Cleared */
> > if (error_en && !err_pending) {
> > /* Event enabled, Interrupt Test Error */
> > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] &
> > R_INTR_TEST_ERROR_MASK) {
> > + if (FIELD_EX32(intr_test_reg, INTR_TEST, ERROR)) {
> > err_irq = 1;
> > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
> > - & R_ERROR_ENABLE_CMDBUSY_MASK) &&
> > - s->regs[IBEX_SPI_HOST_ERROR_STATUS]
> > - & R_ERROR_STATUS_CMDBUSY_MASK) {
> > + } else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CMDBUSY)
> > &&
> > + FIELD_EX32(err_status_reg, ERROR_STATUS,
> > CMDBUSY)) {
> > /* Wrote to COMMAND when not READY */
> > err_irq = 1;
> > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
> > - & R_ERROR_ENABLE_CMDINVAL_MASK) &&
> > - s->regs[IBEX_SPI_HOST_ERROR_STATUS]
> > - & R_ERROR_STATUS_CMDINVAL_MASK) {
> > + } else if (FIELD_EX32(err_en_reg, ERROR_ENABLE,
> > CMDINVAL) &&
> > + FIELD_EX32(err_status_reg, ERROR_STATUS,
> > CMDINVAL)) {
> > /* Invalid command segment */
> > err_irq = 1;
> > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
> > - & R_ERROR_ENABLE_CSIDINVAL_MASK) &&
> > - s->regs[IBEX_SPI_HOST_ERROR_STATUS]
> > - & R_ERROR_STATUS_CSIDINVAL_MASK) {
> > + } else if (FIELD_EX32(err_en_reg, ERROR_ENABLE,
> > CSIDINVAL) &&
> > + FIELD_EX32(err_status_reg, ERROR_STATUS,
> > CSIDINVAL)) {
> > /* Invalid value for CSID */
> > err_irq = 1;
> > }
> > @@ -204,22 +207,19 @@ static void
> > ibex_spi_host_irq(IbexSPIHostState *s)
> >
> > /* Event IRQ Enabled and Event IRQ Cleared */
> > if (event_en && !status_pending) {
> > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] &
> > R_INTR_TEST_SPI_EVENT_MASK) {
> > + if (FIELD_EX32(intr_test_reg, INTR_STATE, SPI_EVENT)) {
> > /* Event enabled, Interrupt Test Event */
> > event_irq = 1;
> > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE]
> > - & R_EVENT_ENABLE_READY_MASK) &&
> > - (s->regs[IBEX_SPI_HOST_STATUS] &
> > R_STATUS_READY_MASK)) {
> > + } else if (FIELD_EX32(event_en_reg, EVENT_ENABLE, READY)
> > &&
> > + FIELD_EX32(status_reg, STATUS, READY)) {
> > /* SPI Host ready for next command */
> > event_irq = 1;
> > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE]
> > - & R_EVENT_ENABLE_TXEMPTY_MASK) &&
> > - (s->regs[IBEX_SPI_HOST_STATUS] &
> > R_STATUS_TXEMPTY_MASK)) {
> > + } else if (FIELD_EX32(event_en_reg, EVENT_ENABLE,
> > TXEMPTY) &&
> > + FIELD_EX32(status_reg, STATUS, TXEMPTY)) {
> > /* SPI TXEMPTY, TXFIFO drained */
> > event_irq = 1;
> > - } else if ((s->regs[IBEX_SPI_HOST_EVENT_ENABLE]
> > - & R_EVENT_ENABLE_RXFULL_MASK) &&
> > - (s->regs[IBEX_SPI_HOST_STATUS] &
> > R_STATUS_RXFULL_MASK)) {
> > + } else if (FIELD_EX32(event_en_reg, EVENT_ENABLE, RXFULL)
> > &&
> > + FIELD_EX32(status_reg, STATUS, RXFULL)) {
> > /* SPI RXFULL, RXFIFO full */
> > event_irq = 1;
> > }
> > @@ -232,10 +232,11 @@ static void
> > ibex_spi_host_irq(IbexSPIHostState *s)
> >
> > static void ibex_spi_host_transfer(IbexSPIHostState *s)
> > {
> > - uint32_t rx, tx;
> > + uint32_t rx, tx, data;
> > /* Get num of one byte transfers */
> > - uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] &
> > R_COMMAND_LEN_MASK)
> > - >> R_COMMAND_LEN_SHIFT);
> > + uint8_t segment_len = FIELD_EX32(s-
> > >regs[IBEX_SPI_HOST_COMMAND],
> > + COMMAND, LEN);
> > +
> > while (segment_len > 0) {
> > if (fifo8_is_empty(&s->tx_fifo)) {
> > /* Assert Stall */
> > @@ -262,22 +263,21 @@ static void
> > ibex_spi_host_transfer(IbexSPIHostState *s)
> > --segment_len;
> > }
> >
> > + data = s->regs[IBEX_SPI_HOST_STATUS];
> > /* Assert Ready */
> > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK;
> > + data = FIELD_DP32(data, STATUS, READY, 1);
> > /* 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));
> > + data = FIELD_DP32(data, STATUS, RXQD,
> > div4_round_up(segment_len));
> > /* 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;
> > + data = FIELD_DP32(data, STATUS, TXQD, fifo8_num_used(&s-
> > >tx_fifo) / 4);
> > /* Clear TXFULL */
> > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK;
> > - /* Assert TXEMPTY and drop remaining bytes that exceed
> > segment_len */
> > - ibex_spi_txfifo_reset(s);
> > + data = FIELD_DP32(data, STATUS, TXFULL, 0);
> > /* Reset RXEMPTY */
> > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXEMPTY_MASK;
> > + data = FIELD_DP32(data, STATUS, RXEMPTY, 0);
> > + /* Update register status */
> > + s->regs[IBEX_SPI_HOST_STATUS] = data;
> > + /* Drop remaining bytes that exceed segment_len */
> > + ibex_spi_txfifo_reset(s);
> >
> > ibex_spi_host_irq(s);
> > }
> > @@ -340,7 +340,7 @@ static void ibex_spi_host_write(void *opaque,
> > hwaddr addr,
> > {
> > IbexSPIHostState *s = opaque;
> > uint32_t val32 = val64;
> > - uint32_t shift_mask = 0xff;
> > + uint32_t shift_mask = 0xff, data = 0, status = 0;
> > uint8_t txqd_len;
> >
> > trace_ibex_spi_host_write(addr, size, val64);
> > @@ -397,22 +397,24 @@ static void ibex_spi_host_write(void *opaque,
> > hwaddr addr,
> > s->regs[addr] = val32;
> >
> > /* STALL, IP not enabled */
> > - if (!(s->regs[IBEX_SPI_HOST_CONTROL] &
> > R_CONTROL_SPIEN_MASK)) {
> > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_CONTROL],
> > + CONTROL, SPIEN))) {
> > return;
> > }
> >
> > /* SPI not ready, IRQ Error */
> > - if (!(s->regs[IBEX_SPI_HOST_STATUS] &
> > R_STATUS_READY_MASK)) {
> > + if (!(FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS],
> > + STATUS, READY))) {
> > s->regs[IBEX_SPI_HOST_ERROR_STATUS] |=
> > R_ERROR_STATUS_CMDBUSY_MASK;
> > ibex_spi_host_irq(s);
> > return;
> > }
> > +
> > /* Assert Not Ready */
> > s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_READY_MASK;
> >
> > - if (((val32 & R_COMMAND_DIRECTION_MASK) >>
> > R_COMMAND_DIRECTION_SHIFT)
> > - != BIDIRECTIONAL_TRANSFER) {
> > - qemu_log_mask(LOG_UNIMP,
> > + if (FIELD_EX32(val32, COMMAND, DIRECTION) !=
> > BIDIRECTIONAL_TRANSFER) {
> > + qemu_log_mask(LOG_UNIMP,
> > "%s: Rx Only/Tx Only are not
> > supported\n", __func__);
> > }
> >
> > @@ -452,8 +454,8 @@ static void ibex_spi_host_write(void *opaque,
> > hwaddr addr,
> > return;
> > }
> > /* Byte ordering is set by the IP */
> > - if ((s->regs[IBEX_SPI_HOST_STATUS] &
> > - R_STATUS_BYTEORDER_MASK) == 0) {
> > + status = s->regs[IBEX_SPI_HOST_STATUS];
> > + if (FIELD_EX32(status, STATUS, BYTEORDER) == 0) {
> > /* LE: LSB transmitted first (default for ibex
> > processor) */
> > shift_mask = 0xff << (i * 8);
> > } else {
> > @@ -464,18 +466,18 @@ static void ibex_spi_host_write(void *opaque,
> > hwaddr addr,
> >
> > fifo8_push(&s->tx_fifo, (val32 & shift_mask) >> (i *
> > 8));
> > }
> > -
> > + status = s->regs[IBEX_SPI_HOST_STATUS];
> > /* Reset TXEMPTY */
> > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXEMPTY_MASK;
> > + status = FIELD_DP32(status, STATUS, TXEMPTY, 0);
> > /* Update TXQD */
> > - txqd_len = (s->regs[IBEX_SPI_HOST_STATUS] &
> > - R_STATUS_TXQD_MASK) >> R_STATUS_TXQD_SHIFT;
> > + txqd_len = FIELD_EX32(status, STATUS, TXQD);
> > /* Partial bytes (size < 4) are padded, in words. */
> > txqd_len += 1;
> > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK;
> > - s->regs[IBEX_SPI_HOST_STATUS] |= txqd_len;
> > + status = FIELD_DP32(status, STATUS, TXQD, txqd_len);
> > /* Assert Ready */
> > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK;
> > + status = FIELD_DP32(status, STATUS, READY, 1);
> > + /* Update register status */
> > + s->regs[IBEX_SPI_HOST_STATUS] = status;
> > break;
> > case IBEX_SPI_HOST_ERROR_ENABLE:
> > s->regs[addr] = val32;
> > --
> > 2.37.2
> >
> >
