std: sys: pal: uefi: Overhaul Time

[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

[fbstj]

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?

[Ayush1325]

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]

@rustbot label +O-UEFI

[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:

z += 719468;

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:

z += 719468;

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]

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:

z += 719468;

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]

ping @joboet

[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:

• 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.
• 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.

[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).

[Ayush1325]

@beetrees Sorry for the late reply. Was busy with college stuff.

I have been pondering things a bit, and I am not sure why we need to do any modification to the time we get from UEFI in Rust. Let's look at Rust's SystemTime API:

Construct a new instance

The only way to do this, is SystemTime::now. One can then use calculations on this get a desired time, but there is no way to construct this structure any other way from user API, at least for now.

So any instance used in comparison and other things will also only come from the same way of construction.

Note: If someone calls SetTime, and goes from Unspecified to UTC or vice versa, and then tries to copare to previous time, then I personally would consider it a user error.

Internal Usage

There might be use cases where we do want to interpret SystemTime Timezone differently, but I feel like that should be on a case by case basis. The base SysteTime implementation should not make any assumption regarding this.

[beetrees]

Construct a new instance

The only way to do this, is SystemTime::now. One can then use calculations on this get a desired time, but there is no way to construct this structure any other way from user API, at least for now.

A SystemTime can also be constructed using SystemTime::UNIX_EPOCH. This allows converting between a Unix timestamp and SystemTime. For example, the Unix timestamp of a SystemTime can be obtained by calling .duration_since(SystemTime::UNIX_EPOCH).

[Ayush1325]

A SystemTime can also be constructed using SystemTime::UNIX_EPOCH. This allows converting between a Unix timestamp and SystemTime. For example, the Unix timestamp of a SystemTime can be obtained by calling .duration_since(SystemTime::UNIX_EPOCH).

Good point. I have updated the PR.