Re: [Qemu-devel] [PATCH v3] make windows notice media change

[Filip Navara <filip.navara@gmail.com>]

2009/7/29 Gleb Natapov <gleb@redhat.com>:
> On Wed, Jul 29, 2009 at 07:10:29PM +0200, Filip Navara wrote:
>> On Wed, Jul 29, 2009 at 6:09 PM, Gleb Natapov<gleb@redhat.com> wrote:
>> > @@ -3250,6 +3253,8 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id)
>> >     /* per IDE drive data */
>> >     for(i = 0; i < 4; i++) {
>> >         ide_load(f, &d->ide_if[i]);
>> > +        if (version_id == 3)
>> > +            qemu_get_8s(f, &d->ide_if[i].cdrom_changed);
>> >     }
>> >     return 0;
>> >  }
>>
>> I'd prefer passing the version to ide_load and doing the actual load there...
>>
> Then you'll break ide_load for md ad pmac.

You would actually unbreak the PowerMAC code. It should save the
cdrom_changed flag the same way as the PC version does.

>> ... but the patch is all wrong and based on wrong assumptions, which is
>> much more fundamental problem. Windows cdrom driver is not that stupid
>> about the change as you think.
> Have you seen the code? How do you know?

Yes, I did. It's part of Windows DDK and it has been there at least since NT 4.

>> The cdrom driver really has a timer and polls the IDE controller, but it
>> doesn't require the intermediate ASC_MEDIUM_NOT_PRESENT state
>> you introduced. It's perfectly ok to return SENSE_UNIT_ATTENTION /
> I have
>> ASC_MEDIUM_MAY_HAVE_CHANGED from GPCMD_TEST_UNIT_READY
>> and Windows will recognize it as medium change.
>>
>> Something like this should work:
>>         if (bdrv_is_inserted(s->bs)) {
>>             if (s->cdrom_changed) {
>>                 ide_atapi_cmd_error(s, SENSE_UNIT_ATTENTION,
>>                                  ASC_MEDIUM_MAY_HAVE_CHANGED);
>>                 s->cdrom_changed = 0;
>>             } else {
>>                 ide_atapi_cmd_ok(s);
>>             }
>>         } else {
>>             ide_atapi_cmd_error(s, SENSE_NOT_READY,
>>                                 ASC_MEDIUM_NOT_PRESENT);
>>         }
>>
>> The benefit is that it will not break guests which issue the request only
>> once.
>>
>  10.8.26 TEST UNIT READY Command
>  The TEST UNIT READY command provides a means to check if the Device is
>  ready. This is not a request for a self-test. If the Device would accept
>  an appropriate medium-access command without returning CHECK CONDITION
>  status, this command shall return a GOOD status. If the Device cannot
>  become operational or is in a state such that an Host Computer action
>  (e.g. START/STOP UNIT command with LoEj = 0 & Start = 1) is required to
>  make the unit ready, the ATAPI CD-ROM Drive shall return CHECK CONDITION
>  status with a sense key of NOT READY.
>
> No mentioning of returning MEDIUM MAY HAVE CHANGED from UNIT READY
> command, so you code it already incorrect. So what should be done in
> this case. Here is what spec says and code actually implements:

Well, my suggestion is wrong, because I didn't read the QEMU IDE code
carefully. It should return GOOD status, which is what ide_atapi_cmd_ok
already did. Notice that ide_atapi_cmd_ok doesn't set s->sense_key and
s->asc, so the values from cdrom_change_cb should be preserved and
Windows driver would still happily received the "MEDIUM MAY HAVE
CHANGED" code. So I wonder what really fails?

>  10.6 Unit Attention Condition
>  If an Host Computer issues a command other than INQUIRY or REQUEST SENSE
>  while a unit attention condition exists for that Host, the ATAPI CD-ROM
>  Drive shall not perform the command and shall report CHECK CONDITION
>  status unless a higher priority status as defined by the ATAPI CD-ROM
>  Drive is also pending (e.g. BUSY).
>
> Cool. So Windows calls REQUEST SENSE after seeing CHECK CONDITION. Gets
> MEDIUM MAY HAVE CHANGED calls TEST UNIT once again see that media is
> present and thinks that CDROM gone crazy.
>
> If you claim that my fix is incorrect (it may very well be) please
> provide working tested solution compliant to spec.

I'm no IDE expert, but your change is workaround that may break
well-behaved guests
because the TEST_UNIT_READY code will intentionally return wrong result and the
guest has no reason to retry the query. The fact that Windows driver
has a timer and
eventually re-queries the status is something one shouldn't depend on.

If I knew how to patch it properly I would have done, but I don't. I'm
more than willing
to explain how Windows behaves, but I can't any patches at the moment
since I have
no Windows virtual machine ready for testing.

Best regards,
Filip Navara