| Message ID | 1232720479-21398-3-git-send-email-ryanh@us.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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 > -- 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
* 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 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 -- 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
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) {
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> -- 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