Re: [Qemu-devel] [PATCH] atapi: Indicate 'medium not ready' to 'medium ready' transition via error

[Amit Shah <amit.shah@redhat.com>]

On (Fri) 15 Apr 2011 [13:05:00], Kevin Wolf wrote:
> Am 15.04.2011 11:33, schrieb Amit Shah:
> > MMC-5 Table F.1 lists errors that can be thrown for the TEST_UNIT_READY
> > command.  Going from medium not ready to medium ready states is
> > communicated by throwing an error.
> > 
> > This adds the missing 'tray opened' event that we fail to report to
> > guests.  After doing this, older Linux guests properly revalidate a disc
> > on the change command.  HSM violation errors, which caused Linux guests
> > to do a soft-reset of the link, also go away:
> > 
> > ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6
> > sr 1:0:0:0: CDB: Test Unit Ready: 00 00 00 00 00 00
> > ata2.00: cmd a0/00:00:00:00:00/00:00:00:00:00/a0 tag 0
> >          res 01/60:00:00:00:00/00:00:00:00:00/a0 Emask 0x3 (HSM violation)
> > ata2.00: status: { ERR }
> > ata2: soft resetting link
> > ata2.00: configured for MWDMA2
> > ata2: EH complete
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  hw/ide/core.c |   32 ++++++++++++++++++++++++++------
> >  1 files changed, 26 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index f028ddb..5a72420 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -1253,9 +1253,16 @@ static void ide_atapi_cmd(IDEState *s)
> >          if (bdrv_is_inserted(s->bs) && !s->cdrom_changed) {
> >              ide_atapi_cmd_ok(s);
> >          } else {
> > -            s->cdrom_changed = 0;
> > -            ide_atapi_cmd_error(s, SENSE_NOT_READY,
> > -                                ASC_MEDIUM_NOT_PRESENT);
> > +            int sense, asc;
> > +
> > +            sense = SENSE_NOT_READY;
> > +            asc = ASC_MEDIUM_NOT_PRESENT;
> > +            if (bdrv_is_inserted(s->bs) && s->cdrom_changed) {
> > +                s->cdrom_changed = 0;
> > +                sense = SENSE_UNIT_ATTENTION;
> > +                asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> > +            }
> > +            ide_atapi_cmd_error(s, sense, asc);
> >          }
> >          break;
> >      case GPCMD_MODE_SENSE_6:
> > @@ -1734,11 +1741,24 @@ static void cdrom_change_cb(void *opaque, int reason)
> >      bdrv_get_geometry(s->bs, &nb_sectors);
> >      s->nb_sectors = nb_sectors;
> >  
> > -    s->sense_key = SENSE_UNIT_ATTENTION;
> > -    s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> > +    /*
> > +     * This function combines three actions a physical cdrom would do to
> > +     * change a disc in a drive:
> > +     *
> > +     * 1. eject the tray,
> > +     * 2. change the disc,
> > +     * 3. close the tray.
> > +     *
> > +     * Guests expect responses from us in the same order.  So first,
> > +     * just mark the disc changed, but provide an 'ejected' event to
> > +     * the guest.  Later, when the guest invokes a TEST_UNIT_READY
> > +     * command, we will provide with a disc change (UNIT_ATTENTION)
> > +     * event.
> > +     */
> >      s->cdrom_changed = 1;
> >      s->events.new_media = true;
> > -    ide_set_irq(s->bus);
> > +
> > +    ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT);
> >  }
> 
> ide_atapi_cmd_error expands to:
> 
>     s->error = sense_key << 4;
>     s->status = READY_STAT | ERR_STAT;
>     s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO |
> ATAPI_INT_REASON_CD;
>     s->sense_key = sense_key;
>     s->asc = asc;
>     ide_set_irq(s->bus);
> 
> Are you sure that you can modify the ATA error register here, even
> though we're not responding to a guest initiated command? I think
> possibly we're even in the middle of processing an independent command,
> which would falsely appear to have failed.
> 
> I'm not even sure that raising an IRQ is correct in cdrom_change_cb.

Hm; what other way is there to signal to a guest of such an event?

Note that the only change in this patch is to set the error status.
The irq was raised even earlier.

Also: not changing the error status can get host and guest out of
sync.  I think I tried that earlier and it didn't work.

		Amit