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

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

[Gleb Natapov]

Windows seems to be very stupid about cdrom media change. It polls
cdrom status and if status goes ready->media not present->ready
it assumes that media was changed. If "media not present" step doesn't
happen even if "medium may have changed" was seen it assumes media
haven't changed. Fake "media not present" step.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
v1->v2:
  do not reuse media_changed. Save/restore on migration.

v2->v3:
  allow migration from 2 to 3.

diff --git a/hw/ide.c b/hw/ide.c
index 1e56786..7da400e 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -418,6 +418,7 @@ typedef struct IDEState {
     /* ATAPI specific */
     uint8_t sense_key;
     uint8_t asc;
+    uint8_t cdrom_changed;
     int packet_transfer_size;
     int elementary_transfer_size;
     int io_buffer_index;
@@ -1644,9 +1645,10 @@ static void ide_atapi_cmd(IDEState *s)
     }
     switch(s->io_buffer[0]) {
     case GPCMD_TEST_UNIT_READY:
-        if (bdrv_is_inserted(s->bs)) {
+        if (bdrv_is_inserted(s->bs) && !s->cdrom_changed) {
             ide_atapi_cmd_ok(s);
         } else {
+            s->cdrom_changed = 0;
             ide_atapi_cmd_error(s, SENSE_NOT_READY,
                                 ASC_MEDIUM_NOT_PRESENT);
         }
@@ -2106,7 +2108,7 @@ static void cdrom_change_cb(void *opaque)
 
     s->sense_key = SENSE_UNIT_ATTENTION;
     s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
-
+    s->cdrom_changed = 1;
     ide_set_irq(s);
 }
 
@@ -3211,6 +3213,7 @@ static void pci_ide_save(QEMUFile* f, void *opaque)
     /* per IDE drive data */
     for(i = 0; i < 4; i++) {
         ide_save(f, &d->ide_if[i]);
+        qemu_put_8s(f, &d->ide_if[i].cdrom_changed);
     }
 }
 
@@ -3219,7 +3222,7 @@ static int pci_ide_load(QEMUFile* f, void *opaque, int version_id)
     PCIIDEState *d = opaque;
     int ret, i;
 
-    if (version_id != 2)
+    if (version_id < 2)
         return -EINVAL;
     ret = pci_device_load(&d->dev, f);
     if (ret < 0)
@@ -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;
 }
@@ -3339,7 +3344,7 @@ void pci_cmd646_ide_init(PCIBus *bus, BlockDriverState **hd_table,
     ide_init2(&d->ide_if[0], hd_table[0], hd_table[1], irq[0]);
     ide_init2(&d->ide_if[2], hd_table[2], hd_table[3], irq[1]);
 
-    register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d);
+    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
     qemu_register_reset(cmd646_reset, d);
     cmd646_reset(d);
 }
--
			Gleb.

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

[Filip Navara]

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...

... 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.

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 /
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.

Best regards,
Filip Navara

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

[Gleb Natapov]

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.

> ... 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?

> 
> 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:

 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.

--
			Gleb.

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

[Filip Navara]

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

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

[Gleb Natapov]

On Wed, Jul 29, 2009 at 08:43:29PM +0200, Filip Navara wrote:
> 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.
> 
Except pmac protocol version is different from pci protocol version.

> >> ... 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.
> 
And you holding all of the detail of this particular code in your head?
Please educate us what QEMU currently does wrong that prevent smart
Windows code from working? This allegedly smart code polls cdrom like crazy BTW.

> >> 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?
>
Please go and read QEMU IDE code once more. Read spec. Run guest and see
what it does. What I see from doing all this is that after setting
sense_key in cdrom_change_cb() Windows issues UNIT READY call which is
_not_ executed according to the spec, but ide_atapi_cmd_check_status()
is executed instead. After that windows correctly calls REQUEST SENSE
and reads SENSE UNIT ATTENTION. At this point condition is cleared.
Then Windows calls UNIT READY once again and at this point
ide_atapi_cmd_ok() is called without sense_key set.


> >  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.
> 
With that I agree. My log message clearly states that this is a
workaround since QEMU code look correct to me (also not ATAPI expert).
And I agree with your concern too. It easily fixed by running a short
timer that will reinject SENSE UNIT ATTENTION interrupt after returning
NOT PRESENT status if there is a need, but for now I didn't wanted to
complicate the logic.

> 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.
> 
You can look at DDK code at tell us were QEMU device emulation is wrong.
After this patch will be applied of cause. Just claiming things without
even looking at spec doesn't help.

--
			Gleb.

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

[Filip Navara]

2009/7/29 Gleb Natapov <gleb@redhat.com>:
> On Wed, Jul 29, 2009 at 08:43:29PM +0200, Filip Navara wrote:
>> 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.
>>
> Except pmac protocol version is different from pci protocol version.

There's no reason not to jump directly from version 1 to version 3.

>> >> ... 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.
>>
> And you holding all of the detail of this particular code in your head?

No. I read the code 4 years ago, so obviously I was unsure about it
now, so I looked it up. That's why it took me few hours to respond on
the patch.

> Please educate us what QEMU currently does wrong that prevent smart
> Windows code from working? This allegedly smart code polls cdrom like crazy BTW.

I didn't say it is smart, what I said is that it is not as stupid as
you claim it is. I also wanted to know why it does the polling, but I
don't know the answer for that question.

>> >> 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?
>>
> Please go and read QEMU IDE code once more. Read spec. Run guest and see
> what it does. What I see from doing all this is that after setting
> sense_key in cdrom_change_cb() Windows issues UNIT READY call which is
> _not_ executed according to the spec

The code is spread across several drivers and the IDE one is not part of the DDK
examples. From what I can see the there is no code for handling
SENSE_UNIT_ATTENTION which didn't result from a command explicitly
sent to the ATAPI drive, so what you see is the periodic timer which issues the
TEST UNIT READY request.

Even if this wasn't according to the IDE spec. the code seems to be able to
recover from error and read the sense data stored in sense_key and asc state
anyway. ide_atapi_cmd_check_status doesn't clear them. I will check it
once I get my Windows installation ready.

> , but ide_atapi_cmd_check_status()
> is executed instead. After that windows correctly calls REQUEST SENSE
> and reads SENSE UNIT ATTENTION.

The REQUEST SENSE is probably the one resulting from the issued
TEST UNIT READY command in the cdrom driver, this is usual behavior
of the scsiport driver (which is responsible for all the SCSI requests in
Windows).

> At this point condition is cleared.

In GPCMD_REQUEST_SENSE handler, right?

Hmm, now it makes me wonder if the REQUEST SENSE was really
the one resulting from the TEST UNIT READY request in cdrom
driver or if it got lost somewhere.

> Then Windows calls UNIT READY once again and at this point
> ide_atapi_cmd_ok() is called without sense_key set.

This one must be from the next timer expiration, the TEST UNIT
READY command is never reissued for the single expiration.

Thanks for the explanation. I will try to reproduce it and see if I can
make any SENSE out of it.

>> >  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).

Actually the Windows behavior is compliant with this. It issues
different command
from INQUIRY / REQUEST SENSE and is supposed to deal with the failure.

>> > 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.
>>
> With that I agree. My log message clearly states that this is a
> workaround since QEMU code look correct to me (also not ATAPI expert).
> And I agree with your concern too. It easily fixed by running a short
> timer that will reinject SENSE UNIT ATTENTION interrupt after returning
> NOT PRESENT status if there is a need, but for now I didn't wanted to
> complicate the logic.

I believe the logic can actually be simplified, if we can find the root cause
of the problem.

>> 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.
>>
> You can look at DDK code at tell us were QEMU device emulation is wrong.
> After this patch will be applied of cause. Just claiming things without
> even looking at spec doesn't help.

I did look at the DDK code and I read the spec. It's not like my claims were
not backed with some actual reading, my suggested fix was. I'm glad you
provided me with the information that could be used for me better
understanding the problem. Hopefully I can come up with some patch or
further information within few days.

Best regards,
Filip Navara

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

[Jamie Lokier]

Filip Navara wrote:
> I didn't say it is smart, what I said is that it is not as stupid as
> you claim it is. I also wanted to know why it does the polling, but I
> don't know the answer for that question.

Maybe some old CD-ROMs don't report ASC_MEDIUM_MAY_HAVE_CHANGED?

Maybe it polls for the same reason that freedesktop HAL polls, but
decided to do it more often for a quicker response?

-- Jamie

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

[Gleb Natapov]

On Wed, Jul 29, 2009 at 11:04:14PM +0200, Filip Navara wrote:
> 2009/7/29 Gleb Natapov <gleb@redhat.com>:
> > On Wed, Jul 29, 2009 at 08:43:29PM +0200, Filip Navara wrote:
> >> 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.
> >>
> > Except pmac protocol version is different from pci protocol version.
> 
> There's no reason not to jump directly from version 1 to version 3.
> 
And md? But I still don't understand what was a conclusion. Do we want
to support migration from older version?

> >> >> ... 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.
> >>
> > And you holding all of the detail of this particular code in your head?
> 
> No. I read the code 4 years ago, so obviously I was unsure about it
> now, so I looked it up. That's why it took me few hours to respond on
> the patch.
> 
> > Please educate us what QEMU currently does wrong that prevent smart
> > Windows code from working? This allegedly smart code polls cdrom like crazy BTW.
> 
> I didn't say it is smart, what I said is that it is not as stupid as
> you claim it is. I also wanted to know why it does the polling, but I
> don't know the answer for that question.
> 
I don't understand how you can say that without pointing out what is the
right solution. Either the code does what I said and then it is stupid
or you know what QEMU does wrong.

> >> >> 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?
> >>
> > Please go and read QEMU IDE code once more. Read spec. Run guest and see
> > what it does. What I see from doing all this is that after setting
> > sense_key in cdrom_change_cb() Windows issues UNIT READY call which is
> > _not_ executed according to the spec
> 
> The code is spread across several drivers and the IDE one is not part of the DDK
> examples. From what I can see the there is no code for handling
> SENSE_UNIT_ATTENTION which didn't result from a command explicitly
> sent to the ATAPI drive, so what you see is the periodic timer which issues the
> TEST UNIT READY request.
This should be ATAPI code not IDE. And how do you know the code Windows
actually use is the code they provide as DDK examples?

> 
> Even if this wasn't according to the IDE spec. the code seems to be able to
> recover from error and read the sense data stored in sense_key and asc state
> anyway. ide_atapi_cmd_check_status doesn't clear them. I will check it
> once I get my Windows installation ready.
ide_atapi_cmd_check_status tells Windows to use REQUEST_SENSE to query
sense_key and Windows does that and still does not reread cdrom.

> 
> > , but ide_atapi_cmd_check_status()
> > is executed instead. After that windows correctly calls REQUEST SENSE
> > and reads SENSE UNIT ATTENTION.
> 
> The REQUEST SENSE is probably the one resulting from the issued
> TEST UNIT READY command in the cdrom driver, this is usual behavior
> of the scsiport driver (which is responsible for all the SCSI requests in
> Windows).
Yes the REQUEST SENSE is a result of TEST UNIT READY command that
returned "check status". Everything according to the spec.

> 
> > At this point condition is cleared.
> 
> In GPCMD_REQUEST_SENSE handler, right?
Yes.

> 
> Hmm, now it makes me wonder if the REQUEST SENSE was really
> the one resulting from the TEST UNIT READY request in cdrom
> driver or if it got lost somewhere.
> 
Yes it was a result of TEST UNIT READY as printf shows.

> > Then Windows calls UNIT READY once again and at this point
> > ide_atapi_cmd_ok() is called without sense_key set.
> 
> This one must be from the next timer expiration, the TEST UNIT
> READY command is never reissued for the single expiration.
> 
I guess so.

> Thanks for the explanation. I will try to reproduce it and see if I can
> make any SENSE out of it.
> 
> >> >  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).
> 
> Actually the Windows behavior is compliant with this. It issues
> different command
> from INQUIRY / REQUEST SENSE and is supposed to deal with the failure.
> 
Yes Windows respond correctly to CHECK CONDITION status.

> >> > 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.
> >>
> > With that I agree. My log message clearly states that this is a
> > workaround since QEMU code look correct to me (also not ATAPI expert).
> > And I agree with your concern too. It easily fixed by running a short
> > timer that will reinject SENSE UNIT ATTENTION interrupt after returning
> > NOT PRESENT status if there is a need, but for now I didn't wanted to
> > complicate the logic.
> 
> I believe the logic can actually be simplified, if we can find the root cause
> of the problem.
If the root cause of the problem is not what I claim it is (stupid
Windows) and I don't exclude this possibility at all and the real reason
is that QEMU has a bug in ATAPI implementation then the correct solution
of course is to fix ATAPI emulation. But till now you have no any
poof for the later and we shouldn't wait for it. I want this patch to be
applied and when/if you'll find correct solution we will replace the
hack with your code.

> 
> >> 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.
> >>
> > You can look at DDK code at tell us were QEMU device emulation is wrong.
> > After this patch will be applied of cause. Just claiming things without
> > even looking at spec doesn't help.
> 
> I did look at the DDK code and I read the spec. It's not like my claims were
> not backed with some actual reading, my suggested fix was. I'm glad you
> provided me with the information that could be used for me better
> understanding the problem. Hopefully I can come up with some patch or
> further information within few days.
> 
I suggest to apply this hack now and replace it with your correct
solution later.

--
			Gleb.

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

[Paul Brook]

> -    if (version_id != 2)
> +    if (version_id < 2)
>          return -EINVAL;

This is still wrong. You can not assume future versions will be backward 
compatible.

Paul