[Qemu-devel] [PATCH v2 0/2] Correct transitions for cd change state

[Qemu-devel] [PATCH v2 0/2] Correct transitions for cd change state

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

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     |   51 ++++++++++++++++++++++++++++++++++++++++++++-------
 hw/ide/internal.h |    1 +
 2 files changed, 45 insertions(+), 7 deletions(-)

-- 
1.7.4

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

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

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/ide/core.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 007a4ee..d55d804 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -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:
-- 
1.7.4

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

[Paolo Bonzini]

On 04/07/2011 07:05 AM, Amit Shah wrote:
> 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<amit.shah@redhat.com>
> ---
>   hw/ide/core.c |    9 +++++----
>   1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 007a4ee..d55d804 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -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:

ACK

Paolo

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

[Stefan Hajnoczi]

On Thu, Apr 07, 2011 at 10:35:18AM +0530, Amit Shah wrote:
> @@ -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.

Stefan

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

[Amit Shah]

On (Thu) 07 Apr 2011 [09:42:30], Stefan Hajnoczi wrote:
> On Thu, Apr 07, 2011 at 10:35:18AM +0530, Amit Shah wrote:
> > @@ -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.

Argh!  I'll update to a generic one as there are more commands that we
need to allow.

		Amit

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

[Kevin Wolf]

Am 07.04.2011 07:05, schrieb 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.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

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.

Kevin

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

[Amit Shah]

On (Thu) 07 Apr 2011 [10:59:20], Kevin Wolf wrote:
> Am 07.04.2011 07:05, schrieb 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.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> 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.

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


There could be something else brewing as well.  We never report a
'tray open' condition; maybe the guest will be happy with such a thing
as well.  Will have to check.

		Amit

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

[Kevin Wolf]

Am 07.04.2011 11:11, schrieb Amit Shah:
> On (Thu) 07 Apr 2011 [10:59:20], Kevin Wolf wrote:
>> Am 07.04.2011 07:05, schrieb 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.
>>>
>>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>>
>> 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.
> 
> 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;
> }

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

Kevin

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

[Amit Shah]

On (Thu) 07 Apr 2011 [12:20:16], Kevin Wolf wrote:
> Am 07.04.2011 11:11, schrieb Amit Shah:
> > On (Thu) 07 Apr 2011 [10:59:20], Kevin Wolf wrote:
> >> Am 07.04.2011 07:05, schrieb 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.
> >>>
> >>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >>
> >> 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.
> > 
> > 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;
> > }
> 
> 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...

No, it doesn't.

It's clear the 2nd patch is needed; the 1st one must be papering over
something else.  Right now I'm inclined to think that it's because we
don't report tray open and no media as separate events to the guest,
as Markus has found out.

		Amit

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

[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 <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 {
+            s->media_change_notified = 1;
         }
     }
     return 0;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index d533fb6..5d58be4 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -411,6 +411,7 @@ struct IDEState {
     uint8_t sense_key;
     uint8_t asc;
     uint8_t cdrom_changed;
+    uint8_t media_change_notified;
     int packet_transfer_size;
     int elementary_transfer_size;
     int io_buffer_index;
-- 
1.7.4

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

[Paolo Bonzini]

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.

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.

Paolo

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

[Amit Shah]

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

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

[Stefan Hajnoczi]

On Thu, Apr 07, 2011 at 10:35:19AM +0530, Amit Shah wrote:
> +	    /*
> +	     * 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.

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

Stefan

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

[Amit Shah]

On (Thu) 07 Apr 2011 [09:41:09], Stefan Hajnoczi wrote:
> On Thu, Apr 07, 2011 at 10:35:19AM +0530, Amit Shah wrote:
> > +	    /*
> > +	     * 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.

Fixed in v3.

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

Thanks; I tested Win XP and RHEL6 guests.

		Amit