Skip to content

uefi: fs: Add file times plumbing #138918

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

uefi: fs: Add file times plumbing #138918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 37 additions & 7 deletions library/std/src/sys/fs/uefi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::hash::Hash;
use crate::io::{self, BorrowedCursor, IoSlice, IoSliceMut, SeekFrom};
use crate::path::{Path, PathBuf};
use crate::sys::time::SystemTime;
use crate::sys::unsupported;
use crate::sys::{unsupported, unsupported_err};

#[expect(dead_code)]
const FILE_PERMISSIONS_MASK: u64 = r_efi::protocols::file::READ_ONLY;
Expand All @@ -18,6 +18,7 @@ pub struct File(!);
pub struct FileAttr {
attr: u64,
size: u64,
times: FileTimes,
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, UEFI's EFI_FILE_INFO always contains the file times, right? Then this shouldn't be FileTimes, but rather unconditionally contain the SystemTimes. That also gets rid of the weird unsupported error in the accessors below.

}

pub struct ReadDir(!);
Expand All @@ -33,7 +34,11 @@ pub struct OpenOptions {
}

#[derive(Copy, Clone, Debug, Default)]
pub struct FileTimes {}
pub struct FileTimes {
accessed: Option<SystemTime>,
modified: Option<SystemTime>,
created: Option<SystemTime>,
}

#[derive(Clone, PartialEq, Eq, Debug)]
// Bool indicates if file is readonly
Expand All @@ -60,15 +65,15 @@ impl FileAttr {
}

pub fn modified(&self) -> io::Result<SystemTime> {
Copy link

Choose a reason for hiding this comment

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

is returning a Result here enforced by higher-level APIs in Rust STD? If not, why not return Option<SystemTime>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it is.

unsupported()
self.times.modified.ok_or(unsupported_err())
}

pub fn accessed(&self) -> io::Result<SystemTime> {
unsupported()
self.times.accessed.ok_or(unsupported_err())
}

pub fn created(&self) -> io::Result<SystemTime> {
unsupported()
self.times.created.ok_or(unsupported_err())
}
}

Expand All @@ -92,8 +97,13 @@ impl FilePermissions {
}

impl FileTimes {
pub fn set_accessed(&mut self, _t: SystemTime) {}
pub fn set_modified(&mut self, _t: SystemTime) {}
pub fn set_accessed(&mut self, t: SystemTime) {
self.accessed = Some(t);
}

pub fn set_modified(&mut self, t: SystemTime) {
self.modified = Some(t);
}
}

impl FileType {
Expand Down Expand Up @@ -386,6 +396,7 @@ mod uefi_fs {
use crate::path::Path;
use crate::ptr::NonNull;
use crate::sys::helpers;
use crate::sys::time::{self, SystemTime};

pub(crate) struct File(NonNull<file::Protocol>);

Expand Down Expand Up @@ -533,4 +544,23 @@ mod uefi_fs {

Ok(())
}

/// EDK2 FAT driver uses EFI_UNSPECIFIED_TIMEZONE to represent localtime. So for proper
/// conversion to SystemTime, we use the current time to get the timezone in such cases.
#[expect(dead_code)]
Copy link

Choose a reason for hiding this comment

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

why dead code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, just to keep the PRs small to review, and maintain continuity in future PRs. Any interactions with these conversions will require implementing GetInfo, which uses a C DST (and hence would require care and proper review) and thus needs to be a separate PR.

Also, the to and fro conversion are strongly related. So best to present them in the same PR.

It might be 2026 by the time that I start upstreaming code that requires conversion from UEFI to systemtime. So I would probably forget a lot of the important information in the current discussion. Having a conversion function already present saves having to repeat the same discussion (or worse, letting wrong code be merged).

fn uefi_to_systemtime(mut time: r_efi::efi::Time) -> SystemTime {
time.timezone = if time.timezone == r_efi::efi::UNSPECIFIED_TIMEZONE {
time::system_time_internal::now().unwrap().timezone
} else {
time.timezone
};
SystemTime::from_uefi(time)
}

/// Convert to UEFI Time with the current timezone.
#[expect(dead_code)]
fn systemtime_to_uefi(time: SystemTime) -> r_efi::efi::Time {
let now = time::system_time_internal::now().unwrap();
time.to_uefi_loose(now.timezone, now.daylight)
}
}
58 changes: 54 additions & 4 deletions library/std/src/sys/pal/uefi/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,20 @@ use crate::time::Duration;

const SECS_IN_MINUTE: u64 = 60;

const MAX_UEFI_TIME: Duration = from_uefi(r_efi::efi::Time {
year: 9999,
month: 12,
day: 31,
hour: 23,
minute: 59,
second: 59,
nanosecond: 999_999_999,
timezone: 1440,
daylight: 0,
pad1: 0,
pad2: 0,
});

#[test]
fn align() {
// UEFI ABI specifies that allocation alignment minimum is always 8. So this can be
Expand Down Expand Up @@ -44,7 +58,7 @@ fn systemtime_start() {
};
assert_eq!(from_uefi(&t), Duration::new(0, 0));
assert_eq!(t, to_uefi(&from_uefi(&t), -1440, 0).unwrap());
assert!(to_uefi(&from_uefi(&t), 0, 0).is_none());
assert!(to_uefi(&from_uefi(&t), 0, 0).is_err());
}

#[test]
Expand All @@ -64,7 +78,7 @@ fn systemtime_utc_start() {
};
assert_eq!(from_uefi(&t), Duration::new(1440 * SECS_IN_MINUTE, 0));
assert_eq!(t, to_uefi(&from_uefi(&t), 0, 0).unwrap());
assert!(to_uefi(&from_uefi(&t), -1440, 0).is_some());
assert!(to_uefi(&from_uefi(&t), -1440, 0).is_err());
Copy link
Member

Choose a reason for hiding this comment

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

Something is wrong in the code, this should succeed (localtime = UTC - timezone, so the local time is after the 1st of january 1900).

}

#[test]
Expand All @@ -82,8 +96,44 @@ fn systemtime_end() {
daylight: 0,
pad2: 0,
};
assert!(to_uefi(&from_uefi(&t), 1440, 0).is_some());
assert!(to_uefi(&from_uefi(&t), 1439, 0).is_none());
assert!(to_uefi(&from_uefi(&t), 1440, 0).is_ok());
assert!(to_uefi(&from_uefi(&t), 1439, 0).is_err());
}

#[test]
fn min_time() {
let inp = Duration::from_secs(1440 * SECS_IN_MINUTE);
let new_tz = to_uefi(&inp, 1440, 0).err().unwrap();
assert_eq!(new_tz, 0);
assert!(to_uefi(&inp, new_tz, 0).is_ok());

let inp = Duration::from_secs(1450 * SECS_IN_MINUTE);
let new_tz = to_uefi(&inp, 1440, 0).err().unwrap();
assert_eq!(new_tz, 10);
assert!(to_uefi(&inp, new_tz, 0).is_ok());

let inp = Duration::from_secs(1450 * SECS_IN_MINUTE + 10);
let new_tz = to_uefi(&inp, 1440, 0).err().unwrap();
assert_eq!(new_tz, 9);
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect, if 1450 * SECS_IN_MINUTE is representable in timezone 10, then times after that should be too, right?

assert!(to_uefi(&inp, new_tz, 0).is_ok());
}

#[test]
fn max_time() {
let inp = MAX_UEFI_TIME;
let new_tz = to_uefi(&inp, -1440, 0).err().unwrap();
assert_eq!(new_tz, 1440);
assert!(to_uefi(&inp, new_tz, 0).is_ok());

let inp = MAX_UEFI_TIME - Duration::from_secs(1440 * SECS_IN_MINUTE);
let new_tz = to_uefi(&inp, -1440, 0).err().unwrap();
assert_eq!(new_tz, 0);
assert!(to_uefi(&inp, new_tz, 0).is_ok());

let inp = MAX_UEFI_TIME - Duration::from_secs(1440 * SECS_IN_MINUTE + 10);
let new_tz = to_uefi(&inp, -1440, 0).err().unwrap();
assert_eq!(new_tz, 0);
assert!(to_uefi(&inp, new_tz, 0).is_ok());
}

// UEFI IoSlice and IoSliceMut Tests
Expand Down
72 changes: 56 additions & 16 deletions library/std/src/sys/pal/uefi/time.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use crate::time::Duration;

const SECS_IN_MINUTE: u64 = 60;

#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
pub struct Instant(Duration);

Expand Down Expand Up @@ -70,13 +72,32 @@ impl SystemTime {
Self(system_time_internal::from_uefi(&t))
}

#[expect(dead_code)]
pub(crate) const fn to_uefi(self, timezone: i16, daylight: u8) -> Option<r_efi::efi::Time> {
system_time_internal::to_uefi(&self.0, timezone, daylight)
pub(crate) const fn to_uefi(
self,
timezone: i16,
daylight: u8,
) -> Result<r_efi::efi::Time, i16> {
// system_time_internal::to_uefi requires a valid timezone. In case of unspecified timezone,
// we just pass 0 since it is assumed that no timezone related adjustments are required.
if timezone == r_efi::efi::UNSPECIFIED_TIMEZONE {
system_time_internal::to_uefi(&self.0, 0, daylight)
} else {
system_time_internal::to_uefi(&self.0, timezone, daylight)
}
}

/// Create UEFI Time with the closest timezone (minute offset) that still allows the time to be
/// represented.
pub(crate) fn to_uefi_loose(self, timezone: i16, daylight: u8) -> r_efi::efi::Time {
match self.to_uefi(timezone, daylight) {
Ok(x) => x,
Err(tz) => self.to_uefi(tz, daylight).unwrap(),
}
}

pub fn now() -> SystemTime {
system_time_internal::now()
.map(Self::from_uefi)
.unwrap_or_else(|| panic!("time not implemented on this platform"))
}

Expand Down Expand Up @@ -104,12 +125,11 @@ pub(crate) mod system_time_internal {
use crate::mem::MaybeUninit;
use crate::ptr::NonNull;

const SECS_IN_MINUTE: u64 = 60;
const SECS_IN_HOUR: u64 = SECS_IN_MINUTE * 60;
const SECS_IN_DAY: u64 = SECS_IN_HOUR * 24;
const TIMEZONE_DELTA: u64 = 1440 * SECS_IN_MINUTE;
const SYSTEMTIME_TIMEZONE: u64 = 1440 * SECS_IN_MINUTE;
Copy link
Member

Choose a reason for hiding this comment

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

The timezone for SystemTime is -1440, since that is the timezone with the earliest representable value. So the sign of this value should be flipped (as well as the sign of the additions/subtractions you use it for).


pub fn now() -> Option<SystemTime> {
pub(crate) fn now() -> Option<Time> {
let runtime_services: NonNull<RuntimeServices> = helpers::runtime_services()?;
let mut t: MaybeUninit<Time> = MaybeUninit::uninit();
let r = unsafe {
Expand All @@ -119,9 +139,7 @@ pub(crate) mod system_time_internal {
return None;
}

let t = unsafe { t.assume_init() };

Some(SystemTime::from_uefi(t))
Some(unsafe { t.assume_init() })
}

/// This algorithm is a modified form of the one described in the post
Expand Down Expand Up @@ -162,7 +180,7 @@ pub(crate) mod system_time_internal {
+ (t.hour as u64) * SECS_IN_HOUR;

// Calculate the offset from 1/1/1900 at timezone -1440 min
let adjusted_localtime_epoc: u64 = localtime_epoch + TIMEZONE_DELTA;
let adjusted_localtime_epoc: u64 = localtime_epoch + SYSTEMTIME_TIMEZONE;

let epoch: u64 = if t.timezone == r_efi::efi::UNSPECIFIED_TIMEZONE {
adjusted_localtime_epoc
Expand All @@ -180,16 +198,27 @@ pub(crate) mod system_time_internal {
///
/// The changes are to use 1900-01-01-00:00:00 with timezone -1440 as anchor instead of UNIX
/// epoch used in the original algorithm.
pub(crate) const fn to_uefi(dur: &Duration, timezone: i16, daylight: u8) -> Option<Time> {
pub(crate) const fn to_uefi(dur: &Duration, timezone: i16, daylight: u8) -> Result<Time, i16> {
const MIN_IN_HOUR: u64 = 60;
const MIN_IN_DAY: u64 = MIN_IN_HOUR * 24;

// Check timzone validity
assert!(timezone <= 1440 && timezone >= -1440);

// FIXME(#126043): use checked_sub_signed once stabilized
// This cannot fail for valid SystemTime due to SYSTEMTIME_TIMEZONE
let secs =
dur.as_secs().checked_add_signed((-timezone as i64) * SECS_IN_MINUTE as i64).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This is the wrong sign – you need to add the new timezone offset. Also, this will panic for positive timezones when the duration is very small – the overflow check should occur below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The UEFI fields are in locatime. And it is given in the spec that LocalTime = UTC - Timezone. When converting from UEFI to our time in secs from epoch, we do LocalTime + Timezone since we want to have the secs when time is in UTC.
So to get back localtime, wouldn't we do UTC secs - Timezone?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I got confused because RFC 3339 uses the exact opposite sign convention. You are indeed correct.

Copy link
Member

Choose a reason for hiding this comment

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

This will still panic for Duration::ZERO in e.g. timezone 1440 though.


// Convert to seconds since 1900-01-01-00:00:00 in timezone.
let Some(secs) = secs.checked_sub(TIMEZONE_DELTA) else { return None };
let Some(secs) = secs.checked_sub(SYSTEMTIME_TIMEZONE) else {
// If the current timezone cannot be used, find the closest timezone that will allow the
// conversion to succeed.
let delta = SYSTEMTIME_TIMEZONE - secs;
let new_tz = timezone
- (delta / SECS_IN_MINUTE + if delta % SECS_IN_MINUTE == 0 { 0 } else { 1 }) as i16;
return Err(new_tz);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a usecase for to_uefi outside of to_uefi_loose? Otherwise you could update the timezone here instead of returning an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I am not sure how to deal with the other case (end of re-presentable time) early. So not sure if there is much benefit if I need to do a recursive call at the end.

I can think about optimizing it more over the weekend if that is required. But it is possible that having this strict version of the function will be required when adding something in the future anyway.

};

let days = secs / SECS_IN_DAY;
let remaining_secs = secs % SECS_IN_DAY;
Expand All @@ -212,9 +241,10 @@ pub(crate) mod system_time_internal {
let minute = ((remaining_secs % SECS_IN_HOUR) / SECS_IN_MINUTE) as u8;
let second = (remaining_secs % SECS_IN_MINUTE) as u8;

// Check Bounds
if y >= 1900 && y <= 9999 {
Some(Time {
// At this point, invalid time will be greater than MAX representable time. It cannot be less
// than minimum time since we already take care of that case above.
if y <= 9999 {
Ok(Time {
year: y as u16,
month: m as u8,
day: d as u8,
Expand All @@ -228,7 +258,17 @@ pub(crate) mod system_time_internal {
pad2: 0,
})
} else {
None
assert!(y == 10000);
assert!(m == 1);

let delta = ((d - 1) as u64 * MIN_IN_DAY
+ hour as u64 * MIN_IN_HOUR
+ minute as u64
+ if second == 0 { 0 } else { 1 }) as i16;
let new_tz = timezone + delta;

assert!(new_tz <= 1440 && new_tz >= -1440);
Err(new_tz)
}
}
}
Expand Down
Loading