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.s...@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
