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

[Kevin Wolf <kwolf@redhat.com>]

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