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

[Pavel Hrdina]

On Mon, 2012-11-26 at 14:55 +0100, Kevin Wolf wrote:
> Am 26.11.2012 13:43, schrieb Pavel Hrdina:
> > On Fri, 2012-11-23 at 13:09 +0100, Kevin Wolf wrote:
> >> Am 21.11.2012 18:17, schrieb Pavel Hrdina:
> >>> 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.
> 
> Hm, yes, I missed the existing ide_drive_post_load() implementation.
> However, this is what the code looks like:
> 
>     if (version_id < 3) {
>         if (s->sense_key == UNIT_ATTENTION &&
>             s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
>             s->cdrom_changed = 1;
>         }
>     }
> 
> version_id for current qemu version is 3, so the intended magic doesn't
> happen.
> 
> >> 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.
> 
> Considering that the goal isn't really to set sense_key/asc so that the
> guest can read it, but just so s->cdrom_changed is right on the
> destination, I agree that it makes sense as it is.
> 
> >> 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.
> 
> I think it's nicer to have only one state. And if I'm not mistaken, we
> can even do without the pre_save/post_load handlers then.
> 
> Kevin

I don't think that we can do it without the pre_save/post_load handlers,
because if the cdrom_changed could be set also to 2 then in case, that
you migrate immediately after the change from "buggy" qemu to the
"fixed" qemu we have in cdrom_changed state 1, but we need state 2. And
otherwise, if you want to migrate from "fixed" qemu to "buggy" qemu
where version_id is lower then 3, you have to set the asc to
ASC_MEDIUM_MAY_HAVE_CHANGED.

I'll make a new version of this patch and then we can discus about it.

Pavel