| Message ID | 1342781633-7288-8-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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",
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
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",