From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49037) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tcz9t-0002kx-3a for qemu-devel@nongnu.org; Mon, 26 Nov 2012 08:55:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tcz9j-0001M2-Fo for qemu-devel@nongnu.org; Mon, 26 Nov 2012 08:55:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:23436) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tcz9j-0001Lx-7k for qemu-devel@nongnu.org; Mon, 26 Nov 2012 08:55:27 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qAQDtQ38005382 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 26 Nov 2012 08:55:26 -0500 Message-ID: <50B374CD.3000404@redhat.com> Date: Mon, 26 Nov 2012 14:55:25 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <874219e1a97886317415a36b767920bd8eed0748.1353518053.git.phrdina@redhat.com> <50AF6776.3030503@redhat.com> <1353933816.3370.13.camel@antique-laptop> In-Reply-To: <1353933816.3370.13.camel@antique-laptop> Content-Type: text/plain; charset=UTF-8 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 26.11.2012 13:43, schrieb Pavel Hrdina: > On Fri, 2012-11-23 at 13:09 +0100, Kevin Wolf wrote: >> Am 21.11.2012 18:17, schrieb Pavel Hrdina: >>> 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. Hm, yes, I missed the existing ide_drive_post_load() implementation. However, this is what the code looks like: if (version_id < 3) { if (s->sense_key == UNIT_ATTENTION && s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) { s->cdrom_changed = 1; } } version_id for current qemu version is 3, so the intended magic doesn't happen. >> 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. Considering that the goal isn't really to set sense_key/asc so that the guest can read it, but just so s->cdrom_changed is right on the destination, I agree that it makes sense as it is. >> 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. I think it's nicer to have only one state. And if I'm not mistaken, we can even do without the pre_save/post_load handlers then. Kevin