From b9febc44580832b4e44acea76604193aba848760 Mon Sep 17 00:00:00 2001 From: Pavel Mores Date: Fri, 17 May 2024 12:21:44 +0200 Subject: [PATCH] runtime-rs: document architecture & implementation conventions in qemu-rs Implementation of QemuCmdLine has a fairly uniform and repetitive structure that's guided by a set of conventions. These conventions have however been mostly implicit so far, leading to a superfluous and annoying request/force-push churn during qemu-rs PR reviews. This commit aims to make things explicit so that contributors can take them into account before an initial PR submission. Signed-off-by: Pavel Mores --- .../hypervisor/src/qemu/cmdline_generator.rs | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs b/src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs index 3abebbee92..e3c4ac9d34 100644 --- a/src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs +++ b/src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs @@ -65,6 +65,81 @@ impl Display for VirtioBusType { } } +// Conventions used in qemu command line generation +// ================================================ +// +// While acknowledging govmm inspiration, this implementation differs in +// several important aspects. +// +// 1:1 correspondence between qemu switches and cmdline generator structs +// ---------------------------------------------------------------------- +// +// There should mostly be a 1:1 mapping between qemu command line switches and +// objects (structs) that represent them. That is, each struct's +// ToQemuParams::qemu_params() is expected to generate a single +// `-whatever `. Notably, a lot of devices are assembled from a +// frontend (`-device`) and a backend (`-object`), each of which should be +// represented and generated by a separate object (struct). If setting up +// a qemu construct (device or other) requires assembly from multiple such +// parts, this is expected to be done in a `QemuCmdLine::add_*()` function. +// +// As an example, a network device might consist of `-netdev tap` and +// `-device virtio-net-pci`. The former is represented by struct Netdev, +// the latter by struct DeviceVirtioNet, both of which are instantiated & +// set up in QemuCmdLine::add_network_device(). +// +// There is a couple of exceptions to this convention. struct Kernel might +// be required to generate multiple qemu cmdline switches to perform full +// kernel set-up, struct Knobs (directly lifted from govmm) does that too +// due to its purpose of being a catch-all grabbag. Both are singletons +// dictated mostly by nature of qemu's cmdline syntax. Going forward, most +// if not all future additions to qemu cmdline generation are expected to +// follow this convention. +// +// This convention is a departure from govmm style which tends to lump all +// information necessary to set up a piece of VM equipment into a single +// object. This might make sense at the first sight, after all a network +// device needs both its frontend and backend so why not represent it with +// a single struct. However, as can be observed in govmm, the resulting +// structs tend to be big and hard to read since the frontend and backend +// params are distinct so the big struct is actually two separate structs +// really. If a need ever arises to have explicit representation of a whole +// device it would still seem preferable to assemble it from the explicit +// individual representations of frontend and backend. +// +// Naming conventions +// ------------------ +// +// The idea is to name entities in this qemu cmdline generator as close +// as possible to the qemu cmdline switches and params that they represent. +// This is to reduce the intellectual overhead of having to map between names +// on qemu command line and in this source code. +// +// Most of the cmdline switch generating structs in this file are expected +// to generate a single qemu cmdline switch as detailed above. It follows +// that most member fields of these objects will represent additional params +// of the qemu switches and their names should ideally be as close to the +// corresponding qemu switch params as possible, within reason and Rust +// syntax limitations (e.g. dashes in qemu param names obviously need to be +// converted to underscores for code to even compile). As an example, +// struct MemoryBackendFile members' names are identical to qemu's +// `-object memory-backend-file,...` params, with the exception of `mem-path` +// which is called `mem_path` to comply with Rust syntax. +// +// The struct names should follow the same logic, their names should ideally +// be as close as possible to the qemu switch they represent (e.g. `-smp` is +// generated by struct Smp), or to common parlance if that's not descriptive +// enough. This applies above all to devices which are commonly set up by +// `-device` & `-object`. In those cases, e.g. `-chardev socket` might be +// referred to as "chardev socket" in common speech so the corresponding struct +// is called ChardevSocket, or `-device vhost-user-fs-pci` will likely be +// pronounced as "device vhost user fs" so its struct is called +// DeviceVhostUserFs, too. Admittedly, this approach can be rather squishy and +// a matter of taste ultimately, but it should still be useful to keep in mind +// that the idea is to keep the names in this source code as close as possible +// to what folks working with qemu in kata are likely to use and be familiar +// with already. + #[derive(Debug)] struct Kernel { // PathBuf would seem more appropriae but since we get the kernel path @@ -1207,6 +1282,15 @@ pub struct QemuCmdLine<'a> { id: String, config: &'a HypervisorConfig, + // In principle, all objects implementing ToQemuParams could be just stored + // in the `devices` container. However, there are several special cases + // that might need to be set up in several steps after having been + // initially constructed (from HypervisorConfig, mostly). For instance, + // adding an NVDIMM needs to modify Machine and query Memory, adding a + // virtiofs share modifies both Memory and Machine, adding a serial console + // modifies Kernel etc. For convenience accessing them, we store these + // singletons in named QemuCmdLine member. The rest should go to the + // anonymous `devices` container. kernel: Kernel, memory: Memory, smp: Smp,