[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] make windows notice media change
From: |
Filip Navara |
Subject: |
Re: [Qemu-devel] [PATCH v3] make windows notice media change |
Date: |
Wed, 29 Jul 2009 20:43:29 +0200 |
2009/7/29 Gleb Natapov <address@hidden>:
> On Wed, Jul 29, 2009 at 07:10:29PM +0200, Filip Navara wrote:
>> On Wed, Jul 29, 2009 at 6:09 PM, Gleb Natapov<address@hidden> 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
Re: [Qemu-devel] [PATCH v3] make windows notice media change, Paul Brook, 2009/07/29