Skip to contentSkip to conversation

make windows notice media change

PATCH Superseded Gleb Natapov 2 participants 3 messages Aug 2, 2009

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

PATCH v4 1/1 make windows notice media change

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 == NewState) {
// Media is in the same state it was before.
return;
}

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.

---
v1->v2:
do not reuse media_changed. Save/restore on migration.

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

v3->v4:
fix migration from 2 to 3 for pmac and md.

hw/ide.c +25 -13
index 6cf04a6..268e589 100644
@@ -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;
@@ -1660,9 +1661,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);
         }
@@ -2122,7 +2124,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);
 }
 
@@ -2890,7 +2892,7 @@ static void ide_save(QEMUFile* f, IDEState *s)
 }
 
 /* load per IDE drive data */
-static void ide_load(QEMUFile* f, IDEState *s)
+static void ide_load(QEMUFile* f, IDEState *s, int version_id)
 {
     s->mult_sectors=qemu_get_be32(f);
     s->identify_set=qemu_get_be32(f);
@@ -2914,6 +2916,13 @@ static void ide_load(QEMUFile* f, IDEState *s)
 
     qemu_get_8s(f, &s->sense_key);
     qemu_get_8s(f, &s->asc);
+    if (version_id == 3) {
+        qemu_get_8s(f, &s->cdrom_changed);
+    } else {
+        if (s->sense_key == SENSE_UNIT_ATTENTION &&
+                       s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED)
+            s->cdrom_changed = 1;
+    }
     /* XXX: if a transfer is pending, we do not save it yet */
 }
 
@@ -3235,7 +3244,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 || version_id != 3)
         return -EINVAL;
     ret = pci_device_load(&d->dev, f);
     if (ret < 0)
@@ -3265,7 +3274,7 @@ 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]);
+        ide_load(f, &d->ide_if[i], version_id);
     }
     return 0;
 }
@@ -3355,7 +3364,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);
 }
@@ -3414,7 +3423,7 @@ void pci_piix3_ide_init(PCIBus *bus, BlockDriverState **hd_table, int devfn,
         if (hd_table[i])
             hd_table[i]->private = &d->dev;
 
-    register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d);
+    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
 }
 
 /* hd_table must contain 4 block drivers */
@@ -3450,7 +3459,7 @@ void pci_piix4_ide_init(PCIBus *bus, BlockDriverState **hd_table, int devfn,
     ide_init_ioport(&d->ide_if[0], 0x1f0, 0x3f6);
     ide_init_ioport(&d->ide_if[2], 0x170, 0x376);
 
-    register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d);
+    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
 }
 
 #if defined(TARGET_PPC)
@@ -3738,7 +3747,7 @@ static int pmac_ide_load(QEMUFile *f, void *opaque, int version_id)
     uint8_t drive1_selected;
     unsigned int i;
 
-    if (version_id != 1)
+    if (version_id != 1 || version_id != 3)
         return -EINVAL;
 
     /* per IDE interface data */
@@ -3748,7 +3757,7 @@ static int pmac_ide_load(QEMUFile *f, void *opaque, int version_id)
 
     /* per IDE drive data */
     for(i = 0; i < 2; i++) {
-        ide_load(f, &s[i]);
+        ide_load(f, &s[i], version_id);
     }
     return 0;
 }
@@ -3779,7 +3788,7 @@ int pmac_ide_init (BlockDriverState **hd_table, qemu_irq irq,
 
     pmac_ide_memory = cpu_register_io_memory(pmac_ide_read,
                                              pmac_ide_write, d);
-    register_savevm("ide", 0, 1, pmac_ide_save, pmac_ide_load, d);
+    register_savevm("ide", 0, 3, pmac_ide_save, pmac_ide_load, d);
     qemu_register_reset(pmac_ide_reset, d);
     pmac_ide_reset(d);
 
@@ -4172,6 +4181,9 @@ static int md_load(QEMUFile *f, void *opaque, int version_id)
     int i;
     uint8_t drive1_selected;
 
+    if (version_id != 0 || version_id != 3)
+        return -EINVAL;
+
     qemu_get_8s(f, &s->opt);
     qemu_get_8s(f, &s->stat);
     qemu_get_8s(f, &s->pins);
@@ -4185,7 +4197,7 @@ static int md_load(QEMUFile *f, void *opaque, int version_id)
     s->ide->cur_drive = &s->ide[(drive1_selected != 0)];
 
     for (i = 0; i < 2; i ++)
-        ide_load(f, &s->ide[i]);
+        ide_load(f, &s->ide[i], version_id);
 
     return 0;
 }
@@ -4416,7 +4428,7 @@ PCMCIACardState *dscm1xxxx_init(BlockDriverState *bdrv)
     md->ide->mdata_size = METADATA_SIZE;
     md->ide->mdata_storage = (uint8_t *) qemu_mallocz(METADATA_SIZE);
 
-    register_savevm("microdrive", -1, 0, md_save, md_load, md);
+    register_savevm("microdrive", -1, 3, md_save, md_load, md);
 
     return &md->card;
 }

--
Gleb.

#1 lore raw
Quoted 96 lines · 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.

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 == NewState) {
 // Media is in the same state it was before.
 return;
}

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.

---
v1->v2:
 do not reuse media_changed. Save/restore on migration.

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

v3->v4:
 fix migration from 2 to 3 for pmac and md.

hw/ide.c +1 -0
index 6cf04a6..268e589 100644
@@ -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;
@@ -1660,9 +1661,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); }

hw/ide.c +9 -2
@@ -2122,7 +2124,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);
  }
 
@@ -2890,7 +2892,7 @@ static void ide_save(QEMUFile* f, IDEState *s)
  }
 
  /* load per IDE drive data */
-static void ide_load(QEMUFile* f, IDEState *s)
+static void ide_load(QEMUFile* f, IDEState *s, int version_id)
  {
     s->mult_sectors=qemu_get_be32(f);
     s->identify_set=qemu_get_be32(f);
@@ -2914,6 +2916,13 @@ static void ide_load(QEMUFile* f, IDEState *s)
 
     qemu_get_8s(f, &s->sense_key);
     qemu_get_8s(f, &s->asc);
+    if (version_id == 3) {
+        qemu_get_8s(f, &s->cdrom_changed);
+    } else {
+        if (s->sense_key == SENSE_UNIT_ATTENTION &&
+                       s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED)
+            s->cdrom_changed = 1;
+    }
     /* XXX: if a transfer is pending, we do not save it yet */
  }
 

hw/ide.c static int pci_ide_load(QEMUFile* f, void *opaque, int version_id) · line 3248

Quoted 6 lines
hw/ide.c +1 -1
@@ -3235,7 +3244,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 || version_id != 3)
         return -EINVAL;

Should be &&

Quoted 39 lines
hw/ide.c +4 -4
     ret = pci_device_load(&d->dev, f);
     if (ret < 0)
@@ -3265,7 +3274,7 @@ 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]);
+        ide_load(f, &d->ide_if[i], version_id);
     }
     return 0;
  }
@@ -3355,7 +3364,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);
  }
@@ -3414,7 +3423,7 @@ void pci_piix3_ide_init(PCIBus *bus, BlockDriverState **hd_table, int devfn,
         if (hd_table[i])
             hd_table[i]->private = &d->dev;
 
-    register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d);
+    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
  }
 
  /* hd_table must contain 4 block drivers */
@@ -3450,7 +3459,7 @@ void pci_piix4_ide_init(PCIBus *bus, BlockDriverState **hd_table, int devfn,
     ide_init_ioport(&d->ide_if[0], 0x1f0, 0x3f6);
     ide_init_ioport(&d->ide_if[2], 0x170, 0x376);
 
-    register_savevm("ide", 0, 2, pci_ide_save, pci_ide_load, d);
+    register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
  }
 
  #if defined(TARGET_PPC)

hw/ide.c static int pmac_ide_load(QEMUFile *f, void *opaque, int version_id) · line 3751

Quoted 6 lines
hw/ide.c +1 -1
@@ -3738,7 +3747,7 @@ static int pmac_ide_load(QEMUFile *f, void *opaque, int version_id)
     uint8_t drive1_selected;
     unsigned int i;
 
-    if (version_id != 1)
+    if (version_id != 1 || version_id != 3)
         return -EINVAL;

Should be &&

Quoted 21 lines
hw/ide.c +2 -2
     /* per IDE interface data */
@@ -3748,7 +3757,7 @@ static int pmac_ide_load(QEMUFile *f, void *opaque, int version_id)
 
     /* per IDE drive data */
     for(i = 0; i < 2; i++) {
-        ide_load(f, &s[i]);
+        ide_load(f, &s[i], version_id);
     }
     return 0;
  }
@@ -3779,7 +3788,7 @@ int pmac_ide_init (BlockDriverState **hd_table, qemu_irq irq,
 
     pmac_ide_memory = cpu_register_io_memory(pmac_ide_read,
                                              pmac_ide_write, d);
-    register_savevm("ide", 0, 1, pmac_ide_save, pmac_ide_load, d);
+    register_savevm("ide", 0, 3, pmac_ide_save, pmac_ide_load, d);
     qemu_register_reset(pmac_ide_reset, d);
     pmac_ide_reset(d);
 

hw/ide.c static int md_load(QEMUFile *f, void *opaque, int version_id) · line 4186

Quoted 6 lines
hw/ide.c +3 -0
@@ -4172,6 +4181,9 @@ static int md_load(QEMUFile *f, void *opaque, int version_id)
     int i;
     uint8_t drive1_selected;
 
+    if (version_id != 0 || version_id != 3)
+        return -EINVAL;
+

Should be &&

Quoted 22 lines
hw/ide.c +2 -3
     qemu_get_8s(f, &s->opt);
     qemu_get_8s(f, &s->stat);
     qemu_get_8s(f, &s->pins);
@@ -4185,7 +4197,7 @@ static int md_load(QEMUFile *f, void *opaque, int version_id)
     s->ide->cur_drive = &s->ide[(drive1_selected != 0)];
 
     for (i = 0; i < 2; i ++)
-        ide_load(f, &s->ide[i]);
+        ide_load(f, &s->ide[i], version_id);
 
     return 0;
  }
@@ -4416,7 +4428,7 @@ PCMCIACardState *dscm1xxxx_init(BlockDriverState *bdrv)
     md->ide->mdata_size = METADATA_SIZE;
     md->ide->mdata_storage = (uint8_t *) qemu_mallocz(METADATA_SIZE);
 
-    register_savevm("microdrive", -1, 0, md_save, md_load, md);
+    register_savevm("microdrive", -1, 3, md_save, md_load, md);
 
     return &md->card;
  }
--
Quoted 1 line

                       Gleb.

Otherwise it looks good. Thanks for taking the time to help me with the debugging.

Best regards,
Filip Navara

#2 lore raw
Quoted 10 lines · Filip Navara

hw/ide.c static int pci_ide_load(QEMUFile* f, void *opaque, int version_id) · line 3248

Quoted 7 lines
hw/ide.c +1 -1
@@ -3235,7 +3244,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 || version_id != 3)
         return -EINVAL;

Should be &&

Oh no.

--
Gleb.