[Qemu-devel] [PATCH 0/2] LSI53C895A and scsi-disk fixes

[Qemu-devel] [PATCH 0/2] LSI53C895A and scsi-disk fixes

[Ryan Harper]

While trying to get SLES10 SP2 installed to a scsi disk, I encountered a couple
of errors.  The first patch just adds support for ISTAT1 register to be read.
The second patch handles re-entrance issues present in the lsi driver and
scsi-disk.  Due to the async/callback structure of the LSI and scsi code, there
are situations where lsi_do_command() and scsi_send_command() can nest.  Patch2
detects and handle these cases.

Applying these two patches allows SLES10 SP2 to function properly.  I've tested
the scsi changes against Ubuntu 8.04.2, 8.10, RHEL5u2, openSuSE11, FreeBSD 7,
Windows XP, Windows XP 64, 2k3 and didn't see any regressions.

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

[Qemu-devel] [PATCH 1/2] lsi: add ISTAT1 register read

[Ryan Harper]

SLES10 SP2 installer complains with:

lsi_scsi: error: readb 0x15

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

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 9f50dcb..912d7d5 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -1368,6 +1368,8 @@ static uint8_t lsi_reg_readb(LSIState *s, int offset)
     CASE_GET_REG32(dsa, 0x10)
     case 0x14: /* ISTAT0 */
         return s->istat0;
+    case 0x15: /* ISTAT1 */
+        return s->istat1;
     case 0x16: /* MBOX0 */
         return s->mbox0;
     case 0x17: /* MBOX1 */

[Qemu-devel] [PATCH 2/2] lsi, scsi-disk: check for reentrance via tag matching

[Ryan Harper]

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) {

[Qemu-devel] Re: [PATCH 2/2] lsi, scsi-disk: check for reentrance via tag matching

[Anthony Liguori]

Ryan Harper wrote:
> 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()
>   

I guess I don't understand why we can't issue the completion from a 
bottom half to take care of the reentrance.  Checking for the case of 
reentrance by detecting state corruption and returning seems very wrong 
to me.  I'm surprised it works.  The code in question:

> /* Read more data from scsi device into buffer.  */
> static void scsi_read_data(SCSIDevice *d, uint32_t tag)
> {
>     SCSIDeviceState *s = d->state;
>     SCSIRequest *r;
>     uint32_t n;
>
>     r = scsi_find_request(s, tag);
>     if (!r) {
>         BADF("Bad read tag 0x%x\n", tag);
>         /* ??? This is the wrong error.  */
>         scsi_command_complete(r, STATUS_CHECK_CONDITION, 
> SENSE_HARDWARE_ERROR);

By calling scsi_command_complete() here (and below) we potentially 
reenter lsi_do_command.  So why not schedule a bottom half here that 
runs scsi_command_complete in the bottom half?

<snip>
>
>     r->buf_len = n * 512;
>     r->aiocb = bdrv_aio_read(s->bdrv, r->sector, r->dma_buf, n,
>                              scsi_read_complete, r);

By virtue of the fact that we do bdrv_aio_read() here, it *has* to be 
safe to run the scsi_command_completes from a bottom half because 
scsi_read_complete is going to get invoked from a bottom half anyway in 
this path.

Am I missing something?

Regards,

Anthony Lgiuori

>     if (r->aiocb == NULL)
>         scsi_command_complete(r, STATUS_CHECK_CONDITION, 
> SENSE_HARDWARE_ERROR);
>     r->sector += n;
>     r->sector_count -= n;
> }




>             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) {
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

[Qemu-devel] Re: [PATCH 2/2] lsi, scsi-disk: check for reentrance via tag matching

[Ryan Harper]

* Anthony Liguori <anthony@codemonkey.ws> [2009-01-23 09:09]:
> 
> By virtue of the fact that we do bdrv_aio_read() here, it *has* to be 
> safe to run the scsi_command_completes from a bottom half because 
> scsi_read_complete is going to get invoked from a bottom half anyway in 
> this path.
> 
> Am I missing something?

Just moving scsi_command_complete to a bottom half won't be sufficent
since lsi_command_complete() is also re-entrent via calls to
lsi_resume_script() which can invoke lsi_do_command().  The issue of
bottom halves for lsi is something I've thought about, but haven't
figured out how to only execute enough lsi scripts to complete the
current command.  Having the device stop executing scripts after sending
back the command completion to the OS was my first though on how to
prevent this loop, but so far, just stopping after just sending
the completion back to the OS isn't something that the OS driver
handles well.

I suppose once we figure out how to stop executing scripts after
sending command completion in a way that the OS drivers understand,
moving that to a bottom half will work fine.


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

[Qemu-devel] Re: [PATCH 2/2] lsi, scsi-disk: check for reentrance via tag matching

[Anthony Liguori]

Ryan Harper wrote:
> * Anthony Liguori <anthony@codemonkey.ws> [2009-01-23 09:09]:
>   
> Just moving scsi_command_complete to a bottom half won't be sufficent
> since lsi_command_complete() is also re-entrent via calls to
> lsi_resume_script() which can invoke lsi_do_command().  The issue of
> bottom halves for lsi is something I've thought about, but haven't
> figured out how to only execute enough lsi scripts to complete the
> current command.  Having the device stop executing scripts after sending
> back the command completion to the OS was my first though on how to
> prevent this loop, but so far, just stopping after just sending
> the completion back to the OS isn't something that the OS driver
> handles well.
>
> I suppose once we figure out how to stop executing scripts after
> sending command completion in a way that the OS drivers understand,
> moving that to a bottom half will work fine.
>   

I think we're talking past each other.  Let me describe what my 
(limited) understanding of the problem is and you can correct me if I've 
got it wrong.

lsi_do_command():
   does some action on the next command
   stores some info in global state
   calls into scsi disk

scsi_disk normally does:
   bdrv_aio_read() with callback of scsi_complete.
   returns

lsi_do_command()
   finishes up command and touching global state
   returns

When IO completes:

scsi_complete:
   executes lsi_do_command() to run next command

We run into trouble when:

lsi_do_command():
   does some action on the next command
   stores some info in global state
   calls into scsi disk

scsi_disk abnormally does:
   scsi_command_complete()

scsi_command_complete:
   executes lsi_do_command() to run next command

lsi_do_command():
   does some action on the next command
   we're fubar because the global state is setup to process another command

So what a bottom half would do is:

lsi_do_command():
   does some action on the next command
   stores some info in global state
   calls into scsi disk

scsi_disk abnormally does:
   schedule bottom half for scsi_command_complete
   returns

lsi_do_command()
   finishes up command and touching global state
   returns

When bottom halves get run

scsi_complete:
   executes lsi_do_command() to run next command

And we avoid getting fubar.

Regards,

Anthony Liguori