On Fri, 2012-11-23 at 13:09 +0100, Kevin Wolf wrote: > Am 21.11.2012 18:17, schrieb Pavel Hrdina: > > If you have a guest with a media in the optical drive and you change the > > media > > the windows guest cannot properly recognize this media change. > > > > Windows needs to detect the sense "NOT_READY with ASC_MEDIUM_NOT_PRESENT" > > before we send the sense "UNIT_ATTENTION with ASC_MEDIUM_MAY_HAVE_CHANGED". > > > > v3: remove timeout as it isn't needed anymore > > > > v2: disable debug messages > > > > Signed-off-by: Pavel Hrdina <[email protected]> > > Hope that we'll get libqos soon so that IDE can be properly tested with > qtest. CD-ROM media change is something that broke more than once. > > The change makes sense to me, it's one of these "how could this ever > work?" cases. However I do have one or two comments: > > > --- > > hw/ide/atapi.c | 16 +++++++++++----- > > hw/ide/core.c | 12 ++++++++++++ > > hw/ide/internal.h | 1 + > > 3 files changed, 24 insertions(+), 5 deletions(-) > > > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > > index 685cbaa..1534afe 100644 > > --- a/hw/ide/atapi.c > > +++ b/hw/ide/atapi.c > > @@ -1124,12 +1124,18 @@ void ide_atapi_cmd(IDEState *s) > > * GET_EVENT_STATUS_NOTIFICATION to detect such tray open/close > > * states rely on this behavior. > > */ > > - if (!s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) { > > - ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT); > > + if (!(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA) && > > + !s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) { > > + > > + if (!s->fake_cdrom_eject) { > > + ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT); > > + s->fake_cdrom_eject = 1; > > + } else { > > + ide_atapi_cmd_error(s, UNIT_ATTENTION, > > ASC_MEDIUM_MAY_HAVE_CHANGED); > > + s->fake_cdrom_eject = 0; > > + s->cdrom_changed = 0; > > + } > > > > - s->cdrom_changed = 0; > > - s->sense_key = UNIT_ATTENTION; > > - s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED; > > return; > > } > > > > diff --git a/hw/ide/core.c b/hw/ide/core.c > > index 7d6b0fa..013671a 100644 > > --- a/hw/ide/core.c > > +++ b/hw/ide/core.c > > @@ -1851,6 +1851,7 @@ static void ide_reset(IDEState *s) > > s->sense_key = 0; > > s->asc = 0; > > s->cdrom_changed = 0; > > + s->fake_cdrom_eject = 0; > > s->packet_transfer_size = 0; > > s->elementary_transfer_size = 0; > > s->io_buffer_index = 0; > > @@ -2143,6 +2144,16 @@ static int transfer_end_table_idx(EndTransferFunc > > *fn) > > return -1; > > } > > > > +static void ide_drive_pre_save(void *opaque) > > +{ > > + IDEState *s = opaque; > > + > > + if (s->cdrom_changed) { > > + s->sense_key = UNIT_ATTENTION; > > + s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED; > > + } > > +} > > If we migrate immediately after the media change, before the OS has sent > another request, we skip the ASC_MEDIUM_NOT_PRESENT step, don't we? As > far as I can tell, adding this step is the real fix, so won't media > change break during migration this way? >
We do not skip the ASC_MEDIUM_NOT_PRESENT. If we migrate immediately after the media change, then in ide_drive_pre_save we set ASC_MEDIUM_MAY_HAVE_CHANGED but on the other side in function ide_drive_post_load we check if ASC_MEDIUM_MAY_HAVE_CHANGED is set and if it is we set cdrom_changed to 1 but fake_cdrom_eject is 0. That means the ASC_MEDIUM_NOT_PRESENT will be returned before ASC_MEDIUM_MAY_HAVE_CHANGED. > The other thing is that if it's valid to set s->sense_key/asc in any > place instead of just during the start of a command (is it? I would > guess so, but I'm not sure), why don't we set ASC_MEDIUM_NOT_PRESENT > already in the change handler? > Well, we can set it in any place, but we have to call ide_atapi_cmd_error to let the guest know about this sense_key/asc only as response to command request. > Another thing I would consider is using cdrom_changed = 0/1/2 instead of > adding fake_cdrom_eject to add another state. Migration would > automatically do the right thing for it, old versions would in both 1 > and 2 state switch to ASC_MEDIUM_MAY_HAVE_CHANGED next, which is correct > in the latter case and is in the former case the same bug as the old > qemu we're migrating to has anyway. > I do it that way at first, but then I rewrite it, because I thought that using new state would be better. But if you agree with cdrom_changed = 0/1/2, I'll change it. Pavel > Kevin
