[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 <address@hidden>
>
> 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 address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>