All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ryan Harper <ryanh@us.ibm.com>
To: qemu-devel@nongnu.org
Cc: Ryan Harper <ryanh@us.ibm.com>, kvm@vger.kernel.org
Subject: [PATCH 2/2] lsi,scsi-disk: check for reentrance via tag matching
Date: Fri, 23 Jan 2009 08:21:19 -0600	[thread overview]
Message-ID: <1232720479-21398-3-git-send-email-ryanh@us.ibm.com> (raw)
In-Reply-To: <1232720479-21398-1-git-send-email-ryanh@us.ibm.com>

SLES10 RC2 install, and other OSes have triggered the following error from the
LSI device:

lsi_scsi: error: Reselect with pending DMA

After some debugging, I noticed that in the lsi code, the phase would change
from PHASE_DO which corresponded to the current Write command we were handling,
to PHASE_DI.  After tracking down all of the places in lsi were we phase change
to PHASE_DI, lsi_do_command was triggering this change after calling send
command.  This was a most odd code path.  Then I saw it:

lsi_do_command()
    device->send_command()
        scsi_command_complete()
            lsi_command_complete()
                lsi_resume_script()
                    lsi_do_command()

This can be detected by tracking s->current_tag when we start lsi_do_command()
and checking against that once we return from send_command().  If the tags are
 different, then the previous command has completed and we exit the function.

A similar reentrance bug happens in scsi-disk.c.

scsi_send_command()
    scsi_command_complete() // we remove the request and put it on the freelist
        lsi_command_complete()
            lsi_resume_script()
                lsi_do_command()
                    scsi_send_command() // fetchings recently free'd request
                                        // and updates r->sector_count
                                        // clobbering the value that was being
                                        // used by the previous user of
                                        // this request
        
As with the lsi device, we can test for this by comparing the tag value from the
start of the function to the request tag (r->tag) and if they differ, then the
current function can exit as that command has already completed.

Fixing these two re-entrance issues allows SLES10 SP2 installer to complete and
guest function fully.

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 912d7d5..34f50df 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -668,6 +668,7 @@ static void lsi_do_command(LSIState *s)
 {
     uint8_t buf[16];
     int n;
+    uint32_t tag = s->current_tag;
 
     DPRINTF("Send command len=%d\n", s->dbc);
     if (s->dbc > 16)
@@ -677,6 +678,15 @@ static void lsi_do_command(LSIState *s)
     s->command_complete = 0;
     n = s->current_dev->send_command(s->current_dev, s->current_tag, buf,
                                      s->current_lun);
+    /* send_command may trigger a completion and call lsi_command_complete()
+     * which can resume SCRIPTS without returning here and actually complete 
+     * this current tag.  We check the current_tag against a copy when we
+     * started this function and if we don't match, just exit the function. */
+    if (tag != s->current_tag) {
+        DPRINTF("%s: tag=0x%x already completed.\n");
+        return;
+    }
+
     if (n > 0) {
         lsi_set_phase(s, PHASE_DI);
         s->current_dev->read_data(s->current_dev, s->current_tag);
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 744573e..4fa09a2 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -785,6 +785,12 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
     }
     if (r->sector_count == 0 && r->buf_len == 0) {
         scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
+        /* scsi_send_command can nest since scsi_command_complete calls
+         * the driver completion function which may proceed to call
+         * send_command a second time.  If that happens the current
+         * task has completed and we can just exit send_command() */
+        if (tag != r->tag)
+            return 0;
     }
     len = r->sector_count * 512 + r->buf_len;
     if (is_write) {

WARNING: multiple messages have this Message-ID (diff)
From: Ryan Harper <ryanh@us.ibm.com>
To: qemu-devel@nongnu.org
Cc: Ryan Harper <ryanh@us.ibm.com>, kvm@vger.kernel.org
Subject: [Qemu-devel] [PATCH 2/2] lsi, scsi-disk: check for reentrance via tag matching
Date: Fri, 23 Jan 2009 08:21:19 -0600	[thread overview]
Message-ID: <1232720479-21398-3-git-send-email-ryanh@us.ibm.com> (raw)
In-Reply-To: <1232720479-21398-1-git-send-email-ryanh@us.ibm.com>

SLES10 RC2 install, and other OSes have triggered the following error from the
LSI device:

lsi_scsi: error: Reselect with pending DMA

After some debugging, I noticed that in the lsi code, the phase would change
from PHASE_DO which corresponded to the current Write command we were handling,
to PHASE_DI.  After tracking down all of the places in lsi were we phase change
to PHASE_DI, lsi_do_command was triggering this change after calling send
command.  This was a most odd code path.  Then I saw it:

lsi_do_command()
    device->send_command()
        scsi_command_complete()
            lsi_command_complete()
                lsi_resume_script()
                    lsi_do_command()

This can be detected by tracking s->current_tag when we start lsi_do_command()
and checking against that once we return from send_command().  If the tags are
 different, then the previous command has completed and we exit the function.

A similar reentrance bug happens in scsi-disk.c.

scsi_send_command()
    scsi_command_complete() // we remove the request and put it on the freelist
        lsi_command_complete()
            lsi_resume_script()
                lsi_do_command()
                    scsi_send_command() // fetchings recently free'd request
                                        // and updates r->sector_count
                                        // clobbering the value that was being
                                        // used by the previous user of
                                        // this request
        
As with the lsi device, we can test for this by comparing the tag value from the
start of the function to the request tag (r->tag) and if they differ, then the
current function can exit as that command has already completed.

Fixing these two re-entrance issues allows SLES10 SP2 installer to complete and
guest function fully.

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 912d7d5..34f50df 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -668,6 +668,7 @@ static void lsi_do_command(LSIState *s)
 {
     uint8_t buf[16];
     int n;
+    uint32_t tag = s->current_tag;
 
     DPRINTF("Send command len=%d\n", s->dbc);
     if (s->dbc > 16)
@@ -677,6 +678,15 @@ static void lsi_do_command(LSIState *s)
     s->command_complete = 0;
     n = s->current_dev->send_command(s->current_dev, s->current_tag, buf,
                                      s->current_lun);
+    /* send_command may trigger a completion and call lsi_command_complete()
+     * which can resume SCRIPTS without returning here and actually complete 
+     * this current tag.  We check the current_tag against a copy when we
+     * started this function and if we don't match, just exit the function. */
+    if (tag != s->current_tag) {
+        DPRINTF("%s: tag=0x%x already completed.\n");
+        return;
+    }
+
     if (n > 0) {
         lsi_set_phase(s, PHASE_DI);
         s->current_dev->read_data(s->current_dev, s->current_tag);
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 744573e..4fa09a2 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -785,6 +785,12 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
     }
     if (r->sector_count == 0 && r->buf_len == 0) {
         scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
+        /* scsi_send_command can nest since scsi_command_complete calls
+         * the driver completion function which may proceed to call
+         * send_command a second time.  If that happens the current
+         * task has completed and we can just exit send_command() */
+        if (tag != r->tag)
+            return 0;
     }
     len = r->sector_count * 512 + r->buf_len;
     if (is_write) {

  parent reply	other threads:[~2009-01-23 14:21 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-01-23 14:21 [PATCH 0/2] LSI53C895A and scsi-disk fixes Ryan Harper
2009-01-23 14:21 ` [Qemu-devel] " Ryan Harper
2009-01-23 14:21 ` [PATCH 1/2] lsi: add ISTAT1 register read Ryan Harper
2009-01-23 14:21   ` [Qemu-devel] " Ryan Harper
2009-01-23 14:21 ` Ryan Harper [this message]
2009-01-23 14:21   ` [Qemu-devel] [PATCH 2/2] lsi, scsi-disk: check for reentrance via tag matching Ryan Harper
2009-01-23 15:00   ` [PATCH 2/2] lsi,scsi-disk: " Anthony Liguori
2009-01-23 15:00     ` [Qemu-devel] Re: [PATCH 2/2] lsi, scsi-disk: " Anthony Liguori
2009-01-23 15:23     ` [PATCH 2/2] lsi,scsi-disk: " Ryan Harper
2009-01-23 15:23       ` [Qemu-devel] Re: [PATCH 2/2] lsi, scsi-disk: " Ryan Harper
2009-01-23 15:42       ` [PATCH 2/2] lsi,scsi-disk: " Anthony Liguori
2009-01-23 15:42         ` [Qemu-devel] Re: [PATCH 2/2] lsi, scsi-disk: " Anthony Liguori

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1232720479-21398-3-git-send-email-ryanh@us.ibm.com \
    --to=ryanh@us.ibm.com \
    --cc=kvm@vger.kernel.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.