impl Sync for FixedRegisters?

[ChocolateLoverRaj]

AcpiPlatform, which contains FixedRegisters, is marked as Sync. So is it safe to also mark FixedRegisters as safe? My use case is storing a Arc<FixedRegisters> in a global variable.

[martin-hughes]

Not by itself, because of the embedded NonNull via PhysicalMapping. To be honest, I think AcpiPlatform probably shouldn't currently be marked Sync.

I think if PhysicalMapping is marked Sync appropriately then the thread-safety flows through MappedGas and the register-block structs to FixedRegisters

But at present PhysicalMapping isn't really thread-safe because of the pub virtual_start: NonNull<T> member. Implementing Deref and DerefMut takes it most of the way there, it just needs a bit more work.

Does that make sense?

[martin-hughes]

Any other questions @ChocolateLoverRaj or happy to close?

[ChocolateLoverRaj]

To be honest, I think AcpiPlatform probably shouldn't currently be marked Sync.

I guess what I'm asking for is a re-evaluation of what should and shouldn't be marked as Send and Sync, and how operating systems can safely store an AML interpreter globally.

[martin-hughes]

Fair question. At the moment, I guess it's mostly a question of your risk appetite! You could newtype unsafe impl onto stuff and probably be fine.

Clearly though, we do want to make it actually fine.

tl;dr: The two issues are PhysicalMapping and WrappedObject.

The following is based on my existing explorations of the code and a git grep "unsafe impl":

I think the main blocker at the moment is PhysicalMapping, see #345 for a little bit of improvement. Assuming it gets merged, I think we should be closer to making PhysicalMapping appropriately Send / Sync, depending on the underlying type. I don't think that'll be too much work to finish it 🤞

AcpiTables has - I assume - the unsafe impl for Send and Sync because of the use of PhysicalMapping. Once PhysicalMapping is fixed, I think AcpiTables follows.

AcpiPlatform relies on PhysicalMapping through FixedRegisters and AcpiTables. I think that's the only dependency.

Interpreter has the lovely comment that I added not so long ago: "// TODO: Make sure to remove these two lines after Interpreter really is Send + Sync." It's most of the way there. Again, it needs PhysicalMapping to be resolved.

In other words, to fix PhysicalMapping means we can accurately describe Send-ness and Sync-ness for all the other types.

However...

There's also the question of WrappedObject. This presents a Send + Sync interface, but it's very easy to use in an unsafe way - gain_mut relies on a token, but does not protect against any number of non-mutable references existing at the same time.

Currently, we rely on correct usage of gain_mut in the interpreter to ensure WrappedObject is safe. I don't think you could gain a mutable reference outside of the crate, but clearly you could store normal references. Not ideal.

I proposed a solution in #307 (to use RW locks), but it has two issues

• It makes the interpreter code a lot worse to read
• We haven't been able to test the performance impact.

I and Isaac are both open to ideas about anything I've written here. Hope it answers your question.

[martin-hughes]

* We haven't been able to test the performance impact.

By which I mean - we haven't got around to testing the performance impact, not that it's impossible to test 😆

[martin-hughes]

I'm working on fixing the PhysicalMapping send-ness, which should unblock "how operating systems can safely store an AML interpreter globally". WrappedObject might need to wait for another day.