Re: [PULL 38/96] hw/ssi: Extend SPI model

[Peter Maydell]

On Mon, 29 Jul 2024 at 11:33, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 7/26/24 01:53, Nicholas Piggin wrote:
> > From: Chalapathi V <chalapathi.v@linux.ibm.com>
> >
> > In this commit SPI shift engine and sequencer logic is implemented.
> > Shift engine performs serialization and de-serialization according to the
> > control by the sequencer and according to the setup defined in the
> > configuration registers. Sequencer implements the main control logic and
> > FSM to handle data transmit and data receive control of the shift engine.




> > +static inline uint8_t get_seq_index(PnvSpi *s)
> > +{
> > +    return GETFIELD(SPI_STS_SEQ_INDEX, s->status);
> > +}
>
> Coverity reports :
>
> >>>     CID 1558827:    (OVERRUN)
> >>>     Overrunning array "s->seq_op" of 8 bytes at byte offset 16 using 
> >>> index "get_seq_index(s) + 1" (which evaluates to 16).
>
>
> get_seq_index() can return a value between 0 and 15 and it is used in a couple
> of places to index array s->seq_op[] which is an 8 bytes array.
>
> Should we increase the size of the seq_op array ?

I think in most places this can't happen, because:

> > +    /*
> > +     * There are only 8 possible operation IDs to iterate through though
> > +     * some operations may cause more than one frame to be sequenced.
> > +     */
> > +    while (get_seq_index(s) < NUM_SEQ_OPS) {

The loop condition enforces that the sequence index is < 8.

> > +        opcode = s->seq_op[get_seq_index(s)];

Coverity isn't smart enough to be able to tell that this
call to get_seq_index() must return the same value as the
previous one and so be in bounds for the array, which is why it's
complaining.

On the other hand:
 * this is also not totally obvious to humans
 * it means we're doing the shift-and-mask ops to get the
   sequence index out every time

Maybe we should keep it in a variable? (Depends whether there's
a bunch of places in the loop that want to update the index, or
if we just do that at the end of the loop.)

Another option would be to have a version of get_seq_index() that
asserts that the index is valid.

The other design approach available here is to have the device
state struct hold the underlying information in a split-out way
(so e.g s->seq_index and other fields), and then assemble those
into a 32-bit value for the status register in the "guest reads/writes
the register codepath).

That said, there is also a case which I suspect really is an
overrun bug:

> > +        case SEQ_OP_SHIFT_N1:
> > +            s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status, 
> > SEQ_STATE_EXECUTE);
> > +            trace_pnv_spi_sequencer_op("SHIFT_N1", get_seq_index(s));
> > +            /*
> > +             * Only allow a shift_n1 when the state is not IDLE or DONE.
> > +             * In either of those two cases the sequencer is not in a 
> > proper
> > +             * state to perform shift operations because the sequencer has:
> > +             * - processed a responder deselect (DONE)
> > +             * - processed a stop opcode (IDLE)
> > +             * - encountered an error (IDLE)
> > +             */
> > +            if ((GETFIELD(SPI_STS_SHIFTER_FSM, s->status) == FSM_IDLE) ||
> > +                (GETFIELD(SPI_STS_SHIFTER_FSM, s->status) == FSM_DONE)) {
> > +                qemu_log_mask(LOG_GUEST_ERROR, "Shift_N1 not allowed in "
> > +                              "shifter state = 0x%llx", GETFIELD(
> > +                        SPI_STS_SHIFTER_FSM, s->status));
> > +                /*
> > +                 * Set sequencer FSM error bit 3 (general_SPI_status[3])
> > +                 * in status reg.
> > +                 */
> > +                s->status = SETFIELD(SPI_STS_GEN_STATUS_B3, s->status, 1);
> > +                trace_pnv_spi_sequencer_stop_requested("invalid shifter 
> > state");
> > +                stop = true;
> > +            } else {
> > +                /*
> > +                 * Look for the special case where there is a shift_n1 set 
> > for
> > +                 * transmit and it is followed by a shift_n2 set for 
> > transmit
> > +                 * AND the combined transmit length of the two operations 
> > is
> > +                 * less than or equal to the size of the TDR register. In 
> > this
> > +                 * case we want to use both this current shift_n1 opcode 
> > and the
> > +                 * following shift_n2 opcode to assemble the frame for
> > +                 * transmission to the responder without requiring a 
> > refill of
> > +                 * the TDR between the two operations.
> > +                 */
> > +                if (PNV_SPI_MASKED_OPCODE(s->seq_op[get_seq_index(s) + 1])
> > +                                == SEQ_OP_SHIFT_N2) {

Here we look at the seq_op[] array for get_seq_index(s) + 1,
so if we're on the last iteration of the loop because the seq
index is 7 we'll read off the end of the array at index 8.

(Presumably the correct behaviour if the shift_n1 is the last
operation is that it's not this special case.)

> > +                    send_n1_alone = false;
> > +                }
> > +                s->status = SETFIELD(SPI_STS_SHIFTER_FSM, s->status,
> > +                                FSM_SHIFT_N1);

thanks
-- PMM