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

[Paolo Bonzini <pbonzini@redhat.com>]

On 24/10/2018 15:06, George Kennedy wrote:
>>
>>
>> Why didn't lsi_do_command invoke lsi_queue_command?  That would set
>> s->current to NULL (on the SCSI level, that means the bus is freed; on
>> the QEMU level, the idea is that lsi_transfer_data would then start a
>> reselection).
> 
> Through the extended period of time with no call to lsi_reselect(), the
> check of "s->command_complete" in lsi_do_command() is always "1" and
> therefore no call to lsi_queue_command() occurs.
> 
> "s->command_complete" is set to "1" in lsi_transfer_data().

Ok, I think we're getting closer.  If the data transfer is from the
device (read), then you can disconnect until the read is complete.

If the data transfer however is to the device (write), there will be
immediately a call to scsi_transfer_data, in order to get the written
data, and s->command_complete is set to 1.

However, this is wrong: the DMA transfer doesn't per se prevent a
disconnect and a later reselect, and this might be the bug.  Basically
we want to get rid of the "s->command_complete = 1" case completely.
The patch to do this change in the SCSI bus handling would look
something like this (untested):

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index d1e6534311..c32ef40e78 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -565,6 +565,21 @@ static void lsi_bad_selection(LSIState *s, uint32_t id)
     lsi_disconnect(s);
 }

+/* Disconnect when DMA has finished but the command did not
+ * complete immediately.
+ */
+static void lsi_maybe_disconnect(LSIState *s)
+{
+    if (!s->current->dma_len && !s->command_complete) {
+        lsi_add_msg_byte(s, 2); /* SAVE DATA POINTER */
+        lsi_add_msg_byte(s, 4); /* DISCONNECT */
+        /* wait data */
+        lsi_set_phase(s, PHASE_MI);
+        s->msg_action = 1;
+        lsi_queue_command(s);
+    }
+}
+
 /* Initiate a SCSI layer data transfer.  */
 static void lsi_do_dma(LSIState *s, int out)
 {
@@ -612,6 +627,7 @@ static void lsi_do_dma(LSIState *s, int out)
     if (s->current->dma_len == 0) {
         s->current->dma_buf = NULL;
         scsi_req_continue(s->current->req);
+        lsi_maybe_disconnect(s);
     } else {
         s->current->dma_buf += count;
         lsi_resume_script(s);
@@ -631,7 +647,6 @@ static void lsi_queue_command(LSIState *s)
     s->current = NULL;

     p->pending = 0;
-    p->out = (s->sstat1 & PHASE_MASK) == PHASE_DO;
 }

 /* Queue a byte for a MSG IN phase.  */
@@ -746,7 +761,7 @@ static void lsi_command_complete(SCSIRequest *req,
     out = (s->sstat1 & PHASE_MASK) == PHASE_DO;
     trace_lsi_command_complete(status);
     s->status = status;
-    s->command_complete = 2;
+    s->command_complete = 1;
     if (s->waiting && s->dbc != 0) {
         /* Raise phase mismatch for short transfers.  */
         lsi_bad_phase(s, out, PHASE_ST);
@@ -781,7 +796,6 @@ static void lsi_transfer_data(SCSIRequest *req,
     /* host adapter (re)connected */
     trace_lsi_transfer_data(req->tag, len);
     s->current->dma_len = len;
-    s->command_complete = 1;
     if (s->waiting) {
         if (s->waiting == 1 || s->dbc == 0) {
             lsi_resume_script(s);
@@ -819,28 +833,12 @@ static void lsi_do_command(LSIState *s)
                                    s->current);

     n = scsi_req_enqueue(s->current->req);
+    s->current->out = (n < 0);
+    lsi_set_phase(s, s->current->out ? PHASE_DO : PHASE_DI);
     if (n) {
-        if (n > 0) {
-            lsi_set_phase(s, PHASE_DI);
-        } else if (n < 0) {
-            lsi_set_phase(s, PHASE_DO);
-        }
         scsi_req_continue(s->current->req);
     }
-    if (!s->command_complete) {
-        if (n) {
-            /* Command did not complete immediately so disconnect.  */
-            lsi_add_msg_byte(s, 2); /* SAVE DATA POINTER */
-            lsi_add_msg_byte(s, 4); /* DISCONNECT */
-            /* wait data */
-            lsi_set_phase(s, PHASE_MI);
-            s->msg_action = 1;
-            lsi_queue_command(s);
-        } else {
-            /* wait command complete */
-            lsi_set_phase(s, PHASE_DI);
-        }
-    }
+    lsi_maybe_disconnect(s);
 }

 static void lsi_do_status(LSIState *s)


Of course, this doesn't include the change to WAIT RESELECT handling.
It only ensures (or attempts to ensure) that the bus is disconnected and
later reselected on long-running I/O, no matter the data direction.

Thanks,

Paolo