[Qemu-devel] [PATCH v2 1/1] atapi: make change media detection for guests easier

[Qemu-devel] [PATCH v2 1/1] atapi: make change media detection for guests easier

[Pavel Hrdina]

If you have a guest with a media in the cdrom and you change it,
the windows and the linux guests cannot properly recognize this
media change.
For this purpose we have to pretend for some time (2s), that we
don't have any media inserted.

v2: disable debug messages

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hw/ide/atapi.c    | 25 ++++++++++++++++++++-----
 hw/ide/core.c     |  1 +
 hw/ide/internal.h |  1 +
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 685cbaa..1f20f10 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -73,7 +73,7 @@ static void lba_to_msf(uint8_t *buf, int lba)
 
 static inline int media_present(IDEState *s)
 {
-    return !s->tray_open && s->nb_sectors > 0;
+    return !s->tray_open && s->nb_sectors > 0 && s->cdrom_changed_timer == NULL;
 }
 
 /* XXX: DVDs that could fit on a CD will be reported as a CD */
@@ -504,7 +504,7 @@ static unsigned int event_status_media(IDEState *s,
     media_status = 0;
     if (s->tray_open) {
         media_status = MS_TRAY_OPEN;
-    } else if (bdrv_is_inserted(s->bs)) {
+    } else if (media_present(s)) {
         media_status = MS_MEDIA_PRESENT;
     }
 
@@ -1091,6 +1091,14 @@ static const struct {
     /* [1] handler detects and reports not ready condition itself */
 };
 
+static void cdrom_changed_timer_cb(void *opaque)
+{
+    IDEState *s = opaque;
+    qemu_del_timer(s->cdrom_changed_timer);
+    qemu_free_timer(s->cdrom_changed_timer);
+    s->cdrom_changed_timer = NULL;
+}
+
 void ide_atapi_cmd(IDEState *s)
 {
     uint8_t *buf;
@@ -1125,11 +1133,18 @@ void ide_atapi_cmd(IDEState *s)
      * states rely on this behavior.
      */
     if (!s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
-        ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
+        ide_atapi_cmd_error(s, UNIT_ATTENTION, ASC_MEDIUM_MAY_HAVE_CHANGED);
 
         s->cdrom_changed = 0;
-        s->sense_key = UNIT_ATTENTION;
-        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
+
+        if (s->cdrom_changed_timer) {
+            cdrom_changed_timer_cb(s);
+        }
+        s->cdrom_changed_timer = qemu_new_timer_ms(vm_clock,
+                                                   cdrom_changed_timer_cb, s);
+        qemu_mod_timer(s->cdrom_changed_timer,
+                       qemu_get_clock_ms(vm_clock) + 2000);
+
         return;
     }
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d6fb69c..caf30c8 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1861,6 +1861,7 @@ static void ide_reset(IDEState *s)
     s->sense_key = 0;
     s->asc = 0;
     s->cdrom_changed = 0;
+    s->cdrom_changed_timer = NULL;
     s->packet_transfer_size = 0;
     s->elementary_transfer_size = 0;
     s->io_buffer_index = 0;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index bf7d313..307ed82 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -382,6 +382,7 @@ struct IDEState {
     bool tray_open;
     bool tray_locked;
     uint8_t cdrom_changed;
+    QEMUTimer *cdrom_changed_timer;
     int packet_transfer_size;
     int elementary_transfer_size;
     int io_buffer_index;
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH v2 1/1] atapi: make change media detection for guests easier

[Pavel Hrdina]

Ping, still not reviewed or applied

On Thu, 2012-10-04 at 16:16 +0200, Pavel Hrdina wrote:
> If you have a guest with a media in the cdrom and you change it,
> the windows and the linux guests cannot properly recognize this
> media change.
> For this purpose we have to pretend for some time (2s), that we
> don't have any media inserted.
> 
> v2: disable debug messages
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hw/ide/atapi.c    | 25 ++++++++++++++++++++-----
>  hw/ide/core.c     |  1 +
>  hw/ide/internal.h |  1 +
>  3 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 685cbaa..1f20f10 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -73,7 +73,7 @@ static void lba_to_msf(uint8_t *buf, int lba)
>  
>  static inline int media_present(IDEState *s)
>  {
> -    return !s->tray_open && s->nb_sectors > 0;
> +    return !s->tray_open && s->nb_sectors > 0 && s->cdrom_changed_timer == NULL;
>  }
>  
>  /* XXX: DVDs that could fit on a CD will be reported as a CD */
> @@ -504,7 +504,7 @@ static unsigned int event_status_media(IDEState *s,
>      media_status = 0;
>      if (s->tray_open) {
>          media_status = MS_TRAY_OPEN;
> -    } else if (bdrv_is_inserted(s->bs)) {
> +    } else if (media_present(s)) {
>          media_status = MS_MEDIA_PRESENT;
>      }
>  
> @@ -1091,6 +1091,14 @@ static const struct {
>      /* [1] handler detects and reports not ready condition itself */
>  };
>  
> +static void cdrom_changed_timer_cb(void *opaque)
> +{
> +    IDEState *s = opaque;
> +    qemu_del_timer(s->cdrom_changed_timer);
> +    qemu_free_timer(s->cdrom_changed_timer);
> +    s->cdrom_changed_timer = NULL;
> +}
> +
>  void ide_atapi_cmd(IDEState *s)
>  {
>      uint8_t *buf;
> @@ -1125,11 +1133,18 @@ void ide_atapi_cmd(IDEState *s)
>       * states rely on this behavior.
>       */
>      if (!s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
> -        ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
> +        ide_atapi_cmd_error(s, UNIT_ATTENTION, ASC_MEDIUM_MAY_HAVE_CHANGED);
>  
>          s->cdrom_changed = 0;
> -        s->sense_key = UNIT_ATTENTION;
> -        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> +
> +        if (s->cdrom_changed_timer) {
> +            cdrom_changed_timer_cb(s);
> +        }
> +        s->cdrom_changed_timer = qemu_new_timer_ms(vm_clock,
> +                                                   cdrom_changed_timer_cb, s);
> +        qemu_mod_timer(s->cdrom_changed_timer,
> +                       qemu_get_clock_ms(vm_clock) + 2000);
> +
>          return;
>      }
>  
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d6fb69c..caf30c8 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1861,6 +1861,7 @@ static void ide_reset(IDEState *s)
>      s->sense_key = 0;
>      s->asc = 0;
>      s->cdrom_changed = 0;
> +    s->cdrom_changed_timer = NULL;
>      s->packet_transfer_size = 0;
>      s->elementary_transfer_size = 0;
>      s->io_buffer_index = 0;
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index bf7d313..307ed82 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -382,6 +382,7 @@ struct IDEState {
>      bool tray_open;
>      bool tray_locked;
>      uint8_t cdrom_changed;
> +    QEMUTimer *cdrom_changed_timer;
>      int packet_transfer_size;
>      int elementary_transfer_size;
>      int io_buffer_index;

Re: [Qemu-devel] [PATCH v2 1/1] atapi: make change media detection for guests easier

[Kevin Wolf]

Am 04.10.2012 16:16, schrieb Pavel Hrdina:
> If you have a guest with a media in the cdrom and you change it,
> the windows and the linux guests cannot properly recognize this
> media change.
> For this purpose we have to pretend for some time (2s), that we
> don't have any media inserted.

This used to work with the existing code, by exposing an intermediate
"no medium" state for exactly one request. Do you know since when this
is broken? Is it a qemu regression, or does it happen only with newer
Windows and Linux versions?

If possible at all, I'd prefer not to use a timer here.

Also I'm relatively sure that your code will break when migrating during
the 2s of "no medium".

Kevin