Repository navigation
Make PhysicalMapping safely Send / Sync - #361
Conversation
This is a prerequisite to removing the `pub` modifiers. Without moving these structs, the fields are visible to the whole crate, which removes much of the benefit of making the interface safer.
|
Definitely should have tested this after rebasing onto the newest |
The internals of `RawPhysicalMapping` and `PhysicalMapping` have been hidden, and can now only ba accessed via accessors. This is largely for the purpose of hiding `virtual_start`, to allow for an interface that can be made Send & Sync. The other fields are hidden for consistency.
d9ce717 to
fc32783
Compare
martin-hughes
changed the title
|
I've had a think about it, and decided that the Ready for review. |
|
|
||
| // If all other types are correctly labelled with Send and/or Sync, then Interpreter should | ||
| // naturally become Send/Sync. | ||
| assert_impl_all!(Interpreter<NullHandler>: Send, Sync); |
There was a problem hiding this comment.
Probably superfluous given that we've added unsafe impls, but I've left it in as a statement of intent / so it won't be forgotten.
IsaacWoods
left a comment
IsaacWoods
left a comment
There was a problem hiding this comment.
Apologies - realised I'd left this as a pending review for a few days! Just some thoughts from my end - thanks for working on this!
| }; | ||
| use core::{mem, ptr}; | ||
| use log::warn; | ||
| pub use physical_mapping::{PhysicalMapping, RawPhysicalMapping}; |
There was a problem hiding this comment.
nit: could we group pub uses together separately to use
| Self { physical_start, virtual_start, region_length, mapped_length } | ||
| } | ||
|
|
||
| pub fn get_physical_start(&self) -> usize { |
There was a problem hiding this comment.
nit: applies throughout - just as convention, getters don't need get (e.g. this should just be fn physical_start)
| let stream = unsafe { | ||
| slice::from_raw_parts( | ||
| mapping.raw.virtual_start.as_ptr().byte_add(mem::size_of::<SdtHeader>()) as *const u8, | ||
| ptr::from_ref(&*mapping).byte_add(size_of::<SdtHeader>()) as *const u8, |
There was a problem hiding this comment.
Applies throughout: I get why the pattern here works (Deref to create a reference to the underlying and then turn that into a pointer) but I wonder if unsafe helpers to create pointers directly as_ptr and as_mut would be clearer?
There was a problem hiding this comment.
Perhaps as_mut_ptr to distinguish it from the existing as_mut from trait AsMut that takes &mut self and returns a mut ref?
(Agree otherwise though, seems sensible)
| // SAFETY: We would really prefer to have a mut ref to mapping here, but that | ||
| // requires `&mut self`. | ||
| // | ||
| // We rely on the write being to memory outside the Rust allocation system in order |
There was a problem hiding this comment.
If we create helpers as the above, this could move to there as a safety requirement on the caller?
* Remove `get_` from names * Add `as_ptr` and `as_mut_ptr` helpers to simplify some use cases.
|
No worries at all, thanks for the comments - all sensible and addressed, with the exception that I created |
|
Lovely, thanks :) At some point we should probably think a little more about provenance (addresses from 'outside' Rust's memory model should likely have their provenance exposed, the rest should likely use the strict provenance APIs a little more etc., but this is obviously a nice improvement in the meantime. |
True - that would be nice to get properly correct! I guess there's scope for a "things to think about" / todo list - perhaps the first wiki page? |
This makes Object Send + Sync, and by extension WrappedObject (although gain_mut is still a potential footgun). If rust-osdev#361 is merged as well, then Interpreter becomes Send + Sync without needing an `unsafe impl` block.
Updates since rust-osdev#361 got merged
2 participants
The aim of this PR is to close #324 by getting the correct Send/Sync annotations on relevant types (such as
PhysicalMappingandWrappedObject).This is a work-in-progress. The outstanding work is to figure out what to do with
NativeMethod.As mentioned in #324,
PhysicalMappingpresented an interface that was not truly Send or Sync due to the public access to thevirtual_start: NonNull<T>.I've made it correct by hiding that field (* although access is still allowed if needed through an
unsafefunction!) / only allowing access through the deref methods, and I've increased the difficulty of misusingPhysicalMappingandRawPhysicalMappingby making their fields be private.That would fix @ChocolateLoverRaj's initial request in #324 to have
SyncforFixedRegisters.If
WrappedObjectisSend + Syncthen this would allowInterpreterto truly beSend + Sync, thus fixing @ChocolateLoverRaj's request to be able to store Interpreter globally. However, viaObjectandNativeMethod,WrappedObjectis notSend + Sync.In this PR I've marked it as though it is - it tries to be, but isn't. For this PR to be ready, those lines really need removing.
Happy to take thoughts/comments from anyone interested: @ChocolateLoverRaj, @IsaacWoods and anyone else.