[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: |
Pavel Hrdina |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier |
Date: |
Mon, 26 Nov 2012 13:43:36 +0100 |
On Fri, 2012-11-23 at 13:09 +0100, Kevin Wolf wrote:
> 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?
>
We do not skip the ASC_MEDIUM_NOT_PRESENT. If we migrate immediately
after the media change, then in ide_drive_pre_save we set
ASC_MEDIUM_MAY_HAVE_CHANGED but on the other side in function
ide_drive_post_load we check if ASC_MEDIUM_MAY_HAVE_CHANGED is set and
if it is we set cdrom_changed to 1 but fake_cdrom_eject is 0. That means
the ASC_MEDIUM_NOT_PRESENT will be returned before
ASC_MEDIUM_MAY_HAVE_CHANGED.
> 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?
>
Well, we can set it in any place, but we have to call
ide_atapi_cmd_error to let the guest know about this sense_key/asc only
as response to command request.
> 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.
>
I do it that way at first, but then I rewrite it, because I thought that
using new state would be better. But if you agree with cdrom_changed =
0/1/2, I'll change it.
Pavel
> Kevin