[Top][All Lists]
[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