Add support for PVH boot protocol #11
Conversation
aljimenezb
changed the title
aljimenezb
force-pushed
the
pvh-boot
branch
2 times, most recently
from
a08f097 to
26cfe46
Compare
|
@bonzini Can you give this a quick look through? |
|
Looks good to me. The main issue is that it does not cover the task of preparing the PVH parameters struct, I'll open an issues about that. |
Right, I looked to see if there was some common place to add this code (and also setting up initial vCPU registers), since these areas are very similar in Firecracker and Intel Cloud Hypervisor (likely inherited from crosvm). I thought the vmm-vcpu crate would be a good place for this but found it empty. Looking at the issue you opened now.. |
|
We also need to add tests for the newly added feature. For some examples, you can check the existing pipeline: https://github.com/rust-vmm/linux-loader/blob/master/.buildkite/pipeline.yml It's not a hard block, but I would prefer the PR that uses the rust-vmm-ci pipeline to be merged first because it also updates the container (which updates the rust version & adds tests for aarch64): #14 |
aljimenezb
force-pushed
the
pvh-boot
branch
2 times, most recently
from
96d19b3 to
e54933d
Compare
|
@andreeaflorescu I amended the last commit to use an ELF binary with an additional note header that is unrelated to PVH. The goal is to increase coverage by exercising the code path that ignores such notes. Still, the coverage test fails because coverage drops by 0.20% for x86 and 1% for arm. I believe the minor drops in coverage are due to the start_info.rs definitions which do not have layout tests, but I am not sure how the tests work exactly. The previous test left some files in the workspace where I could examine the report and see the coverage, but after updating to the rust-vmm-ci pipeline I can't find those anymore. Any advice on what I am doing wrong/how to move forward? |
aljimenezb
force-pushed
the
pvh-boot
branch
2 times, most recently
from
334e311 to
2f51246
Compare
|
Added layout tests (autogenerated by bindgen) for the hvm_start_info and hvm_memmap_table_entry structs. Also changed the name from hvm_mmap_table_entry to hvm_memmap_table_entry to keep the same name as in the Xen header file (https://xenbits.xenproject.org/docs/unstable/hypercall/x86_64/include,public,arch-x86,hvm,start_info.h.html) To investigate why the coverage check fails with a drop of 0.20% for x86 and 1% for arm, I modified test_coverage.py in my local repo so the kcov_output directory is not cleaned up after the test runs. I can see that in the x86 case, there are only 3 lines I added that are not covered. |
aljimenezb
force-pushed
the
pvh-boot
branch
2 times, most recently
from
165d4f4 to
e106a2c
Compare
|
I found an ARM box and ran the coverage tests with the instrumented test_coverage.py so that I could look at the reports afterwards. Before applying changes in this PR:
=======================================================
(Ignoring the spurious change in /linux-loader/src/cmdline/mod.rs, which is caused by an empty line which is now reported as covered) The results above show that the drop in coverage for the aarch64 case is caused primarily by the new data structures in start_info.rs and elf.rs which are not utilized in the aarch64 code. |
Sounds good, let's compile out what is not used on aarch64. If the coverage still fails for weird reasons (it does that from time to time), you can also decrease the coverage. |
Done. I tried my best to avoid decreasing the coverage, and luckily the numbers worked out after removing start_info.rs from the aarch64 case, and adding more unit tests on x86. I actually had to increase the coverage value for x86 on my last commit. Please let me know if there is anything else that is needed before merging. |
|
I would very much like to see #24 merged before this one, so that we can work on code that's clearly structured per architecture and kernel format (this code would then go to |
Unless anyone else has objections, I believe this PR is ready to merge, but I'm ok with whatever you decide is best. As you mentioned, I think the PVH code probably fits best in |
After actually looking at the code, I realize that it isn't. I will update the description. |
| // Rust definitions needed to enable PVH boot protocol | ||
| #[repr(C)] | ||
| #[derive(Debug, Copy, Clone, Default)] | ||
| pub struct hvm_start_info { |
There was a problem hiding this comment.
disclaimer I don't really know how this works.
I noticed that the structures in this file aren't actually used. Is that intended? How are you suppose to use them? I noticed that we do export them, so I was wondering if we can have some sort of example with them.
There was a problem hiding this comment.
The structures are meant to be exported and used by the VMM implementation when writing the necessary info (cmdline address, ACPI RSDP, memory maps, etc) to guest memory to implement the specific boot protocol. A current example is struct boot_params, which is also defined in linux-loader and then imported by the VMMs. If you were to try and establish a correspondence between the two, then:
hvm_start_info <==> boot_params
hvm_memmap_table_entry <==> boot_e820_entry
As Paolo mentioned before, PVH uses smaller set of boot parameters than the Linux boot protocol; you can see by the definitions that hvm_start_info is much more concise than boot_params.
Currently, VMMs like Intel Cloud Hypervisor and Firecracker use the Linux boot protocol and struct boot_params. The goal of this change is to allow them to use the PVH protocol as well, since it is a standard Linux interface and boots as fast (or faster) than the direct ELF boot method currently in use. I have an open PR on Intel Cloud Hypervisor and will create one for Firecracker soon.
Right now, for both direct ELF boot and PVH, the VMM is responsible for putting the data structures in guest memory and initializing the vCPU state as required by the specific protocol. Paolo opened #15 to add functionality that takes care of those tasks as well on this crate. IIUC, that would provide the sort of use case example that is missing for both boot_params and hvm_start_info and friends... And of course I can also add comments on start_info.rs if you think that is better.
There was a problem hiding this comment.
Thanks for the really nice explanation. I think it is worth specifying this somewhere so that noobs like myself can have it easier.
|
@bonzini can you take a look at this PR? |
| // The PVH entry point is a 32-bit address, so the descriptor field | ||
| // must be capable of storing all such addresses. | ||
| if (nhdr.n_descsz as usize) < mem::size_of::<u32>() { | ||
| return Err(Error::InvalidPvhNote); | ||
| } | ||
|
|
||
| let mut pvh_addr_bytes = [0; mem::size_of::<u32>()]; | ||
|
|
||
| // Read 32-bit address stored in the PVH note descriptor field. | ||
| kernel_image | ||
| .read_exact(&mut pvh_addr_bytes) | ||
| .map_err(|_| Error::ReadNoteHeader)?; | ||
|
|
||
| Ok(Some(GuestAddress( | ||
| u32::from_le_bytes(pvh_addr_bytes).into(), | ||
| ))) |
There was a problem hiding this comment.
I changed this section slightly since the previous revision to make it more generic. The previous code works correctly for the primary use case: a 64-bit Linux ELF header which encodes the PVH entry point address in an 8 byte field, but it did not work with ELF binaries that encode the address using a 4 byte field (e.g. rust-hypervisor-firmware has an WIP PR that does this).
The PVH specification technically does not define the size of the note header descriptor field, only that it must contain a 32-bit address. So regardless of the size of the containing field in the note header, a little endian architecture will store the 32-bit address in the first 4 bytes of the buffer and I changed the code to retrieve that value.
Please let me know if you see any issues with this approach.
Define ELF Note header structures and necessary for parsing the PVH entry point address encoded in the kernel ELF header. Generated the elf.rs file again by running bindgen: bindgen --with-derive-default elf.h > elf.rs From upstream linux include/uapi/linux/elf.h at commit: 3cc6e2c599cdca573a8f347aea5da4c855ff5a78 and then edited to eliminate unnecessary definitions, add comments, and relocate definitions and tests for clarity. Signed-off-by: Alejandro Jimenez <[email protected]>
Introduce the layout and define the start_info, module list and memory map table entry structures used by the PVH boot protocol. The hvm_start_info structure is akin to bootparams in Linux boot protocol, specifying the small set of parameters required by the PVH protocol. Signed-off-by: Alejandro Jimenez <[email protected]>
Parse the ELF header looking for a PVH Note section and retrieve the encoded PVH entry point address if there is one. The entry point address is returned in KernelLoaderResult alongside the typical ELF entry point used for direct boot. A VMM implementing KernelLoader can now determine whether a PVH entry point is available and choose to configure its guests to boot using either PVH or Linux 64-bit protocol. Signed-off-by: Alejandro Jimenez <[email protected]>
Add test cases to verify the functionality that parses the ELF Note header to look for a PVH entry point address if one is encoded. Parse a minimal ELF binary that encodes a predefined address of 0x1e1fe1f, and verify that the same value is read. Also test the case in which a note header is present but no PVH entry point is encoded, as well as a case where the PVH entry address is encoded in the note header using a field of incorrect size. The minimal ELF source code (elfnote.S): #define ELFNOTE_START(name, type, flags) \ .pushsection .note.name, flags, @note ; \ .balign 4 ; \ .long 2f - 1f /* namesz */ ; \ .long 4484f - 3f /* descsz */ ; \ .long type ; \ 1:.asciz #name ; \ 2:.balign 4 ; \ 3: #define ELFNOTE_END \ 4484:.balign 4 ; \ .popsection ; #define ELFNOTE(name, type, desc) \ ELFNOTE_START(name, type, "a") \ desc ; \ ELFNOTE_END #define XEN_ELFNOTE_PHYS32_ENTRY 18 #define NT_VERSION 1 ELFNOTE(dummy, NT_VERSION, .quad 0xcafecafe) ELFNOTE(PVHNote, XEN_ELFNOTE_PHYS32_ENTRY, .quad 0x1e1fe1f) .section ".text","ax" .global _start _start: Built with: $ gcc elfnote.S -s -nostdlib -o test_elfnote.bin The elfnote.S source above is modified to generate the binaries for the rest of the test cases. Signed-off-by: Alejandro Jimenez <[email protected]>
Adding support for parsing the PVH entry point address if encoded in the kernel ELF binary, and returning it as an additional field in KernelLoaderResult. This allows the VMM to choose which boot protocol to use.
These changes have been tested alongside with patches for Intel Cloud Hypervisor that successfully boot a guest using the PVH entry point.
The PR is marked as WIP since there are still a few open questions:
Some definitions from Xen's xen/include/public/arch-x86/hvm/start_info.h (this header also exists in Linux), and comments were manually copied into a new file start_info.rs. When using bindgen to generate start_info.rs from the C header, the output file is not very legible so I opted for the manual approach to keep it readable. Is this acceptable or should bindgen be used?
I took the function align_up() from the x86_64 crate (https://docs.rs/x86_64/0.9.2/x86_64/index.html) and slightly modified it. The goal was to avoid adding a new dependency to an external crate. Is this appropriate?
Copyright messages: I'm unsure of the proper way, so I just pasted the copyright notice at the top of every file I touched, regardless of the length/importance of the change. What are the guidelines on how to do this correctly?
Closes #3