Repository navigation
impl Sync for FixedRegisters? #324
Description
Activity
Not by itself, because of the embedded
NonNullviaPhysicalMapping. To be honest, I thinkAcpiPlatformprobably shouldn't currently be markedSync.I think if
PhysicalMappingis markedSyncappropriately then the thread-safety flows throughMappedGasand the register-block structs toFixedRegistersBut at present
PhysicalMappingisn't really thread-safe because of thepub virtual_start: NonNull<T>member. ImplementingDerefandDerefMuttakes it most of the way there, it just needs a bit more work.Does that make sense?
Any other questions @ChocolateLoverRaj or happy to close?
To be honest, I think
AcpiPlatformprobably shouldn't currently be markedSync.I guess what I'm asking for is a re-evaluation of what should and shouldn't be marked as
SendandSync, and how operating systems can safely store an AML interpreter globally.Fair question. At the moment, I guess it's mostly a question of your risk appetite! You could newtype
unsafe implonto stuff and probably be fine.Clearly though, we do want to make it actually fine.
tl;dr: The two issues are
PhysicalMappingandWrappedObject.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 makingPhysicalMappingappropriately Send / Sync, depending on the underlying type. I don't think that'll be too much work to finish it 🤞AcpiTableshas - I assume - theunsafe implfor Send and Sync because of the use ofPhysicalMapping. OncePhysicalMappingis fixed, I thinkAcpiTablesfollows.AcpiPlatformrelies onPhysicalMappingthroughFixedRegistersandAcpiTables. I think that's the only dependency.Interpreterhas 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 needsPhysicalMappingto be resolved.In other words, to fix
PhysicalMappingmeans 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_mutrelies 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_mutin the interpreter to ensureWrappedObjectis 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.
* 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 😆
I'm working on fixing the
PhysicalMappingsend-ness, which should unblock "how operating systems can safely store an AML interpreter globally".WrappedObjectmight need to wait for another day.One thing I hadn't considered but which has come up:
NativeMethod. This currently imposes no bounds, so can't be said to beSend. As suchObjectcan't beSend, so nor canWrappedObject.It may be that the only way around this is to make
NativeMethodhave aSendbound on it.Ideally we could have a non-Send
NativeMethodforBaseInterpreterand only apply theSendbound when used withInterpreter(which isSend)... but I'm not sure if that's really possible!One thing I hadn't considered but which has come up: NativeMethod. This currently imposes no bounds, so can't be said to be Send. As such Object can't be Send, so nor can WrappedObject
Hm, good point. The only current use of
NativeMethodis to implement_OSII believe (which is pure) but there is obviously no intention to limit it to this; likely I didn't think about the need for that at the time. We store thedyn Fnwithin anArc, and so I guess it will need to be boundedSend + Syncfor theArc(and thereforeObject) to beSend?Ideally we could have a non-Send NativeMethod for BaseInterpreter and only apply the Send bound when used with Interpreter (which is Send)... but I'm not sure if that's really possible!
I think the bounds this would introduce on
ObjectandWrappedObjectwould be rather unwieldy...I guess it will need to be bounded Send + Sync for the Arc (and therefore Object) to be Send?
I think so, yes
I think the bounds this would introduce on Object and WrappedObject would be rather unwieldy...
Classic understatement I think 😉
I suppose it's relatively unlikely that firmware would call a native method other than the few in the spec - how would it know such a method existed? Any exception would need to have a pretty tight coupling between the firmware and the OSPM side, although I suppose on Windows you could use the WPBT table to do help with that.
I guess the question is, is constraining
NativeMethodto Send + Sync OK, given we offer a non-Send interpreter? I think in most cases it is, and we can always work on the horrendous constraints needed to relax that another time.Reacted by Isaac Woods
AcpiPlatform, which contains FixedRegisters, is marked as
Sync. So is it safe to also mark FixedRegisters as safe? My use case is storing aArc<FixedRegisters>in a global variable.