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

[Alex Williamson]

On Sun, 2010-12-12 at 20:07 +0530, Juan Quintela wrote:
> "Michael S. Tsirkin" <address@hidden> wrote:
> > On Sun, Dec 12, 2010 at 05:23:39PM +0530, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <address@hidden> 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.  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).
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