GitVita
New repositoryImport repositoryNew issue
Your repositories Your dashboard Sign in

microsoft/openvmm

Public

mirrored from https://github.com/microsoft/openvmmAvailable

Watch0Fork0Star0
CodeCommitsIssuesPull requestsActionsInsightsSecurity
e8acaefba35969bbca0ffae96452cd05b47aa621

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

445lines · modeblame

RawDownload
Latest commit unavailable.
304657e9OpenVMM Team1 years ago1# Coding conventions
2
3fb1f627Daniel Prilik1 years ago3One 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.
304657e9OpenVMM Team1 years ago6
3fb1f627Daniel Prilik1 years ago7[Do your part](https://youtu.be/-_7FaWnlhS4?t=10) and keep OpenVMM clean!
304657e9OpenVMM Team1 years ago8
3fb1f627Daniel Prilik1 years ago9## `rustfmt`
304657e9OpenVMM Team1 years ago10
3fb1f627Daniel Prilik1 years ago11_Checked Automatically:_ **Yes** (via [`cargo xtask fmt rustfmt`](../dev_tools/xtask.md))
304657e9OpenVMM Team1 years ago12
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
25103b44Daniel Prilik1 years ago27```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```
304657e9OpenVMM Team1 years ago35
3fb1f627Daniel Prilik1 years ago36Assuming 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.
304657e9OpenVMM Team1 years ago39
3fb1f627Daniel Prilik1 years ago40## House Rules
304657e9OpenVMM Team1 years ago41
3fb1f627Daniel Prilik1 years ago42_Checked Automatically:_ **Yes** (via [`cargo xtask fmt house-rules`](../dev_tools/xtask.md))
304657e9OpenVMM Team1 years ago43
3fb1f627Daniel Prilik1 years ago44"House-rules" are a set of misc code lints that are specific to OpenVMM, which
45are enforced using a custom in-house tool:
304657e9OpenVMM Team1 years ago46
3fb1f627Daniel Prilik1 years ago47- 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!)
19a5bd79Steven Malis1 years ago54- justify usage of `expect(unsafe_code)` with an UNSAFETY comment
304657e9OpenVMM Team1 years ago55
3fb1f627Daniel Prilik1 years ago56Some of these lints are self explanatory, whereas others are described in more
57detail elsewhere on this page.
304657e9OpenVMM Team1 years ago58
3fb1f627Daniel Prilik1 years ago59## Unused `Cargo.toml` Dependencies
304657e9OpenVMM Team1 years ago60
3fb1f627Daniel Prilik1 years ago61_Checked Automatically:_ **Yes** (via [`cargo xtask fmt unused-deps`](../dev_tools/xtask.md))
304657e9OpenVMM Team1 years ago62
63We have an in-repo fork of
64[`cargo-machete`](https://github.com/bnjbvr/cargo-machete) that ensures
65`Cargo.toml` files only include dependencies that are actually being used.
66
67Avoiding unused dependencies makes it easier to reason about what a crate is
68doing just by looking at its dependencies, and also helps cut-down on
69incremental compile times.
70
3fb1f627Daniel Prilik1 years ago71## Formatting (`Cargo.toml`)
304657e9OpenVMM Team1 years ago72
3fb1f627Daniel Prilik1 years ago73_Checked Automatically:_ **No**
304657e9OpenVMM Team1 years ago74
75When defining dependencies in `Cargo.toml` files, please organize dependencies
76into the following groups:
77
78- crate-specific "subcrates"
79- crates under vm/
80- crates under vm/vmcore/
81- crates under support/
82- external dependencies
83
84The rationale here is that crates should be grouped according to how related to
85how _widely applicable_ they are. i.e: crates from crates.io and support are
86widely applicable outside of OpenVMM, whereas crates under vmcore/ only make
87sense within the context of OpenVMM, and crate-specific subcrates are - by
88definition - only applicable to the crate they are being imported from.
89
90Additionally, we make use of the
91[workspace dependencies](https://doc.rust-lang.org/nightly/cargo/reference/workspaces.html#the-dependencies-table)
92feature to ensure that all our dependencies stay in sync. This requires defining
93dependencies in your crate's `Cargo.toml` file and in the project's root `Cargo.toml`.
94
95So, for example:
96
97```toml
98[package]
3fb1f627Daniel Prilik1 years ago99name = "openvmm"
304657e9OpenVMM Team1 years ago100
101[dependencies]
102# crate-specific subcrates
3fb1f627Daniel Prilik1 years ago103openvmm_core.workspace = true
304657e9OpenVMM Team1 years ago104...
105
106# /vmcore
107vmcore.workspace = true
108...
109
110# /vm/devices
111firmware_uefi_custom_vars.workspace = true
112storvsp.workspace = true
113...
114
115# /support
116guid.workspace = true
117inspect.workspace = true
118inspect_proto.workspace = true
119...
120
121# external dependencies
122anyhow.workspace = true
123cfg-if.workspace = true
124clap.workspace = true
125...
126```
127
3fb1f627Daniel Prilik1 years ago128## Linting (via `clippy`)
129
130_Checked Automatically:_ **Yes**
304657e9OpenVMM Team1 years ago131
132OpenVMM uses [`cargo clippy`](https://github.com/rust-lang/rust-clippy) to
133supplement rustc's built-in lints.
134
135Assuming you've followed the guide and set up `rust-analyzer` to
3fb1f627Daniel Prilik1 years ago136[use clippy](../getting_started/suggested_dev_env.md#enabling-clippy), you should see clippy lints
304657e9OpenVMM Team1 years ago137appear inline when working on Rust code.
138
139The CI runs `cargo clippy` on every crate in the repo prior to building the
140project, and will fast-fail if it catches any warnings / errors.
141
142### Suppressing Lints
143
144In general, lints should be fixed by modifying the code to satisfy the lint.
a3f6faf0Steven Malis11 months ago145However, there are cases where a lint may need to be `expect`'d inline.
304657e9OpenVMM Team1 years ago146
147In these cases, you _must_ provide a inline comment providing reasonable
148justification for the suppressed lint.
149
150e.g:
151
152```rust
153// x86_64-unknown-linux-musl targets have a different type defn for
154// `libc::cmsghdr`, hence why these lints are being suppressed.
a3f6faf0Steven Malis11 months ago155#[expect(clippy::needless_update, clippy::useless_conversion)]
304657e9OpenVMM Team1 years ago156libc::cmsghdr {
157cmsg_level: libc::SOL_SOCKET,
158cmsg_type: libc::SCM_RIGHTS,
159cmsg_len: (size_of::<libc::cmsghdr>() + size_of_val(fds))
160.try_into()
161.unwrap(),
162..std::mem::zeroed()
163}
164```
165
3fb1f627Daniel Prilik1 years ago166### OpenVMM's `clippy` Configuration
304657e9OpenVMM Team1 years ago167
168We stick fairly close to the default set of rustc / clippy lints, though there
169are some default lints that we've decided to disabled project wide, and other
170non-default lints which we've explicitly opted into.
171
3fb1f627Daniel Prilik1 years ago172See the `[workspace.lints]` sections of OpenVMM's root
173[`Cargo.toml`](https://github.com/microsoft/openvmm?path=/Cargo.toml)
304657e9OpenVMM Team1 years ago174for a list of globally enabled/disabled lints, along with justification as to
175why certain lints have been enabled/disabled.
176
3fb1f627Daniel Prilik1 years ago177## Unsafe Code Policy
304657e9OpenVMM Team1 years ago178
25103b44Daniel Prilik1 years ago179```admonish danger
180When possible, try to avoid introducing new `unsafe` code!
181
182Before rolling your own `unsafe` code, check to see if a safe abstraction
183already exists, either in-tree, on crates.io\*, or in the standard library.
184
185\*subject to an unsafe-code audit
186```
304657e9OpenVMM Team1 years ago187
188Rather than synthesizing our own unsafe code conventions, we follow the
189guidelines outlined in the following two resources:
190
191- [The Rust `unsafe` docs](https://doc.rust-lang.org/std/keyword.unsafe.html)
192- [The Fuchsia "Unsafe code in Rust" Guidelines](https://fuchsia.googlesource.com/fuchsia/+/refs/heads/main/docs/development/languages/rust/unsafe.md)
193
194In a nutshell:
195
196- `unsafe` functions are required to include `/// # Safety` documentation
197describing any preconditions the caller must uphold when calling the function.
198- `unsafe {}` blocks are required to include a `// SAFETY:` comment describing
199how the preconditions for calling the `unsafe` function(s) within the block
200are being satisfied.
19a5bd79Steven Malis1 years ago201- `expect(unsafe_code)` annotations are required to include an `// UNSAFETY:`
304657e9OpenVMM Team1 years ago202comment justifying why the code in question needs to use `unsafe`. This
203annotation must be placed at the module or crate level.
204
205These requirements are enforced by CI, and will cause the build to fail if
206required documentation is missing.
207
208Editing a file containing unsafe code will trigger CI to automatically add the
209OpenVMM Unsafe Approvers group to your PR. This is to ensure that all unsafe code
210is audited for correctness by area experts.
211
3fb1f627Daniel Prilik1 years ago212## Uses of `cfg(target_arch = ...)` must be justified
304657e9OpenVMM Team1 years ago213
3fb1f627Daniel Prilik1 years ago214_Checked Automatically:_ **Yes** (via [`cargo xtask fmt house-rules`](../dev_tools/xtask.md))
304657e9OpenVMM Team1 years ago215
3fb1f627Daniel Prilik1 years ago216Unless you're working on something that's genuinely tied to the host's CPU
217architecture, you should use `cfg(guest_arch = ...)` instead of `cfg(target_arch = ...)`.
304657e9OpenVMM Team1 years ago218
3fb1f627Daniel Prilik1 years ago219OpenVMM is a multi-architecture VMM framework, capable of running running x64
220guests on a x64 host, as well as Aarch64 guests on Aarch64 hosts (with the
221potential of adding additional platforms in the future).
304657e9OpenVMM Team1 years ago222
3fb1f627Daniel Prilik1 years ago223At the moment, OpenVMM requires that the host architecture and guest
224architecture match. That said, it's possible that at some point in the future,
225OpenVMM may also support _mismatched_ guest/host architectures, via an emulated
226CPU virtualization backend (akin to QEMU).
304657e9OpenVMM Team1 years ago227
3fb1f627Daniel Prilik1 years ago228Having an emulated CPU backend would enable OpenVMM to support such useful
229scenarios as:
304657e9OpenVMM Team1 years ago230
231- running Arch64 guests on x86 machines
232- running x86 guests on ARM
233- running something exotic (e.g: RISC-V) on x86/ARM machines
234- ...assuming we had the bandwidth to implement + maintain something like that
235- running OpenVMM on systems without hardware-accelerated CPU virtualization enabled
236
237With these scenarios in mind, it would be short-sighted to rely entirely on
238`cfg(target_arch = ...)` to gate guest-facing arch-specific functionality, as it
239would inexorably tie the guest's arch to the host's arch, making any future
240initiatives to pry the two apart significantly more difficult!
241
242As such, the `OpenVMM` repo includes infrastructure to specify a custom,
243OpenVMM-specific `cfg(guest_arch = ...)` cfg parameter.
244
245By default, `cfg(guest_arch = ...)` will act the same way as `cfg(target_arch =
246...)`, but it can be swapped to a different architecture by setting the
6386cc75John Starks1 years ago247`OPENVMM_GUEST_TARGET` env var at compile-time.
304657e9OpenVMM Team1 years ago248
249There are very few reasons to use `cfg(target_arch = ...)` within the OpenVMM
250repo, and to enforce this rule, we have an in-house `xtask fmt house-rules`
251check that lints each use of `cfg(target_arch = ...)` to include a
252"justification" for why it's being used.
253
254e.g: `cfg(target_arch = ...)` would be applicable when feature-gating a CPU
255intrinsic (such as CPUID, or a SIMD instruction), or when implementing a
256`*-sys` crate where the underlying C API/ABI varies between architectures.
257
258...otherwise, use `cfg(guest_arch = ...)`!
259
b60f2e75Matt Kurjanowicz1 years ago260## Avoid `Default` when using `zerocopy::FromZeros`
3fb1f627Daniel Prilik1 years ago261
262_Checked Automatically:_ **No**
304657e9OpenVMM Team1 years ago263
264The rule:
265
b60f2e75Matt Kurjanowicz1 years ago266- A type can `derive(Default)` **XOR** `derive(FromZeros)`.
267- A type that is `FromZeros` can also `impl Default`, but it must be a
304657e9OpenVMM Team1 years ago268conscious, explicit choice, with justification (read: inline comment) as to
269why that particular default value was chosen.
b60f2e75Matt Kurjanowicz1 years ago270- N.B. `derive(IntoBytes)` or `derive(FromBytes)` imply `derive(FromZeros)`. That is:
271this convention applies to those types as well.
304657e9OpenVMM Team1 years ago272
273The why:
274
275- The all-zero type is often not a semantically valid `Default` value for a type
276- There are plenty of types that don't have a `Default` value, but _do_ have a
277valid all-zero repr
278
3fb1f627Daniel Prilik1 years ago279### Additional context
304657e9OpenVMM Team1 years ago280
281As per the Rust docs for [`Default::default`](https://doc.rust-lang.org/std/default/trait.Default.html#tymethod.default):
282
25103b44Daniel Prilik1 years ago283```admonish quote
284`fn default() -> Self`
285
286Returns the “default value” for a type.
287
288Default values are often some kind of initial value, identity value, or anything
289else that may make sense as a default.
290```
304657e9OpenVMM Team1 years ago291
292Notably, **default should _not_ be used as some shorthand to "zero initialize"
293values!** For most non-trivial structs, the all-zero representation is _not_ a
294semantically valid `Default`!
295
296This is true in many contexts... but one that's particularly relevant in OpenVMM
297is that of FFI via C-style APIs and ABIs.
298
299In C, it's very common for types to undergo multi-stage instantiation, where
300they are initially allocated as all-zero, and then get "filled in" by some
301secondary init code. Notably: it's quite rare for that initial "all-zero" struct
302to be a valid instance of the type!
303
3fb1f627Daniel Prilik1 years ago304#### Example: C FFI
304657e9OpenVMM Team1 years ago305
306For example, a common pattern in C libraries might look something like:
307
308```c
309struct Handle {
310uint16_t opaque_handle;
311}
312
313struct Handle handle = {0};
314init_handle(&handle);
315
316update_handle(handle, options);
317do_thing(handle);
318```
319
320In this example: it would be an error to invoke `update_handle` or `do_thing`
321with an all-zero handle, as the type hadn't finished being fully initialized.
322
323If we wanted to use this library from Rust, a "naive" approach would be to do
324something like:
325
326```rust
327#[repr(C)]
328#[derive(Default)]
329struct Handle {
330opaque_handle: u16,
331}
332
333let mut handle = Handle::default(); // BAD!
334unsafe { init_handle(&handle) };
335
336unsafe { update_handle(handle, options) };
337unsafe { do_thing(handle) };
338```
339
340While this _technically_ works... using `Default` here is kinda bogus!
341
342After all - `Handle::default()` doesn't actually call `init_handle`, which means
343the value returned by `default()` doesn't match the "promise" of the trait!
344Namely: the returned value is not semantically valid yet!
345
346In many other Rust codebase, this "overloading" of `Default` to represent both
347semantically valid value _and_ all-zero values (in FFI) is par for the course,
348as once structs get more complicated, having a `derive` that is able to fully
349init a "uninitialized" struct in-memory is quite handy...
350
351In OpenVMM, we don't do this. Instead, we use a separate trait to init all-zero
352structs.
353
b60f2e75Matt Kurjanowicz1 years ago354**In OpenVMM, we use `FromZeros` and `FromZeros::new_zeroed()` to work with types
304657e9OpenVMM Team1 years ago355that have valid all-zero representations, _without_ implying that those types
356also have valid all-zero _default_ values!**
357
358So, for the example above:
359
360```rust
361#[repr(C)]
b60f2e75Matt Kurjanowicz1 years ago362#[derive(zerocopy::FromZeros)]
304657e9OpenVMM Team1 years ago363struct Handle {
364opaque_handle: u16
365}
366
367let mut handle = Handle::new_zeroed(); // GOOD!
368unsafe { init_handle(&handle) };
369```
370
371Now, it's impossible for code elsewhere to obtain a `Handle` via
372`Handle::default`, and mistakenly forget to invoke `init_handle` on it.
373
374...but if it so happens that we _do_ want a `Default` impl for `Handle`, we can
375do so by _manually_ implementing `derive(Default)` ourselves:
376
377```rust
b60f2e75Matt Kurjanowicz1 years ago378// Default + FromZeros: `default` returns fully initialized handle
304657e9OpenVMM Team1 years ago379impl Default for Handle {
380fn default() -> Handle {
381let mut handle = Handle::new_zeroed();
382unsafe { init_handle(&handle) };
383handle
384}
385}
386```
387
3fb1f627Daniel Prilik1 years ago388## Avoid Requiring `Debug` on Traits
389
390_Checked Automatically:_ **No**
304657e9OpenVMM Team1 years ago391
392**TL;DR:** Don't do this:
393
394```rust
395trait MyTrait: std::fmt::Debug
396```
397
398Implementations of the standard library's `Debug` trait can be surprisingly large,
399and the final binary size of OpenHCL and related binaries is a major concern
400for us. Unused implementations of this trait are usually removed during the
401optimization process (like all dead code), making this a non-issue. However when
402traits, and more specifically trait objects, are involved, the compiler has a
403much more difficult time proving that implementations are unused. This can
404result in large amounts of functionally dead code ending up in the final
405binaries.
406
407If you need to implement `Debug` for a struct containing such a trait object, you
408will need to do so manually, so that you can skip over that field.
409
410Moreover, it's usually not good form to leave tracing statements that log a
411struct's `Debug` representation in production. Prefer tracing just the fields
412you're interested in, and/or connecting objects to the `Inspect` graph.
413
3fb1f627Daniel Prilik1 years ago414## Crate Naming
415
416_Checked Automatically:_ **Yes** (via [`cargo xtask fmt house-rules`](../dev_tools/xtask.md))
304657e9OpenVMM Team1 years ago417
418**Crates must be named with underscores, not dashes and underscores used in
419folder names.**
420
421- Bad: `my-cool-crate`
422- Good: `my_cool_crate`
423
424Rust does not allow dashes in imports, with any dashes getting replaced with
425underscores when used in the code. Avoiding dashes altogether makes it easier to
426grep for crate names, and makes things more consistent across the repo.
427
428This convention is enforced by CI
429
430**Do not name crates with the words "base, util, common" or other terms that are
431overly general.**
432
433For example, consider a crate that provides a common data structure used by
434multiple devices:
435
436- Bad: `devices_common`
437- Good: `range_map`
438
439Libraries that contain the following eventually become a mishmash of unrelated
440functionality that is located there for convenience. This
441[blog post](https://dave.cheney.net/2019/01/08/avoid-package-names-like-base-util-or-common)
442goes more in-depth as to why.
443
444Instead, name things based on what they logically provide, like functionality or
445data types.