coconut-svsm / svsm

COCONUT-SVSM
MIT License
123 stars 43 forks source link

docs: Document page tables, visibility, per-CPU guards #272

Closed Zildj1an closed 7 months ago

Zildj1an commented 9 months ago

Document memory management of page tables, page visibility and per-CPU mapping guards. This is related to issue #74 Document COCONUT-SVSM and continues the mm documentation efforts that started with the merged PR 243. For the purpose of that issue, I will consider the memory subsystem sufficiently documented after this PR (even if not every single detail has been documented) and would like to shift focus to documenting protocols.

00xc commented 9 months ago

The new CI changes in #275 will complain about trailing /// with no comments, i.e.

/// Some docs
///
fn my_function() {
     ...
}
Zildj1an commented 9 months ago

The new CI changes in #275 will complain about trailing /// with no comments, i.e.

/// Some docs
///
fn my_function() {
     ...
}

Wouldn't that trigger numerous complaints for already committed comments? Also, does this imply that a block of documentation like the following:

/// Unmaps a 2MiB page from the page table sub-tree
///
/// # Arguments
///
/// * `vaddr` - The virtual address to unmap. Must be aligned to 2MiB.
///
/// # Returns
///
/// Returns a copy of the PTEntry that mapped the virtual address, if any.
///
/// # Panics
///
/// This method panics when `vaddr` is not aligned to 2MiB.

we will have to change it to:

/// Unmaps a 2MiB page from the page table sub-tree
/// # Arguments
/// * `vaddr` - The virtual address to unmap. Must be aligned to 2MiB.
/// # Returns
/// Returns a copy of the PTEntry that mapped the virtual address, if any.
/// # Panics
/// This method panics when `vaddr` is not aligned to 2MiB.

or is this just for the last line in a documentation block? If not, this makes the documentation MUCH less readable from source, IMO. And what is the benefit?

stefano-garzarella commented 9 months ago

@Zildj1an IIUC it's just the last line of the comment block that should not be empty

Zildj1an commented 9 months ago

@Zildj1an IIUC it's just the last line of the comment block that should not be empty

Okay, that's a relief :) Updating PR now

00xc commented 9 months ago

The new CI changes in #275 will complain about trailing /// with no comments, i.e.

/// Some docs
///
fn my_function() {
     ...
}

Wouldn't that trigger numerous complaints for already committed comments?

Yes, there is already a commit fixing those cases in the PR introducing the check.

Also, does this imply that a block of documentation like the following:

/// Unmaps a 2MiB page from the page table sub-tree
///
/// # Arguments
///
/// * `vaddr` - The virtual address to unmap. Must be aligned to 2MiB.
///
/// # Returns
///
/// Returns a copy of the PTEntry that mapped the virtual address, if any.
///
/// # Panics
///
/// This method panics when `vaddr` is not aligned to 2MiB.

we will have to change it to:

/// Unmaps a 2MiB page from the page table sub-tree
/// # Arguments
/// * `vaddr` - The virtual address to unmap. Must be aligned to 2MiB.
/// # Returns
/// Returns a copy of the PTEntry that mapped the virtual address, if any.
/// # Panics
/// This method panics when `vaddr` is not aligned to 2MiB.

or is this just for the last line in a documentation block? If not, this makes the documentation MUCH less readable from source, IMO. And what is the benefit?

As Stefano points out, this is only for the last line :)

Zildj1an commented 9 months ago

Left some suggestions. Overall I'd say we don't need to be too verbose adding Arguments, Returns and Errors sections on simple or self-explanatory functions. Also, I still see several leftover blank lines at the end of documentation comments.

I've already left a comment about this; IMHO documenting return values, potential errors and arguments is positive. The function name might be self-evident, e.g. "use A to do B" but whether the parameter to pass is "A" or, e.g. , its index on the alphabet is not self-evident. At most it is obvious for the current contributors but not necessarily for new ones; which are the target audience of this documentation. Also, markers like '# Returns' or '# Arguments' can help non-human eyes, CI, automated tools, etc. An example is Sphinx and the rest of tools we use for the Linux kernel documentation.

00xc commented 9 months ago

Left some suggestions. Overall I'd say we don't need to be too verbose adding Arguments, Returns and Errors sections on simple or self-explanatory functions. Also, I still see several leftover blank lines at the end of documentation comments.

I've already left a comment about this; IMHO documenting return values, potential errors and arguments is positive. The function name might be self-evident, e.g. "use A to do B" but whether the parameter to pass is "A" or, e.g. , its index on the alphabet is not self-evident.

Yes, but a lot of the cases I pointed out are something like:

/// # Returns
///
/// Returns a [`Thing`], or an [`SvsmError`] if an error occured.
fn get_thing() -> Result<Thing, SvsmError> { ... }

Which does not really add anything over the function signature as, well, that's the use of Result. So, unless we are detailing which conditions result in an error, I'd say this is not really needed.

At most it is obvious for the current contributors but not necessarily for new ones; which are the target audience of this documentation. Also, markers like '# Returns' or '# Arguments' can help non-human eyes, CI, automated tools, etc. An example is Sphinx and the rest of tools we use for the Linux kernel documentation.

As far as I know, the use of a lot of these tools is to generate documentation, which cargo doc already does. For anything more advanced, like IDEs and LSPs, there is rust-analyzer. I'm not aware of any tooling in the Rust ecosystem that makes use of these sections, other than # Safety and # Panics.

Zildj1an commented 7 months ago

I'm opening a new PR for this, with the appropriate changes following @00xc feedback