rsdp: clarify PhysicalMapping documentation

[A0lson]

• Make new method so RSDP search (for BIOS systems) can return just the physical address. This alone is useful when the physical address of the found table is desirable (such as for passing the ACPI table address from a bootloader to an OS, etc...)

• For the BIOS-based searhc method, map the final RSDP properly based on length

[rcerc]

Could this be implemented by calling physical_start on the PhysicalMapping returned by (the old implementation of) search_for_on_bios? As you mentioned, it would be useful for a bootloader to be able to simply get the physical address of the RSDP, especially without having to implement an AcpiHandler, but I don't think the latter is possible because the RSDP must be found by searching on legacy BIOS systems (which requires the rsdp crate to map physical memory). Please let me know if I am misinterpreting these changes!

[A0lson]

I myself am not sure I understand how PhysicalMapping was intended and would really appreciate a bit of clarification. It seems to me the goal would be for it to return a virtual address pointing to the desired physical_start of region_length. However, interpretation of the other two variables is a bit confusing to me...

With alignment/paging details, exactly may things be altered? As concrete example:
If the goal were physical_start=0x54321 and region_length=0x1234, with 0x1000 aligned pages, would something like virtual_start=0x80000321 and mapped_length=0x1CDF be expected? I wasn't sure if the PhysicalMapping object would always be expected to have physical_start and region_length matching the "requested" ones, or if could physical_start point to the physical address at the beginning of the mapping (such as 0x54000 here)and instead have aregion_length of0x2000`.

Especially in light of your suggestion, I think the difference matters greatly as it would imply whether or not the physical_start of a mapping could be treated as the original physical address intended by the mapping.

Otherwise, if physical_start could be relied upon, getting it from the mapping seems fine too.

[rcerc]

Hmm... I had not thought of that second interpretation before. @IsaacWoods would probably know what the documentation means. Frankly, I struggle to see why physical_start, region_length and mapped_length must be included in the PhysicalMapping struct; I think it would be more flexible and less misleading if instead PhysicalMapping consisted of just a virtual_start and arbitrary data field, the latter used by the implementer of AcpiHandler to keep track of mappings and who could also directly give it a custom Drop implementation to recycle a mapping instead of unmap_physical_region:

pub struct PhysicalMapping<H: AcpiHandler, T> {
    pub data: H::PhysicalMappingData,
    virtual_start: NonNull<T>,
}

Then, AcpiHandler could resemble this:

pub trait AcpiHandler: Clone {
    type PhysicalMappingData;

    unsafe fn map_physical_region<T>(&self, physical_address: usize, size: usize) -> PhysicalMapping<Self, T>;

    unsafe fn resize<T>(physical_mapping: &mut PhysicalMapping<Self, T>, new_size: usize);
}

It might require a little more work on the user's side (to keep around a physical address, reference to the AcpiHandler, length, etc. in PhysicalMappingData), but I personally prefer this interface.

[IsaacWoods]

Could this be implemented by calling physical_start on the PhysicalMapping returned by (the old implementation of) search_for_on_bios?

It could.

especially without having to implement an AcpiHandler

If you're in a simple environment (e.g. bootloader with identity mapping), an AcpiHandler can be very basic, and so doesn't feel like a huge burden to implement imo.

I myself am not sure I understand how PhysicalMapping was intended and would really appreciate a bit of clarification.

Frankly, I struggle to see why physical_start, region_length and mapped_length must be included in the PhysicalMapping struct; I think it would be more flexible and less misleading

We could definitely improve the documentation as people have found the mapping code difficult, but it's really not too complex. Your first interpretation is correct, if I understand correctly.

• physical_start is the physical address you've been asked to map. In your example, this would be 0x54321
• virtual_start is the address that this physical address has mapped to. This may not be the start of the region! In your example, it would be 0x80000321 if the page 0x80000000 was mapped to the frame 0x54000.
• region_size is the number of bytes you've been asked to map. In your example, 0x1234.
• mapped_size is the number of bytes you've actually mapped. It's purely used for your own housekeeping. This is where I think the confusion is, as your example doesn't seem correct. If 0x1234 bytes were requested, you'd need two pages at 0x1000 granularity, so mapped_size would be 0x2000.

This covers a wide range of mapping mechanisms (mainly, actually mapping it into arbitrary virtual memory, and using a direct map behind the scenes), and so I don't really see the need for storing arbitrary data. I think that, especially for newcomers, this would make the problem seem much more complex than needed. If you do need it, you can always store a map in your handler from physical address to some random data, but I'd want to see an example where this was unwieldy to make this breaking change.

[rcerc]

Thank you for the clarification. I think it would be helpful to expand the documentation. That mapped_length is the length from the start of the first used page to the end of the last page mapped contiguously surprises me because it is not as relevant to the library (for the library is not aware of where that first page starts and thus how to interpret mapped_length). On the other hand, if mapped_length were the length from the virtual address of the structure that was requested to the end of the last page mapped contiguously, then we could have logic such as that existing in SdtHeader::validate_lazy to not request another mapping if the structure is already accessible in its entirety.

[A0lson]

Sorry for the noise, I was too quick to push the update a short while ago.

After the feedback, exposing the physical start of AcpiTables was an attractive solution as it was much smaller change. However, this is actually the RSDT address, not the RSDP address. The from_validated_rsdp macro ends up unmapping the RSDP and returning a mapping to the RSDT. That seems sensible for the purposes of table parsing, but is not conducive for locating the RSDP.

The latest revision slightly tweaks the documentation but is otherwise similar to my initial revision that split the method. If you have other suggestions, I'm all ears.

[rcerc]

Yeah, using AcpiTables::search_for_rsdp_bios would not provide the right address. However, does Rsdp::search_for_on_bios not return a PhysicalMapping to the RSDP on which physical_start can be called?

unsafe { rsdp::Rsdp::search_for_on_bios(acpi_handler) }
    .unwrap()
    .physical_start()
    .as_ptr() as usize

[A0lson]

Good point, I forgot I can just import the rsdp crate directly and use that. Works great!

(This PR is now just a documentation update for the PhysicalMapping)

[rcerc]

It is good that the documentation is clarified. I left a few comments in case you are interested.

[rcerc]

A newline should precede this if you meant it to be on the line in the documentation after 'Construct a new PhysicalMapping.'

Suggested change

	/// `mapped_length` describes usable span from `virtual_start` to end of last mapped page (for paging), and thus may exceed `physical_length`.
	/// `mapped_length` describes usable span from `virtual_start` to end of last mapped page (for paging), and thus may exceed `region_length`.

[rcerc]

Do you think documentation for these fields should be moved to PhysicalMapping::new? Its arguments are named the same as the fields, and the documentation under PhysicalMapping::new has the benefit that it is publicly facing.

[rcerc]

Maybe it could be mentioned that physical_start is the 'physical address of the structure requested to be mapped' and that virtual_start is the 'virtual address of that structure' to avoid any uncertainty.

[IsaacWoods]

Thanks very much to both of you for your input! I definitely agree that the documentation in this area needed improving, and liked suggestions from both of you. I wanted to add a few more bits, and so have incorporated all of the comments into one commit, a77817f with you both as co-authors.

Thanks again!