From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51602) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tbs4Z-0005wY-NC for qemu-devel@nongnu.org; Fri, 23 Nov 2012 07:09:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tbs4X-0004BX-G0 for qemu-devel@nongnu.org; Fri, 23 Nov 2012 07:09:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:17217) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tbs4X-0004BI-5i for qemu-devel@nongnu.org; Fri, 23 Nov 2012 07:09:29 -0500 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qANC9SNk005935 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 23 Nov 2012 07:09:28 -0500 Message-ID: <50AF6776.3030503@redhat.com> Date: Fri, 23 Nov 2012 13:09:26 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <874219e1a97886317415a36b767920bd8eed0748.1353518053.git.phrdina@redhat.com> In-Reply-To: <874219e1a97886317415a36b767920bd8eed0748.1353518053.git.phrdina@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/1] atapi: make change media detection for guests easier List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Hrdina Cc: qemu-devel@nongnu.org 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 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? 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? 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. Kevin