Changeset View
Standalone View
sys/amd64/vmm/io/vrtc.c
Show First 20 Lines • Show All 178 Lines • ▼ Show 20 Lines | |||||
} | } | ||||
static void | static void | ||||
secs_to_rtc(time_t rtctime, struct vrtc *vrtc, int force_update) | secs_to_rtc(time_t rtctime, struct vrtc *vrtc, int force_update) | ||||
{ | { | ||||
struct clocktime ct; | struct clocktime ct; | ||||
struct timespec ts; | struct timespec ts; | ||||
struct rtcdev *rtc; | struct rtcdev *rtc; | ||||
int hour; | int hour, r; | ||||
KASSERT(VRTC_LOCKED(vrtc), ("%s: vrtc not locked", __func__)); | KASSERT(VRTC_LOCKED(vrtc), ("%s: vrtc not locked", __func__)); | ||||
if (rtctime < 0) { | if (rtctime < 0) { | ||||
KASSERT(rtctime == VRTC_BROKEN_TIME, | KASSERT(rtctime == VRTC_BROKEN_TIME, | ||||
("%s: invalid vrtc time %#lx", __func__, rtctime)); | ("%s: invalid vrtc time %#lx", __func__, rtctime)); | ||||
return; | return; | ||||
} | } | ||||
/* | /* | ||||
* If the RTC is halted then the guest has "ownership" of the | * If the RTC is halted then the guest has "ownership" of the | ||||
* date/time fields. Don't update the RTC date/time fields in | * date/time fields. Don't update the RTC date/time fields in | ||||
* this case (unless forced). | * this case (unless forced). | ||||
*/ | */ | ||||
if (rtc_halted(vrtc) && !force_update) | if (rtc_halted(vrtc) && !force_update) | ||||
return; | return; | ||||
ts.tv_sec = rtctime; | ts.tv_sec = rtctime; | ||||
ts.tv_nsec = 0; | ts.tv_nsec = 0; | ||||
clock_ts_to_ct(&ts, &ct); | r = clock_ts_to_ct(&ts, &ct); | ||||
if (r) | |||||
panic("invalid RTC time %ld seconds %ld nanoseconds", | |||||
imp: could this be reached from setting the date via the command line? If so, then it shouldn't… | |||||
asomersAuthorUnsubmitted Done Inline ActionsAs far as I can tell, no. I think only BHyve uses this. And this panic isn't new; there was always a panic in this path, at least in INVARIANTS builds. asomers: As far as I can tell, no. I think only BHyve uses this. And this panic isn't new; there was… | |||||
ts.tv_sec, ts.tv_nsec); | |||||
KASSERT(ct.sec >= 0 && ct.sec <= 59, ("invalid clocktime sec %d", | KASSERT(ct.sec >= 0 && ct.sec <= 60, ("invalid clocktime sec %d", | ||||
impUnsubmitted Not Done Inline ActionsThis is wrong. It can never be 60 here. time_t has no ability to represent a leap second since POSIX does not define an encoding for it, let alone a unique one (There's a formula in the standard, but it's specifically not for leap seconds). imp: This is wrong. It can never be 60 here. time_t has no ability to represent a leap second since… | |||||
asomersAuthorUnsubmitted Done Inline ActionsAllright, I'll adjust all of the assertions and checks to exclude the possibility of leap seconds. asomers: Allright, I'll adjust all of the assertions and checks to exclude the possibility of leap… | |||||
ct.sec)); | ct.sec)); | ||||
KASSERT(ct.min >= 0 && ct.min <= 59, ("invalid clocktime min %d", | KASSERT(ct.min >= 0 && ct.min <= 59, ("invalid clocktime min %d", | ||||
ct.min)); | ct.min)); | ||||
KASSERT(ct.hour >= 0 && ct.hour <= 23, ("invalid clocktime hour %d", | KASSERT(ct.hour >= 0 && ct.hour <= 23, ("invalid clocktime hour %d", | ||||
ct.hour)); | ct.hour)); | ||||
KASSERT(ct.dow >= 0 && ct.dow <= 6, ("invalid clocktime wday %d", | KASSERT(ct.dow >= 0 && ct.dow <= 6, ("invalid clocktime wday %d", | ||||
ct.dow)); | ct.dow)); | ||||
KASSERT(ct.day >= 1 && ct.day <= 31, ("invalid clocktime mday %d", | KASSERT(ct.day >= 1 && ct.day <= 31, ("invalid clocktime mday %d", | ||||
ct.day)); | ct.day)); | ||||
KASSERT(ct.mon >= 1 && ct.mon <= 12, ("invalid clocktime month %d", | KASSERT(ct.mon >= 1 && ct.mon <= 12, ("invalid clocktime month %d", | ||||
ct.mon)); | ct.mon)); | ||||
KASSERT(ct.year >= POSIX_BASE_YEAR, ("invalid clocktime year %d", | KASSERT(ct.year >= POSIX_BASE_YEAR, ("invalid clocktime year %d", | ||||
ct.year)); | ct.year)); | ||||
Not Done Inline ActionsThe values are already validated by clock_ts_to_ct. In my opinion, we can drop those checks to avoid checking them twice. corvink: The values are already validated by `clock_ts_to_ct`. In my opinion, we can drop those checks… | |||||
Done Inline ActionsThat was my first impulse. OTOH, there's no guarantee that clock_ts_to_ct will continue to validate them the same way that this function does (and you'll notice that it doesn't validate year quite the same way). And since these are only assertions, they don't even get included in production builds, so I left them in. asomers: That was my first impulse. OTOH, there's no guarantee that `clock_ts_to_ct` will continue to… | |||||
Not Done Inline Actionsclock_ts_to_ct should always return a valid clocktime if it succeeds. If the vrtc isn't able to handle a valid clocktime, it shouldn't ask for it. The asserts are already included. So, it's fine to keep them. We could just think of removing these unneccessary checks while we're already here. corvink: `clock_ts_to_ct` should always return a valid clocktime if it succeeds. If the vrtc isn't able… | |||||
Done Inline ActionsWhat do you mean by "keep the asserts", but "remove the unneccessary checks"? What's the difference between checks and asserts? asomers: What do you mean by "keep the asserts", but "remove the unneccessary checks"? What's the… | |||||
Not Done Inline ActionsSry, for being unclear. We can think of removing the asserts because we're touching this code but if we keep them, it's fine. corvink: Sry, for being unclear. We can think of removing the asserts because we're touching this code… | |||||
rtc = &vrtc->rtcdev; | rtc = &vrtc->rtcdev; | ||||
rtc->sec = rtcset(rtc, ct.sec); | rtc->sec = rtcset(rtc, ct.sec); | ||||
rtc->min = rtcset(rtc, ct.min); | rtc->min = rtcset(rtc, ct.min); | ||||
if (rtc->reg_b & RTCSB_24HR) { | if (rtc->reg_b & RTCSB_24HR) { | ||||
hour = ct.hour; | hour = ct.hour; | ||||
} else { | } else { | ||||
▲ Show 20 Lines • Show All 836 Lines • Show Last 20 Lines |
could this be reached from setting the date via the command line? If so, then it shouldn't panic.
Also, if this is in the boot path, it will keep us from booting at all.