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

[Kevin Wolf <kwolf@redhat.com>]

Am 21.11.2012 18:17, schrieb Pavel Hrdina:
> If you have a guest with a media in the optical drive and you change the media
> the windows guest cannot properly recognize this media change.
> 
> Windows needs to detect the sense "NOT_READY with ASC_MEDIUM_NOT_PRESENT"
> before we send the sense "UNIT_ATTENTION with ASC_MEDIUM_MAY_HAVE_CHANGED".
> 
> v3: remove timeout as it isn't needed anymore
> 
> v2: disable debug messages
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>

Hope that we'll get libqos soon so that IDE can be properly tested with
qtest. CD-ROM media change is something that broke more than once.

The change makes sense to me, it's one of these "how could this ever
work?" cases. However I do have one or two comments:

> ---
>  hw/ide/atapi.c    | 16 +++++++++++-----
>  hw/ide/core.c     | 12 ++++++++++++
>  hw/ide/internal.h |  1 +
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 685cbaa..1534afe 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -1124,12 +1124,18 @@ void ide_atapi_cmd(IDEState *s)
>       * GET_EVENT_STATUS_NOTIFICATION to detect such tray open/close
>       * 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);
> +    if (!(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA) &&
> +        !s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
> +
> +        if (!s->fake_cdrom_eject) {
> +            ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
> +            s->fake_cdrom_eject = 1;
> +        } else {
> +            ide_atapi_cmd_error(s, UNIT_ATTENTION, ASC_MEDIUM_MAY_HAVE_CHANGED);
> +            s->fake_cdrom_eject = 0;
> +            s->cdrom_changed = 0;
> +        }
>  
> -        s->cdrom_changed = 0;
> -        s->sense_key = UNIT_ATTENTION;
> -        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
>          return;
>      }
>  
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 7d6b0fa..013671a 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1851,6 +1851,7 @@ static void ide_reset(IDEState *s)
>      s->sense_key = 0;
>      s->asc = 0;
>      s->cdrom_changed = 0;
> +    s->fake_cdrom_eject = 0;
>      s->packet_transfer_size = 0;
>      s->elementary_transfer_size = 0;
>      s->io_buffer_index = 0;
> @@ -2143,6 +2144,16 @@ static int transfer_end_table_idx(EndTransferFunc *fn)
>      return -1;
>  }
>  
> +static void ide_drive_pre_save(void *opaque)
> +{
> +    IDEState *s = opaque;
> +
> +    if (s->cdrom_changed) {
> +        s->sense_key = UNIT_ATTENTION;
> +        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> +    }
> +}

If we migrate immediately after the media change, before the OS has sent
another request, we skip the ASC_MEDIUM_NOT_PRESENT step, don't we? As
far as I can tell, adding this step is the real fix, so won't media
change break during migration this way?

The other thing is that if it's valid to set s->sense_key/asc in any
place instead of just during the start of a command (is it? I would
guess so, but I'm not sure), why don't we set ASC_MEDIUM_NOT_PRESENT
already in the change handler?

Another thing I would consider is using cdrom_changed = 0/1/2 instead of
adding fake_cdrom_eject to add another state. Migration would
automatically do the right thing for it, old versions would in both 1
and 2 state switch to ASC_MEDIUM_MAY_HAVE_CHANGED next, which is correct
in the latter case and is in the former case the same bug as the old
qemu we're migrating to has anyway.

Kevin