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

[Filip Navara <filip.navara@gmail.com>]

On Sun, Aug 2, 2009 at 10:06 AM, Gleb Natapov<gleb@redhat.com> 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 == 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.
>
> 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.
>
> v3->v4:
>  fix 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 {
>     /* 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;

Should be &&

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

Should be &&

>
>     /* 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;
> +

Should be &&

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

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

Best regards,
Filip Navara