[Qemu-devel] Re: [PATCH] rtl8139: IO memory is not part of vmstate

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Dec 12, 2010 at 05:23:39PM +0530, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> 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 <alex.williamson@redhat.com> 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 <alex.williamson@redhat.com>
> >> > > ---
> >> > >
> >> > >  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:-)

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.

> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > 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),