From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=55227 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PRozo-00066X-Cg for qemu-devel@nongnu.org; Sun, 12 Dec 2010 11:42:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PRozl-0003WW-C9 for qemu-devel@nongnu.org; Sun, 12 Dec 2010 11:41:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34212) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PRozl-0003WI-55 for qemu-devel@nongnu.org; Sun, 12 Dec 2010 11:41:57 -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.13.8/8.13.8) with ESMTP id oBCGftTc007549 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 12 Dec 2010 11:41:56 -0500 Date: Sun, 12 Dec 2010 18:41:33 +0200 From: "Michael S. Tsirkin" Message-ID: <20101212164133.GB4148@redhat.com> References: <20101209191623.15450.19696.stgit@s20.home> <1291932857.2926.20.camel@x201> <20101212102812.GA13033@redhat.com> <20101212120120.GA15016@redhat.com> <1292171345.2857.43.camel@x201> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1292171345.2857.43.camel@x201> Subject: [Qemu-devel] Re: [PATCH] rtl8139: IO memory is not part of vmstate List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: qemu-devel@nongnu.org, quintela@redhat.com On Sun, Dec 12, 2010 at 09:29:05AM -0700, Alex Williamson wrote: > On Sun, 2010-12-12 at 20:07 +0530, Juan Quintela wrote: > > "Michael S. Tsirkin" wrote: > > > On Sun, Dec 12, 2010 at 05:23:39PM +0530, Juan Quintela wrote: > > >> "Michael S. Tsirkin" wrote: > > >> > On Thu, Dec 09, 2010 at 03:14:17PM -0700, Alex Williamson wrote: > > > > >> > How about we keep migrating the index for the benefit of > > >> > old versions, but ignore the value on load? > > >> > Something like the following: > > >> > > >> This was my 1st suggestion to Alex O:-) > > > > > > The difference here is that instead of sending garbage to the > > > old version we send an actual index value. > > > > > >> So, I am in. he think this is bad for upstream, I don't think so (but > > >> I understand that it is oppinable). > > >> > > >> Later, Juan. > > > > > > I think it makes sense to fix this for the stable branch, > > > and I think we should try as hard as we can to avoid bumping up the > > > version number there. > > > > > > For master we can bump the version number but it might be easier to > > > just keep the code the same there. > > > > I think that your solution is better. For older versions, it works as > > expected. For new versions, problem is fixed. Solution is not the > > "purest", but you can say the same about uping the version for a state > > that is exactly the same length & fields O:-) > > I disagree, without bumping the version number, we can never guarantee > the problem is behind us. We can always migrate to the bad version, > which puts our users at risk. Well, they can also migrate between old versions, right? > The responsible behavior is to allow > forward migrations and prevent migrations to a version with an issue > known to compromise VM integrity. Perhaps I feel more strongly about > this because I actually had to debug this problem. Obvious in > retrospect, but a huge pain in the butt to get there. > > I had sent Juan a similar patch to the one Michael proposed, but with > the following change: > > + if (s->dev.qdev.hotplugged) { > + s->rtl8139_mmio_io_addr_dummy = IO_MEM_UNASSIGNED; > + } else { > + s->rtl8139_mmio_io_addr_dummy = s->rtl8139_mmio_io_addr; > + } > > With this, we at least limit the damage that the hotplugged NIC can do, > but all it takes is a reboot of the VM to touch the BARs before the > device becomes unusable (which is better than taking out the whole VM). Looks good to me. > If we need something like this for stable, I will begrudgingly agree, > but for master I think we have to bump the version. Thanks, > > Alex