[PATCH 2/3] hw/ssi: fixup coverity issue
From: Wilfred Mallawa This patch addresses the coverity issues specified in [1], as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been implemented to clean up the code. Additionally, the `EVENT_ENABLE` register is correctly updated to addr of `0x34`. [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html Fixes: Coverity CID 1488107 Signed-off-by: Wilfred Mallawa --- hw/ssi/ibex_spi_host.c | 141 +++-- 1 file changed, 78 insertions(+), 63 deletions(-) diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c index 601041d719..8c35bfa95f 100644 --- a/hw/ssi/ibex_spi_host.c +++ b/hw/ssi/ibex_spi_host.c @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) FIELD(ERROR_STATUS, CMDINVAL, 3, 1) FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) -REG32(EVENT_ENABLE, 0x30) +REG32(EVENT_ENABLE, 0x34) FIELD(EVENT_ENABLE, RXFULL, 0, 1) FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) FIELD(EVENT_ENABLE, RXWM, 2, 1) @@ -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); +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); +s->regs[IBEX_SPI_HOST_STATUS] = data; } static void ibex_spi_host_reset(DeviceState *dev) @@ -162,37 +164,41 @@ 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; +bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], + INTR_ENABLE, ERROR); + +bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], + INTR_ENABLE, SPI_EVENT); + +bool err_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], + INTR_STATE, ERROR); + +bool status_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], + 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(s->regs[IBEX_SPI_HOST_INTR_TEST], 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(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], + ERROR_ENABLE, CMDBUSY) && +FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], + 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(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], + ERROR_ENABLE, CMDINVAL) && +FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], + 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(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], + ERROR_ENABLE, CSIDINVAL) && +FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], + ERROR_STATUS, CSIDINVAL
Re: [PATCH 2/3] hw/ssi: fixup coverity issue
On Thu, Aug 11, 2022 at 8:58 AM Wilfred Mallawa wrote: > > From: Wilfred Mallawa > > This patch addresses the coverity issues specified in [1], > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been > implemented to clean up the code. > > Additionally, the `EVENT_ENABLE` register is correctly updated > to addr of `0x34`. > > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html > > Fixes: Coverity CID 1488107 > > Signed-off-by: Wilfred Mallawa nits: please add "ibex_spi" to the tag, like hw/ssi: ibex_spi: > --- > hw/ssi/ibex_spi_host.c | 141 +++-- > 1 file changed, 78 insertions(+), 63 deletions(-) > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > index 601041d719..8c35bfa95f 100644 > --- a/hw/ssi/ibex_spi_host.c > +++ b/hw/ssi/ibex_spi_host.c > @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) > FIELD(ERROR_STATUS, CMDINVAL, 3, 1) > FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) > FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) > -REG32(EVENT_ENABLE, 0x30) > +REG32(EVENT_ENABLE, 0x34) > FIELD(EVENT_ENABLE, RXFULL, 0, 1) > FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) > FIELD(EVENT_ENABLE, RXWM, 2, 1) > @@ -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); > +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); > +s->regs[IBEX_SPI_HOST_STATUS] = data; > } > > static void ibex_spi_host_reset(DeviceState *dev) > @@ -162,37 +164,41 @@ 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; > +bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > + INTR_ENABLE, ERROR); > + > +bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > + INTR_ENABLE, SPI_EVENT); > + > +bool err_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], > + INTR_STATE, ERROR); > + > +bool status_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], > + 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(s->regs[IBEX_SPI_HOST_INTR_TEST], 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(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > + ERROR_ENABLE, CMDBUSY) && > +FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], > + 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(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > + ERROR_ENABLE, CMDINVAL) && > +FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], > + 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[IBE
Re: [PATCH 2/3] hw/ssi: fixup coverity issue
On Thu, Aug 11, 2022 at 09:02:00AM +1000, Wilfred Mallawa wrote: > From: Wilfred Mallawa > > This patch addresses the coverity issues specified in [1], > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been > implemented to clean up the code. > > Additionally, the `EVENT_ENABLE` register is correctly updated > to addr of `0x34`. This sounds like separate patch material. > > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html > > Fixes: Coverity CID 1488107 > > Signed-off-by: Wilfred Mallawa > --- > hw/ssi/ibex_spi_host.c | 141 +++-- > 1 file changed, 78 insertions(+), 63 deletions(-) > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > index 601041d719..8c35bfa95f 100644 > --- a/hw/ssi/ibex_spi_host.c > +++ b/hw/ssi/ibex_spi_host.c > @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) > FIELD(ERROR_STATUS, CMDINVAL, 3, 1) > FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) > FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) > -REG32(EVENT_ENABLE, 0x30) > +REG32(EVENT_ENABLE, 0x34) > FIELD(EVENT_ENABLE, RXFULL, 0, 1) > FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) > FIELD(EVENT_ENABLE, RXWM, 2, 1) > @@ -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); > +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); > +s->regs[IBEX_SPI_HOST_STATUS] = data; > } > > static void ibex_spi_host_reset(DeviceState *dev) > @@ -162,37 +164,41 @@ 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; > +bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > + INTR_ENABLE, ERROR); > + > +bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > + INTR_ENABLE, SPI_EVENT); > + > +bool err_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], > + INTR_STATE, ERROR); > + > +bool status_pending = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_STATE], > + INTR_STATE, SPI_EVENT); uint32_t reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE]; bool error_en = FIELD_EX32(reg, INTR_ENABLE, ERROR); bool event_en = FIELD_EX32(reg, INTR_ENABLE, SPI_EVENT); ... would like nicer, IMHO. > + > 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(s->regs[IBEX_SPI_HOST_INTR_TEST], 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(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > + ERROR_ENABLE, CMDBUSY) && > +FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], > + 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(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > + ERROR_ENABLE, CMDINVAL) && > +FIELD_EX32(s->regs[IBEX_SPI_HOST_ERROR_STATUS], > + ERROR_STATUS, CMDINVAL)) { > /* Invalid co
Re: [PATCH 2/3] hw/ssi: fixup coverity issue
On Thu, 2022-08-11 at 10:55 +0800, Bin Meng wrote: > On Thu, Aug 11, 2022 at 8:58 AM Wilfred Mallawa > wrote: > > > > From: Wilfred Mallawa > > > > This patch addresses the coverity issues specified in [1], > > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been > > implemented to clean up the code. > > > > Additionally, the `EVENT_ENABLE` register is correctly updated > > to addr of `0x34`. > > > > [1] > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html > > > > Fixes: Coverity CID 1488107 > > > > Signed-off-by: Wilfred Mallawa > > nits: please add "ibex_spi" to the tag, like hw/ssi: ibex_spi: will add in v2 :) > > > --- > > hw/ssi/ibex_spi_host.c | 141 +++-- > > > > 1 file changed, 78 insertions(+), 63 deletions(-) > > > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > > index 601041d719..8c35bfa95f 100644 > > --- a/hw/ssi/ibex_spi_host.c > > +++ b/hw/ssi/ibex_spi_host.c > > @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) > > FIELD(ERROR_STATUS, CMDINVAL, 3, 1) > > FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) > > FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) > > -REG32(EVENT_ENABLE, 0x30) > > +REG32(EVENT_ENABLE, 0x34) > > FIELD(EVENT_ENABLE, RXFULL, 0, 1) > > FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) > > FIELD(EVENT_ENABLE, RXWM, 2, 1) > > @@ -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); > > + 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); > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > } > > > > static void ibex_spi_host_reset(DeviceState *dev) > > @@ -162,37 +164,41 @@ 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; > > + bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > > + INTR_ENABLE, ERROR); > > + > > + bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > > + INTR_ENABLE, SPI_EVENT); > > + > > + bool err_pending = FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_INTR_STATE], > > + INTR_STATE, ERROR); > > + > > + bool status_pending = FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_INTR_STATE], > > + 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(s->regs[IBEX_SPI_HOST_INTR_TEST], > > 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(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > + ERROR_ENABLE, CMDBUSY) && > > + FIELD_EX32(s- > > >regs[IBEX_SPI_HOST_ERROR_STATUS], > > + 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(s->regs[IBEX_SPI_HOST_ERROR_ENABLE], > > + ERROR_ENABLE, CMDINVAL) && > > +
Re: [PATCH 2/3] hw/ssi: fixup coverity issue
On Fri, Aug 12, 2022 at 02:21:40AM +, Wilfred Mallawa wrote: > On Thu, 2022-08-11 at 16:23 +0200, Andrew Jones wrote: > > On Thu, Aug 11, 2022 at 09:02:00AM +1000, Wilfred Mallawa wrote: > > > From: Wilfred Mallawa > > > > > > This patch addresses the coverity issues specified in [1], > > > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been > > > implemented to clean up the code. > > > > > > Additionally, the `EVENT_ENABLE` register is correctly updated > > > to addr of `0x34`. > > > > This sounds like separate patch material. > > > > > > > > [1] > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html > > > > > > Fixes: Coverity CID 1488107 > > > > > > Signed-off-by: Wilfred Mallawa > > > --- > > > hw/ssi/ibex_spi_host.c | 141 +++-- > > > > > > 1 file changed, 78 insertions(+), 63 deletions(-) > > > > > > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c > > > index 601041d719..8c35bfa95f 100644 > > > --- a/hw/ssi/ibex_spi_host.c > > > +++ b/hw/ssi/ibex_spi_host.c > > > @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) > > > FIELD(ERROR_STATUS, CMDINVAL, 3, 1) > > > FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) > > > FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) > > > -REG32(EVENT_ENABLE, 0x30) > > > +REG32(EVENT_ENABLE, 0x34) > > > FIELD(EVENT_ENABLE, RXFULL, 0, 1) > > > FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) > > > FIELD(EVENT_ENABLE, RXWM, 2, 1) > > > @@ -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); > > > + 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); > > > + s->regs[IBEX_SPI_HOST_STATUS] = data; > > > } > > > > > > static void ibex_spi_host_reset(DeviceState *dev) > > > @@ -162,37 +164,41 @@ 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; > > > + bool error_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > > > + INTR_ENABLE, ERROR); > > > + > > > + bool event_en = FIELD_EX32(s->regs[IBEX_SPI_HOST_INTR_ENABLE], > > > + INTR_ENABLE, SPI_EVENT); > > > + > > > + bool err_pending = FIELD_EX32(s- > > > >regs[IBEX_SPI_HOST_INTR_STATE], > > > + INTR_STATE, ERROR); > > > + > > > + bool status_pending = FIELD_EX32(s- > > > >regs[IBEX_SPI_HOST_INTR_STATE], > > > + INTR_STATE, SPI_EVENT); > > > > uint32_t reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE]; > > bool error_en = FIELD_EX32(reg, INTR_ENABLE, ERROR); > > bool event_en = FIELD_EX32(reg, INTR_ENABLE, SPI_EVENT); > > ... > > > > would like nicer, IMHO. > as per the comment below, do you mean to make all the conditions > variables here? and substitute those for the if statements instead of > the `FIELD_..` macros? For the above code, the suggestion I gave is what I'm thinking. For the below code, keep the FIELD macros in place, but shrink the length of the line by doing uint32_t enable = s->regs[IBEX_SPI_HOST_ERROR_ENABLE]; uint32_t status = s->regs[IBEX_SPI_HOST_ERROR_STATUS]; at the top, and then } else if (FIELD_EX32(enable, ERROR_ENABLE, CMDBUSY) && FIELD_EX32(status, ERROR_STATUS, CMDBUSY)) { ... > > > > > > > + > > > 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(s->regs[IBEX_SPI_HOST_INT