-
Notifications
You must be signed in to change notification settings - Fork 56
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
Move Note and GDT definitions into Rust #37
Conversation
cd7f3f9
to
f578136
Compare
Right now, if layout.ld or target.json is changed, the firmware will not automatically relink when running "cargo xbuild". This can make working on layout.ld quite tedious, as you have to manually change the Rust code to get the firmware to rebuild. We solve this by adding a tiny build script which simply rebuilds the program if either layout.ld or target.json changes. We also stop using "-s" (aka "--strip-all") in the target options. Instead, we add a second /DISCARD/ section in the linker script. This puts all of our logic about sections in a single place. It also makes it easier for a developer to get the symbols (if they want them). Signed-off-by: Joe Richey <[email protected]>
This ensures that the stack region is mapped by the ELF loader and doesn't conflict with any hypervisor memory regions. To do this without increasing binary size, we place the stack right after the .bss section. Note that we still need an assert to make sure that our minimal page table setup in ram32.s covers our program + stack. Also, being able to see the stack as a section makes debugging easier. Signed-off-by: Joe Richey <[email protected]>
The structure of the ELF note can be done with pure Rust code. We can definie the Name and Desc types and use a static struct to hold the note. Due to Rust's limitations on "pointer-to-integer cast", we have to have Desc have a function pointer type, which means that field is now 8 bytes long instead of 4. However, this doesn't seem to be an issue. The binary still works w/ PVH Boot on QEMU and CH. Signed-off-by: Joe Richey <[email protected]>
The GDT is just some data in static memory, so there's not a good reason to have this code in assembly. Ideally, we would use types from the x86_64 crate for this. However, - We can't use x86_64::structures::gdt::DescriptorFlags for the contents of the descriptors, as that definition is missing flags. - We can't use x86_64::structures::DescriptorTablePointer for the GDT pointers because its base is a u64, and Rust doesn't allow "pointer-to-integer cast" in statics. So we have to roll our own. The definitions aren't that bad. The only downside is that we have to add the bitflags dependency, but it was already a dependancy of x86_64, so that's also not bad. Signed-off-by: Joe Richey <[email protected]>
@rbradford I fixed this and the CI is now passing. It's ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nit typo if you want to fix it.
fn main() { | ||
println!("cargo:rerun-if-changed=target.json"); | ||
println!("cargo:rerun-if-changed=layout.ld"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
desc_size: u32, | ||
kind: u32, | ||
name: Name, | ||
desc: Desc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly when it was 4 bytes that forced a change in the Cloud Hypervisor PVH code to support it. It's ambiguous to me what it should be. But if it works... :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size of Desc seems to depend on how the code in question is compiled. Linux sets a field of type _ASM_PTR
, which is 8 bytes (at least on my kernel). The PVH Specification seems to indicate the type should be .long
aka 4 bytes. I think it's fine for hypervisors to just support any size <= 8 for this field. QEMU just does a raw read of a size_t
without checking descsz
.
There type of namesz
, descsz
, and kind
seems to usually be u32
(even for 64-bit ELF). That's what Linux does, NetBSD does, and what Oracle says to do. However, the gABI says they should be u64
s but that seems to get ignored. See this mailing list discussion for more info.
The final complexity is alignment of the structure (and hence the containing phdr's alignment). Some loaders expect this to always be 4, others allow for other alignments but compute the structure padding incorrectly:
- QEMU does it wrong
- The
linux-loader
crate used bycloud-hypervisor
also does it wrong. - See LLVM bug1 and bug2
Using an alignment of 4 prevents any issues (as there will just not be any padding). This is what Linux does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment about alignment to pvh.rs
.
Signed-off-by: Joe Richey <[email protected]>
These structures are just data, so there's not really a need for them to use assembly code. This change makes the layout of the GDT and ELF Note easier to debug/change. Now, you can just dump them to serial output like any other struct. This also makes adding the 32-bit GDT in #24 much easier.
Additional small cleanups:
.stack
section (instead of using hacky asserts).