-
Notifications
You must be signed in to change notification settings - Fork 137
Add RTC timezone mode option #709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Implement CLI option to select RTC time mode between UTC and local time. This allows the emulated Goldfish RTC to report either UTC or local time with timezone offset to the guest Linux kernel. Changes: - Add -x rtc:<mode> argument parser in main.c (modes: utc, localtime) - Define rtc_time_mode_t enum in riscv.h for shared usage - Add use_localtime field to rtc_t structure - Update rtc_new() to accept use_localtime parameter - Apply tm_gmtoff offset in rtc_get_now_nsec() for localtime mode Usage: ./rv32emu -k <kernel_path> -i <rootfs_path> -x rtc:<utc|localtime>
Add tests for the RTC timezone mode option (-x rtc:<mode>): - Explicit UTC mode test: Verify -x rtc:utc behaves same as default - Localtime offset comparison test: Compare guest hour with host local hour using ±1 hour tolerance for robustness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/main.c">
<violation number="1" location="src/main.c:146">
P2: `-x rtc:<mode>` fails after max vblk devices because the vblk limit check runs before distinguishing rtc vs vblk options.</violation>
<violation number="2" location="src/main.c:148">
P2: RTC mode parsing accepts any prefix (e.g., "rtc:utcfoo"), not just exact "utc" or "localtime".</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
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| else | ||
| else if (!strncmp("rtc:", optarg, 4)) { | ||
| const char *mode = optarg + 4; /* strlen("rtc:") */ | ||
| if (!strncmp(mode, "localtime", 9)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: RTC mode parsing accepts any prefix (e.g., "rtc:utcfoo"), not just exact "utc" or "localtime".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main.c, line 148:
<comment>RTC mode parsing accepts any prefix (e.g., "rtc:utcfoo"), not just exact "utc" or "localtime".</comment>
<file context>
@@ -141,7 +143,15 @@ static bool parse_args(int argc, char **args)
- else
+ else if (!strncmp("rtc:", optarg, 4)) {
+ const char *mode = optarg + 4; /* strlen("rtc:") */
+ if (!strncmp(mode, "localtime", 9))
+ opt_rtc_mode = RTC_LOCALTIME;
+ else if (!strncmp(mode, "utc", 3))
</file context>
| opt_virtio_blk_img[opt_virtio_blk_idx++] = | ||
| optarg + 5; /* strlen("vblk:") */ | ||
| else | ||
| else if (!strncmp("rtc:", optarg, 4)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: -x rtc:<mode> fails after max vblk devices because the vblk limit check runs before distinguishing rtc vs vblk options.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main.c, line 146:
<comment>`-x rtc:<mode>` fails after max vblk devices because the vblk limit check runs before distinguishing rtc vs vblk options.</comment>
<file context>
@@ -141,7 +143,15 @@ static bool parse_args(int argc, char **args)
opt_virtio_blk_img[opt_virtio_blk_idx++] =
optarg + 5; /* strlen("vblk:") */
- else
+ else if (!strncmp("rtc:", optarg, 4)) {
+ const char *mode = optarg + 4; /* strlen("rtc:") */
+ if (!strncmp(mode, "localtime", 9))
</file context>
jserv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Benchmarks
Details
| Benchmark suite | Current: 78cf51f | Previous: 3641924 | Ratio |
|---|---|---|---|
Dhrystone |
1869.667 DMIPS |
1619.667 DMIPS |
0.87 |
CoreMark |
1000.228 iterations/sec |
949.751 iterations/sec |
0.95 |
This comment was automatically generated by workflow using github-action-benchmark.
|
Shouldn't RTC hardware be timezone-agnostic? |
visitorckw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective, it is quite counter-intuitive for hardware behavior to change based on the host's timezone.
We might need more explanation on the intended use cases: Which type of users would require this feature, and in what specific scenarios would it be beneficial? Furthermore, are there any real-world hardware implementations that exhibit similar behavior?
Introduce a new command-line option
-x rtc:<mode>to configure the Real-Time Clock (RTC) timezone behavior. This allows the guest system's RTC to synchronize with either the host's UTC time or local time.-x rtc:<utc|localtime>option to select RTC timezone modeSummary by cubic
Add a CLI option to select RTC timezone mode (UTC or localtime) so guests can read either UTC or local time from the Goldfish RTC. Default stays UTC; localtime applies the host timezone offset.
Written for commit 78cf51f. Summary will update on new commits.