[Qemu-devel] [PATCH v3] lsi_scsi: Reselection needed to remove pending commands from queue

[Qemu-devel] [PATCH v3] lsi_scsi: Reselection needed to remove pending commands from queue

[George Kennedy]

Under heavy IO (e.g. fio) the queue is not checked frequently enough for
pending commands. As a result some pending commands are timed out by the
linux sym53c8xx driver, which sends SCSI Abort messages for the timed out
commands. The SCSI Abort messages result in linux errors, which show up
on the console and in /var/log/messages.

e.g.
sd 0:0:3:0: [sdd] tag#33 ABORT operation started
scsi target0:0:3: control msgout:
80 20 47 d
sd 0:0:3:0: ABORT operation complete.
scsi target0:0:4: message d sent on bad reselection

When the current command completes, check if there is a pending command
on the queue and if a pending command exists, set a flag indicating
that a Wait Reselect command is needed to handle a queued pending command.

When a Wait Reselect is needed, intercept and save the current DMA Scripts
Ptr (DSP) contents and load it instead with the pointer to the Reselection
Scripts.  When Reselection has completed, restore the original DSP contents.

Signed-off-by: George Kennedy <george.kennedy@oracle.com>
---
 hw/scsi/lsi53c895a.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index d1e6534..b783d72 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -219,6 +219,8 @@ typedef struct {
     int command_complete;
     QTAILQ_HEAD(, lsi_request) queue;
     lsi_request *current;
+    bool want_resel;    /* need resel to handle queued completed cmds */
+    uint32_t resel_dsp; /* DMA Scripts Ptr (DSP) of reselection scsi scripts */
 
     uint32_t dsa;
     uint32_t temp;
@@ -298,6 +300,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 +460,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 +490,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 +772,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->want_resel = get_pending_req(s) ? true : false;
+
     lsi_resume_script(s);
 }
 
@@ -1064,11 +1079,13 @@ 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;
-        }
+    if (s->current) {
+        return;
+    }
+
+    p = get_pending_req(s);
+    if (p) {
+        lsi_reselect(s, p);
     }
     if (s->current == NULL) {
         s->waiting = 1;
@@ -1082,6 +1099,7 @@ static void lsi_execute_script(LSIState *s)
     uint32_t addr, addr_high;
     int opcode;
     int insn_processed = 0;
+    uint32_t save_dsp = 0;
 
     s->istat1 |= LSI_ISTAT1_SRUN;
 again:
@@ -1260,7 +1278,14 @@ again:
                 s->scntl1 &= ~LSI_SCNTL1_CON;
                 break;
             case 2: /* Wait Reselect */
+                if (!s->resel_dsp) {
+                    s->resel_dsp = s->dsp - 8;
+                }
                 if (!lsi_irq_on_rsl(s)) {
+                    if (save_dsp) {
+                        s->dsp = save_dsp;
+                        save_dsp = 0;
+                    }
                     lsi_wait_reselect(s);
                 }
                 break;
@@ -1476,6 +1501,10 @@ again:
 
             if (insn & (1 << 28)) {
                 addr = s->dsa + sextract32(addr, 0, 24);
+            } else if (s->want_resel && s->resel_dsp && !(insn & (1 << 24))) {
+                save_dsp = s->dsp;
+                s->dsp = s->resel_dsp;
+                s->want_resel = false;
             }
             n = (insn & 7);
             reg = (insn >> 16) & 0xff;
-- 
1.8.3.1

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

[Paolo Bonzini]

On 30/10/2018 22:42, George Kennedy wrote:
> Under heavy IO (e.g. fio) the queue is not checked frequently enough for
> pending commands. As a result some pending commands are timed out by the
> linux sym53c8xx driver, which sends SCSI Abort messages for the timed out
> commands. The SCSI Abort messages result in linux errors, which show up
> on the console and in /var/log/messages.
> 
> e.g.
> sd 0:0:3:0: [sdd] tag#33 ABORT operation started
> scsi target0:0:3: control msgout:
> 80 20 47 d
> sd 0:0:3:0: ABORT operation complete.
> scsi target0:0:4: message d sent on bad reselection
> 
> When the current command completes, check if there is a pending command
> on the queue and if a pending command exists, set a flag indicating
> that a Wait Reselect command is needed to handle a queued pending command.
> 
> When a Wait Reselect is needed, intercept and save the current DMA Scripts
> Ptr (DSP) contents and load it instead with the pointer to the Reselection
> Scripts.  When Reselection has completed, restore the original DSP contents.
> 
> Signed-off-by: George Kennedy <george.kennedy@oracle.com>
> ---
>  hw/scsi/lsi53c895a.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1e6534..b783d72 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -219,6 +219,8 @@ typedef struct {
>      int command_complete;
>      QTAILQ_HEAD(, lsi_request) queue;
>      lsi_request *current;
> +    bool want_resel;    /* need resel to handle queued completed cmds */
> +    uint32_t resel_dsp; /* DMA Scripts Ptr (DSP) of reselection scsi scripts */
>  
>      uint32_t dsa;
>      uint32_t temp;
> @@ -298,6 +300,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 +460,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 +490,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 +772,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->want_resel = get_pending_req(s) ? true : false;
> +
>      lsi_resume_script(s);
>  }
>  
> @@ -1064,11 +1079,13 @@ 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;
> -        }
> +    if (s->current) {
> +        return;
> +    }
> +
> +    p = get_pending_req(s);
> +    if (p) {
> +        lsi_reselect(s, p);
>      }
>      if (s->current == NULL) {
>          s->waiting = 1;
> @@ -1082,6 +1099,7 @@ static void lsi_execute_script(LSIState *s)
>      uint32_t addr, addr_high;
>      int opcode;
>      int insn_processed = 0;
> +    uint32_t save_dsp = 0;
>  
>      s->istat1 |= LSI_ISTAT1_SRUN;
>  again:
> @@ -1260,7 +1278,14 @@ again:
>                  s->scntl1 &= ~LSI_SCNTL1_CON;
>                  break;
>              case 2: /* Wait Reselect */
> +                if (!s->resel_dsp) {
> +                    s->resel_dsp = s->dsp - 8;
> +                }

What is the reason for the "if"?

>                  if (!lsi_irq_on_rsl(s)) {
> +                    if (save_dsp) {
> +                        s->dsp = save_dsp;
> +                        save_dsp = 0;
> +                    }
>                      lsi_wait_reselect(s);
>                  }
>                  break;
> @@ -1476,6 +1501,10 @@ again:
>  
>              if (insn & (1 << 28)) {
>                  addr = s->dsa + sextract32(addr, 0, 24);
> +            } else if (s->want_resel && s->resel_dsp && !(insn & (1 << 24))) {
> +                save_dsp = s->dsp;
> +                s->dsp = s->resel_dsp;
> +                s->want_resel = false;
>              }

Can you explain why this is placed under the LOAD AND STORE instruction?

Your previous patch has, more specifically, a store from the scratch
register into memory.  In the Linux scripts this occurs twice, both
after a DISCONNECT instruction.  Is that the place where you want to hook?

(All this is a bit of a hack of course, and I would rather have the
actual solution that adds the appropriate RESELECTION and MESSAGE IN
phases whenever the device has finished the data transfer.  But with a
FIXME comment I guess it might be okay).

Paolo

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

[George Kennedy]

On 10/31/2018 5:21 AM, Paolo Bonzini wrote:
> On 30/10/2018 22:42, George Kennedy wrote:
>> Under heavy IO (e.g. fio) the queue is not checked frequently enough for
>> pending commands. As a result some pending commands are timed out by the
>> linux sym53c8xx driver, which sends SCSI Abort messages for the timed out
>> commands. The SCSI Abort messages result in linux errors, which show up
>> on the console and in /var/log/messages.
>>
>> e.g.
>> sd 0:0:3:0: [sdd] tag#33 ABORT operation started
>> scsi target0:0:3: control msgout:
>> 80 20 47 d
>> sd 0:0:3:0: ABORT operation complete.
>> scsi target0:0:4: message d sent on bad reselection
>>
>> When the current command completes, check if there is a pending command
>> on the queue and if a pending command exists, set a flag indicating
>> that a Wait Reselect command is needed to handle a queued pending command.
>>
>> When a Wait Reselect is needed, intercept and save the current DMA Scripts
>> Ptr (DSP) contents and load it instead with the pointer to the Reselection
>> Scripts.  When Reselection has completed, restore the original DSP contents.
>>
>> Signed-off-by: George Kennedy <george.kennedy@oracle.com>
>> ---
>>   hw/scsi/lsi53c895a.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 40 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
>> index d1e6534..b783d72 100644
>> --- a/hw/scsi/lsi53c895a.c
>> +++ b/hw/scsi/lsi53c895a.c
>> @@ -219,6 +219,8 @@ typedef struct {
>>       int command_complete;
>>       QTAILQ_HEAD(, lsi_request) queue;
>>       lsi_request *current;
>> +    bool want_resel;    /* need resel to handle queued completed cmds */
>> +    uint32_t resel_dsp; /* DMA Scripts Ptr (DSP) of reselection scsi scripts */
>>   
>>       uint32_t dsa;
>>       uint32_t temp;
>> @@ -298,6 +300,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 +460,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 +490,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 +772,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->want_resel = get_pending_req(s) ? true : false;
>> +
>>       lsi_resume_script(s);
>>   }
>>   
>> @@ -1064,11 +1079,13 @@ 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;
>> -        }
>> +    if (s->current) {
>> +        return;
>> +    }
>> +
>> +    p = get_pending_req(s);
>> +    if (p) {
>> +        lsi_reselect(s, p);
>>       }
>>       if (s->current == NULL) {
>>           s->waiting = 1;
>> @@ -1082,6 +1099,7 @@ static void lsi_execute_script(LSIState *s)
>>       uint32_t addr, addr_high;
>>       int opcode;
>>       int insn_processed = 0;
>> +    uint32_t save_dsp = 0;
>>   
>>       s->istat1 |= LSI_ISTAT1_SRUN;
>>   again:
>> @@ -1260,7 +1278,14 @@ again:
>>                   s->scntl1 &= ~LSI_SCNTL1_CON;
>>                   break;
>>               case 2: /* Wait Reselect */
>> +                if (!s->resel_dsp) {
>> +                    s->resel_dsp = s->dsp - 8;
>> +                }
> What is the reason for the "if"?
The save of the Rselection Script address only needs to be done once
>
>>                   if (!lsi_irq_on_rsl(s)) {
>> +                    if (save_dsp) {
>> +                        s->dsp = save_dsp;
>> +                        save_dsp = 0;
>> +                    }
>>                       lsi_wait_reselect(s);
>>                   }
>>                   break;
>> @@ -1476,6 +1501,10 @@ again:
>>   
>>               if (insn & (1 << 28)) {
>>                   addr = s->dsa + sextract32(addr, 0, 24);
>> +            } else if (s->want_resel && s->resel_dsp && !(insn & (1 << 24))) {
>> +                save_dsp = s->dsp;
>> +                s->dsp = s->resel_dsp;
>> +                s->want_resel = false;
>>               }
> Can you explain why this is placed under the LOAD AND STORE instruction?
>
> Your previous patch has, more specifically, a store from the scratch
> register into memory.  In the Linux scripts this occurs twice, both
> after a DISCONNECT instruction.  Is that the place where you want to hook?
Yes. I can revert to what was in the previous patch. I had figured that, 
like the change to capture the Reselection Script address, the point 
where the DSP address gets loaded with the Script Reselection address 
could be handled in a similar manner.
>
> (All this is a bit of a hack of course, and I would rather have the
> actual solution that adds the appropriate RESELECTION and MESSAGE IN
> phases whenever the device has finished the data transfer.  But with a
> FIXME comment I guess it might be okay).
I will add a FIXME with a description of the problem that we had 
discussed along with how we ultimately want the code flow to work.

Thank you,
George
>
> Paolo
>

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

[Paolo Bonzini]

On 31/10/2018 12:29, George Kennedy wrote:
>>>               case 2: /* Wait Reselect */
>>> +                if (!s->resel_dsp) {
>>> +                    s->resel_dsp = s->dsp - 8;
>>> +                }
>>
>> What is the reason for the "if"?
>
> The save of the Rselection Script address only needs to be done once

That's not correct.  There could be more than one WAIT RESELECT instruction.

(I'm also not sure why the "- 8", which causes the WAIT RESELECT opcode
to run again.  I think that, instead of that "- 8", you're missing a
call to lsi_wait_reselect where you set s->dsp = s->resel_dsp.  It makes
sense: after disconnecting from one target, you immediately fake a
reselect from the target.  This isn't quite right, the reselection
should be in lsi_transfer_data, but it's a start).

>>>                   if (!lsi_irq_on_rsl(s)) {
>>> +                    if (save_dsp) {
>>> +                        s->dsp = save_dsp;
>>> +                        save_dsp = 0;
>>> +                    }
>>>                       lsi_wait_reselect(s);
>>>                   }
>>>                   break;
>>> @@ -1476,6 +1501,10 @@ again:
>>>                 if (insn & (1 << 28)) {
>>>                   addr = s->dsa + sextract32(addr, 0, 24);
>>> +            } else if (s->want_resel && s->resel_dsp && !(insn & (1
>>> << 24))) {
>>> +                save_dsp = s->dsp;
>>> +                s->dsp = s->resel_dsp;
>>> +                s->want_resel = false;
>>>               }
>> Can you explain why this is placed under the LOAD AND STORE instruction?
>>
>> Your previous patch has, more specifically, a store from the scratch
>> register into memory.  In the Linux scripts this occurs twice, both
>> after a DISCONNECT instruction.  Is that the place where you want to
>> hook?
>
> Yes. I can revert to what was in the previous patch. I had figured that,
> like the change to capture the Reselection Script address, the point
> where the DSP address gets loaded with the Script Reselection address
> could be handled in a similar manner.

There is only one correct way to handle SCRIPTS instructions, and it's
the one that matches the data sheet.  In other words, even if we agree
to an incomplete solution it still must have some foundation in the
datasheet and parallel SCSI spec.  We have to adjust for the coalescing
of initiator/target/scripts that occurs in lsi53c895a.c, but that should
be it.

In particular, the reselection script is captured in WAIT RESELECT
because the data sheet says so:

  If the LSI53C895A is reselected, it fetches the next
  instruction from the address pointed to by the DMA
  SCRIPTS Pointer (DSP) register.

but LOAD AND STORE has nothing to do with reselection.

Paolo