From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MWFVw-0006D3-1D for qemu-devel@nongnu.org; Wed, 29 Jul 2009 16:12:40 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MWFVr-0006AM-3Q for qemu-devel@nongnu.org; Wed, 29 Jul 2009 16:12:39 -0400 Received: from [199.232.76.173] (port=54345 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MWFVq-0006AB-U6 for qemu-devel@nongnu.org; Wed, 29 Jul 2009 16:12:35 -0400 Received: from mx2.redhat.com ([66.187.237.31]:59394) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MWFVq-0007bz-Cf for qemu-devel@nongnu.org; Wed, 29 Jul 2009 16:12:34 -0400 Date: Wed, 29 Jul 2009 23:12:30 +0300 From: Gleb Natapov Subject: Re: [Qemu-devel] [PATCH v3] make windows notice media change Message-ID: <20090729201230.GB7382@redhat.com> References: <20090729160902.GG30449@redhat.com> <5b31733c0907291010k6ce95d54g6cbaaa954f22409b@mail.gmail.com> <20090729173820.GA7382@redhat.com> <5b31733c0907291143h75b04339r173b90647126acc3@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <5b31733c0907291143h75b04339r173b90647126acc3@mail.gmail.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Filip Navara Cc: qemu-devel@nongnu.org On Wed, Jul 29, 2009 at 08:43:29PM +0200, Filip Navara wrote: > 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 *opa= que, int version_id) > >> > =9A =9A /* per IDE drive data */ > >> > =9A =9A for(i =3D 0; i < 4; i++) { > >> > =9A =9A =9A =9A ide_load(f, &d->ide_if[i]); > >> > + =9A =9A =9A =9Aif (version_id =3D=3D 3) > >> > + =9A =9A =9A =9A =9A =9Aqemu_get_8s(f, &d->ide_if[i].cdrom_changed); > >> > =9A =9A } > >> > =9A =9A return 0; > >> > =9A} > >> > >> I'd prefer passing the version to ide_load and doing the actual load t= here... > >> > > Then you'll break ide_load for md ad pmac. >=20 > You would actually unbreak the PowerMAC code. It should save the > cdrom_changed flag the same way as the PC version does. >=20 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? >=20 > Yes, I did. It's part of Windows DDK and it has been there at least since= NT 4. >=20 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: > >> =9A =9A =9A =9A if (bdrv_is_inserted(s->bs)) { > >> =9A =9A =9A =9A =9A =9A if (s->cdrom_changed) { > >> =9A =9A =9A =9A =9A =9A =9A =9A ide_atapi_cmd_error(s, SENSE_UNIT_ATTE= NTION, > >> =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9AASC= _MEDIUM_MAY_HAVE_CHANGED); > >> =9A =9A =9A =9A =9A =9A =9A =9A s->cdrom_changed =3D 0; > >> =9A =9A =9A =9A =9A =9A } else { > >> =9A =9A =9A =9A =9A =9A =9A =9A ide_atapi_cmd_ok(s); > >> =9A =9A =9A =9A =9A =9A } > >> =9A =9A =9A =9A } else { > >> =9A =9A =9A =9A =9A =9A ide_atapi_cmd_error(s, SENSE_NOT_READY, > >> =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A ASC_ME= DIUM_NOT_PRESENT); > >> =9A =9A =9A =9A } > >> > >> The benefit is that it will not break guests which issue the request o= nly > >> once. > >> > > =9A10.8.26 TEST UNIT READY Command > > =9AThe TEST UNIT READY command provides a means to check if the Device = is > > =9Aready. This is not a request for a self-test. If the Device would ac= cept > > =9Aan appropriate medium-access command without returning CHECK CONDITI= ON > > =9Astatus, this command shall return a GOOD status. If the Device cannot > > =9Abecome operational or is in a state such that an Host Computer action > > =9A(e.g. START/STOP UNIT command with LoEj =3D 0 & Start =3D 1) is requ= ired to > > =9Amake the unit ready, the ATAPI CD-ROM Drive shall return CHECK CONDI= TION > > =9Astatus 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: >=20 > 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. > > =9A10.6 Unit Attention Condition > > =9AIf an Host Computer issues a command other than INQUIRY or REQUEST S= ENSE > > =9Awhile a unit attention condition exists for that Host, the ATAPI CD-= ROM > > =9ADrive shall not perform the command and shall report CHECK CONDITION > > =9Astatus unless a higher priority status as defined by the ATAPI CD-ROM > > =9ADrive 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. >=20 > 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 a= nd 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. >=20 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. >=20 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.