From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MWE7k-0002so-KT for qemu-devel@nongnu.org; Wed, 29 Jul 2009 14:43:36 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MWE7g-0002p6-Va for qemu-devel@nongnu.org; Wed, 29 Jul 2009 14:43:36 -0400 Received: from [199.232.76.173] (port=34395 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MWE7g-0002p3-PB for qemu-devel@nongnu.org; Wed, 29 Jul 2009 14:43:32 -0400 Received: from mail-ew0-f210.google.com ([209.85.219.210]:39740) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MWE7g-00035X-02 for qemu-devel@nongnu.org; Wed, 29 Jul 2009 14:43:32 -0400 Received: by ewy6 with SMTP id 6so192961ewy.34 for ; Wed, 29 Jul 2009 11:43:30 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20090729173820.GA7382@redhat.com> References: <20090729160902.GG30449@redhat.com> <5b31733c0907291010k6ce95d54g6cbaaa954f22409b@mail.gmail.com> <20090729173820.GA7382@redhat.com> Date: Wed, 29 Jul 2009 20:43:29 +0200 Message-ID: <5b31733c0907291143h75b04339r173b90647126acc3@mail.gmail.com> Subject: Re: [Qemu-devel] [PATCH v3] make windows notice media change From: Filip Navara Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gleb Natapov Cc: qemu-devel@nongnu.org 2009/7/29 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 wrote: >> > @@ -3250,6 +3253,8 @@ static int pci_ide_load(QEMUFile* f, void *opaqu= e, int version_id) >> > =A0 =A0 /* per IDE drive data */ >> > =A0 =A0 for(i =3D 0; i < 4; i++) { >> > =A0 =A0 =A0 =A0 ide_load(f, &d->ide_if[i]); >> > + =A0 =A0 =A0 =A0if (version_id =3D=3D 3) >> > + =A0 =A0 =A0 =A0 =A0 =A0qemu_get_8s(f, &d->ide_if[i].cdrom_changed); >> > =A0 =A0 } >> > =A0 =A0 return 0; >> > =A0} >> >> I'd prefer passing the version to ide_load and doing the actual load the= re... >> > 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 N= T 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: >> =A0 =A0 =A0 =A0 if (bdrv_is_inserted(s->bs)) { >> =A0 =A0 =A0 =A0 =A0 =A0 if (s->cdrom_changed) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ide_atapi_cmd_error(s, SENSE_UNIT_ATTENT= ION, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ASC_M= EDIUM_MAY_HAVE_CHANGED); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->cdrom_changed =3D 0; >> =A0 =A0 =A0 =A0 =A0 =A0 } else { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ide_atapi_cmd_ok(s); >> =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 } else { >> =A0 =A0 =A0 =A0 =A0 =A0 ide_atapi_cmd_error(s, SENSE_NOT_READY, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ASC_MEDI= UM_NOT_PRESENT); >> =A0 =A0 =A0 =A0 } >> >> The benefit is that it will not break guests which issue the request onl= y >> once. >> > =A010.8.26 TEST UNIT READY Command > =A0The TEST UNIT READY command provides a means to check if the Device is > =A0ready. This is not a request for a self-test. If the Device would acce= pt > =A0an appropriate medium-access command without returning CHECK CONDITION > =A0status, this command shall return a GOOD status. If the Device cannot > =A0become operational or is in a state such that an Host Computer action > =A0(e.g. START/STOP UNIT command with LoEj =3D 0 & Start =3D 1) is requir= ed to > =A0make the unit ready, the ATAPI CD-ROM Drive shall return CHECK CONDITI= ON > =A0status 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? > =A010.6 Unit Attention Condition > =A0If an Host Computer issues a command other than INQUIRY or REQUEST SEN= SE > =A0while a unit attention condition exists for that Host, the ATAPI CD-RO= M > =A0Drive shall not perform the command and shall report CHECK CONDITION > =A0status unless a higher priority status as defined by the ATAPI CD-ROM > =A0Drive 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