feat: Don't take explicit references to Handler

[mkroening]

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 Handler and PhysicalMapping always requires that H always be present by value, which is cloned from &H. This PR:

1. Makes Handler::unmap_physical_region unsafe. Since physical mappings can be forged without restrictions, this method must be unsafe.
2. Extracts RawPhysicalMapping. This is akin to a raw pointer, which is independent of the allocator (handler). PhysicalMapping stays the equivalent of a Box.
3. Makes 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.
4. Makes Handler::unmap_physical_region() take &self and raw physical mappings. That incidentally also makes Handler dyn-compatible, which is nice.
5. Adds a forwarding 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 contain Self, hence the introduction of raw physical mappings.
6. Updates APIs to take handlers by value. I hope I got all relevant ones.

Essentially, Handler is remodeled after Rust's Allocator and Box types. This allows users to create physical mappings without cloning by supplying &Handler if 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:

• feat: don't take explicit references to ConfigRegionAccess pci_types#22
• fix: don't take explicit reference in update_interrupt pci_types#24

[martin-hughes]

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:

1. Could MappedGas use RawPhysicalMapping, since it already holds a handler? Perhaps that's an optimisation too far though.
2. I think there's a safety improvement to be had if RawPhysicalMapping was less forgeable. Do the various fields need to be pub or could they be replaced with get_[x]()? The NonNull could also have get_mut(&mut self)
   • This isn't new because of your PR - but since I was looking at your PR it made me think of it, and I'm curious if you thought about it

Edit: Forgot that PhysicalMapping has impl DerefMut that covers the struck out bit already

[IsaacWoods]

Yeah I think this is reasonable! Thanks @mkroening!

I think there's a safety improvement to be had if RawPhysicalMapping was less forgeable.

I think this could well be an improvement.

I also wonder if we should split PhysicalMapping and Handler back out into a separate handler module at this point - re-exports from the root again. That file is quite large now. We can do after this is merged, maybe with the above.

Some conflicts I'm afraid, otherwise will merge :)

[mkroening]

Sorry for the wait! Rebase is done now. :)

• Could MappedGas use RawPhysicalMapping, since it already holds a handler? Perhaps that's an optimisation too far though.

• I think there's a safety improvement to be had if RawPhysicalMapping was less forgeable. Do the various fields need to be pub or could they be replaced with get_[x]()? The NonNull could also have get_mut(&mut self)

  • This isn't new because of your PR - but since I was looking at your PR it made me think of it, and I'm curious if you thought about it

Both make sense to me. 👍
I also thought about the second one but thought it might be better done in a separate PR.

I also wonder if we should split PhysicalMapping and Handler back out into a separate handler module at this point - re-exports from the root again. That file is quite large now. We can do after this is merged, maybe with the above.

Makes sense to me. 👍

[martin-hughes]

I'm afraid I've been inspired by some of your other recent PRs... to point out there's a few clippy warnings:

(Summarised)

warning: non-canonical implementation of `clone` on a `Copy` type
   --> src\lib.rs:371:29

warning: unsafe function's docs are missing a `# Safety` section
   --> src\lib.rs:400:5

warning: `?Sized` bound is ignored because of a `Sized` requirement
   --> src\lib.rs:591:19

Plus some related others

But otherwise from a glance this looks good, thanks again! If Isaac doesn't get to it first, I'll check it over again in a day or two.

[mkroening]

Ah, thanks for pointing them out! I am used to the CI telling me that I should fix something. I resolved the issues now. :)