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

[Michael S. Tsirkin]

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" <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.

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