Re: [Qemu-devel] [PATCH v6] lsi: Reselection needed to remove pending commands from queue

[Paolo Bonzini]

On 07/11/2018 23:04, George Kennedy wrote:
> Your latest suggestion was the "missing link". Calling lsi_wait_reselect()
> after a WAIT DISCONNECT Script instruction when there are commands on 
> the pending queue is all the is needed. The patch has been greatly
> reduced in size and complexity.

Yeah, it's mixing initiator and target in slightly unhealthy ways, but
it's not something that you introduced.

(Can you also send, perhaps offlist, the other patch you were working on
which tried to introduce the right sequence of disconnect/reselect
before disk accesses and DMA respectively?  I'd like to keep it for
reference).

>  hw/scsi/lsi53c895a.c | 43 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1e6534..7f9ed2f 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -219,6 +219,7 @@ typedef struct {
>      int command_complete;
>      QTAILQ_HEAD(, lsi_request) queue;
>      lsi_request *current;
> +    bool handle_pending;    /* handle queued pending commands */
>  
>      uint32_t dsa;
>      uint32_t temp;
> @@ -298,6 +299,18 @@ static inline int lsi_irq_on_rsl(LSIState *s)
>      return (s->sien0 & LSI_SIST0_RSL) && (s->scid & LSI_SCID_RRE);
>  }
>  
> +static lsi_request *get_pending_req(LSIState *s)
> +{
> +    lsi_request *p;
> +
> +    QTAILQ_FOREACH(p, &s->queue, next) {
> +        if (p->pending) {
> +            return p;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  static void lsi_soft_reset(LSIState *s)
>  {
>      trace_lsi_reset();
> @@ -446,7 +459,6 @@ static void lsi_update_irq(LSIState *s)
>  {
>      int level;
>      static int last_level;
> -    lsi_request *p;
>  
>      /* It's unclear whether the DIP/SIP bits should be cleared when the
>         Interrupt Status Registers are cleared or when istat0 is read.
> @@ -477,12 +489,12 @@ static void lsi_update_irq(LSIState *s)
>      lsi_set_irq(s, level);
>  
>      if (!level && lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON)) {
> +        lsi_request *p;
> +
>          trace_lsi_update_irq_disconnected();
> -        QTAILQ_FOREACH(p, &s->queue, next) {
> -            if (p->pending) {
> -                lsi_reselect(s, p);
> -                break;
> -            }
> +        p = get_pending_req(s);
> +        if (p) {
> +            lsi_reselect(s, p);
>          }
>      }
>  }
> @@ -759,6 +771,8 @@ static void lsi_command_complete(SCSIRequest *req, 
> uint32_t status, size_t resid
>          lsi_request_free(s, s->current);
>          scsi_req_unref(req);
>      }
> +    s->handle_pending = get_pending_req(s) ? true : false;
> +
>      lsi_resume_script(s);
>  }
>  
> @@ -1064,11 +1078,15 @@ static void lsi_wait_reselect(LSIState *s)
>  
>      trace_lsi_wait_reselect();
>  
> -    QTAILQ_FOREACH(p, &s->queue, next) {
> -        if (p->pending) {
> -            lsi_reselect(s, p);
> -            break;
> -        }
> +    s->handle_pending = false;
> +
> +    if (s->current) {
> +        return;
> +    }
> +
> +    p = get_pending_req(s);
> +    if (p) {
> +        lsi_reselect(s, p);
>      }
>      if (s->current == NULL) {
>          s->waiting = 1;
> @@ -1258,6 +1276,9 @@ again:
>              case 1: /* Disconnect */
>                  trace_lsi_execute_script_io_disconnect();
>                  s->scntl1 &= ~LSI_SCNTL1_CON;
> +                if (s->handle_pending) {

Can you explain the if?  Is it to avoid that "s->waiting = 1" assignment?

Also, could the disconnect case check get_pending_req directly, and call
lsi_reselect?  That is, replacing this "if" with

        if (!s->current) {
            p = get_pending_req(s);
            if (p) {
                lsi_reselect(s, p);
            }
        }

instead of calling lsi_wait_reselect?

Thanks,

Paolo

> +                    lsi_wait_reselect(s);
> +                }
>                  break;
>              case 2: /* Wait Reselect */
>                  if (!lsi_irq_on_rsl(s)) {
>