Skip to content
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.

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

acpi: Document ACPI tables and ELF files handling #89

Closed
wants to merge 3 commits into from
Closed

acpi: Document ACPI tables and ELF files handling #89

wants to merge 3 commits into from

Conversation

Zildj1an
Copy link
Contributor

@Zildj1an Zildj1an commented Sep 8, 2023

Document ACPI tables structs and code, including function usage examples. This is related to issue #74 titled "Document COCONUT-SVSM". Also document ELF files handling

Document ACPI tables structs and code, including function usage examples.
This is related to issue #74 titled "Document COCONUT-SVSM".

Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
@cclaudio
Copy link
Member

cclaudio commented Sep 8, 2023

When applicable, I think the two things below can be useful for people that need to work on this file:

  • a comment at the top describing what spec we used in the file (e.g. Name, version)
  • a comment in the structure indicating the table number in the spec it refers to

@Zildj1an
Copy link
Contributor Author

Regarding @cclaudio comment, what do you think @joergroedel ? Should we link to any specific ACPI documentation?

@joergroedel
Copy link
Member

Good question, for now the ACPI table definitions we actually use are there for decades, so I don't see much added value by adding version comments/links. The question is whether we will need to support features from more recent ACPI versions, in that case it would make sense to add this information, and also have it at the old tables for consistency.

@Zildj1an
Copy link
Contributor Author

Thanks @joergroedel in that case I'm satisfied with the current state of the commit. I fixed a merge issue and that's the reason why we have a merge commit without my signature.

Document ELF files handling, code and structures, including some function
usage examples. This relates to issue #74 titled "Document COCONUT-SVSM".

Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
@Zildj1an Zildj1an changed the title acpi: Document ACPI tables acpi: Document ACPI tables and ELF files handling Sep 12, 2023
@Zildj1an
Copy link
Contributor Author

I just updated this pull request, now we also document ELF files handling (see new commit) I will not continue documenting until this is merged to avoid bottleneck

@00xc
Copy link
Member

00xc commented Sep 13, 2023

Thanks @joergroedel in that case I'm satisfied with the current state of the commit. I fixed a merge issue and that's the reason why we have a merge commit without my signature.

I think this could be fixed by rebasing instead, no? We don't have guidelines on merges vs rebases for the project so this is fine for me, just mentioning it.

Comment on lines +401 to +402
/// let elf_file = Elf64File::read(&elf_file_buf).unwrap();
/// ``
Copy link
Member

Choose a reason for hiding this comment

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

There's a missing closing backtick here. cargo doc will warn about it:

warning: could not parse code block as Rust code
   --> src/elf/mod.rs:398:9
    |
398 |       /// ```rust
    |  _________^
399 | |     /// use your_module::Elf64File;
400 | |     ///
401 | |     /// let elf_file = Elf64File::read(&elf_file_buf).unwrap();
402 | |     /// ``
    | |__________^
    |
    = note: error from rustc: unknown start of token: `
    = note: `#[warn(rustdoc::invalid_rust_codeblocks)]` on by default

@00xc
Copy link
Member

00xc commented Sep 13, 2023

The doctests for this PR are broken at the moment. Moreover, existing doctests in the main branch are also broken, so I suggest we fix those before merging these. I've opened #93 for this purpose.

You can run the doctests via cargo test --target=x86_64-unknown-linux-gnu --doc.

@Zildj1an
Copy link
Contributor Author

Thanks @00xc I will reopen this PR without the issues mentioned.

@Zildj1an Zildj1an closed this Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants