feat: Don't take explicit references to Handler - #345
Conversation
82a4c1e to
0092a0f
Compare
Since physical mappings can be forged without restrictions, this method must be unsafe.
0092a0f to
08a8bfc
Compare
|
I like the idea here, but I will defer to @IsaacWoods on whether he's happy with the design. (Isaac, up to you if you want to do the full review as well, or I can do) A couple of thoughts sprung to mind as I was looking at this - but they might be sensible to leave for a future PR:
Edit: Forgot that |
|
Yeah I think this is reasonable! Thanks @mkroening!
I think this could well be an improvement. I also wonder if we should split Some conflicts I'm afraid, otherwise will merge :) |
This PR is rather large and best reviewed commit by commit. Please let me know if I should split them up or explain the changes better. :)
The current design of
HandlerandPhysicalMappingalways requires thatHalways be present by value, which is cloned from&H. This PR:Handler::unmap_physical_regionunsafe. Since physical mappings can be forged without restrictions, this method must be unsafe.RawPhysicalMapping. This is akin to a raw pointer, which is independent of the allocator (handler).PhysicalMappingstays the equivalent of aBox.Handler::map_physical_region()return raw physical mappings. That way, the handler can be provided by value by the caller instead of having to clone from a reference.Handler::unmap_physical_region()take&selfand raw physical mappings. That incidentally also makes Handler dyn-compatible, which is nice.impl Handler for &H. This is the core of the PR and the motivation for the previous commits. This requires that returned types do not containSelf, hence the introduction of raw physical mappings.Essentially,
Handleris remodeled after Rust'sAllocatorandBoxtypes. This allows users to create physical mappings without cloning by supplying&Handlerif necessary. Cloning shared references is just a copy.My motivation is not strong with this one. I just saw this and thought this might be nicer, without strictly needing these changes. The old design works too, though. Please let me know what you think. :)
This PR is similar to the following pci_types PRs:
ConfigRegionAccesspci_types#22update_interruptpci_types#24