Correct transitions for cd change state

PATCH In review Amit Shah 4 participants 6 messages Apr 7, 2011 – Apr 8, 2011

lore

Share & tools

lore

Amit Shah

author

2011-04-07 08:46 UTC #0 lore raw

v3 cover Correct transitions for cd change state

Hello,

These two patches fix the cd media size change bugs.

The test scenario is:

1. create an iso image from a file
2. create a second iso image from a bigger file
3. mount 1st cd in guest
4. unmount it
5. change cd via qemu monitor
6. mount 2nd cd
7. copy file on cd to local disk -- shows errors.

v2: Set media_change_notified to 1 if SENSE_UNIT_ATTENTION is not set after migration. This is the only thing needed for migration compat.

v3:
- Use cdrom_changed to hold all state changes instead of introducing a new variable. (Paolo Bonzini)
- Slightly better comment and commit messages

Please apply.

Amit Shah (2):
cdrom: Allow the TEST_UNIT_READY command after a cdrom change
cdrom: Make disc change event visible to guests

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

Signature · 1 line

1.7.4

Amit Shah

author

2011-04-07 08:46 UTC #1 lore raw

PATCH v3 1/2 cdrom: Allow the TEST_UNIT_READY command after a cdrom change

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.

This also gets rid of one cause of the HSM violation errors in Linux guests. Those errors came up because we had the UNIT_ATTENTION event pending and we replied with an error message to a command that should be allowed in such a condition. The guest then did a soft reset to get to a sane state.

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

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

hw/ide/core.c +5 -4

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) {
-	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;
     }
     switch(s->io_buffer[0]) {
     case GPCMD_TEST_UNIT_READY:

Signature · 1 line

1.7.4

Ryan Harper

2011-04-08 20:45 UTC #2 lore raw

Re: [Qemu-devel] [PATCH v3 1/2] cdrom: Allow the TEST_UNIT_READY command after a cdrom change

Quoted 21 lines · Amit Shah

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.
This also gets rid of one cause of the HSM violation errors in Linux guests. Those errors came up because we had the UNIT_ATTENTION event pending and we replied with an error message to a command that should be allowed in such a condition. The guest then did a soft reset to get to a sane state.
Signed-off-by Amit Shah <[email protected]>
---
 hw/ide/core.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
hw/ide/core.c +0 -0
index 007a4ee..d55d804 100644

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

Quoted 2 lines

hw/ide/core.c +0 -0

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

Wasn't this comment going to get updated to be more generic (ie more than just the 3 commands below may be needed?)

Quoted 16 lines

hw/ide/core.c +5 -4

     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;
     }
     switch(s->io_buffer[0]) {
     case GPCMD_TEST_UNIT_READY:

Signature · 1 line

1.7.4

Signature · 4 lines

Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
[email protected]

Amit Shah

author

2011-04-07 08:46 UTC #3 lore raw

PATCH v3 2/2 cdrom: Make disc change event visible to guests

In reply to Amit Shah

Commit 93c8cfd9e67a62711b86f4c93747566885eb7928 sent 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 follow the 2nd arrow in:

'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 |   47 ++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 42 insertions(+), 5 deletions(-)

hw/ide/core.c +42 -5

index d55d804..3f00cff 100644
@@ -1113,10 +1113,47 @@ 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;
+
+            /*
+             * Check if there's any pending media change notification
+             * to be given.
+             *
+             * We want the guest to notice an empty tray after a cd
+             * change, so that the guest can trigger its new media
+             * paths.  So send one MEDIUM_NOT_PRESENT message after a
+             * cd change.
+             *
+             * After we've sent that message, the guest will poke at
+             * us again.  Send the UNIT_ATTENTION message then.  Once
+             * this is done, reset the UNIT_ATTENTION message to
+             * ensure we don't keep repeating it.
+             */
+            switch(s->cdrom_changed) {
+            case 0:
+                ide_atapi_cmd_ok(s);
+                break;
+            case 1:
+                s->sense_key = SENSE_UNIT_ATTENTION;
+                s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
+                ide_atapi_cmd_ok(s);
+
+		sense = SENSE_NONE;
+                s->cdrom_changed--;
+                break;
+            case 2:
+                ide_atapi_cmd_error(s, SENSE_NOT_READY,
+                                    ASC_MEDIUM_NOT_PRESENT);
+                s->cdrom_changed--;
+                break;
+            }
+            s->sense_key = sense;
+            s->asc = asc;
         } else {
-            s->cdrom_changed = 0;
             ide_atapi_cmd_error(s, SENSE_NOT_READY,
                                 ASC_MEDIUM_NOT_PRESENT);
         }

@@ -1612,7 +1649,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->cdrom_changed = 2;
     ide_set_irq(s->bus);
 }

@@ -2712,7 +2749,7 @@ static int ide_drive_post_load(void *opaque, int version_id)
     if (version_id < 3) {
         if (s->sense_key == SENSE_UNIT_ATTENTION &&
             s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
-            s->cdrom_changed = 1;
+            s->cdrom_changed = 2;
         }
     }
     return 0;

Signature · 1 line

1.7.4

Stefan Hajnoczi

Tested-by on the v3 series

2011-04-07 09:26 UTC #4 lore raw

In reply to Amit Shah

Windows Server 2003 works - media change is detected and size is updated correctly.

Tested-by Stefan Hajnoczi <[email protected]>

Paolo Bonzini

Reviewed-by on the v3 series

2011-04-07 09:27 UTC #5 lore raw

In reply to Amit Shah

Quoted 22 lines · Amit Shah

Hello,
These two patches fix the cd media size change bugs.
The test scenario is:
1. create an iso image from a file
2. create a second iso image from a bigger file
3. mount 1st cd in guest
4. unmount it
5. change cd via qemu monitor
6. mount 2nd cd
7. copy file on cd to local disk -- shows errors.
v2: Set media_change_notified to 1 if SENSE_UNIT_ATTENTION is not set after migration. This is the only thing needed for migration compat.
v3:
- Use cdrom_changed to hold all state changes instead of introducing a new variable. (Paolo Bonzini)
- Slightly better comment and commit messages

Quoted 9 lines

Please apply.
Amit Shah (2):
cdrom: Allow the TEST_UNIT_READY command after a cdrom change
cdrom: Make disc change event visible to guests
  hw/ide/core.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++---------
  1 files changed, 47 insertions(+), 9 deletions(-)

Thanks!

Reviewed-by Paolo Bonzini <[email protected]>

Paolo