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

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

[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);
 }
 
 static void ide_cmd_lba48_transform(IDEState *s, int lba48)
-- 
1.7.4.2

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

[Markus Armbruster]

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>

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

[Kevin Wolf]

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

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

[Amit Shah]

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