* Peter Maydell (peter.mayd...@linaro.org) wrote:
> The PL031 RTC tracks the difference between the guest RTC
> and the host RTC using a tick_offset field. For migration,
> however, we currently always migrate the offset between
> the guest and the vm_clock, even if the RTC clock is not
> the same as the vm_clock; this was an attempt to retain
> migration backwards compatibility.
>
> Unfortunately this results in the RTC behaving oddly across
> a VM state save and restore -- since the VM clock stands still
> across save-then-restore, regardless of how much real world
> time has elapsed, the guest RTC ends up out of sync with the
> host RTC in the restored VM.
>
> Fix this by migrating the raw tick_offset. To retain migration
> compatibility as far as possible, we have a new property
> migrate-tick-offset; by default this is 'true' and we will
> migrate the true tick offset in a new subsection; if the
> incoming data has no subsection we fall back to the old
> vm_clock-based offset information, so old->new migration
> compatibility is preserved. For complete new->old migration
> compatibility, the property is set to 'false' for 4.0 and
> earlier machine types (this will only affect 'virt-4.0'
> and below, as none of the other pl031-using machines are
> versioned).
>
> Reported-by: Russell King <r...@armlinux.org.uk>
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
Yes, I think so;
Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> ---
> I think this is correct, and it makes the "rtc clock should
> not stay still across a save/reload" work, but I feel like
> there might be some subtlety I've missed here. Review
> definitely needed...
>
> include/hw/timer/pl031.h | 2 +
> hw/core/machine.c | 1 +
> hw/timer/pl031.c | 92 ++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 91 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/timer/pl031.h b/include/hw/timer/pl031.h
> index 8857c24ca5d..8c3f555ee28 100644
> --- a/include/hw/timer/pl031.h
> +++ b/include/hw/timer/pl031.h
> @@ -33,6 +33,8 @@ typedef struct PL031State {
> */
> uint32_t tick_offset_vmstate;
> uint32_t tick_offset;
> + bool tick_offset_migrated;
> + bool migrate_tick_offset;
>
> uint32_t mr;
> uint32_t lr;
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2be19ec0cd5..37a1111da1d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -34,6 +34,7 @@ GlobalProperty hw_compat_4_0[] = {
> { "virtio-vga", "edid", "false" },
> { "virtio-gpu-pci", "edid", "false" },
> { "virtio-device", "use-started", "false" },
> + { "pl031", "migrate-tick-offset", "false" },
> };
> const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
>
> diff --git a/hw/timer/pl031.c b/hw/timer/pl031.c
> index 3378084f4a8..1a7e2ee06b3 100644
> --- a/hw/timer/pl031.c
> +++ b/hw/timer/pl031.c
> @@ -199,29 +199,94 @@ static int pl031_pre_save(void *opaque)
> {
> PL031State *s = opaque;
>
> - /* tick_offset is base_time - rtc_clock base time. Instead, we want to
> - * store the base time relative to the QEMU_CLOCK_VIRTUAL for
> backwards-compatibility. */
> + /*
> + * The PL031 device model code uses the tick_offset field, which is
> + * the offset between what the guest RTC should read and what the
> + * QEMU rtc_clock reads:
> + * guest_rtc = rtc_clock + tick_offset
> + * and so
> + * tick_offset = guest_rtc - rtc_clock
> + *
> + * We want to migrate this offset, which sounds straightforward.
> + * Unfortunately older versions of QEMU migrated a conversion of this
> + * offset into an offset from the vm_clock. (This was in turn an
> + * attempt to be compatible with even older QEMU versions, but it
> + * has incorrect behaviour if the rtc_clock is not the same as the
> + * vm_clock.) So we put the actual tick_offset into a migration
> + * subsection, and the backwards-compatible time-relative-to-vm_clock
> + * in the main migration state.
> + *
> + * Calculate base time relative to QEMU_CLOCK_VIRTUAL:
> + */
> int64_t delta = qemu_clock_get_ns(rtc_clock) -
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> s->tick_offset_vmstate = s->tick_offset + delta / NANOSECONDS_PER_SECOND;
>
> return 0;
> }
>
> +static int pl031_pre_load(void *opaque)
> +{
> + PL031State *s = opaque;
> +
> + s->tick_offset_migrated = false;
> + return 0;
> +}
> +
> static int pl031_post_load(void *opaque, int version_id)
> {
> PL031State *s = opaque;
>
> - int64_t delta = qemu_clock_get_ns(rtc_clock) -
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> - s->tick_offset = s->tick_offset_vmstate - delta / NANOSECONDS_PER_SECOND;
> + /*
> + * If we got the tick_offset subsection, then we can just use
> + * the value in that. Otherwise the source is an older QEMU and
> + * has given us the offset from the vm_clock; convert it back to
> + * an offset from the rtc_clock. This will cause time to incorrectly
> + * go backwards compared to the host RTC, but this is unavoidable.
> + */
> +
> + if (!s->tick_offset_migrated) {
> + int64_t delta = qemu_clock_get_ns(rtc_clock) -
> + qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + s->tick_offset = s->tick_offset_vmstate -
> + delta / NANOSECONDS_PER_SECOND;
> + }
> pl031_set_alarm(s);
> return 0;
> }
>
> +static int pl031_tick_offset_post_load(void *opaque, int version_id)
> +{
> + PL031State *s = opaque;
> +
> + s->tick_offset_migrated = true;
> + return 0;
> +}
> +
> +static bool pl031_tick_offset_needed(void *opaque)
> +{
> + PL031State *s = opaque;
> +
> + return s->migrate_tick_offset;
> +}
> +
> +static const VMStateDescription vmstate_pl031_tick_offset = {
> + .name = "pl031/tick-offset",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = pl031_tick_offset_needed,
> + .post_load = pl031_tick_offset_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(tick_offset, PL031State),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static const VMStateDescription vmstate_pl031 = {
> .name = "pl031",
> .version_id = 1,
> .minimum_version_id = 1,
> .pre_save = pl031_pre_save,
> + .pre_load = pl031_pre_load,
> .post_load = pl031_post_load,
> .fields = (VMStateField[]) {
> VMSTATE_UINT32(tick_offset_vmstate, PL031State),
> @@ -231,14 +296,33 @@ static const VMStateDescription vmstate_pl031 = {
> VMSTATE_UINT32(im, PL031State),
> VMSTATE_UINT32(is, PL031State),
> VMSTATE_END_OF_LIST()
> + },
> + .subsections = (const VMStateDescription*[]) {
> + &vmstate_pl031_tick_offset,
> + NULL
> }
> };
>
> +static Property pl031_properties[] = {
> + /*
> + * True to correctly migrate the tick offset of the RTC. False to
> + * obtain backward migration compatibility with older QEMU versions,
> + * at the expense of the guest RTC going backwards compared with the
> + * host RTC when the VM is saved/restored if using -rtc host.
> + * (Even if set to 'true' older QEMU can migrate forward to newer QEMU;
> + * 'false' also permits newer QEMU to migrate to older QEMU.)
> + */
> + DEFINE_PROP_BOOL("migrate-tick-offset",
> + PL031State, migrate_tick_offset, true),
> + DEFINE_PROP_END_OF_LIST()
> +};
> +
> static void pl031_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> dc->vmsd = &vmstate_pl031;
> + dc->props = pl031_properties;
> }
>
> static const TypeInfo pl031_info = {
> --
> 2.20.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
