GitVita
New repositoryImport repositoryNew issue
Your repositories Your dashboard Sign in

microsoft/openvmm

Public

mirrored fromhttps://github.com/microsoft/openvmmAvailable

Watch0Fork0Star0
CodeCommitsIssuesPull requestsActionsInsightsSecurity
fe2bbc9829558a07c5e06d2b6ececc383b6593bf

Branches

Tags

  • No tags available.
0Branches0Tags
Go to file
Add file
Code

Clone

HTTPS

Download ZIP

openvmm/Guide/src/dev_guide/contrib

Guide/src/dev_guide/contrib/code.md

481lines · modecode

RawDownload
Latest commit unavailable.
1# Coding conventions
2
3One of our major goals with OpenVMM is to provide a high quality coding
4experience for contributors, starting first-and-foremost by having a consistent
5set of coding conventions in the project.
6
7[Do your part](https://youtu.be/-_7FaWnlhS4?t=10) and keep OpenVMM clean!
8
9## `rustfmt`
10
11_Checked Automatically:_ **Yes** (via [`cargo xtask fmt --pass rustfmt`](../dev_tools/xtask.md))
12
13OpenVMM source must be formatted using
14[rustfmt](https://github.com/rust-lang/rustfmt), which automatically and
15mechanically applies standard formatting to all the code. This eliminates time
16spent discussing or reviewing stylistic issues in pull requests.
17
18The CI will run `rustfmt --check` to enforce consistent formatting, and will
19fail if it notices any discrepancies.
20
21Unfortunately, `rustfmt` isn't infinitely customizable, and there are several
22rules that are must be manually enforced:
23
24- All lines must end with LF, not CRLF.
25- Top-level `use` imports should be non-nested.
26
27```admonish note
28Some of these manually-enforced conventions were introduced late into
29OpenVMM's development, and there may be chunks of the codebase that do not
30adhere to these conventions.
31
32If you're working in a file and notice that it isn't following a certain
33convention, please take a moment to fix it!
34```
35
36Assuming you've followed the [suggested dev env setup](../getting_started/suggested_dev_env.md)
37and set up `rust-analyzer` to format-on-save, you should rarely have to think
38about formatting in `.rs` files.
39
40## Custom Lints
41
42_Checked Automatically:_ **Yes** (via [`cargo xtask fmt --pass lints`](../dev_tools/xtask.md))
43
44Custom lints are a set of miscellaneous code lints that are specific to OpenVMM, which
45are enforced using a custom in-house tool:
46
47- enforce the presence of the standard Microsoft copyright header
48- enforce in-repo crate names don't use '-' in their name (use '_' instead)
49- enforce Cargo.toml files don't include autogenerated "see more keys" comments
50- enforce Cargo.toml files don't contain author or version fields
51- enforce files end with a single trailing newline
52- deny usage of `#[repr(packed)]` (you want `#[repr(C, packed)]`)
53- justify usage of `cfg(target_arch = ...)` (use `guest_arch` instead!)
54- justify usage of `expect(unsafe_code)` with an UNSAFETY comment
55- and more!
56
57Some of these lints are self explanatory, whereas others are described in more
58detail elsewhere on this page.
59
60## Unused `Cargo.toml` Dependencies
61
62_Checked Automatically:_ **Yes** (via [`cargo xtask fmt --pass unused-deps`](../dev_tools/xtask.md))
63
64We have an in-repo fork of
65[`cargo-machete`](https://github.com/bnjbvr/cargo-machete) that ensures
66`Cargo.toml` files only include dependencies that are actually being used.
67
68Avoiding unused dependencies makes it easier to reason about what a crate is
69doing just by looking at its dependencies, and also helps cut-down on
70incremental compile times.
71
72## Formatting (`Cargo.toml`)
73
74_Checked Automatically:_ **No**
75
76When defining dependencies in `Cargo.toml` files, please organize dependencies
77into the following groups:
78
79- crate-specific "subcrates"
80- crates under vm/
81- crates under vm/vmcore/
82- crates under support/
83- external dependencies
84
85The rationale here is that crates should be grouped according to how related to
86how _widely applicable_ they are. i.e: crates from crates.io and support are
87widely applicable outside of OpenVMM, whereas crates under vmcore/ only make
88sense within the context of OpenVMM, and crate-specific subcrates are - by
89definition - only applicable to the crate they are being imported from.
90
91Additionally, we make use of the
92[workspace dependencies](https://doc.rust-lang.org/nightly/cargo/reference/workspaces.html#the-dependencies-table)
93feature to ensure that all our dependencies stay in sync. This requires defining
94dependencies in your crate's `Cargo.toml` file and in the project's root `Cargo.toml`.
95
96So, for example:
97
98```toml
99[package]
100name = "openvmm"
101
102[dependencies]
103# crate-specific subcrates
104openvmm_core.workspace = true
105...
106
107# /vmcore
108vmcore.workspace = true
109...
110
111# /vm/devices
112firmware_uefi_custom_vars.workspace = true
113storvsp.workspace = true
114...
115
116# /support
117guid.workspace = true
118inspect.workspace = true
119inspect_proto.workspace = true
120...
121
122# external dependencies
123anyhow.workspace = true
124cfg-if.workspace = true
125clap.workspace = true
126...
127```
128
129## Linting (via `clippy`)
130
131_Checked Automatically:_ **Yes**
132
133OpenVMM uses [`cargo clippy`](https://github.com/rust-lang/rust-clippy) to
134supplement rustc's built-in lints.
135
136Assuming you've followed the guide and set up `rust-analyzer` to
137[use clippy](../getting_started/suggested_dev_env.md#enabling-clippy), you should see clippy lints
138appear inline when working on Rust code.
139
140The CI runs `cargo clippy` on every crate in the repo prior to building the
141project, and will fast-fail if it catches any warnings / errors.
142
143### Suppressing Lints
144
145In general, lints should be fixed by modifying the code to satisfy the lint.
146However, there are cases where a lint may need to be `expect`'d inline.
147
148In these cases, you _must_ provide a inline comment providing reasonable
149justification for the suppressed lint.
150
151e.g:
152
153```rust,ignore
154// x86_64-unknown-linux-musl targets have a different type defn for
155// `libc::cmsghdr`, hence why these lints are being suppressed.
156#[expect(clippy::needless_update, clippy::useless_conversion)]
157libc::cmsghdr {
158 cmsg_level: libc::SOL_SOCKET,
159 cmsg_type: libc::SCM_RIGHTS,
160 cmsg_len: (size_of::<libc::cmsghdr>() + size_of_val(fds))
161 .try_into()
162 .unwrap(),
163 ..std::mem::zeroed()
164}
165```
166
167### OpenVMM's `clippy` Configuration
168
169We stick fairly close to the default set of rustc / clippy lints, though there
170are some default lints that we've decided to disabled project wide, and other
171non-default lints which we've explicitly opted into.
172
173See the `[workspace.lints]` sections of OpenVMM's root
174[`Cargo.toml`](https://github.com/microsoft/openvmm?path=/Cargo.toml)
175for a list of globally enabled/disabled lints, along with justification as to
176why certain lints have been enabled/disabled.
177
178## Unsafe Code Policy
179
180```admonish danger
181When possible, try to avoid introducing new `unsafe` code!
182
183Before rolling your own `unsafe` code, check to see if a safe abstraction
184already exists, either in-tree, on crates.io\*, or in the standard library.
185
186\*subject to an unsafe-code audit
187```
188
189Rather than synthesizing our own unsafe code conventions, we follow the
190guidelines outlined in the following two resources:
191
192- [The Rust `unsafe` docs](https://doc.rust-lang.org/std/keyword.unsafe.html)
193- [The Fuchsia "Unsafe code in Rust" Guidelines](https://fuchsia.googlesource.com/fuchsia/+/refs/heads/main/docs/development/languages/rust/unsafe.md)
194
195In a nutshell:
196
197- `unsafe` functions are required to include `/// # Safety` documentation
198 describing any preconditions the caller must uphold when calling the function.
199- `unsafe {}` blocks are required to include a `// SAFETY:` comment describing
200 how the preconditions for calling the `unsafe` function(s) within the block
201 are being satisfied.
202- `expect(unsafe_code)` annotations are required to include an `// UNSAFETY:`
203 comment justifying why the code in question needs to use `unsafe`. This
204 annotation must be placed at the module or crate level.
205
206These requirements are enforced by CI, and will cause the build to fail if
207required documentation is missing.
208
209Editing a file containing unsafe code will trigger CI to label your PR and warn
210reviewers that the PR touches unsafe code. This is to ensure that all unsafe code
211is audited for correctness by area experts.
212
213## Uses of `cfg(target_arch = ...)` must be justified
214
215_Checked Automatically:_ **Yes** (via [`cargo xtask fmt --pass lints`](../dev_tools/xtask.md))
216
217Unless you're working on something that's genuinely tied to the host's CPU
218architecture, you should use `cfg(guest_arch = ...)` instead of `cfg(target_arch = ...)`.
219
220OpenVMM is a multi-architecture VMM framework, capable of running running x64
221guests on a x64 host, as well as Aarch64 guests on Aarch64 hosts (with the
222potential of adding additional platforms in the future).
223
224At the moment, OpenVMM requires that the host architecture and guest
225architecture match. That said, it's possible that at some point in the future,
226OpenVMM may also support _mismatched_ guest/host architectures, via an emulated
227CPU virtualization backend (akin to QEMU).
228
229Having an emulated CPU backend would enable OpenVMM to support such useful
230scenarios as:
231
232- running Arch64 guests on x86 machines
233- running x86 guests on ARM
234- running something exotic (e.g: RISC-V) on x86/ARM machines
235 - ...assuming we had the bandwidth to implement + maintain something like that
236- running OpenVMM on systems without hardware-accelerated CPU virtualization enabled
237
238With these scenarios in mind, it would be short-sighted to rely entirely on
239`cfg(target_arch = ...)` to gate guest-facing arch-specific functionality, as it
240would inexorably tie the guest's arch to the host's arch, making any future
241initiatives to pry the two apart significantly more difficult!
242
243As such, the `OpenVMM` repo includes infrastructure to specify a custom,
244OpenVMM-specific `cfg(guest_arch = ...)` cfg parameter.
245
246By default, `cfg(guest_arch = ...)` will act the same way as `cfg(target_arch =
247...)`, but it can be swapped to a different architecture by setting the
248`OPENVMM_GUEST_TARGET` env var at compile-time.
249
250There are very few reasons to use `cfg(target_arch = ...)` within the OpenVMM
251repo, and to enforce this rule, we have an in-house `xtask fmt --pass lints`
252check that lints each use of `cfg(target_arch = ...)` to include a
253"justification" for why it's being used.
254
255e.g: `cfg(target_arch = ...)` would be applicable when feature-gating a CPU
256intrinsic (such as CPUID, or a SIMD instruction), or when implementing a
257`*-sys` crate where the underlying C API/ABI varies between architectures.
258
259...otherwise, use `cfg(guest_arch = ...)`!
260
261## Avoid `Default` when using `zerocopy::FromZeros`
262
263_Checked Automatically:_ **No**
264
265The rule:
266
267- A type can `derive(Default)` **XOR** `derive(FromZeros)`.
268- A type that is `FromZeros` can also `impl Default`, but it must be a
269 conscious, explicit choice, with justification (read: inline comment) as to
270 why that particular default value was chosen.
271- N.B. `derive(IntoBytes)` or `derive(FromBytes)` imply `derive(FromZeros)`. That is:
272 this convention applies to those types as well.
273
274The why:
275
276- The all-zero type is often not a semantically valid `Default` value for a type
277- There are plenty of types that don't have a `Default` value, but _do_ have a
278 valid all-zero repr
279
280### Additional context
281
282As per the Rust docs for [`Default::default`](https://doc.rust-lang.org/std/default/trait.Default.html#tymethod.default):
283
284```admonish quote
285`fn default() -> Self`
286
287Returns the “default value” for a type.
288
289Default values are often some kind of initial value, identity value, or anything
290else that may make sense as a default.
291```
292
293Notably, **default should _not_ be used as some shorthand to "zero initialize"
294values!** For most non-trivial structs, the all-zero representation is _not_ a
295semantically valid `Default`!
296
297This is true in many contexts... but one that's particularly relevant in OpenVMM
298is that of FFI via C-style APIs and ABIs.
299
300In C, it's very common for types to undergo multi-stage instantiation, where
301they are initially allocated as all-zero, and then get "filled in" by some
302secondary init code. Notably: it's quite rare for that initial "all-zero" struct
303to be a valid instance of the type!
304
305#### Example: C FFI
306
307For example, a common pattern in C libraries might look something like:
308
309```c
310struct Handle {
311 uint16_t opaque_handle;
312}
313
314struct Handle handle = {0};
315init_handle(&handle);
316
317update_handle(handle, options);
318do_thing(handle);
319```
320
321In this example: it would be an error to invoke `update_handle` or `do_thing`
322with an all-zero handle, as the type hadn't finished being fully initialized.
323
324If we wanted to use this library from Rust, a "naive" approach would be to do
325something like:
326
327```rust,ignore
328#[repr(C)]
329#[derive(Default)]
330struct Handle {
331 opaque_handle: u16,
332}
333
334let mut handle = Handle::default(); // BAD!
335unsafe { init_handle(&handle) };
336
337unsafe { update_handle(handle, options) };
338unsafe { do_thing(handle) };
339```
340
341While this _technically_ works... using `Default` here is kinda bogus!
342
343After all - `Handle::default()` doesn't actually call `init_handle`, which means
344the value returned by `default()` doesn't match the "promise" of the trait!
345Namely: the returned value is not semantically valid yet!
346
347In many other Rust codebase, this "overloading" of `Default` to represent both
348semantically valid value _and_ all-zero values (in FFI) is par for the course,
349as once structs get more complicated, having a `derive` that is able to fully
350init a "uninitialized" struct in-memory is quite handy...
351
352In OpenVMM, we don't do this. Instead, we use a separate trait to init all-zero
353structs.
354
355**In OpenVMM, we use `FromZeros` and `FromZeros::new_zeroed()` to work with types
356that have valid all-zero representations, _without_ implying that those types
357also have valid all-zero _default_ values!**
358
359So, for the example above:
360
361```rust,ignore
362#[repr(C)]
363#[derive(zerocopy::FromZeros)]
364struct Handle {
365 opaque_handle: u16
366}
367
368let mut handle = Handle::new_zeroed(); // GOOD!
369unsafe { init_handle(&handle) };
370```
371
372Now, it's impossible for code elsewhere to obtain a `Handle` via
373`Handle::default`, and mistakenly forget to invoke `init_handle` on it.
374
375...but if it so happens that we _do_ want a `Default` impl for `Handle`, we can
376do so by _manually_ implementing `derive(Default)` ourselves:
377
378```rust,ignore
379// Default + FromZeros: `default` returns fully initialized handle
380impl Default for Handle {
381 fn default() -> Handle {
382 let mut handle = Handle::new_zeroed();
383 unsafe { init_handle(&handle) };
384 handle
385 }
386}
387```
388
389## Avoid Requiring `Debug` on Traits
390
391_Checked Automatically:_ **No**
392
393**TL;DR:** Don't do this:
394
395```rust,ignore
396trait MyTrait: std::fmt::Debug
397```
398
399Implementations of the standard library's `Debug` trait can be surprisingly large,
400and the final binary size of OpenHCL and related binaries is a major concern
401for us. Unused implementations of this trait are usually removed during the
402optimization process (like all dead code), making this a non-issue. However when
403traits, and more specifically trait objects, are involved, the compiler has a
404much more difficult time proving that implementations are unused. This can
405result in large amounts of functionally dead code ending up in the final
406binaries.
407
408If you need to implement `Debug` for a struct containing such a trait object, you
409will need to do so manually, so that you can skip over that field.
410
411Moreover, it's usually not good form to leave tracing statements that log a
412struct's `Debug` representation in production. Prefer tracing just the fields
413you're interested in, and/or connecting objects to the `Inspect` graph.
414
415## Crate Naming
416
417_Checked Automatically:_ **Yes** (via [`cargo xtask fmt --pass lints`](../dev_tools/xtask.md))
418
419**Crates must be named with underscores, not dashes and underscores used in
420folder names.**
421
422- Bad: `my-cool-crate`
423- Good: `my_cool_crate`
424
425Rust does not allow dashes in imports, with any dashes getting replaced with
426underscores when used in the code. Avoiding dashes altogether makes it easier to
427grep for crate names, and makes things more consistent across the repo.
428
429This convention is enforced by CI
430
431**Do not name crates with the words "base, util, common" or other terms that are
432overly general.**
433
434For example, consider a crate that provides a common data structure used by
435multiple devices:
436
437- Bad: `devices_common`
438- Good: `range_map`
439
440Libraries that contain the following eventually become a mishmash of unrelated
441functionality that is located there for convenience. This
442[blog post](https://dave.cheney.net/2019/01/08/avoid-package-names-like-base-util-or-common)
443goes more in-depth as to why.
444
445Instead, name things based on what they logically provide, like functionality or
446data types.
447
448## Release Gates Workflow
449
450_Triggered Manually:_ **Yes** (via GitHub labels)
451
452In addition to the standard PR gates that run in debug mode, OpenVMM also provides
453a "Release Gates" workflow that runs the same checks but compiled in release mode.
454This workflow takes significantly longer to run but can catch issues that only
455manifest in optimized builds.
456
457### When to Use Release Gates
458
459The release gates workflow should be used when:
460
461- Making changes to performance-critical code paths
462- Modifying compiler flags or build configuration
463- Implementing low-level optimizations
464- Debugging issues that only appear in release builds
465- Before merging large refactoring changes
466
467### How to Trigger Release Gates
468
469To run the release gates on your PR:
470
4711. Ensure your PR is ready for review (not in draft mode)
4722. Add the `release-ci-required` label to your PR
4733. The workflow will automatically trigger and run all checks in release mode
474
475The workflow will only run when the specific label is present, so you have full
476control over when to use this more resource-intensive testing.
477
478### Label Management
479
480Only repository maintainers can add labels to PRs. If you need release gates
481run on your PR, ask a maintainer to add the `release-ci-required` label for you.
482