impl Sync for FixedRegisters? #324

Activity

1. martin-hughes commented on Aug 27, 2026

   

   martin-hughes

   on Aug 27, 2026

   Contributor

   More actions

   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?
2. martin-hughes commented on Sep 9, 2026

   

   martin-hughes

   on Sep 9, 2026

   Contributor

   More actions

   Any other questions @ChocolateLoverRaj or happy to close?
3. ChocolateLoverRaj commented on Sep 16, 2026

   

   ChocolateLoverRaj

   on Sep 16, 2026

   ContributorAuthor

   More actions

   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.
4. martin-hughes commented on Sep 16, 2026

   

   martin-hughes

   on Sep 16, 2026

   Contributor

   More actions

   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.
5. martin-hughes commented on Sep 16, 2026

   

   martin-hughes

   on Sep 16, 2026

   Contributor

   More actions

   * 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 😆
6. martin-hughes

   self-assigned this

   on Sep 20, 2026
7. martin-hughes commented on Sep 20, 2026

   

   martin-hughes

   on Sep 20, 2026

   Contributor

   More actions

   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.
8. martin-hughes commented on Sep 22, 2026

   

   martin-hughes

   on Sep 22, 2026

   Contributor

   More actions

   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.

   It may be that the only way around this is to make NativeMethod have a Send bound on it.

   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!
9. IsaacWoods commented on Sep 24, 2026

   

   IsaacWoods

   on Sep 24, 2026

   Member

   More actions

   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 NativeMethod is to implement _OSI I 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 the dyn Fn within an Arc, and so I guess it will need to be bounded Send + Sync for the Arc (and therefore Object) to be Send?

   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 Object and WrappedObject would be rather unwieldy...
10. martin-hughes commented on Sep 24, 2026

    

    martin-hughes

    on Sep 24, 2026

    Contributor

    More actions

    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 NativeMethod to 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.

    👍React with 👍1Reacted by Isaac Woods
11. martin-hughes commented on Oct 2, 2026

    

    martin-hughes

    on Oct 2, 2026

    Contributor

    More actions

    OK, I've opened #366 regarding the WrappedObject stuff, PR #361 remains for this issue.
12. IsaacWoods

    closed this as completedin #361on Oct 4, 2026