ImmutAfterInit: misuse runtime detection

[00xc]

• Use SyncUnsafeCell instead of UnsafeCell + unsafe impl Sync.
• Pass Copy types by value in ImmutAfterInitCell methods.
• Remove a double initialization before implementing runtime detection of this condition.
• Back ImmutAfterInitCell by an Option in order to be able to detect uninitialized cells.
• Return Result from public API calls to handle double initialization and uninitialized access.

[nicstange]

FWIW, I have a very similar, conflicting commit as your a7da78a in my pending ELF PR: e2acf29

The reasoning for having the initializer functions to take references rather than values had been to not rely on compiler optimizations too much when initializing large objects like the SnpCpuidTable blob. That is, my concern had been that temporaries on the stack could perhaps get created.

[00xc]

FWIW, I have a very similar, conflicting commit as your a7da78a in my pending ELF PR: e2acf29

The reasoning for having the initializer functions to take references rather than values had been to not rely on compiler optimizations too much when initializing large objects like the SnpCpuidTable blob. That is, my concern had been that temporaries on the stack could perhaps get created.

I see. I think this is a common pattern for Rust and should not be an issue, especially on release builds.

[00xc]

TODO for me:

• Rebase based on nicstange's changes, when/if they are merged (or at least based on e2acf29)
• Refactor to use an initialized flag instead of an Option. The main issue is that there are no guarantees on the layout of the Option, so, in debug builds, we cannot copy into it from a pointer without first copying to the stack, and then from the stack into the Option (via something like Option::replace()), which will be in static storage for regular users of the API.

[00xc]

I've simplified the PR:

• No longer uses SyncUnsafeCell (it introduces a dependency on a nightly feature).
• No longer takes advantage of the Copy trait (it is no longer used in the main branch, plus it proved to have some problems). Initialization is done by copying from a pointer as before.
• No longer uses an Option to track state, but an AtomicBool, since it implements Sync and has a simple API.

[nicstange]

I like this PR, but have three suggestions though. I don't want to delay the merge any further just because of nitpicking, so it might make sense to merge it as-is and do a follow-up patch -- up to Joerg to decide.

1.) You removed the #[repr(transparent)] from ImmutAfterInitCell. Have you tried whether #[cfg_attr(not(debug_assertions), repr(transparent))] would work? This way the ImmutAfterInitCell would remain to be guaranteed a zero-cost wrapper for non-debug builds.

2.) For not(debug_assertions), I would turn the new ImmutAfterInitError into a core::convert::Infallible. That would potentially enable the compiler to optimize the error checks at call sites away for non-debug builds. Not sure compilers are clever enough today already, but when Infallible is made the never-type (!) one day, they probably would.

3.) In my personal opinion, removing the unsafe marker from the fn init() can be debated about: from my POV it serves as an indicator to users that they need to think carefully when invoking it, i.e. that there is no possible code path where an ImmutAfterInitCell can get dereferenced while still in uninitialized state. Your patch set here certainly helps a lot in establishing confidence into that, but it still doesn't provide a hard guarantee, which can be obtained only by reviewing the code. Having the unsafe{} markers at the potentially problematic call sites, would at least help to draw reviewers' attention at the right spots. In either case, the move to drop unsafe from fn init() might perhaps have deserved a separate patch on its own. Just saying ;)

[00xc]

I like this PR, but have three suggestions though. I don't want to delay the merge any further just because of nitpicking, so it might make sense to merge it as-is and do a follow-up patch -- up to Joerg to decide.

1.) You removed the #[repr(transparent)] from ImmutAfterInitCell. Have you tried whether #[cfg_attr(not(debug_assertions), repr(transparent))] would work? This way the ImmutAfterInitCell would remain to be guaranteed a zero-cost wrapper for non-debug builds.

I thought about this, but I did not like the idea of having different layout guarantees for debug and release builds. Anyhow, for release builds the struct only has one field, I do not see how it can be a non-zero-cost wrapper.

2.) For not(debug_assertions), I would turn the new ImmutAfterInitError into a core::convert::Infallible. That would potentially enable the compiler to optimize the error checks at call sites away for non-debug builds. Not sure compilers are clever enough today already, but when Infallible is made the never-type (!) one day, they probably would.

Done in the updated PR.

3.) In my personal opinion, removing the unsafe marker from the fn init() can be debated about: from my POV it serves as an indicator to users that they need to think carefully when invoking it, i.e. that there is no possible code path where an ImmutAfterInitCell can get dereferenced while still in uninitialized state. Your patch set here certainly helps a lot in establishing confidence into that, but it still doesn't provide a hard guarantee, which can be obtained only by reviewing the code. Having the unsafe{} markers at the potentially problematic call sites, would at least help to draw reviewers' attention at the right spots.

I mean, technically the unsafe part is dereferencing the cell, not so much initializing it. I think having it being unsafe might raise awareness as you mention, but it also clutters the code in places where UB is really not possible. If we want to be extra safe we could add a dedicated method to access the cell contents and have that be unsafe.

[00xc]

@nicstange do you think we could also remove the T: Copy constraint? I don't think it is needed at all.

[00xc]

With 10b7610 we could perhaps have a slightly better initialization of the filesystem. Instead of:

svsm/src/fs/fs.rs

Lines 129 to 136 in 631ae3b

	static mut FS_ROOT: SvsmFs = SvsmFs::new();
	pub fn initialize_fs() {
	let root_dir = Arc::new(RamDirectory::new());
	unsafe {
	FS_ROOT.initialize(&root_dir);
	}
	}

We could remove SvsmFs::initialize() and have something like:

static FS_ROOT: ImmutAfterInitCell<SvsmFs> = ImmutAfterInitCell::uninit();

pub fn initialize_fs() { 
    let root_dir = Arc::new(RamDirectory::new()); 
    FS_ROOT.init(&root_dir);
}