Boot configurator: trait (and associated objects) that write boot params in guest memory

[alxiord]

#15

This PR introduces a new configurator module, meant to build (TBD) and write (included in this PR) boot parameters in guest memory. To begin with, the VMM will have to build its own boot params, encapsulate them into ByteValued objects, then pass them on to linux-loader.

VMM workflow:

let load_result = kernel_loader.load<ReadImpl, GuestMemoryImpl>(
        guest_memory,
        kernel_start_addr,
        kernel_image,
        himem_start_addr
)?;
let (boot_params_header, boot_params_sections): (HeaderImpl, Vec<SectionImpl>) = Vmm::build_boot_params()?;
BootConfiguratorImpl::write_bootparams::<HeaderImpl, SectionImpl, GuestMemoryImpl>(
        (boot_params_header, header_addr),
        Some((boot_params_sections, sections_addr)),
        guest_memory
)?;

The PR adds support for 3 boot protocols:

x86_64 Linux boot

• header: boot_params struct, everything that goes into the zeropage
• sections: unused (e820 map included in header)

let params: boot_params = Vmm::build_boot_params_linux()?;
LinuxBootConfigurator::write_bootparams::<boot_params, boot_params, GuestMemoryImpl>(
        (params, zeropg_addr),
        None,
        guest_memory
)?;

x86_64 PVH boot

• header: hvm_start_info struct
• sections: vector of hvm_memmap_table_entry structs: memory map table entries

let (start_info, memmap_entries): (hvm_start_info, Vec<hvm_memmap_table_entry>) = Vmm::build_boot_params_pvh()?;
PvhBootConfigurator::write_bootparams::<hvm_start_info, hvm_memmap_table_entry, GuestMemoryImpl>(
        (start_info, start_info_addr),
        Some((memmap_entries, memmap_addr)),
        guest_memory
)?;

aarch64 boot

• header: byte blob containing the compiled FDT
• sections: unused

struct FdtBlob { ... }
impl ByteValued for FdtBlob { ... }
...
let fdt_blob: FdtBlob= Vmm::build_fdt()?;
FdtBootConfigurator::write_bootparams::<FdtBlob, FdtBlob, GuestMemoryImpl>(
        (fdt_blob, fdt_addr),
        None,
        guest_memory
)?;

[alxiord]

Coverage test fails, but I'd like to get some preliminary feedback before investing the extra effort into additional unit tests.

[aljimenezb]

I looked mostly at the PVH section since that is what I am most familiar with, but the design looks clean and easy to understand even for Rust beginners like me. Thank you! Please see my comment regarding the initrd support.

[aljimenezb]

Another section type that must be specified for PVH is the list of auxiliary boot modules described by a vector of hvm_modlist_entry structs. This is the mechanism used in PVH for initrd support.

A VMM that implements initrd support with PVH, must write to guest memory a single hvm_modlist_entry structure that specifies the paddr and size of the initramfs loaded in guest memory. The hvm_start_info struct then has a field pointing to this hvm_modlist_entry, similar to how boot_params has fields for ramdisk_image and ramdisk_size.

As far as I know, there are no other current uses of this module mechanism in Linux, so only a single hvm_modlist_entry is needed in guests that use initrd. Any additional functionality required at boot time is typically embedded in the kernel binary or as part of the initramfs. As an example, I am told that microcode blobs used to be specified using a similar approach in the past, where now they are usually part of initramfs.

So for the current use cases only one hvm_modlist_entry is needed, but in theory we should be able to handle an arbitrary (to a reasonable extent) number of modules. We would either need another parameter (boot_modules?) that would be unused by the Linux and ARM implementations, or to extend this sections parameter to be able to describe both the memory map and the modules entries.

[alxiord]

Thank you for the detailed explanation! I updated this PR with some more abstractions over the tuples I initially used, and now the BootConfigurator works with a BootParams object that has a (mandatory) header (boot_params / hvm_start_info / fdt blob), optional sections (PVH memmap) and optional modules (PVH modlist for initrd).

[bonzini]

Yes, I was also going to remark that the boot parameters should all be encapsulated in a single struct.

[bonzini]

Yes, I was also going to remark that the boot parameters should all be encapsulated in a single struct.

[alxiord]

I used this PR as a starting point to play around with encapsulating bootparams creation too, towards the vision of having all kernel loading logic in this crate. I started with elf + Linux as it's the easiest of combinations (building the fdt for ARM is, I think, beyond this crate).

The code is in this branch: https://github.com/aghecenco/linux-loader/tree/build_params

elf + Linux boot: LinuxBootParams

Tried it out with Firecracker: https://github.com/aghecenco/firecracker/tree/linloader
configure_system is simplified

And cloud-hypervisor: https://github.com/aghecenco/cloud-hypervisor/tree/linloader
configure_64bit_boot is simplified

[alxiord]

Note to self: to avoid this level of verbosity, maybe impl ByteValued for () {} in vm-memory

[alxiord]

@bonzini the latest version uses a single BootParams object with (up to) 3 generics for the various components.
@aljimenezb the modlist is an Optional member of the BootParams, filled in only for PVH.
Could you please take another look?

[aljimenezb]

@aghecenco Sorry it took me a bit to reply back. The new types look good to me, but probably others with more Rust experience should take a look. One minor suggestion is to avoid having members of BootParams and BootSections that are both called sections, to potentially avoid lines like:

let memmap_list = params.sections.unwrap().sections;

Perhaps the second field in the BootParams struct can be renamed to mem_maps or something similar, since it is only used for PVH to specify memory maps anyway.

Also I noticed that the write_bootparams() implementation in PvhBootConfigurator did not have support yet for copying the module list into guest memory, so I added the (untested!) code here:
aljimenezb@9f932dc
feel free to copy/paste if needed :)

[bonzini]

I think adding more generic parameters is not the right approach.

BootParams can be kept generic if you make it a convenience for building a Vec<(Box<[u8]>, GuestAddress)>. It can have generic methods set_header(&mut self, ...) add_section(&mut self, ...) add_module(&mut self, ...) that take any ByteValued and make a copy into a Box<[u8]> (I think vec![0; size].into_boxed_slice() works for that). Then write_bootparams does not need to be generic at all because it just iterates along the Vec<(Box<[u8]>, GuestAddress)> and writes each slice to the guest memory.

[alxiord]

@bonzini I got rid of the 3 generics; the struct still separates header from sections and modules to allow for specific errors when writing the params to guest memory.
@alexandruag please take a(nother) look too.

[bonzini]

Nice! My only remaining qualm is that add_sections actually sets the whole Vec, so I would prefer an add_section method that can be called multiple times. Likewise for modules.

[bonzini]

There can still be add_sections and add_modules methods, taking a Vec, for convenience. However it's a bit limiting to assume a single type for all sections/modules. add_section and add_module could return the top address for the section and module to simplify adding heterogenous sections/modules.

[bonzini]

Looks great, thanks!

[alxiord]

I added a new commit that replaces the tuples in the original BootParams struct with individual fields, for increased code clarity. I kept the set_modules and set_sections (very convenient when we already have them) but extended BootParams to allow additions of individual sections / modules of various types (as long as they're ByteValued) at any valid address. Examples in configurator/mod.rs should show how the functions are to be used. Some responsibility is left to the caller, as individual modules and sections can now be placed anywhere after the starting address, potentially overlapping others, or leaving gaps. The caller should use the returned GuestAddresses to make sure everything is where they want it to be.

@bonzini many thanks for all the feedback so far, this PR has come a long way since the beginning. Can you take another look? The 3 types of configurators haven't changed much, but the sections/modules rework has built up.

[alxiord]

Hi @bonzini @alexandruag, could you please take a(nother) look?

[bonzini]

I don't have approval privileges, but I like the basic API and tests are comprehensive, so it looks good.

[jiangliu]

Do we need to validate guest memory address here?

[alxiord]

Added validation. write_slice would have returned an error, but the other use cases explicitly validate that the bootparams fit in guest memory, so I added the same here.

Changes addressed