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) {
next prev 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: linkBe 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.