Correct transitions for cd change state

We restrict the commands that a guest can send us after a cdrom change event. The current list includes REQUEST_SENSE and INQUIRY commands. Guests can also issue TEST_UNIT_READY to inquire for the status, so allow this command as well.

Signed-off-by Amit Shah<[email protected]>

---
  hw/ide/core.c |    9 +++++----
  1 files changed, 5 insertions(+), 4 deletions(-)

hw/ide/core.c +0 -2

index 007a4ee..d55d804 100644
@@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s)
      /* If there's a UNIT_ATTENTION condition pending, only
         REQUEST_SENSE and INQUIRY commands are allowed to complete. */
      if (s->sense_key == SENSE_UNIT_ATTENTION&&
-	s->io_buffer[0] != GPCMD_REQUEST_SENSE&&
-	s->io_buffer[0] != GPCMD_INQUIRY) {

hw/ide/core.c static void ide_atapi_cmd(IDEState *s) · line 1112

Quoted 13 lines · Amit Shah

hw/ide/core.c +5 -4

@@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s)
     /* If there's a UNIT_ATTENTION condition pending, only
        REQUEST_SENSE and INQUIRY commands are allowed to complete. */
     if (s->sense_key == SENSE_UNIT_ATTENTION &&
-	s->io_buffer[0] != GPCMD_REQUEST_SENSE &&
-	s->io_buffer[0] != GPCMD_INQUIRY) {
-	ide_atapi_cmd_check_status(s);
-	return;
+        s->io_buffer[0] != GPCMD_REQUEST_SENSE &&
+        s->io_buffer[0] != GPCMD_INQUIRY &&
+        s->io_buffer[0] != GPCMD_TEST_UNIT_READY) {
+        ide_atapi_cmd_check_status(s);
+        return;

Looks good but please update the comment.

We restrict the commands that a guest can send us after a cdrom change event. The current list includes REQUEST_SENSE and INQUIRY commands. Guests can also issue TEST_UNIT_READY to inquire for the status, so allow this command as well.

Signed-off-by Amit Shah <[email protected]>

Am 07.04.2011 07:05, schrieb Amit Shah:

Quoted 6 lines

We restrict the commands that a guest can send us after a cdrom change event. The current list includes REQUEST_SENSE and INQUIRY commands. Guests can also issue TEST_UNIT_READY to inquire for the status, so allow this command as well.

Signed-off-by Amit Shah <[email protected]>

Hm... MMC-5, section 4.1.6.1 seems to conflict with this:

"If a Host issues a command other than GET CONFIGURATION, GET EVENT STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention condition exists for that Host, the Drive shall not perform the command and shall report CHECK CONDITION status unless a higher priority status as defined by the Drive is also pending."

So while you're right that our list is incomplete, TEST UNIT READY doesn't seem to be among the missing commands.

Quoted 10 lines

Am 07.04.2011 07:05, schrieb Amit Shah:

Quoted 6 lines

We restrict the commands that a guest can send us after a cdrom change event. The current list includes REQUEST_SENSE and INQUIRY commands. Guests can also issue TEST_UNIT_READY to inquire for the status, so allow this command as well.

Signed-off-by Amit Shah <[email protected]>

Hm... MMC-5, section 4.1.6.1 seems to conflict with this:

Quoted 8 lines

"If a Host issues a command other than GET CONFIGURATION, GET EVENT STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention condition exists for that Host, the Drive shall not perform the command and shall report CHECK CONDITION status unless a higher priority status as defined by the Drive is also pending."

So while you're right that our list is incomplete, TEST UNIT READY doesn't seem to be among the missing commands.

Hm - older Linux guests (pre 2.6.38) and Windows guests, as Gleb's commit mentioned, rely on this command to get CD change notifications:

/* identical to scsi_test_unit_ready except that it doesn't
* eat the NOT_READY returns for removable media */
int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)

{
        int retries = MAX_RETRIES;
        int the_result;
        u8 cmd[] = {TEST_UNIT_READY, 0, 0, 0, 0, 0 };

        /* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION
         * conditions are gone, or a timeout happens
         */
        do {
                the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL,
                                              0, sshdr, SR_TIMEOUT,
                                              retries--, NULL);
                if (scsi_sense_valid(sshdr) &&
                    sshdr->sense_key == UNIT_ATTENTION)

                        sdev->changed = 1;

        } while (retries > 0 &&
                 (!scsi_status_is_good(the_result) ||
                  (scsi_sense_valid(sshdr) &&
                   sshdr->sense_key == UNIT_ATTENTION)));
        return the_result;
}

Am 07.04.2011 11:11, schrieb Amit Shah:

Quoted 40 lines · Kevin Wolf

Quoted 10 lines

Am 07.04.2011 07:05, schrieb Amit Shah:

Quoted 6 lines

We restrict the commands that a guest can send us after a cdrom change event. The current list includes REQUEST_SENSE and INQUIRY commands. Guests can also issue TEST_UNIT_READY to inquire for the status, so allow this command as well.

Signed-off-by Amit Shah <[email protected]>

Hm... MMC-5, section 4.1.6.1 seems to conflict with this:

Quoted 8 lines

"If a Host issues a command other than GET CONFIGURATION, GET EVENT STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention condition exists for that Host, the Drive shall not perform the command and shall report CHECK CONDITION status unless a higher priority status as defined by the Drive is also pending."

So while you're right that our list is incomplete, TEST UNIT READY doesn't seem to be among the missing commands.

Hm - older Linux guests (pre 2.6.38) and Windows guests, as Gleb's commit mentioned, rely on this command to get CD change notifications:

/* identical to scsi_test_unit_ready except that it doesn't
* eat the NOT_READY returns for removable media */
int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)

{
        int retries = MAX_RETRIES;
        int the_result;
        u8 cmd[] = {TEST_UNIT_READY, 0, 0, 0, 0, 0 };

        /* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION
         * conditions are gone, or a timeout happens
         */
        do {
                the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL,
                                              0, sshdr, SR_TIMEOUT,
                                              retries--, NULL);
                if (scsi_sense_valid(sshdr) &&
                    sshdr->sense_key == UNIT_ATTENTION)

Quoted 8 lines

                        sdev->changed = 1;

        } while (retries > 0 &&
                 (!scsi_status_is_good(the_result) ||
                  (scsi_sense_valid(sshdr) &&
                   sshdr->sense_key == UNIT_ATTENTION)));
        return the_result;
}

I think the scsi_execute_req() call might issue a REQUEST SENSE internally and therefore clear the unit attention condition. Tried to check that in the source, but I'm hopelessly lost in the kernel...

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<[email protected]>

---
  hw/ide/core.c     |   42 +++++++++++++++++++++++++++++++++++++++---
  hw/ide/internal.h |    1 +
  2 files changed, 40 insertions(+), 3 deletions(-)

hw/ide/core.c +37 -3

index d55d804..abb577c 100644
@@ -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;

Quoted 81 lines · Amit Shah

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<[email protected]>

---
 hw/ide/core.c     |   42 +++++++++++++++++++++++++++++++++++++++---
 hw/ide/internal.h |    1 +
 2 files changed, 40 insertions(+), 3 deletions(-)

hw/ide/core.c +36 -3

index d55d804..abb577c 100644
@@ -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);
 }
 

hw/ide/core.c static int ide_drive_post_load(void *opaque, int version_id) · line 2750

Quoted 13 lines

hw/ide/core.c +2 -0

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

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.

hw/ide/core.c static void ide_atapi_cmd(IDEState *s) · line 1134

Quoted 13 lines · Amit Shah

hw/ide/core.c +13 -0

+	    /*
+	     * 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.
+	     */

Indentation is off here.

hw/ide/core.c static void ide_atapi_cmd(IDEState *s) · line 1145

Quoted 11 lines

hw/ide/core.c +11 -0

+            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;

Indentation is off here.

Otherwise looks good. I will test a Windows 2003 Server guest.