On Wed, Jan 15, 2025 at 7:23 AM Rodrigo Dias Correa <r...@drigo.nl> wrote:
>
> Instead of migrating the raw tick_offset, goldfish_rtc migrates a
> recalculated value based on QEMU_CLOCK_VIRTUAL. As QEMU_CLOCK_VIRTUAL
> stands still across a save-and-restore cycle, the guest RTC becomes out
> of sync with the host RTC when the VM is restored.
>
> As described in the bug description, it looks like this calculation was
> copied from pl031 RTC, which had its tick_offset migration fixed by
> Commit 032cfe6a79c8 ("pl031: Correctly migrate state when using -rtc
> clock=host").
>
> Migrate the tick_offset directly, adding it as a version-dependent field
> to VMState. Keep the old behavior when migrating from previous versions.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2033
> Signed-off-by: Rodrigo Dias Correa <r...@drigo.nl>
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> As a new field was added to VMState, after this patch, it won't be possible
> to
> migrate to old versions. I'm not sure if this is needed. Please, let me know.
> ---
> hw/rtc/goldfish_rtc.c | 43 +++++++++++++------------------------------
> 1 file changed, 13 insertions(+), 30 deletions(-)
>
> diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c
> index fa1d9051f4..0f1b53e0e4 100644
> --- a/hw/rtc/goldfish_rtc.c
> +++ b/hw/rtc/goldfish_rtc.c
> @@ -178,38 +178,21 @@ static void goldfish_rtc_write(void *opaque, hwaddr
> offset,
> trace_goldfish_rtc_write(offset, value);
> }
>
> -static int goldfish_rtc_pre_save(void *opaque)
> -{
> - uint64_t delta;
> - GoldfishRTCState *s = opaque;
> -
> - /*
> - * We want to migrate this offset, which sounds straightforward.
> - * Unfortunately, we cannot directly pass tick_offset because
> - * rtc_clock on destination Host might not be same source Host.
> - *
> - * To tackle, this we pass tick_offset relative to vm_clock from
> - * source Host and make it relative to rtc_clock at destination Host.
> - */
> - delta = qemu_clock_get_ns(rtc_clock) -
> - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> - s->tick_offset_vmstate = s->tick_offset + delta;
> -
> - return 0;
> -}
> -
> static int goldfish_rtc_post_load(void *opaque, int version_id)
> {
> - uint64_t delta;
> GoldfishRTCState *s = opaque;
>
> - /*
> - * We extract tick_offset from tick_offset_vmstate by doing
> - * reverse math compared to pre_save() function.
> - */
> - delta = qemu_clock_get_ns(rtc_clock) -
> - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> - s->tick_offset = s->tick_offset_vmstate - delta;
> + if (version_id < 3) {
> + /*
> + * Previous versions didn't migrate tick_offset directly. Instead,
> they
> + * migrated tick_offset_vmstate, which is a recalculation based on
> + * QEMU_CLOCK_VIRTUAL. We use tick_offset_vmstate when migrating from
> + * older versions.
> + */
> + uint64_t delta = qemu_clock_get_ns(rtc_clock) -
> + qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + s->tick_offset = s->tick_offset_vmstate - delta;
> + }
>
> goldfish_rtc_set_alarm(s);
>
> @@ -239,8 +222,7 @@ static const MemoryRegionOps goldfish_rtc_ops[2] = {
>
> static const VMStateDescription goldfish_rtc_vmstate = {
> .name = TYPE_GOLDFISH_RTC,
> - .version_id = 2,
> - .pre_save = goldfish_rtc_pre_save,
> + .version_id = 3,
> .post_load = goldfish_rtc_post_load,
> .fields = (const VMStateField[]) {
> VMSTATE_UINT64(tick_offset_vmstate, GoldfishRTCState),
> @@ -249,6 +231,7 @@ static const VMStateDescription goldfish_rtc_vmstate = {
> VMSTATE_UINT32(irq_pending, GoldfishRTCState),
> VMSTATE_UINT32(irq_enabled, GoldfishRTCState),
> VMSTATE_UINT32(time_high, GoldfishRTCState),
> + VMSTATE_UINT64_V(tick_offset, GoldfishRTCState, 3),
> VMSTATE_END_OF_LIST()
> }
> };
> --
> 2.34.1
>
>
