[Qemu-devel] [PATCH 0/2] Fix rtl8139 migration with hotplug

[Qemu-devel] [PATCH 0/2] Fix rtl8139 migration with hotplug

[Alex Williamson]

Ok, I think this might actually make everyone happy, but I've been
known to be wrong about that many times before.  Juan challenged me
to find an rtl8139 migration scenario that fails when hotplug is
not involved (and not switch device creation order since that's a
usage bug).  I couldn't come up with one.  We had been arguing that
a subsection didn't make sense for the change to rtl8139 vmstate
because the needed function would be {return 1}. but what if we
could detect if the VM had done any other hotplugs and only include
the subsection in those cases.  That's what this short series does.

So, I hope Juan is happy because this preserves the migration ABI
for the majority of the use cases, and I hope Michael is happy
because it does so using a subsection.  Thanks,

Alex

---

Alex Williamson (2):
      rtl8139: Use subsection to restrict migration after hotplug
      qdev: Track runtime machine modifications


 hw/qdev.c    |   10 ++++++++++
 hw/qdev.h    |    1 +
 hw/rtl8139.c |   28 +++++++++++++++++++++++++++-
 3 files changed, 38 insertions(+), 1 deletions(-)

[Qemu-devel] [PATCH 1/2] qdev: Track runtime machine modifications

[Alex Williamson]

Create a trivial interface to track whether the machine has been
modified since boot.  Adding or removing devices will trigger this
to return true.  An example usage scenario for such an interface is
the rtl8139 driver which includes a cpu_register_io_memory() value
in it's migration stream.  For the majority of migrations, where
no hotplug has occured in the machine, this works correctly.  Once
the machine is modified, we can use this interface to detect that
and include a subsection for the device to prevent migrations to
rtl8139 versions with this bug.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/qdev.c |   10 ++++++++++
 hw/qdev.h |    1 +
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 35858cb..e6e7a57 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -32,6 +32,8 @@
 #include "blockdev.h"
 
 static int qdev_hotplug = 0;
+static bool qdev_hot_added = false;
+static bool qdev_hot_removed = false;
 
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 static BusState *main_system_bus;
@@ -93,6 +95,7 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
     if (qdev_hotplug) {
         assert(bus->allow_hotplug);
         dev->hotplugged = 1;
+        qdev_hot_added = true;
     }
     dev->instance_id_alias = -1;
     dev->state = DEV_STATE_CREATED;
@@ -305,6 +308,8 @@ int qdev_unplug(DeviceState *dev)
     }
     assert(dev->info->unplug != NULL);
 
+    qdev_hot_removed = true;
+
     return dev->info->unplug(dev);
 }
 
@@ -370,6 +375,11 @@ void qdev_machine_creation_done(void)
     qdev_hotplug = 1;
 }
 
+bool qdev_machine_modified(void)
+{
+    return qdev_hot_added || qdev_hot_removed;
+}
+
 /* Get a character (serial) device interface.  */
 CharDriverState *qdev_init_chardev(DeviceState *dev)
 {
diff --git a/hw/qdev.h b/hw/qdev.h
index 579328a..66b8aee 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -123,6 +123,7 @@ int qdev_unplug(DeviceState *dev);
 void qdev_free(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
 void qdev_machine_creation_done(void);
+bool qdev_machine_modified(void);
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
 void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);

[Qemu-devel] [PATCH 2/2] rtl8139: Use subsection to restrict migration after hotplug

[Alex Williamson]

rtl8139 includes a cpu_register_io_memory acquired value in it's
migration data.  This is not only unecessary, but we should treat
these values as unique to the VM instances since the value depends
on call order.  In most cases, this miraculously still works.
However, if devices are added or removed from the system, it may
represent an ordering change, which could cause the target rtl8139
device to make use of another device's cpu_register_io_memory value.
If we detect that a hot-add/remove has occured, include a subsection
to restrict migrations only to driver versions known to include this
fix.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/rtl8139.c |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index d92981d..4decadb 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -494,6 +494,8 @@ typedef struct RTL8139State {
     QEMUTimer *timer;
     int64_t TimerExpire;
 
+    /* Support migration to/from old versions */
+    int rtl8139_mmio_io_addr_dummy;
 } RTL8139State;
 
 static void rtl8139_set_next_tctr_time(RTL8139State *s, int64_t current_time);
@@ -3173,6 +3175,21 @@ static int rtl8139_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool rtl8139_hotplug_ready_needed(void *opaque)
+{
+    return qdev_machine_modified();
+}
+
+static const VMStateDescription vmstate_rtl8139_hotplug_ready ={
+    .name = "rtl8139/hotplug_ready",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void rtl8139_pre_save(void *opaque)
 {
     RTL8139State* s = opaque;
@@ -3182,6 +3199,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->rtl8139_mmio_io_addr_dummy = s->rtl8139_mmio_io_addr;
 }
 
 static const VMStateDescription vmstate_rtl8139 = {
@@ -3234,7 +3252,7 @@ static const VMStateDescription vmstate_rtl8139 = {
 
         VMSTATE_UNUSED(4),
         VMSTATE_MACADDR(conf.macaddr, RTL8139State),
-        VMSTATE_INT32(rtl8139_mmio_io_addr, RTL8139State),
+        VMSTATE_INT32(rtl8139_mmio_io_addr_dummy, RTL8139State),
 
         VMSTATE_UINT32(currTxDesc, RTL8139State),
         VMSTATE_UINT32(currCPlusRxDesc, RTL8139State),
@@ -3263,6 +3281,14 @@ static const VMStateDescription vmstate_rtl8139 = {
 
         VMSTATE_UINT32_V(cplus_enabled, RTL8139State, 4),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_rtl8139_hotplug_ready,
+            .needed = rtl8139_hotplug_ready_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 

[Qemu-devel] Re: [PATCH 0/2] Fix rtl8139 migration with hotplug

[Michael S. Tsirkin]

On Thu, Dec 16, 2010 at 11:01:49AM -0700, Alex Williamson wrote:
> Ok, I think this might actually make everyone happy, but I've been
> known to be wrong about that many times before.  Juan challenged me
> to find an rtl8139 migration scenario that fails when hotplug is
> not involved (and not switch device creation order since that's a
> usage bug).  I couldn't come up with one.  We had been arguing that
> a subsection didn't make sense for the change to rtl8139 vmstate
> because the needed function would be {return 1}. but what if we
> could detect if the VM had done any other hotplugs and only include
> the subsection in those cases.  That's what this short series does.
> 
> So, I hope Juan is happy because this preserves the migration ABI
> for the majority of the use cases, and I hope Michael is happy
> because it does so using a subsection.  Thanks,
> 
> Alex

I think it's a clever hack.

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
> 
> Alex Williamson (2):
>       rtl8139: Use subsection to restrict migration after hotplug
>       qdev: Track runtime machine modifications
> 
> 
>  hw/qdev.c    |   10 ++++++++++
>  hw/qdev.h    |    1 +
>  hw/rtl8139.c |   28 +++++++++++++++++++++++++++-
>  3 files changed, 38 insertions(+), 1 deletions(-)

[Qemu-devel] Re: [PATCH 0/2] Fix rtl8139 migration with hotplug

[Juan Quintela]

Alex Williamson <alex.williamson@redhat.com> wrote:
> Ok, I think this might actually make everyone happy, but I've been
> known to be wrong about that many times before.  Juan challenged me
> to find an rtl8139 migration scenario that fails when hotplug is
> not involved (and not switch device creation order since that's a
> usage bug).  I couldn't come up with one.  We had been arguing that
> a subsection didn't make sense for the change to rtl8139 vmstate
> because the needed function would be {return 1}. but what if we
> could detect if the VM had done any other hotplugs and only include
> the subsection in those cases.  That's what this short series does.
>
> So, I hope Juan is happy because this preserves the migration ABI
> for the majority of the use cases, and I hope Michael is happy
> because it does so using a subsection.  Thanks,
>
> Alex

I am happy.
Thanks a lot.

Acked-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [RESEND PATCH 0/2] Fix rtl8139 migration with hotplug

[Alex Williamson]

rtl8139 includes a cpu_register_io_memory() in it's vmstate.  This
is problematic if the guest undergoes hotplug operations, which can
shift io indexes between migration source and target.  Add an
interface to detect this condition and add a subsection, when
necessary, to ensure we migrate to a safe target.

This resolves Juan's concerns about maintaining the migration ABI
and Michael's concerns wrt subsections.  Acked by both.  Please apply.
Thanks,

Alex

---

Alex Williamson (2):
      rtl8139: Use subsection to restrict migration after hotplug
      qdev: Track runtime machine modifications


 hw/qdev.c    |   10 ++++++++++
 hw/qdev.h    |    1 +
 hw/rtl8139.c |   28 +++++++++++++++++++++++++++-
 3 files changed, 38 insertions(+), 1 deletions(-)

[Qemu-devel] [RESEND PATCH 1/2] qdev: Track runtime machine modifications

[Alex Williamson]

Create a trivial interface to track whether the machine has been
modified since boot.  Adding or removing devices will trigger this
to return true.  An example usage scenario for such an interface is
the rtl8139 driver which includes a cpu_register_io_memory() value
in it's migration stream.  For the majority of migrations, where
no hotplug has occured in the machine, this works correctly.  Once
the machine is modified, we can use this interface to detect that
and include a subsection for the device to prevent migrations to
rtl8139 versions with this bug.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Juan Quintela <quintela@redhat.com>
---

 hw/qdev.c |   10 ++++++++++
 hw/qdev.h |    1 +
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 6fc9b02..e450c21 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -32,6 +32,8 @@
 #include "blockdev.h"
 
 static int qdev_hotplug = 0;
+static bool qdev_hot_added = false;
+static bool qdev_hot_removed = false;
 
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 static BusState *main_system_bus;
@@ -93,6 +95,7 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
     if (qdev_hotplug) {
         assert(bus->allow_hotplug);
         dev->hotplugged = 1;
+        qdev_hot_added = true;
     }
     dev->instance_id_alias = -1;
     dev->state = DEV_STATE_CREATED;
@@ -294,6 +297,8 @@ int qdev_unplug(DeviceState *dev)
     }
     assert(dev->info->unplug != NULL);
 
+    qdev_hot_removed = true;
+
     return dev->info->unplug(dev);
 }
 
@@ -394,6 +399,11 @@ void qdev_machine_creation_done(void)
     qdev_hotplug = 1;
 }
 
+bool qdev_machine_modified(void)
+{
+    return qdev_hot_added || qdev_hot_removed;
+}
+
 /* Get a character (serial) device interface.  */
 CharDriverState *qdev_init_chardev(DeviceState *dev)
 {
diff --git a/hw/qdev.h b/hw/qdev.h
index aaaf55a..273bbac 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -132,6 +132,7 @@ int qdev_unplug(DeviceState *dev);
 void qdev_free(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
 void qdev_machine_creation_done(void);
+bool qdev_machine_modified(void);
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
 void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);

Re: [Qemu-devel] [RESEND PATCH 1/2] qdev: Track runtime machine modifications

[Markus Armbruster]

Alex Williamson <alex.williamson@redhat.com> writes:

> Create a trivial interface to track whether the machine has been
> modified since boot.  Adding or removing devices will trigger this
> to return true.  An example usage scenario for such an interface is
> the rtl8139 driver which includes a cpu_register_io_memory() value
> in it's migration stream.  For the majority of migrations, where
> no hotplug has occured in the machine, this works correctly.  Once
> the machine is modified, we can use this interface to detect that
> and include a subsection for the device to prevent migrations to
> rtl8139 versions with this bug.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Juan Quintela <quintela@redhat.com>
> ---
>
>  hw/qdev.c |   10 ++++++++++
>  hw/qdev.h |    1 +
>  2 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 6fc9b02..e450c21 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -32,6 +32,8 @@
>  #include "blockdev.h"
>  
>  static int qdev_hotplug = 0;
> +static bool qdev_hot_added = false;
> +static bool qdev_hot_removed = false;
>  
>  /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
>  static BusState *main_system_bus;
> @@ -93,6 +95,7 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
>      if (qdev_hotplug) {
>          assert(bus->allow_hotplug);
>          dev->hotplugged = 1;
> +        qdev_hot_added = true;
>      }
>      dev->instance_id_alias = -1;
>      dev->state = DEV_STATE_CREATED;
> @@ -294,6 +297,8 @@ int qdev_unplug(DeviceState *dev)
>      }
>      assert(dev->info->unplug != NULL);
>  
> +    qdev_hot_removed = true;
> +
>      return dev->info->unplug(dev);
>  }
>  
> @@ -394,6 +399,11 @@ void qdev_machine_creation_done(void)
>      qdev_hotplug = 1;
>  }
>  
> +bool qdev_machine_modified(void)
> +{
> +    return qdev_hot_added || qdev_hot_removed;
> +}
> +
>  /* Get a character (serial) device interface.  */
>  CharDriverState *qdev_init_chardev(DeviceState *dev)
>  {

Why do you track add/remove separately, in qdev_hot_added and
qdev_hot_removed?  I don't mind, just curious.

[...]

Re: [Qemu-devel] [RESEND PATCH 1/2] qdev: Track runtime machine modifications

[Alex Williamson]

On Wed, 2011-01-12 at 13:09 +0100, Markus Armbruster wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > Create a trivial interface to track whether the machine has been
> > modified since boot.  Adding or removing devices will trigger this
> > to return true.  An example usage scenario for such an interface is
> > the rtl8139 driver which includes a cpu_register_io_memory() value
> > in it's migration stream.  For the majority of migrations, where
> > no hotplug has occured in the machine, this works correctly.  Once
> > the machine is modified, we can use this interface to detect that
> > and include a subsection for the device to prevent migrations to
> > rtl8139 versions with this bug.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > Acked-by: Juan Quintela <quintela@redhat.com>
> > ---
> >
> >  hw/qdev.c |   10 ++++++++++
> >  hw/qdev.h |    1 +
> >  2 files changed, 11 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index 6fc9b02..e450c21 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -32,6 +32,8 @@
> >  #include "blockdev.h"
> >  
> >  static int qdev_hotplug = 0;
> > +static bool qdev_hot_added = false;
> > +static bool qdev_hot_removed = false;
> >  
> >  /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
> >  static BusState *main_system_bus;
> > @@ -93,6 +95,7 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
> >      if (qdev_hotplug) {
> >          assert(bus->allow_hotplug);
> >          dev->hotplugged = 1;
> > +        qdev_hot_added = true;
> >      }
> >      dev->instance_id_alias = -1;
> >      dev->state = DEV_STATE_CREATED;
> > @@ -294,6 +297,8 @@ int qdev_unplug(DeviceState *dev)
> >      }
> >      assert(dev->info->unplug != NULL);
> >  
> > +    qdev_hot_removed = true;
> > +
> >      return dev->info->unplug(dev);
> >  }
> >  
> > @@ -394,6 +399,11 @@ void qdev_machine_creation_done(void)
> >      qdev_hotplug = 1;
> >  }
> >  
> > +bool qdev_machine_modified(void)
> > +{
> > +    return qdev_hot_added || qdev_hot_removed;
> > +}
> > +
> >  /* Get a character (serial) device interface.  */
> >  CharDriverState *qdev_init_chardev(DeviceState *dev)
> >  {
> 
> Why do you track add/remove separately, in qdev_hot_added and
> qdev_hot_removed?  I don't mind, just curious.

I thought the distinction might be useful at some point, but we don't
use it yet.  Thanks,

Alex

[Qemu-devel] [RESEND PATCH 2/2] rtl8139: Use subsection to restrict migration after hotplug

[Alex Williamson]

rtl8139 includes a cpu_register_io_memory acquired value in it's
migration data.  This is not only unecessary, but we should treat
these values as unique to the VM instances since the value depends
on call order.  In most cases, this miraculously still works.
However, if devices are added or removed from the system, it may
represent an ordering change, which could cause the target rtl8139
device to make use of another device's cpu_register_io_memory value.
If we detect that a hot-add/remove has occured, include a subsection
to restrict migrations only to driver versions known to include this
fix.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Juan Quintela <quintela@redhat.com>
---

 hw/rtl8139.c |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index a8aed89..a22530c 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -495,6 +495,8 @@ typedef struct RTL8139State {
     QEMUTimer *timer;
     int64_t TimerExpire;
 
+    /* Support migration to/from old versions */
+    int rtl8139_mmio_io_addr_dummy;
 } RTL8139State;
 
 static void rtl8139_set_next_tctr_time(RTL8139State *s, int64_t current_time);
@@ -3162,6 +3164,21 @@ static int rtl8139_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool rtl8139_hotplug_ready_needed(void *opaque)
+{
+    return qdev_machine_modified();
+}
+
+static const VMStateDescription vmstate_rtl8139_hotplug_ready ={
+    .name = "rtl8139/hotplug_ready",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void rtl8139_pre_save(void *opaque)
 {
     RTL8139State* s = opaque;
@@ -3171,6 +3188,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->rtl8139_mmio_io_addr_dummy = s->rtl8139_mmio_io_addr;
 }
 
 static const VMStateDescription vmstate_rtl8139 = {
@@ -3223,7 +3241,7 @@ static const VMStateDescription vmstate_rtl8139 = {
 
         VMSTATE_UNUSED(4),
         VMSTATE_MACADDR(conf.macaddr, RTL8139State),
-        VMSTATE_INT32(rtl8139_mmio_io_addr, RTL8139State),
+        VMSTATE_INT32(rtl8139_mmio_io_addr_dummy, RTL8139State),
 
         VMSTATE_UINT32(currTxDesc, RTL8139State),
         VMSTATE_UINT32(currCPlusRxDesc, RTL8139State),
@@ -3252,6 +3270,14 @@ static const VMStateDescription vmstate_rtl8139 = {
 
         VMSTATE_UINT32_V(cplus_enabled, RTL8139State, 4),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_rtl8139_hotplug_ready,
+            .needed = rtl8139_hotplug_ready_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 

[Qemu-devel] Re: [RESEND PATCH 0/2] Fix rtl8139 migration with hotplug

[Michael S. Tsirkin]

On Tue, Jan 04, 2011 at 12:37:37PM -0700, Alex Williamson wrote:
> rtl8139 includes a cpu_register_io_memory() in it's vmstate.  This
> is problematic if the guest undergoes hotplug operations, which can
> shift io indexes between migration source and target.  Add an
> interface to detect this condition and add a subsection, when
> necessary, to ensure we migrate to a safe target.
> 
> This resolves Juan's concerns about maintaining the migration ABI
> and Michael's concerns wrt subsections.  Acked by both.  Please apply.
> Thanks,
> 
> Alex

Applied, thanks!

> ---
> 
> Alex Williamson (2):
>       rtl8139: Use subsection to restrict migration after hotplug
>       qdev: Track runtime machine modifications
> 
> 
>  hw/qdev.c    |   10 ++++++++++
>  hw/qdev.h    |    1 +
>  hw/rtl8139.c |   28 +++++++++++++++++++++++++++-
>  3 files changed, 38 insertions(+), 1 deletions(-)