[Qemu-devel] Re: [PATCH v2 2/2] cdrom: Make disc change event visible to guests

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

On (Thu) 07 Apr 2011 [09:31:05], Paolo Bonzini wrote:
> On 04/07/2011 07:05 AM, Amit Shah wrote:
> >Commit 93c8cfd9e67a62711b86f4c93747566885eb7928 tried to send a 'no
> >disc' event after a cdrom change so that guests notice a cd change event
> >between two 'cd present' states.  However, we don't go from
> >
> >  'cd present' ->  'no cd' ->  'cd present'
> >
> >as the SENSE_UNIT_ATTENTION sense_key is written over by the
> >ide_atapi_cmd_error() function.
> >
> >So for the disc change event, let us ensure the error() function doesn't
> >trample over that value so we do get to report it the next time around.
> >Also, ensure we go from 'no cd' to 'cd present' state.
> >
> >With this, older Linux guests (<  2.6.38) notice cd changes just fine.
> >For newer Linux guests (2.6.38+) cd change events break again and that
> >will be fixed by implementing the GET_EVENT_STATUS_NOTIFICATION command.
> >
> >Signed-off-by: Amit Shah<amit.shah@redhat.com>
> >---
> >  hw/ide/core.c     |   42 +++++++++++++++++++++++++++++++++++++++---
> >  hw/ide/internal.h |    1 +
> >  2 files changed, 40 insertions(+), 3 deletions(-)
> >
> >diff --git a/hw/ide/core.c b/hw/ide/core.c
> >index d55d804..abb577c 100644
> >--- a/hw/ide/core.c
> >+++ b/hw/ide/core.c
> >@@ -1113,10 +1113,42 @@ static void ide_atapi_cmd(IDEState *s)
> >      }
> >      switch(s->io_buffer[0]) {
> >      case GPCMD_TEST_UNIT_READY:
> >-        if (bdrv_is_inserted(s->bs)&&  !s->cdrom_changed) {
> >-            ide_atapi_cmd_ok(s);
> >+        if (bdrv_is_inserted(s->bs)) {
> >+            int sense, asc;
> >+
> >+            sense = s->sense_key;
> >+            asc = s->asc;
> >+
> >+	    /*
> >+	     * First, check if there's any pending media change
> >+	     * notification to be given.
> >+	     *
> >+	     * We want the guest to notice an empty tray between a cd
> >+	     * change, so send one MEDIUM_NOT_PRESENT message after a
> >+	     * cd change.
> >+	     *
> >+	     * After we've sent that message, the guest will poke at
> >+	     * us again and send the UNIT_ATTENTION message then.
> >+	     * Once this is done, reset the UNIT_ATTENTION message to
> >+	     * ensure we don't keep repeating it.
> >+	     */
> >+            if (!s->media_change_notified) {
> >+                ide_atapi_cmd_error(s, SENSE_NOT_READY,
> >+                                    ASC_MEDIUM_NOT_PRESENT);
> >+                s->media_change_notified = 1;
> >+            } else if (s->cdrom_changed) {
> >+                s->sense_key = SENSE_UNIT_ATTENTION;
> >+                s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> >+                ide_atapi_cmd_ok(s);
> >+
> >+                s->cdrom_changed = 0;
> >+		sense = SENSE_NONE;
> >+            } else {
> >+                ide_atapi_cmd_ok(s);
> >+            }
> >+            s->sense_key = sense;
> >+            s->asc = asc;
> >          } else {
> >-            s->cdrom_changed = 0;
> >              ide_atapi_cmd_error(s, SENSE_NOT_READY,
> >                                  ASC_MEDIUM_NOT_PRESENT);
> >          }
> >@@ -1613,6 +1645,7 @@ static void cdrom_change_cb(void *opaque, int reason)
> >      s->sense_key = SENSE_UNIT_ATTENTION;
> >      s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> >      s->cdrom_changed = 1;
> >+    s->media_change_notified = 0;
> >      ide_set_irq(s->bus);
> >  }
> >
> >@@ -2474,6 +2507,7 @@ static void ide_reset(IDEState *s)
> >      s->sense_key = 0;
> >      s->asc = 0;
> >      s->cdrom_changed = 0;
> >+    s->media_change_notified = 0;
> >      s->packet_transfer_size = 0;
> >      s->elementary_transfer_size = 0;
> >      s->io_buffer_index = 0;
> >@@ -2713,6 +2747,8 @@ static int ide_drive_post_load(void *opaque, int version_id)
> >          if (s->sense_key == SENSE_UNIT_ATTENTION&&
> >              s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
> >              s->cdrom_changed = 1;
> >+        } else {
> 
> This is within a version_id < 3 condition, so media_change_notified
> is never set to 1 when loading from a current QEMU.

Right.

> Integrating the new flag into cdrom_changed (0 = ok, 1 = cdrom
> changed and NOT_PRESENT sent, 2 = cdrom changed and NOT_PRESENT not
> sent) should also work for older guests.  They just treat 2 the same
> as 1.

Yes, thanks -- I was thinking the same.

v3 coming up.

		Amit