Skip to content

rsdp: clarify PhysicalMapping documentation #162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

rsdp: clarify PhysicalMapping documentation #162

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions rsdp/src/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ where
{
physical_start: usize,
virtual_start: NonNull<T>,
region_length: usize, // Can be equal or larger than size_of::<T>()
mapped_length: usize, // Differs from `region_length` if padding is added for alignment
region_length: usize, // Desired physical memory span, can be equal or larger than size_of::<T>()
mapped_length: usize, // Usuable mapped length starting from `virtual_start`
Comment on lines +13 to +14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

handler: H,
}

Expand All @@ -20,14 +20,14 @@ where
H: AcpiHandler,
{
/// Construct a new `PhysicalMapping`.
/// `mapped_length` may differ from `region_length` if padding is added for alignment.
/// `mapped_length` describes usable span from `virtual_start` to end of last mapped page (for paging), and thus may exceed `physical_length`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

///
/// ## Safety
///
/// This function must only be called by an `AcpiHandler` of type `H` to make sure that it's safe to unmap the mapping.
///
/// - `virtual_start` must be a valid pointer.
/// - `region_length` must be equal to or larger than `size_of::<T>()`.
/// - `region_length` is the desired physical memory span, must be equal to or larger than `size_of::<T>()`.
/// - `handler` must be the same `AcpiHandler` that created the mapping.
Comment on lines 29 to 31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pub unsafe fn new(
physical_start: usize,
Expand Down