Fix physical mapping soundness

[Freax13]

This pr makes PhysicalMapping sound:

• Previously one could safely construct a PhysicalMapping with virtual_start set to an invalid pointer and dereference it. This pr adds PhysicalMapping::new which is unsafe.
• AcpiHandler::unmap_physical_region no longer takes self. Implementations should use PhysicalMapping::handler to get the correct mapper. While it wasn't necessarily unsound to call unmap_physical_region on another PhysicalMapper removing self makes it's easier to implement unmap_physical_region safely.

This is a breaking change.

[IsaacWoods]

Previously one could safely construct a PhysicalMapping with virtual_start set to an invalid pointer and dereference it.

Ouch - this is an embarrassing oversight on my part. My thought process was that the library only accepted PhysicalMappings from AcpiHandler::map_physical_region, which is unsafe, and so from acpi's perspective, the guarantee that the pointer has been correctly constructed is on that unsafe usage. I stand by that from acpi's perspective, but that glosses over the fact that we're exposing an unsound PhysicalMapping type, which can easily be safe in its own right.

So while breaking, I think I'm for the first change.

Implementing unmap_physical_region in terms of the mapping's handler is also pretty clever, but non-obvious without context - we should at the very least mention that this is how you should access the handler in the documentation of unmap_physical_mapping, as well as the reasoning for doing it like this. It does seem pretty unlikely for a bug to occur here - a user would need to misunderstand and think they needed to call unmap_physical_mapping manually (maybe we should mention more explicitely in the docs that it is called on drop), then also have multiple instances of the same type of AcpiHandler (which is unlikely), and then finally pass the wrong one when unmapping a mapping. I'm unsure this is a good tradeoff - what do you think?

[IsaacWoods]

Could you format this in a bullet list like above?

[IsaacWoods]

I think we should either add some docs here about how to access the handler and why it works like that, or just that you should not call this yourself, because it is called automatically when a mapping is dropped.

[Freax13]

It does seem pretty unlikely for a bug to occur here - a user would need to misunderstand and think they needed to call unmap_physical_mapping manually (maybe we should mention more explicitely in the docs that it is called on drop), then also have multiple instances of the same type of AcpiHandler (which is unlikely), and then finally pass the wrong one when unmapping a mapping. I'm unsure this is a good tradeoff - what do you think?

I agree that it's very unlikely that a user would use this api wrongly, however as someone implementing AcpiHandler I don't want to think on how to handle it if it would happen. I could an assert to check if both handlers are the same or we could just get rid of the parameter. That was my thought process behind this change.

I agree that the docs should be improved - what do you think of the following docs?

/// Unmap the given physical mapping. This is called when a `PhysicalMapping` is dropped, you should **not** manually call this.
///
/// Note: A reference to the handler used to construct `region` can be acquired by calling [`PhysicalMapping::handler`].
fn unmap_physical_region<T>(region: &PhysicalMapping<Self, T>);

Ultimately, if you want me to undo the second change, I'd understand, it's a bit weird in that it makes implementing the trait slightly easier though slightly more confusing at first.

[IsaacWoods]

I think saying "must only be called by an AcpiHandler of type H to..." would be clearer

[IsaacWoods]

Hm, I think I'm convinced. With the doc changes (those are great, thanks!), I don't think this is particularly confusing and does prevent a footgun, that as you say I'd hate to try and protect against from when implementing AcpiHandler.

If you add the docs and address that last formatting nit, I think this is ready to merge. Thanks for these changes!

[IsaacWoods]

Great, thanks!

This will be published as a breaking change to rsdp and acpi by the end of the month, as it'll be good to get it into the newsletter to warn users of the breakage. However, I'd like to review a few things if we're going to publish a new breaking change to acpi anyway - do you need this published to crates.io before then or are you happy to continue patching a copy in for a few weeks?

[Freax13]

No need to rush, I'm fine just using a patched version until then, thanks.