qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Kevin Wolf
Subject: Re: [Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier
Date: Fri, 23 Nov 2012 13:09:26 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0

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 <address@hidden>

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]