qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/3] hw/ssi: fixup coverity issue


From: Wilfred Mallawa
Subject: Re: [PATCH 2/3] hw/ssi: fixup coverity issue
Date: Fri, 12 Aug 2022 02:21:40 +0000

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 <wilfred.mallawa@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.
> > 
> > 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 <wilfred.mallawa@wdc.com>
> > ---
> >  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? 
> 
> 
> > +
> >      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)) {
> 
> Same comment as above. If we set local variables for
> IBEX_SPI_HOST_ERROR_ENABLE and IBEX_SPI_HOST_ERROR_STATUS at the top
> of
> the ladder, then it should be easier to read.
> 
> 
> >              /* Invalid value for CSID */
> >              err_irq = 1;
> >          }
> > @@ -204,22 +210,26 @@ 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(s->regs[IBEX_SPI_HOST_INTR_TEST],
> > +                       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(s->regs[IBEX_SPI_HOST_EVENT_ENABLE],
> > +                              EVENT_ENABLE,  READY) &&
> > +                    FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS],
> > +                               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(s->regs[IBEX_SPI_HOST_EVENT_ENABLE],
> > +                              EVENT_ENABLE,  TXEMPTY) &&
> > +                    FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS],
> > +                               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(s->regs[IBEX_SPI_HOST_EVENT_ENABLE],
> > +                              EVENT_ENABLE,  RXFULL) &&
> > +                    FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS],
> > +                               STATUS,  RXFULL)) {
> 
> same comment
> 
> >              /* SPI RXFULL, RXFIFO  full */
> >              event_irq = 1;
> >          }
> > @@ -232,10 +242,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 +273,23 @@ 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);
> > +    /* Set TXEMPTY */
> > +    data = FIELD_DP32(data, STATUS, TXEMPTY, 1);
> 
> Why add this here? This is still done in ibex_spi_txfifo_reset()
good catch! will fixup.
> 
> > +    /* Drop remaining bytes that exceed segment_len */
> > +    ibex_spi_txfifo_reset(s);
> > +    /* Update register status */
> > +    s->regs[IBEX_SPI_HOST_STATUS] = data;
> >  
> >      ibex_spi_host_irq(s);
> >  }
> > @@ -340,7 +352,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;
> >      uint8_t txqd_len;
> >  
> >      trace_ibex_spi_host_write(addr, size, val64);
> > @@ -397,21 +409,23 @@ 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) {
> > +        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 +466,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) {
> > +            if (FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], STATUS,
> > +                           BYTEORDER) == 0) {
> 
> Can use local variable for IBEX_SPI_HOST_STATUS to avoid these three
> wrapped
> if conditions
Not sure if I'm following, do you mean to use a local var that holds
the condition? something like:

    bool byte_order_le = FIELD_EX32(s->regs[IBEX_SPI_HOST_STATUS], 
                                    STATUS, BYTEORDER) == 0;
    if (bo_le) {..} else {...}

 In which case we would still need a conditional to check for   
byteorder?
> 
> >                  /* LE: LSB transmitted first (default for ibex
> > processor) */
> >                  shift_mask = 0xff << (i * 8);
> >              } else {
> > @@ -465,17 +479,18 @@ static void ibex_spi_host_write(void *opaque,
> > hwaddr addr,
> >              fifo8_push(&s->tx_fifo, (val32 & shift_mask) >> (i *
> > 8));
> >          }
> >  
> > +        data = s->regs[IBEX_SPI_HOST_STATUS];
> 
> Could probably change the name of data to status or whatever and use
> it
> all the way through.
`data` here is ambiguous as it is used for other registers at the top
of the function and not just the status ones. We could create a new 
var `status` just for this bottom part...if it helps readibility?
> 
> >          /* Reset TXEMPTY */
> > -        s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXEMPTY_MASK;
> > +        data = FIELD_DP32(data, 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(s->regs[IBEX_SPI_HOST_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;
> > +        data = FIELD_DP32(data, STATUS, TXQD, txqd_len);
> >          /* Assert Ready */
> > -        s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK;
> > +        data = FIELD_DP32(data, STATUS, READY, 1);
> > +        /* Update register status */
> > +        s->regs[IBEX_SPI_HOST_STATUS] = data;
> >          break;
> >      case IBEX_SPI_HOST_ERROR_ENABLE:
> >          s->regs[addr] = val32;
> > -- 
> > 2.37.1
> > 
> > 
> 
> Thanks,
> drew
Thanks!
Wilfred

reply via email to

[Prev in Thread] Current Thread [Next in Thread]