Patchwork [7/7] RTC: Allow to migrate from old QEMU

login
register
mail settings
Submitter Paolo Bonzini
Date July 20, 2012, 10:53 a.m.
Message ID <1342781633-7288-8-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/172202/
State New
Headers show

Comments

Paolo Bonzini - July 20, 2012, 10:53 a.m.
From: "Zhang, Yang Z" <yang.z.zhang@intel.com>

The new logic is compatible with old, and it should not block migration
from old QEMU.  However, the new version cannot migrate to the old one.

Cc: Juan Quintela <quintela@redhat.com>
Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mc146818rtc.c |   42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)
Michael Roth - July 20, 2012, 5:38 p.m.
On Fri, Jul 20, 2012 at 12:53:53PM +0200, Paolo Bonzini wrote:
> From: "Zhang, Yang Z" <yang.z.zhang@intel.com>
> 
> The new logic is compatible with old, and it should not block migration
> from old QEMU.  However, the new version cannot migrate to the old one.

Do we not plan to support 1.2 -> 1.1 with -M pc-1.1? I'm not sure what
the "official" stance is, but I think we could do it by making the behavior
configurable and defaulting to the old behavior for -M pc-1.1, and using
VMSTATE_*_TEST(..., rtc_on_demand_enabled) to send the new fields
conditionally.

> 
> Cc: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/mc146818rtc.c |   42 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index f0b75cb..d3871d8 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -697,11 +697,47 @@ static int rtc_post_load(void *opaque, int version_id)
>      return 0;
>  }
> 
> +static int rtc_load_old(QEMUFile *f, void *opaque, int version_id)
> +{
> +    RTCState *s = opaque;
> +    uint8_t buf[77];
> +
> +    if (version_id > 2) {
> +        return -EINVAL;
> +    }
> +
> +    qemu_get_buffer(f, s->cmos_data, sizeof(s->cmos_data));
> +    qemu_get_8s(f, &s->cmos_index);
> +
> +    /* Skip loading of s->current_tm, we already have the
> +     * information in cmos_data.
> +     */
> +    qemu_get_buffer(f, buf, 7*4);
> +
> +    qemu_get_timer(f, s->periodic_timer);
> +    s->next_periodic_time = qemu_get_be64(f);
> +
> +    /* Skip loading of next_second_time, second_timer, second_timer2.  */
> +    qemu_get_buffer(f, buf, 3*8);
> +
> +    rtc_set_time(s);
> +    s->offset = 0;
> +    check_update_timer(s);
> +
> +    if (version_id >= 2) {
> +        s->irq_coalesced = qemu_get_be32(f);
> +        s->period = qemu_get_be32(f);
> +    }
> +
> +    return rtc_post_load(opaque, version_id);
> +}
> +
>  static const VMStateDescription vmstate_rtc = {
>      .name = "mc146818rtc",
> -    .version_id = 2,
> -    .minimum_version_id = 1,
> +    .version_id = 3,
> +    .minimum_version_id = 3,
>      .minimum_version_id_old = 1,
> +    .load_state_old = rtc_load_old,
>      .post_load = rtc_post_load,
>      .fields      = (VMStateField []) {
>          VMSTATE_BUFFER(cmos_data, RTCState),
> @@ -837,7 +873,7 @@ static int rtc_initfn(ISADevice *dev)
>      memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
>      isa_register_ioport(dev, &s->io, base);
> 
> -    qdev_set_legacy_instance_id(&dev->qdev, base, 2);
> +    qdev_set_legacy_instance_id(&dev->qdev, base, 3);
>      qemu_register_reset(rtc_reset, s);
> 
>      object_property_add(OBJECT(s), "date", "struct tm",
> -- 
> 1.7.10.2
> 
>
Paolo Bonzini - July 20, 2012, 7:02 p.m.
Il 20/07/2012 19:38, Michael Roth ha scritto:
>> > The new logic is compatible with old, and it should not block migration
>> > from old QEMU.  However, the new version cannot migrate to the old one.
> Do we not plan to support 1.2 -> 1.1 with -M pc-1.1?

No, we don't.  That's why in general subsections are preferred to
version bumps, as long as the subsection is sent rarely (to fix bugs) or
only for features that are never enabled in the old version.  Actually I
think all of our subsections are of the former kind.

Paolo
Juan Quintela - July 23, 2012, 5:12 a.m.
Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: "Zhang, Yang Z" <yang.z.zhang@intel.com>
>
> The new logic is compatible with old, and it should not block migration
> from old QEMU.  However, the new version cannot migrate to the old one.
>
> Cc: Juan Quintela <quintela@redhat.com>

I guess that you removed the tm structs.

This are the current fields.

    .fields      = (VMStateField []) {
        VMSTATE_BUFFER(cmos_data, RTCState),
        VMSTATE_UINT8(cmos_index, RTCState),
        VMSTATE_INT32(current_tm.tm_sec, RTCState),
        VMSTATE_INT32(current_tm.tm_min, RTCState),
        VMSTATE_INT32(current_tm.tm_hour, RTCState),
        VMSTATE_INT32(current_tm.tm_wday, RTCState),
        VMSTATE_INT32(current_tm.tm_mday, RTCState),
        VMSTATE_INT32(current_tm.tm_mon, RTCState),
        VMSTATE_INT32(current_tm.tm_year, RTCState),

you can change this to:

        VMSTATE_UNUSED(7*4);

Some for the others.

        VMSTATE_TIMER(periodic_timer, RTCState),
        VMSTATE_INT64(next_periodic_time, RTCState),
        VMSTATE_INT64(next_second_time, RTCState),
        VMSTATE_TIMER(second_timer, RTCState),
        VMSTATE_TIMER(second_timer2, RTCState),
        VMSTATE_UINT32_V(irq_coalesced, RTCState, 2),
        VMSTATE_UINT32_V(period, RTCState, 2),
        VMSTATE_END_OF_LIST()
    }


>  static const VMStateDescription vmstate_rtc = {
>      .name = "mc146818rtc",
> -    .version_id = 2,
> -    .minimum_version_id = 1,
> +    .version_id = 3,
> +    .minimum_version_id = 3,
>      .minimum_version_id_old = 1,
> +    .load_state_old = rtc_load_old,
>      .post_load = rtc_post_load,
>      .fields      = (VMStateField []) {
>          VMSTATE_BUFFER(cmos_data, RTCState),
> @@ -837,7 +873,7 @@ static int rtc_initfn(ISADevice *dev)

no changes in the other part of vmstate?  that is at least strange?  I
guess they are on the other patches on the series.  Will take a look.


>      memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
>      isa_register_ioport(dev, &s->io, base);
>  
> -    qdev_set_legacy_instance_id(&dev->qdev, base, 2);
> +    qdev_set_legacy_instance_id(&dev->qdev, base, 3);
>      qemu_register_reset(rtc_reset, s);
>  
>      object_property_add(OBJECT(s), "date", "struct tm",
Paolo Bonzini - July 23, 2012, 7:30 a.m.
Il 23/07/2012 07:12, Juan Quintela ha scritto:
>     .fields      = (VMStateField []) {
>         VMSTATE_BUFFER(cmos_data, RTCState),
>         VMSTATE_UINT8(cmos_index, RTCState),
>         VMSTATE_INT32(current_tm.tm_sec, RTCState),
>         VMSTATE_INT32(current_tm.tm_min, RTCState),
>         VMSTATE_INT32(current_tm.tm_hour, RTCState),
>         VMSTATE_INT32(current_tm.tm_wday, RTCState),
>         VMSTATE_INT32(current_tm.tm_mday, RTCState),
>         VMSTATE_INT32(current_tm.tm_mon, RTCState),
>         VMSTATE_INT32(current_tm.tm_year, RTCState),
> 
> you can change this to:
> 
>         VMSTATE_UNUSED(7*4);

I think this is not safe.  In the load_old case, we ignore the struct tm
because we call rtc_set_time and check_update_timer.  We need this to
make up some values of base_rtc, last_update and offset.

In the normal case we need both the CMOS data and the timer data.
Though perhaps I can make a further change and remove current_tm
altogether.  I'll take a look this week; however that will make it even
harder to produce the old version.

Paolo

Patch

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index f0b75cb..d3871d8 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -697,11 +697,47 @@  static int rtc_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static int rtc_load_old(QEMUFile *f, void *opaque, int version_id)
+{
+    RTCState *s = opaque;
+    uint8_t buf[77];
+
+    if (version_id > 2) {
+        return -EINVAL;
+    }
+
+    qemu_get_buffer(f, s->cmos_data, sizeof(s->cmos_data));
+    qemu_get_8s(f, &s->cmos_index);
+
+    /* Skip loading of s->current_tm, we already have the
+     * information in cmos_data.
+     */
+    qemu_get_buffer(f, buf, 7*4);
+
+    qemu_get_timer(f, s->periodic_timer);
+    s->next_periodic_time = qemu_get_be64(f);
+
+    /* Skip loading of next_second_time, second_timer, second_timer2.  */
+    qemu_get_buffer(f, buf, 3*8);
+
+    rtc_set_time(s);
+    s->offset = 0;
+    check_update_timer(s);
+
+    if (version_id >= 2) {
+        s->irq_coalesced = qemu_get_be32(f);
+        s->period = qemu_get_be32(f);
+    }
+
+    return rtc_post_load(opaque, version_id);
+}
+
 static const VMStateDescription vmstate_rtc = {
     .name = "mc146818rtc",
-    .version_id = 2,
-    .minimum_version_id = 1,
+    .version_id = 3,
+    .minimum_version_id = 3,
     .minimum_version_id_old = 1,
+    .load_state_old = rtc_load_old,
     .post_load = rtc_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_BUFFER(cmos_data, RTCState),
@@ -837,7 +873,7 @@  static int rtc_initfn(ISADevice *dev)
     memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
     isa_register_ioport(dev, &s->io, base);
 
-    qdev_set_legacy_instance_id(&dev->qdev, base, 2);
+    qdev_set_legacy_instance_id(&dev->qdev, base, 3);
     qemu_register_reset(rtc_reset, s);
 
     object_property_add(OBJECT(s), "date", "struct tm",