| Message ID | cf1b3572fb325c9a918f3b62686e2f770cbe88d9.1302860026.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
Amit Shah <amit.shah@redhat.com> writes: > 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> Looks good, and performs as advertized in my tests. Reviewed-by: Markus Armbruster <armbru@redhat.com> Tested-by: Markus Armbruster <armbru@redhat.com>
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. Kevin
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
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); } static void ide_cmd_lba48_transform(IDEState *s, int lba48)
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(-)