kern/57980: gfrtc(4) hardclock runs late (on virt68k)

[isaki%pastel-flower.jp@localhost]

>Number:         57980
>Category:       kern
>Synopsis:       gfrtc(4) hardclock runs late (on virt68k)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Mar 02 05:55:00 +0000 2024
>Originator:     Tetsuya Isaki
>Release:        NetBSD-current around 20240225
>Organization:
>Environment:
NetBSD 10.99.10 virt68k
>Description:
In the current gfrtc(4) implementation, the time runs late by
two problems.

First one, the timespan between
 a) alarm time has arrived, and
 b) read new TIME_{LOW,HIGH} on the next interrupt handler
    (gfrtc_mainbus_hardclock() in arch/virt68k/dev/gfrtc_mainbus.c)
is not accumulated.
This causes that the hardclock runs late every interrupt.

Second one.  Current our gfrtc(4) uses an oneshot timer everytime.
In this method, if the host time between a) and b) described above
is prolonged (and it can well happen on emulators), we have no way to
know that prolonged host time.
This causes that the guest time can never catch up with host time
again once this happens.

Here is a patch that resolves both.
- If the past time is written to ALARM_* register, QEMU
  implementation raises the interrupt immediately.  So the guest can
  catch up with the host time soon.
- It changes gfrtc_set_alarm() MI-MD interface.  But I think no problem.

--- sys/arch/virt68k/dev/gfrtc_mainbus.c	12 Jan 2024 06:23:20 -0000	1.2
+++ sys/arch/virt68k/dev/gfrtc_mainbus.c	25 Feb 2024 08:53:37 -0000
@@ -58,6 +58,7 @@
 	struct gfrtc_softc	sc_gfrtc;
 	struct clock_attach_args sc_clock_args;
 	uint64_t		sc_interval_ns;
+	uint64_t		sc_alarm_time;
 	void			(*sc_handler)(struct clockframe *);
 	struct evcnt *		sc_evcnt;
 	void *			sc_ih;
@@ -68,8 +69,9 @@
 	/* Clear the interrupt condition. */				\
 	gfrtc_clear_interrupt(&sc->sc_gfrtc);				\
 									\
-	/* Get the next alarm set ASAP. */				\
-	gfrtc_set_alarm(&sc->sc_gfrtc, sc->sc_interval_ns);		\
+	/* Get the next alarm set. */					\
+	sc->sc_alarm_time += sc->sc_interval_ns;			\
+	gfrtc_set_alarm(&sc->sc_gfrtc, sc->sc_alarm_time);		\
 									\
 	/* Increment the counter and call the handler. */		\
 	sc->sc_evcnt->ev_count++;					\
@@ -103,7 +105,8 @@
 	gfrtc_enable_interrupt(&sc->sc_gfrtc);
 
 	/* Start the first alarm! */
-	gfrtc_set_alarm(&sc->sc_gfrtc, sc->sc_interval_ns);
+	sc->sc_alarm_time = gfrtc_get_time(&sc->sc_gfrtc) + sc->sc_interval_ns;
+	gfrtc_set_alarm(&sc->sc_gfrtc, sc->sc_alarm_time);
 }
 
 static int
--- sys/dev/goldfish/gfrtc.c
+++ sys/dev/goldfish/gfrtc.c
@@ -141,10 +141,8 @@ gfrtc_get_time(struct gfrtc_softc * const sc)
 
 
 void
-gfrtc_set_alarm(struct gfrtc_softc * const sc, const uint64_t nsec)
+gfrtc_set_alarm(struct gfrtc_softc * const sc, const uint64_t next)
 {
-	uint64_t next = gfrtc_get_time(sc) + nsec;
-
 	GOLDFISH_RTC_WRITE(sc, GOLDFISH_RTC_ALARM_HIGH, (uint32_t)(next >> 32));
 	GOLDFISH_RTC_WRITE(sc, GOLDFISH_RTC_ALARM_LOW, (uint32_t)next);
 }


>How-To-Repeat:
On virt68k:
# ntpdate server
# date
(Wait 60 seconds on your real watch)
# date

>Fix:
See above.