From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MXWJW-0007Et-No for qemu-devel@nongnu.org; Sun, 02 Aug 2009 04:21:06 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MXWJR-0007EA-SZ for qemu-devel@nongnu.org; Sun, 02 Aug 2009 04:21:05 -0400 Received: from [199.232.76.173] (port=48191 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MXWJR-0007E3-PE for qemu-devel@nongnu.org; Sun, 02 Aug 2009 04:21:01 -0400 Received: from mail-ew0-f210.google.com ([209.85.219.210]:64734) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MXWJR-0006Kb-8w for qemu-devel@nongnu.org; Sun, 02 Aug 2009 04:21:01 -0400 Received: by ewy6 with SMTP id 6so560438ewy.34 for ; Sun, 02 Aug 2009 01:21:00 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20090802080608.GQ30449@redhat.com> References: <20090802080608.GQ30449@redhat.com> Date: Sun, 2 Aug 2009 10:21:00 +0200 Message-ID: <5b31733c0908020121t11bc3a1fmc7dbe537428bc964@mail.gmail.com> Subject: Re: [Qemu-devel] [PATCH v4] 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 On Sun, Aug 2, 2009 at 10:06 AM, Gleb Natapov wrote: > 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. > > Filip Navara did a great job debugging this issue in Windows and this is > what he found out: > > BINGO! ... The media present notifications were broken ever since > Windows 2000 it seems. The media change is detected properly and it's > passed to ClassSetMediaChangeState function which in turn calls > ClasspInternalSetMediaChangeState. This function is responsible for > changing some internal state of the device object and sending the PnP > events which later result in application notifications. It has this > tiny bit of code (not copied byte for byte): > > if (oldMediaState =3D=3D NewState) { > =A0// Media is in the same state it was before. > =A0return; > } > > so the end result is that for the case of UNIT NEEDS ATTENTION / > MEDIUM MAY HAVE CHANGED without NOT READY in-between is really broken. > It results in the internal media change counter incremented, so the > media contents are re-read when necessary, instead of relying on the > cache, but the notifications to applications are never sent. > > Signed-off-by: Gleb Natapov > --- > v1->v2: > =A0do not reuse media_changed. Save/restore on migration. > > v2->v3: > =A0allow migration from 2 to 3. > > v3->v4: > =A0fix migration from 2 to 3 for pmac and md. > > diff --git a/hw/ide.c b/hw/ide.c > index 6cf04a6..268e589 100644 > --- a/hw/ide.c > +++ b/hw/ide.c > @@ -418,6 +418,7 @@ typedef struct IDEState { > =A0 =A0 /* ATAPI specific */ > =A0 =A0 uint8_t sense_key; > =A0 =A0 uint8_t asc; > + =A0 =A0uint8_t cdrom_changed; > =A0 =A0 int packet_transfer_size; > =A0 =A0 int elementary_transfer_size; > =A0 =A0 int io_buffer_index; > @@ -1660,9 +1661,10 @@ static void ide_atapi_cmd(IDEState *s) > =A0 =A0 } > =A0 =A0 switch(s->io_buffer[0]) { > =A0 =A0 case GPCMD_TEST_UNIT_READY: > - =A0 =A0 =A0 =A0if (bdrv_is_inserted(s->bs)) { > + =A0 =A0 =A0 =A0if (bdrv_is_inserted(s->bs) && !s->cdrom_changed) { > =A0 =A0 =A0 =A0 =A0 =A0 ide_atapi_cmd_ok(s); > =A0 =A0 =A0 =A0 } else { > + =A0 =A0 =A0 =A0 =A0 =A0s->cdrom_changed =3D 0; > =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_MEDIU= M_NOT_PRESENT); > =A0 =A0 =A0 =A0 } > @@ -2122,7 +2124,7 @@ static void cdrom_change_cb(void *opaque) > > =A0 =A0 s->sense_key =3D SENSE_UNIT_ATTENTION; > =A0 =A0 s->asc =3D ASC_MEDIUM_MAY_HAVE_CHANGED; > - > + =A0 =A0s->cdrom_changed =3D 1; > =A0 =A0 ide_set_irq(s); > =A0} > > @@ -2890,7 +2892,7 @@ static void ide_save(QEMUFile* f, IDEState *s) > =A0} > > =A0/* load per IDE drive data */ > -static void ide_load(QEMUFile* f, IDEState *s) > +static void ide_load(QEMUFile* f, IDEState *s, int version_id) > =A0{ > =A0 =A0 s->mult_sectors=3Dqemu_get_be32(f); > =A0 =A0 s->identify_set=3Dqemu_get_be32(f); > @@ -2914,6 +2916,13 @@ static void ide_load(QEMUFile* f, IDEState *s) > > =A0 =A0 qemu_get_8s(f, &s->sense_key); > =A0 =A0 qemu_get_8s(f, &s->asc); > + =A0 =A0if (version_id =3D=3D 3) { > + =A0 =A0 =A0 =A0qemu_get_8s(f, &s->cdrom_changed); > + =A0 =A0} else { > + =A0 =A0 =A0 =A0if (s->sense_key =3D=3D SENSE_UNIT_ATTENTION && > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->asc =3D=3D ASC_MEDIUM_MA= Y_HAVE_CHANGED) > + =A0 =A0 =A0 =A0 =A0 =A0s->cdrom_changed =3D 1; > + =A0 =A0} > =A0 =A0 /* XXX: if a transfer is pending, we do not save it yet */ > =A0} > > @@ -3235,7 +3244,7 @@ static int pci_ide_load(QEMUFile* f, void *opaque, = int version_id) > =A0 =A0 PCIIDEState *d =3D opaque; > =A0 =A0 int ret, i; > > - =A0 =A0if (version_id !=3D 2) > + =A0 =A0if (version_id !=3D 2 || version_id !=3D 3) > =A0 =A0 =A0 =A0 return -EINVAL; Should be && > =A0 =A0 ret =3D pci_device_load(&d->dev, f); > =A0 =A0 if (ret < 0) > @@ -3265,7 +3274,7 @@ static int pci_ide_load(QEMUFile* f, void *opaque, = int version_id) > > =A0 =A0 /* per IDE drive data */ > =A0 =A0 for(i =3D 0; i < 4; i++) { > - =A0 =A0 =A0 =A0ide_load(f, &d->ide_if[i]); > + =A0 =A0 =A0 =A0ide_load(f, &d->ide_if[i], version_id); > =A0 =A0 } > =A0 =A0 return 0; > =A0} > @@ -3355,7 +3364,7 @@ void pci_cmd646_ide_init(PCIBus *bus, BlockDriverSt= ate **hd_table, > =A0 =A0 ide_init2(&d->ide_if[0], hd_table[0], hd_table[1], irq[0]); > =A0 =A0 ide_init2(&d->ide_if[2], hd_table[2], hd_table[3], irq[1]); > > - =A0 =A0register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d); > + =A0 =A0register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d); > =A0 =A0 qemu_register_reset(cmd646_reset, d); > =A0 =A0 cmd646_reset(d); > =A0} > @@ -3414,7 +3423,7 @@ void pci_piix3_ide_init(PCIBus *bus, BlockDriverSta= te **hd_table, int devfn, > =A0 =A0 =A0 =A0 if (hd_table[i]) > =A0 =A0 =A0 =A0 =A0 =A0 hd_table[i]->private =3D &d->dev; > > - =A0 =A0register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d); > + =A0 =A0register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d); > =A0} > > =A0/* hd_table must contain 4 block drivers */ > @@ -3450,7 +3459,7 @@ void pci_piix4_ide_init(PCIBus *bus, BlockDriverSta= te **hd_table, int devfn, > =A0 =A0 ide_init_ioport(&d->ide_if[0], 0x1f0, 0x3f6); > =A0 =A0 ide_init_ioport(&d->ide_if[2], 0x170, 0x376); > > - =A0 =A0register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d); > + =A0 =A0register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d); > =A0} > > =A0#if defined(TARGET_PPC) > @@ -3738,7 +3747,7 @@ static int pmac_ide_load(QEMUFile *f, void *opaque,= int version_id) > =A0 =A0 uint8_t drive1_selected; > =A0 =A0 unsigned int i; > > - =A0 =A0if (version_id !=3D 1) > + =A0 =A0if (version_id !=3D 1 || version_id !=3D 3) > =A0 =A0 =A0 =A0 return -EINVAL; Should be && > > =A0 =A0 /* per IDE interface data */ > @@ -3748,7 +3757,7 @@ static int pmac_ide_load(QEMUFile *f, void *opaque,= int version_id) > > =A0 =A0 /* per IDE drive data */ > =A0 =A0 for(i =3D 0; i < 2; i++) { > - =A0 =A0 =A0 =A0ide_load(f, &s[i]); > + =A0 =A0 =A0 =A0ide_load(f, &s[i], version_id); > =A0 =A0 } > =A0 =A0 return 0; > =A0} > @@ -3779,7 +3788,7 @@ int pmac_ide_init (BlockDriverState **hd_table, qem= u_irq irq, > > =A0 =A0 pmac_ide_memory =3D cpu_register_io_memory(pmac_ide_read, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0pmac_ide_write, d); > - =A0 =A0register_savevm("ide", 0, 1, pmac_ide_save, pmac_ide_load, d); > + =A0 =A0register_savevm("ide", 0, 3, pmac_ide_save, pmac_ide_load, d); > =A0 =A0 qemu_register_reset(pmac_ide_reset, d); > =A0 =A0 pmac_ide_reset(d); > > @@ -4172,6 +4181,9 @@ static int md_load(QEMUFile *f, void *opaque, int v= ersion_id) > =A0 =A0 int i; > =A0 =A0 uint8_t drive1_selected; > > + =A0 =A0if (version_id !=3D 0 || version_id !=3D 3) > + =A0 =A0 =A0 =A0return -EINVAL; > + Should be && > =A0 =A0 qemu_get_8s(f, &s->opt); > =A0 =A0 qemu_get_8s(f, &s->stat); > =A0 =A0 qemu_get_8s(f, &s->pins); > @@ -4185,7 +4197,7 @@ static int md_load(QEMUFile *f, void *opaque, int v= ersion_id) > =A0 =A0 s->ide->cur_drive =3D &s->ide[(drive1_selected !=3D 0)]; > > =A0 =A0 for (i =3D 0; i < 2; i ++) > - =A0 =A0 =A0 =A0ide_load(f, &s->ide[i]); > + =A0 =A0 =A0 =A0ide_load(f, &s->ide[i], version_id); > > =A0 =A0 return 0; > =A0} > @@ -4416,7 +4428,7 @@ PCMCIACardState *dscm1xxxx_init(BlockDriverState *b= drv) > =A0 =A0 md->ide->mdata_size =3D METADATA_SIZE; > =A0 =A0 md->ide->mdata_storage =3D (uint8_t *) qemu_mallocz(METADATA_SIZE= ); > > - =A0 =A0register_savevm("microdrive", -1, 0, md_save, md_load, md); > + =A0 =A0register_savevm("microdrive", -1, 3, md_save, md_load, md); > > =A0 =A0 return &md->card; > =A0} > -- > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Gleb. Otherwise it looks good. Thanks for taking the time to help me with the debugging. Best regards, Filip Navara