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

[Pavel Hrdina]

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