std: sys: pal: uefi: Overhaul Time by Ayush1325 · Pull Request #139806 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

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

[ Show hidden characters]({{ revealButtonHref }})

Ayush1325

Use UEFI time format for SystemTime.

All calculations and comparisons are being done using UnixTime since UEFI Time format does not seem good fit for those calculations.

I have tested the conversions and calculations, but I am not sure if there is a way to run unit tests for platform specific code in Rust source.

The only real benefit from using UEFI Time representation is that to and fro conversion will preserve daylight and timezone values.

r? @joboet

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Apr 14, 2025

fbstj

}
pub(crate) const fn checked_add(&self, dur: Duration) -> Self {
let temp: u32 = self.usecs + dur.subsec_nanos();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're combining something called usecs with something called nanos that doesn't seem to be correct?

EDIT: it looks like usecs should be called nanos thruout the file?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have renamed it to nanos now. The reason for using nanos instead of normal microseconds (which is uses in unix) is because nanoseconds are stored in the UEFI Time normally.

@Ayush1325

@joboet

This still doesn't solve the problem that checked_add and checked_sub can return Some even though the time does not fit into the bounds of the UEFI time structure. The other problem is that the unspecified timezone is not handled correctly – it's not equal to UTC and comparing/operating on a time with a specified timezone (such as the UNIX epoch) with a time with unspecified timezone is not a defined operation. A panic might be justified in that case.

Then, there is a design question: I think it might be more advantageous to convert the time and date directly into seconds (just like in the old version) as all of the operations use seconds, and it's unlikely that one would create a SystemTime without performing arithmetic on it. You'd still need to preserve the timezone information in SystemTime though, as it's necessary for bound-checking.

@Ayush1325

This still doesn't solve the problem that checked_add and checked_sub can return Some even though the time does not fit into the bounds of the UEFI time structure.

You mean they should fail for bounds outside of those defined here? Or should it fail when the values don't fit in the underlying data structure?

UINT16 Year; // 1900 - 9999
UINT8 Month; // 1 - 12
UINT8 Day; // 1 - 31
UINT8 Hour; // 0 - 23
UINT8 Minute; // 0 - 59
UINT8 Second; // 0 - 59
UINT32 Nanosecond; // 0 - 999,999,999
INT16 TimeZone; // --1440 to 1440 or 2047

The other problem is that the unspecified timezone is not handled correctly – it's not equal to UTC and comparing/operating on a time with a specified timezone (such as the UNIX epoch) with a time with unspecified timezone is not a defined operation. A panic might be justified in that case.

Ok, but should unspecified to unspecified calculation be fine? Currently the timezone on my qemu also shows up as unspecified, so I think they should be allowed.

Then, there is a design question: I think it might be more advantageous to convert the time and date directly into seconds (just like in the old version) as all of the operations use seconds, and it's unlikely that one would create a SystemTime without performing arithmetic on it. You'd still need to preserve the timezone information in SystemTime though, as it's necessary for bound-checking.

Ok, I was thinking the same as all that to and fro conversion for everything was getting a bit too much.

@joboet

This still doesn't solve the problem that checked_add and checked_sub can return Some even though the time does not fit into the bounds of the UEFI time structure.

You mean they should fail for bounds outside of those defined here? Or should it fail when the values don't fit in the underlying data structure?

I think we should use the defined bounds (so years from 1900–9999, etc.). SetTime is specified to report an error if the time is outside these bounds, and I think it makes sense to respect that.

The other problem is that the unspecified timezone is not handled correctly – it's not equal to UTC and comparing/operating on a time with a specified timezone (such as the UNIX epoch) with a time with unspecified timezone is not a defined operation. A panic might be justified in that case.

Ok, but should unspecified to unspecified calculation be fine? Currently the timezone on my qemu also shows up as unspecified, so I think they should be allowed.

I agree. Only unspecified-specified calculations are undefined.

Then, there is a design question: I think it might be more advantageous to convert the time and date directly into seconds (just like in the old version) as all of the operations use seconds, and it's unlikely that one would create a SystemTime without performing arithmetic on it. You'd still need to preserve the timezone information in SystemTime though, as it's necessary for bound-checking.

Ok, I was thinking the same as all that to and fro conversion for everything was getting a bit too much.

😄 yeah... I'd maybe still choose a different timebase than UNIX time. You could e.g. use 1900-01-01-00:00,0[local] as base, which would allow you to use Duration again, since all valid times are forward from that date.

@Ayush1325

😄 yeah... I'd maybe still choose a different timebase than UNIX time. You could e.g. use 1900-01-01-00:00,0[local] as base, which would allow you to use Duration again, since all valid times are forward from that date.

So how should conversion be done? UEFI -> Unix -> Anchor to 1900-01-01-00:00,0 ?
I am not sure how to account for leap years and all that stuff without going through Unix (which has already been documented extensively)

@joboet

The source for the algorithm you cite already does a conversion from a timebase of 0000-03-01 to 1970-01-01 by adding/subtracting an offset from the day count.

The first step in the computation is to shift the epoch from 1970-01-01 to 0000-03-01:

We'd simply need to find the offset for 1900-01-01, use it in the algorithm instead and adjust the UNIX_EPOCH constant accordingly.

@joboet

The source for the algorithm you cite already does a conversion from a timebase of 0000-03-01 to 1970-01-01 by adding/subtracting an offset from the day count.

The first step in the computation is to shift the epoch from 1970-01-01 to 0000-03-01:

We'd simply need to find the offset for 1900-01-01, use it in the algorithm instead and adjust the UNIX_EPOCH constant accordingly.

By my calculation (well, by inputting the time into the algorithm), the day offset is 693901 for 1900-01-01.

@Ayush1325

Use a time representation with 1900-01-01-00:00:00 as anchor. This is the earliest time supported in UEFI.

Signed-off-by: Ayush Singh ayush@beagleboard.org

@Ayush1325

The source for the algorithm you cite already does a conversion from a timebase of 0000-03-01 to 1970-01-01 by adding/subtracting an offset from the day count.

The first step in the computation is to shift the epoch from 1970-01-01 to 0000-03-01:

We'd simply need to find the offset for 1900-01-01, use it in the algorithm instead and adjust the UNIX_EPOCH constant accordingly.

By my calculation (well, by inputting the time into the algorithm), the day offset is 693901 for 1900-01-01.

I have adjusted both conversion offsets and run some initial tests with time anchored to 1900-01-01. Additionally, I have added checks to ensure that the results of calculations are representable in UEFI Time.

@Ayush1325

@beetrees

I don't think keeping track of the timezone in SystemTime is the right solution here. In particular (AFAICT), the FAT driver in EDK II ignores the timezone (FAT doesn't specify what timezone timestamps are stored in, so Windows and Linux interpret the file times in the current system timezone), meaning that after this PR, setting a file's modification time to two "equal" SystemTimes can result in a different actual timestamp being stored if the "equal" SystemTimes have different timezones. Instead, I think a better idea would be to always convert the timestamp to UTC before storing it in SystemTime, and:

@Ayush1325

I don't think keeping track of the timezone in SystemTime is the right solution here. In particular (AFAICT), the FAT driver in EDK II ignores the timezone (FAT doesn't specify what timezone timestamps are stored in, so Windows and Linux interpret the file times in the current system timezone), meaning that after this PR, setting a file's modification time to two "equal" SystemTimes can result in a different actual timestamp being stored if the "equal" SystemTimes have different timezones. Instead, I think a better idea would be to always convert the timestamp to UTC before storing it in SystemTime, and:

So, the timestamp in SystemTime::dur is UTC even right now. Timezone is only used for the following:

  1. Converting back to UEFI Time.
  2. Comparison between SystemTime. Basically, it is fine to compare times from unspecified timezones, and it is fine to compare between specified timezones, but comparison between unspecified timezone and specified timezone is not allowed.
* When fetching the current time, use UTC as the timezone if the timezone is unspecified. This is how EDK II handles it in RTC drivers AFAICT, so would be consistent with the rest of the ecosystem.

Well, that does explain the weird behavior I was observing on OVMF. I can see that I get unspecified timezone, even though I can see the timestamp is UTC.

Why does RuntimeServices->GetTime ever return unspecified time to begin with if unspecified is to be treated the same as UTC?

* When fetching a file time from the filesystem, use the timezone from the current time if the filesystem file time has an unspecified timezone. This ensures FAT timestamps are interpreted correctly.

* When storing a file time to the filesystem, use the timezone from the current time. This ensures FAT timestamps are correctly stored.

Do you have any idea what Project Mu does? If everyone disregards the spec, I guess we might as well. But we probably still need to check.

@beetrees

Why does RuntimeServices->GetTime ever return unspecified time to begin with if unspecified is to be treated the same as UTC?

Some RTCs always store the current time in UTC, and their drivers will adjust the returned time for the TimeZone if the TimeZone is not EFI_UNSPECIFIED_TIMEZONE, leading to UTC being returned when the time zone is unspecified in these cases. The unspecified time zone is used as the default time zone, so will occur when the time zone hasn't ever been set with SetTime (of course, if the time has never been set the actual time is unlikely to be correct either).

Do you have any idea what Project Mu does? If everyone disregards the spec, I guess we might as well. But we probably still need to check.

Project Mu is based on EDK II and it appears to have not changed how FAT timestamps are handled (it also seems to have the same RTC drivers more or less AFAICT).

Labels

O-UEFI

UEFI

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.