NetBSD Problem Report #57980

From www@netbsd.org  Sat Mar  2 05:54:43 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 6F4451A9239
	for <gnats-bugs@gnats.NetBSD.org>; Sat,  2 Mar 2024 05:54:43 +0000 (UTC)
Message-Id: <20240302055442.83FD71A923A@mollari.NetBSD.org>
Date: Sat,  2 Mar 2024 05:54:42 +0000 (UTC)
From: isaki@pastel-flower.jp
Reply-To: isaki@pastel-flower.jp
To: gnats-bugs@NetBSD.org
Subject: gfrtc(4) hardclock runs late (on virt68k)
X-Send-Pr-Version: www-1.0

>Number:         57980
>Category:       kern
>Synopsis:       gfrtc(4) hardclock runs late (on virt68k)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    isaki
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Mar 02 05:55:00 +0000 2024
>Closed-Date:    Tue Mar 05 11:23:05 +0000 2024
>Last-Modified:  Tue Mar 05 11:23:05 +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.

>Release-Note:

>Audit-Trail:
From: "Tetsuya Isaki" <isaki@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57980 CVS commit: src/sys
Date: Tue, 5 Mar 2024 11:19:31 +0000

 Module Name:	src
 Committed By:	isaki
 Date:		Tue Mar  5 11:19:30 UTC 2024

 Modified Files:
 	src/sys/arch/virt68k/dev: gfrtc_mainbus.c
 	src/sys/dev/goldfish: gfrtc.c

 Log Message:
 Fix two problems that the time runs late on virt68k.
 - The time between the time the alarm occurred and the time read by
   TIME_* register in the next interrupt handler was not accumulated.
 - With the one-shot timer method, once the host time prolongs, the
   guest time will never be able to catch up with the host time again.
 New one does:
 - The driver maintains its (guest's) time (as sc_alarm_time) and always
   set the next alarm sc_interval_ns after the previous alarm.
 - gfrtc_set_alarm() takes an absolute time instead of a relative time
   as the argument.
 PR kern/57980.  Confirmed on QEMU.


 To generate a diff of this commit:
 cvs rdiff -u -r1.2 -r1.3 src/sys/arch/virt68k/dev/gfrtc_mainbus.c
 cvs rdiff -u -r1.4 -r1.5 src/sys/dev/goldfish/gfrtc.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Responsible-Changed-From-To: kern-bug-people->isaki
Responsible-Changed-By: isaki@NetBSD.org
Responsible-Changed-When: Tue, 05 Mar 2024 11:23:05 +0000
Responsible-Changed-Why:
I take.


State-Changed-From-To: open->closed
State-Changed-By: isaki@NetBSD.org
State-Changed-When: Tue, 05 Mar 2024 11:23:05 +0000
State-Changed-Why:
Committed.


>Unformatted:

Home	PR Database Search