Implement Goldfish RTC #613
Open
+807
−106
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The system date after boot is not synchronized with the host's wall clock, resulting in incorrect timestamps shown by commands such as date. When the system has the hwclock utility, it cannot function because the /dev/rtc? device file is missing. In addition, programs might rely on RTC alarm events for wake-up operations, which require access to /dev/rtc? device file. This commit implements an RTC device using the Goldfish RTC model, chosen for its simplicity and native support in the Linux kernel. By adding RTC support, the following capabilities are enabled: - System boot-up synchronization: The RTC driver initializes the system time from the at boot, ensuring the guestOS starts with an accurate and synchronized timestamp. - Wakeup events: The RTC can generate alarm interrupts to wake up the process. - The date command, hwclock utility, and alarm configuration via ioctl (see the rtc(4) man page) are now functional. - Provide accurate timestamps for filesystem operations such as when writing or modifying files on a virtio block device. The sysprog21#605 hardcoded vblk_mmio_base_hi to 0x41, which limits adding new subnodes (e.g., RTC) under the SoC node. This value should instead be dynamically set during load_dtb(). Another limitation is in the next_addr probing logic, which currently uses subnode's name + 7. This assumes a fixed-length name and fails for variable-length names (e.g., rtc@4100000, should be name + 4). The fix is to locate the '@' character in the subnode name and use its position + 1 as the offset. Additionally, when no virtio block device is specified, virtio_mmio_base_hi will default to 0. In this case, use vblk_cnt for condition checking during MMIO handling to avoid incorrect behavior.
The do_buildroot process now supports patching from scratch, applying the desired packages while building rootfs.cpio with the make build-linux-image target. The desired packages should be stored in the tests/system/br_pkgs/ . The package patching from submodule might be supported in the future.
rtc_alarm.c: The newly modified Linux configuration enables the procfs interface for the /dev/rtc? real-time clock device. This example demonstrates how to interact with the RTC device step by step. The program: 1. Opens the /dev/rtc? device. 2. Reads and displays its current status via the procfs interface. 3. Sets an alarm to trigger 5 seconds later. 4. Enables the alarm interrupt. 5. Reads the status again. 6. Waits for the alarm to trigger. 7. Finally reads the status once more and disables the alarm interrupt. During this process, will observe the alarm_IRQ field transition from no -> yes -> no, indicating that the RTC interrupt has been successfully asserted and then handled by the emulated real time clock. rtc_settime.c: A simple user-space tool to test the RTC_SET_TIME ioctl on /dev/rtc?. It sets the RTC to the current UTC system time by default or to a custom year provided via command-line arguments. This helps verify that the real time clock driver correctly handles time updates. The buildroot.config is also modified to enable them by default. The example packages are stored in the tests/system/br_pkgs/ .
jserv
reviewed
Sep 21, 2025
There was a problem hiding this comment.
Benchmarks
| Benchmark suite | Current: 5ee534a | Previous: 2476710 | Ratio |
|---|---|---|---|
Dhrystone |
1306 Average DMIPS over 10 runs |
1327 Average DMIPS over 10 runs |
1.02 |
Coremark |
917.35 Average iterations/sec over 10 runs |
900.071 Average iterations/sec over 10 runs |
0.98 |
This comment was automatically generated by workflow using github-action-benchmark.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 15 files
Prompt for AI agents (all 12 issues)
Understand the root cause of the following 12 issues and fix them.
<file name="tools/build-linux-image.sh">
<violation number="1" location="tools/build-linux-image.sh:95">
Appends to Config.in are not idempotent; repeated runs will duplicate entries. Add a presence check before appending.</violation>
<violation number="2" location="tools/build-linux-image.sh:106">
Restrict find to C source files to avoid packaging non-C files.</violation>
</file>
<file name="src/riscv.c">
<violation number="1" location="src/riscv.c:354">
Unconditional assert on '@' in FDT node name can crash if a subnode lacks a unit-address; handle absence gracefully.</violation>
<violation number="2" location="src/riscv.c:356">
Missing validation of strtoul result for FDT unit-address; add endptr check to ensure hex parse succeeded.</violation>
<violation number="3" location="src/riscv.c:373">
vblk MMIO range computation should avoid defining a range when vblk_cnt == 0 to prevent misrouting MMIO accesses.</violation>
<violation number="4" location="src/riscv.c:676">
Ensure vblk_mmio_base_hi/max_hi are initialized to safe defaults (e.g., 0) when vblk_cnt == 0 to avoid undefined behavior.</violation>
</file>
<file name="tests/system/br_pkgs/rtc/rtc_settime.c">
<violation number="1" location="tests/system/br_pkgs/rtc/rtc_settime.c:50">
Possible NULL dereference: check gmtime() result before dereferencing utc_tm.</violation>
</file>
<file name="src/common.h">
<violation number="1" location="src/common.h:32">
Left shift can be undefined when n >= bit-width of unsigned long; guard against out-of-range shift to avoid UB.</violation>
</file>
<file name=".ci/boot-linux.sh">
<violation number="1" location=".ci/boot-linux.sh:39">
Misleading exit code: use exit 3 for command timeout to match "Fail to run commands" message.</violation>
<violation number="2" location=".ci/boot-linux.sh:48">
Labels like "rtc_alarm"/"rtc_settime" are being passed as rv32emu CLI args, likely causing unknown-argument failures.</violation>
</file>
<file name="assets/system/configs/buildroot.config">
<violation number="1" location="assets/system/configs/buildroot.config:51">
Unknown Buildroot symbol BR2_PACKAGE_RTC_ALARM is enabled but no package definition exists in the repo; this will be ignored by Kconfig or break reproducibility. Disable or add proper Buildroot package metadata.</violation>
<violation number="2" location="assets/system/configs/buildroot.config:52">
Unknown Buildroot symbol BR2_PACKAGE_RTC_SETTIME is enabled but no package definition exists in the repo; this will be ignored by Kconfig or break reproducibility. Disable or add proper Buildroot package metadata.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| # This function patches the packages when building the rootfs.cpio from scratch | ||
| function do_patch_buildroot | ||
| { | ||
| for c in $(find ${BR_PKG_DIR} -type f); do |
There was a problem hiding this comment.
Restrict find to C source files to avoid packaging non-C files.
Prompt for AI agents
Address the following comment on tools/build-linux-image.sh at line 106:
<comment>Restrict find to C source files to avoid packaging non-C files.</comment>
<file context>
@@ -23,32 +23,127 @@ PARALLEL="-j$(nproc)"
+# This function patches the packages when building the rootfs.cpio from scratch
+function do_patch_buildroot
+{
+ for c in $(find ${BR_PKG_DIR} -type f); do
+ local basename="$(basename ${c})"
+ local pkg_name="${basename%.*}"
</file context>
Suggested change
| for c in $(find ${BR_PKG_DIR} -type f); do | |
| for c in $(find "${BR_PKG_DIR}" -type f -name '*.c'); do |
| { | ||
| local pkg_name=$1 | ||
|
|
||
| cat << EOF >> "${SRC_DIR}/buildroot/package/Config.in" |
There was a problem hiding this comment.
Appends to Config.in are not idempotent; repeated runs will duplicate entries. Add a presence check before appending.
Prompt for AI agents
Address the following comment on tools/build-linux-image.sh at line 95:
<comment>Appends to Config.in are not idempotent; repeated runs will duplicate entries. Add a presence check before appending.</comment>
<file context>
@@ -23,32 +23,127 @@ PARALLEL="-j$(nproc)"
+{
+ local pkg_name=$1
+
+ cat << EOF >> "${SRC_DIR}/buildroot/package/Config.in"
+menu "${pkg_name}"
+ source "package/${pkg_name}/Config.in"
</file context>
| attr->vblk_mmio_base_hi = 0x41; | ||
| attr->vblk_mmio_max_hi = attr->vblk_mmio_base_hi + attr->vblk_cnt; | ||
| /* setup rtc */ | ||
| attr->rtc = rtc_new(); |
There was a problem hiding this comment.
Ensure vblk_mmio_base_hi/max_hi are initialized to safe defaults (e.g., 0) when vblk_cnt == 0 to avoid undefined behavior.
Prompt for AI agents
Address the following comment on src/riscv.c at line 676:
<comment>Ensure vblk_mmio_base_hi/max_hi are initialized to safe defaults (e.g., 0) when vblk_cnt == 0 to avoid undefined behavior.</comment>
<file context>
@@ -665,9 +672,8 @@ riscv_t *rv_create(riscv_user_t rv_attr)
- attr->vblk_mmio_base_hi = 0x41;
- attr->vblk_mmio_max_hi = attr->vblk_mmio_base_hi + attr->vblk_cnt;
+ /* setup rtc */
+ attr->rtc = rtc_new();
attr->vblk = malloc(sizeof(virtio_blk_state_t *) * attr->vblk_cnt);
</file context>
|
|
||
| /* set the VBLK MMIO valid range */ | ||
| attr->vblk_mmio_base_hi = next_addr >> 20; | ||
| attr->vblk_mmio_max_hi = attr->vblk_mmio_base_hi + attr->vblk_cnt; |
There was a problem hiding this comment.
vblk MMIO range computation should avoid defining a range when vblk_cnt == 0 to prevent misrouting MMIO accesses.
Prompt for AI agents
Address the following comment on src/riscv.c at line 373:
<comment>vblk MMIO range computation should avoid defining a range when vblk_cnt == 0 to prevent misrouting MMIO accesses.</comment>
<file context>
@@ -365,6 +368,10 @@ static void load_dtb(char **ram_loc, vm_attr_t *attr)
+ /* set the VBLK MMIO valid range */
+ attr->vblk_mmio_base_hi = next_addr >> 20;
+ attr->vblk_mmio_max_hi = attr->vblk_mmio_base_hi + attr->vblk_cnt;
+
/* adding new virtio block nodes */
</file context>
Suggested change
| attr->vblk_mmio_max_hi = attr->vblk_mmio_base_hi + attr->vblk_cnt; | |
| attr->vblk_mmio_max_hi = attr->vblk_cnt ? (attr->vblk_mmio_base_hi + attr->vblk_cnt - 1) : 0; |
| char *at_pos = strchr(name, '@'); | ||
| assert(at_pos); | ||
|
|
||
| uint32_t addr = strtoul(at_pos + 1, NULL, 16); |
There was a problem hiding this comment.
Missing validation of strtoul result for FDT unit-address; add endptr check to ensure hex parse succeeded.
Prompt for AI agents
Address the following comment on src/riscv.c at line 356:
<comment>Missing validation of strtoul result for FDT unit-address; add endptr check to ensure hex parse succeeded.</comment>
<file context>
@@ -350,7 +350,10 @@ static void load_dtb(char **ram_loc, vm_attr_t *attr)
+ char *at_pos = strchr(name, '@');
+ assert(at_pos);
+
+ uint32_t addr = strtoul(at_pos + 1, NULL, 16);
if (addr == next_addr)
next_addr = addr + addr_offset;
</file context>
| #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0])) | ||
|
|
||
| #define MASK(n) (~((~0U << (n)))) | ||
| #define MASK(n) (~((~0UL << (n)))) |
There was a problem hiding this comment.
Left shift can be undefined when n >= bit-width of unsigned long; guard against out-of-range shift to avoid UB.
Prompt for AI agents
Address the following comment on src/common.h at line 32:
<comment>Left shift can be undefined when n >= bit-width of unsigned long; guard against out-of-range shift to avoid UB.</comment>
<file context>
@@ -29,7 +29,7 @@
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
-#define MASK(n) (~((~0U << (n))))
+#define MASK(n) (~((~0UL << (n))))
#if defined(_MSC_VER)
</file context>
Suggested change
| #define MASK(n) (~((~0UL << (n)))) | |
| #define MASK(n) ((n) >= (sizeof(unsigned long) * 8) ? ~0UL : ((1UL << (n)) - 1UL)) |
| TEST_OPTIONS=("base (${OPTS_BASE})") | ||
| EXPECT_CMDS=(' | ||
| expect "buildroot login:" { send "root\n" } timeout { exit 1 } | ||
| expect "# " { send "dmesg | grep rtc\n" } timeout { exit 2 } |
There was a problem hiding this comment.
Misleading exit code: use exit 3 for command timeout to match "Fail to run commands" message.
Prompt for AI agents
Address the following comment on .ci/boot-linux.sh at line 39:
<comment>Misleading exit code: use exit 3 for command timeout to match "Fail to run commands" message.</comment>
<file context>
@@ -33,10 +36,93 @@ OPTS_BASE+=" -i build/linux-image/rootfs.cpio"
TEST_OPTIONS=("base (${OPTS_BASE})")
EXPECT_CMDS=('
expect "buildroot login:" { send "root\n" } timeout { exit 1 }
+ expect "# " { send "dmesg | grep rtc\n" } timeout { exit 2 }
+ expect "rtc0" { } timeout { exit 3 }
+ expect "# " { send "date -u +%Y\n" } timeout { exit 2 }
</file context>
| BR2_PACKAGE_UTIL_LINUX=y | ||
| BR2_PACKAGE_UTIL_LINUX_HWCLOCK=y | ||
|
|
||
| BR2_PACKAGE_RTC_ALARM=y |
There was a problem hiding this comment.
Unknown Buildroot symbol BR2_PACKAGE_RTC_ALARM is enabled but no package definition exists in the repo; this will be ignored by Kconfig or break reproducibility. Disable or add proper Buildroot package metadata.
Prompt for AI agents
Address the following comment on assets/system/configs/buildroot.config at line 51:
<comment>Unknown Buildroot symbol BR2_PACKAGE_RTC_ALARM is enabled but no package definition exists in the repo; this will be ignored by Kconfig or break reproducibility. Disable or add proper Buildroot package metadata.</comment>
<file context>
@@ -45,3 +45,8 @@ BR2_TARGET_ROOTFS_CPIO_NONE=y
+BR2_PACKAGE_UTIL_LINUX=y
+BR2_PACKAGE_UTIL_LINUX_HWCLOCK=y
+
+BR2_PACKAGE_RTC_ALARM=y
+BR2_PACKAGE_RTC_SETTIME=y
</file context>
Suggested change
| BR2_PACKAGE_RTC_ALARM=y | |
| # BR2_PACKAGE_RTC_ALARM is not set |
| BR2_PACKAGE_UTIL_LINUX_HWCLOCK=y | ||
|
|
||
| BR2_PACKAGE_RTC_ALARM=y | ||
| BR2_PACKAGE_RTC_SETTIME=y |
There was a problem hiding this comment.
Unknown Buildroot symbol BR2_PACKAGE_RTC_SETTIME is enabled but no package definition exists in the repo; this will be ignored by Kconfig or break reproducibility. Disable or add proper Buildroot package metadata.
Prompt for AI agents
Address the following comment on assets/system/configs/buildroot.config at line 52:
<comment>Unknown Buildroot symbol BR2_PACKAGE_RTC_SETTIME is enabled but no package definition exists in the repo; this will be ignored by Kconfig or break reproducibility. Disable or add proper Buildroot package metadata.</comment>
<file context>
@@ -45,3 +45,8 @@ BR2_TARGET_ROOTFS_CPIO_NONE=y
+BR2_PACKAGE_UTIL_LINUX_HWCLOCK=y
+
+BR2_PACKAGE_RTC_ALARM=y
+BR2_PACKAGE_RTC_SETTIME=y
</file context>
Suggested change
| BR2_PACKAGE_RTC_SETTIME=y | |
| # BR2_PACKAGE_RTC_SETTIME is not set |
This change adds CI tests to verify RTC functionality, covering the
following cases:
1. Verify that /dev/rtc0 is present and logged in dmesg.
2. Ensure /dev/rtc0 is interactable:
- Use rtc_settime to set the year to both before(1980) and
after(2030) the current year, then verify that the RTC retains the
changes.
- Use rtc_alarm to confirm that the alarm_IRQ status transitions
correctly: no -> yes -> no.
3. Validate alignment of the UTC year between host and guest using the
date command.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request introduces full RTC support, integrating a new Goldfish RTC device, userspace utilities, Buildroot updates, and comprehensive CI test coverage to ensure correctness and long-term reliability.
The Goldfish RTC device has been implemented to provide accurate system time and alarm functionality in the guest OS. At boot, the system clock is synchronized with the host’s wall clock, ensuring the guest starts with an accurate and consistent timestamp. This enables standard tools such as
dateandhwclockto function correctly and guarantees precise timestamps for filesystem operations like file creation and modification. Alarm events are now supported, allowing processes to schedule wake-up events via RTC interrupts. Alongside this feature, improvements were made to the DTB parsing logic to handle variable-length subnode names and to fix issues in MMIO handling when no virtio block devices are present. The Linux kernel configuration was also updated to enable the Goldfish RTC driver by default.Buildroot was extended to include the
hwclockutility and to support patching custom packages directly into the root filesystem. These packages are stored undertests/system/br_pkgs/, simplifying the workflow for testing and debugging RTC behavior while allowing additional test tools to be integrated easily in the future.Two new userspace utilities were introduced for validating and demonstrating RTC functionality. The
rtc_alarmprogram walks through enabling, triggering, and disabling RTC alarms, showing the alarm_IRQ state transition from no -> yes -> no. Thertc_settimeutility provides a simple interface to test theRTC_SET_TIMEioctl by setting the RTC to either the current UTC time or a custom year supplied via command-line arguments. These tools are automatically built and included in the root filesystem to streamline testing.Comprehensive CI test coverage was added to automatically verify RTC functionality. The workflow validates that
/dev/rtc0is present and properly registered indmesg, ensures the RTC retains updates by setting it to years both before and after the current year, and confirms that alarm interrupts behave correctly. These checks are performed using thertc_settimeandrtc_alarmutilities, verifying that time changes persist and alarm events trigger as expected. The CI process also checks that the guest’s UTC year remains aligned with the host’s wall clock. Together, these changes deliver a fully functional RTC feature set with automated testing to prevent regressions and maintain stability over time.Testing
date,rtc_alarm,rtc_settimein guestOS and the sample outputExpected Result
Date correctly synchronizes with the host wall clock in UTC format, alarms trigger as expected, and RTC_SET_TIME events are handled properly."
Summary by cubic
Add a Goldfish RTC device with host-synced UTC time and alarm interrupts, fully integrated with Linux, Buildroot, and CI. The guest now exposes /dev/rtc0, so date/hwclock work and alarms trigger via PLIC.
New Features
Bug Fixes