From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=37340 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PRkVd-0007EQ-Bn for qemu-devel@nongnu.org; Sun, 12 Dec 2010 06:54:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PRkVb-0007OS-VT for qemu-devel@nongnu.org; Sun, 12 Dec 2010 06:54:33 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42546) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PRkVb-0007Nc-MG for qemu-devel@nongnu.org; Sun, 12 Dec 2010 06:54:31 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oBCBsTNj001773 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 12 Dec 2010 06:54:29 -0500 From: Juan Quintela In-Reply-To: <20101212102812.GA13033@redhat.com> (Michael S. Tsirkin's message of "Sun, 12 Dec 2010 12:28:12 +0200") References: <20101209191623.15450.19696.stgit@s20.home> <1291932857.2926.20.camel@x201> <20101212102812.GA13033@redhat.com> Date: Sun, 12 Dec 2010 17:23:39 +0530 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: [Qemu-devel] Re: [PATCH] rtl8139: IO memory is not part of vmstate Reply-To: quintela@redhat.com List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Alex Williamson , qemu-devel@nongnu.org "Michael S. Tsirkin" wrote: > On Thu, Dec 09, 2010 at 03:14:17PM -0700, Alex Williamson wrote: >> On Thu, 2010-12-09 at 22:49 +0100, Juan Quintela wrote: >> > Alex Williamson wrote: >> > > The cpu_register_io_memory() value is unique to the VM instance and >> > > should not be restored after migration/save. Doing so means we >> > > could be pointing at arbitrary device's io regions after migration/restore. >> > > >> > > In this case, if we start a VM with a single rtl8139, hot add a 2nd, >> > > migrate the VM, then hot remove the added NIC, the 1st NIC stops >> > > working and the VM segfaults on reboot. >> > > >> > > Signed-off-by: Alex Williamson >> > > --- >> > > >> > > hw/rtl8139.c | 4 ++-- >> > > 1 files changed, 2 insertions(+), 2 deletions(-) >> > > >> > > diff --git a/hw/rtl8139.c b/hw/rtl8139.c >> > > index d92981d..9c5fc84 100644 >> > > --- a/hw/rtl8139.c >> > > +++ b/hw/rtl8139.c >> > > @@ -3186,7 +3186,7 @@ static void rtl8139_pre_save(void *opaque) >> > > >> > > static const VMStateDescription vmstate_rtl8139 = { >> > > .name = "rtl8139", >> > > - .version_id = 4, >> > > + .version_id = 5, >> > >> > No need to change version, format is still the same. >> > >> > > .minimum_version_id = 3, >> > > .minimum_version_id_old = 3, >> > > .post_load = rtl8139_post_load, >> > > @@ -3234,7 +3234,7 @@ static const VMStateDescription vmstate_rtl8139 = { >> > > >> > > VMSTATE_UNUSED(4), >> > > VMSTATE_MACADDR(conf.macaddr, RTL8139State), >> > > - VMSTATE_INT32(rtl8139_mmio_io_addr, RTL8139State), >> > > + VMSTATE_UNUSED(4), >> > >> > If we migrate from an old guest: we just ignore the value. >> > If we migrate to one old guest, we send garbage, but as you told that we >> > were already sending garbage, it looks ok, no? >> >> NAK, if we don't bump the version, we don't know if we're migration >> to/from a version 4 that includes the io address or not. We have no >> good way to debug different binaries on different systems. It seems to >> work today if we don't involve hotplug, so I think we have to maintain >> version 4 as including the io address, and let version 5 drop it. I >> tested old to new migrations, and as you expect, it does work. >> >> Alex > > 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:-) 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. > > Signed-off-by: Michael S. Tsirkin > > diff --git a/hw/rtl8139.c b/hw/rtl8139.c > index d92981d..e999dd9 100644 > --- a/hw/rtl8139.c > +++ b/hw/rtl8139.c > @@ -494,6 +494,13 @@ typedef struct RTL8139State { > QEMUTimer *timer; > int64_t TimerExpire; > > + /* Hack for migration compatibility: older > + * versions of rtl8139 put mmio index in save image, > + * and override the index that we have with that one. > + * This does not make any sense but happens to > + * work sometimes, if we are lucky and index matches. > + * On load, we can simply ignore this value. */ > + int compat_rtl8139_mmio_io_addr; > } RTL8139State; > > static void rtl8139_set_next_tctr_time(RTL8139State *s, int64_t current_time); > @@ -3182,6 +3189,7 @@ static void rtl8139_pre_save(void *opaque) > rtl8139_set_next_tctr_time(s, current_time); > s->TCTR = muldiv64(current_time - s->TCTR_base, PCI_FREQUENCY, > get_ticks_per_sec()); > + s->compat_rtl8139_mmio_io_addr = s->rtl8139_mmio_io_addr; > } > > static const VMStateDescription vmstate_rtl8139 = { > @@ -3234,7 +3242,7 @@ static const VMStateDescription vmstate_rtl8139 = { > > VMSTATE_UNUSED(4), > VMSTATE_MACADDR(conf.macaddr, RTL8139State), > - VMSTATE_INT32(rtl8139_mmio_io_addr, RTL8139State), > + VMSTATE_INT32(compat_rtl8139_mmio_io_addr, RTL8139State), > > VMSTATE_UINT32(currTxDesc, RTL8139State), > VMSTATE_UINT32(currCPlusRxDesc, RTL8139State),