|
| 1 | +.. SPDX-License-Identifier: GPL-2.0 |
| 2 | +
|
| 3 | +KVM x86 |
| 4 | +======= |
| 5 | + |
| 6 | +Foreword |
| 7 | +-------- |
| 8 | +KVM strives to be a welcoming community; contributions from newcomers are |
| 9 | +valued and encouraged. Please do not be discouraged or intimidated by the |
| 10 | +length of this document and the many rules/guidelines it contains. Everyone |
| 11 | +makes mistakes, and everyone was a newbie at some point. So long as you make |
| 12 | +an honest effort to follow KVM x86's guidelines, are receptive to feedback, |
| 13 | +and learn from any mistakes you make, you will be welcomed with open arms, not |
| 14 | +torches and pitchforks. |
| 15 | + |
| 16 | +TL;DR |
| 17 | +----- |
| 18 | +Testing is mandatory. Be consistent with established styles and patterns. |
| 19 | + |
| 20 | +Trees |
| 21 | +----- |
| 22 | +KVM x86 is currently in a transition period from being part of the main KVM |
| 23 | +tree, to being "just another KVM arch". As such, KVM x86 is split across the |
| 24 | +main KVM tree, ``git.kernel.org/pub/scm/virt/kvm/kvm.git``, and a KVM x86 |
| 25 | +specific tree, ``github.com/kvm-x86/linux.git``. |
| 26 | + |
| 27 | +Generally speaking, fixes for the current cycle are applied directly to the |
| 28 | +main KVM tree, while all development for the next cycle is routed through the |
| 29 | +KVM x86 tree. In the unlikely event that a fix for the current cycle is routed |
| 30 | +through the KVM x86 tree, it will be applied to the ``fixes`` branch before |
| 31 | +making its way to the main KVM tree. |
| 32 | + |
| 33 | +Note, this transition period is expected to last quite some time, i.e. will be |
| 34 | +the status quo for the foreseeable future. |
| 35 | + |
| 36 | +Branches |
| 37 | +~~~~~~~~ |
| 38 | +The KVM x86 tree is organized into multiple topic branches. The purpose of |
| 39 | +using finer-grained topic branches is to make it easier to keep tabs on an area |
| 40 | +of development, and to limit the collateral damage of human errors and/or buggy |
| 41 | +commits, e.g. dropping the HEAD commit of a topic branch has no impact on other |
| 42 | +in-flight commits' SHA1 hashes, and having to reject a pull request due to bugs |
| 43 | +delays only that topic branch. |
| 44 | + |
| 45 | +All topic branches, except for ``next`` and ``fixes``, are rolled into ``next`` |
| 46 | +via a Cthulhu merge on an as-needed basis, i.e. when a topic branch is updated. |
| 47 | +As a result, force pushes to ``next`` are common. |
| 48 | + |
| 49 | +Lifecycle |
| 50 | +~~~~~~~~~ |
| 51 | +Fixes that target the current release, a.k.a. mainline, are typically applied |
| 52 | +directly to the main KVM tree, i.e. do not route through the KVM x86 tree. |
| 53 | + |
| 54 | +Changes that target the next release are routed through the KVM x86 tree. Pull |
| 55 | +requests (from KVM x86 to main KVM) are sent for each KVM x86 topic branch, |
| 56 | +typically the week before Linus' opening of the merge window, e.g. the week |
| 57 | +following rc7 for "normal" releases. If all goes well, the topic branches are |
| 58 | +rolled into the main KVM pull request sent during Linus' merge window. |
| 59 | + |
| 60 | +The KVM x86 tree doesn't have its own official merge window, but there's a soft |
| 61 | +close around rc5 for new features, and a soft close around rc6 for fixes (for |
| 62 | +the next release; see above for fixes that target the current release). |
| 63 | + |
| 64 | +Timeline |
| 65 | +~~~~~~~~ |
| 66 | +Submissions are typically reviewed and applied in FIFO order, with some wiggle |
| 67 | +room for the size of a series, patches that are "cache hot", etc. Fixes, |
| 68 | +especially for the current release and or stable trees, get to jump the queue. |
| 69 | +Patches that will be taken through a non-KVM tree (most often through the tip |
| 70 | +tree) and/or have other acks/reviews also jump the queue to some extent. |
| 71 | + |
| 72 | +Note, the vast majority of review is done between rc1 and rc6, give or take. |
| 73 | +The period between rc6 and the next rc1 is used to catch up on other tasks, |
| 74 | +i.e. radio silence during this period isn't unusual. |
| 75 | + |
| 76 | +Pings to get a status update are welcome, but keep in mind the timing of the |
| 77 | +current release cycle and have realistic expectations. If you are pinging for |
| 78 | +acceptance, i.e. not just for feedback or an update, please do everything you |
| 79 | +can, within reason, to ensure that your patches are ready to be merged! Pings |
| 80 | +on series that break the build or fail tests lead to unhappy maintainers! |
| 81 | + |
| 82 | +Development |
| 83 | +----------- |
| 84 | + |
| 85 | +Base Tree/Branch |
| 86 | +~~~~~~~~~~~~~~~~ |
| 87 | +Fixes that target the current release, a.k.a. mainline, should be based on |
| 88 | +``git://git.kernel.org/pub/scm/virt/kvm/kvm.git master``. Note, fixes do not |
| 89 | +automatically warrant inclusion in the current release. There is no singular |
| 90 | +rule, but typically only fixes for bugs that are urgent, critical, and/or were |
| 91 | +introduced in the current release should target the current release. |
| 92 | + |
| 93 | +Everything else should be based on ``kvm-x86/next``, i.e. there is no need to |
| 94 | +select a specific topic branch as the base. If there are conflicts and/or |
| 95 | +dependencies across topic branches, it is the maintainer's job to sort them |
| 96 | +out. |
| 97 | + |
| 98 | +The only exception to using ``kvm-x86/next`` as the base is if a patch/series |
| 99 | +is a multi-arch series, i.e. has non-trivial modifications to common KVM code |
| 100 | +and/or has more than superficial changes to other architectures' code. Multi- |
| 101 | +arch patch/series should instead be based on a common, stable point in KVM's |
| 102 | +history, e.g. the release candidate upon which ``kvm-x86 next`` is based. If |
| 103 | +you're unsure whether a patch/series is truly multi-arch, err on the side of |
| 104 | +caution and treat it as multi-arch, i.e. use a common base. |
| 105 | + |
| 106 | +Coding Style |
| 107 | +~~~~~~~~~~~~ |
| 108 | +When it comes to style, naming, patterns, etc., consistency is the number one |
| 109 | +priority in KVM x86. If all else fails, match what already exists. |
| 110 | + |
| 111 | +With a few caveats listed below, follow the tip tree maintainers' preferred |
| 112 | +:ref:`maintainer-tip-coding-style`, as patches/series often touch both KVM and |
| 113 | +non-KVM x86 files, i.e. draw the attention of KVM *and* tip tree maintainers. |
| 114 | + |
| 115 | +Using reverse fir tree, a.k.a. reverse Christmas tree or reverse XMAS tree, for |
| 116 | +variable declarations isn't strictly required, though it is still preferred. |
| 117 | + |
| 118 | +Except for a handful of special snowflakes, do not use kernel-doc comments for |
| 119 | +functions. The vast majority of "public" KVM functions aren't truly public as |
| 120 | +they are intended only for KVM-internal consumption (there are plans to |
| 121 | +privatize KVM's headers and exports to enforce this). |
| 122 | + |
| 123 | +Comments |
| 124 | +~~~~~~~~ |
| 125 | +Write comments using imperative mood and avoid pronouns. Use comments to |
| 126 | +provide a high level overview of the code, and/or to explain why the code does |
| 127 | +what it does. Do not reiterate what the code literally does; let the code |
| 128 | +speak for itself. If the code itself is inscrutable, comments will not help. |
| 129 | + |
| 130 | +SDM and APM References |
| 131 | +~~~~~~~~~~~~~~~~~~~~~~ |
| 132 | +Much of KVM's code base is directly tied to architectural behavior defined in |
| 133 | +Intel's Software Development Manual (SDM) and AMD's Architecture Programmer’s |
| 134 | +Manual (APM). Use of "Intel's SDM" and "AMD's APM", or even just "SDM" or |
| 135 | +"APM", without additional context is a-ok. |
| 136 | + |
| 137 | +Do not reference specific sections, tables, figures, etc. by number, especially |
| 138 | +not in comments. Instead, if necessary (see below), copy-paste the relevant |
| 139 | +snippet and reference sections/tables/figures by name. The layouts of the SDM |
| 140 | +and APM are constantly changing, and so the numbers/labels aren't stable. |
| 141 | + |
| 142 | +Generally speaking, do not explicitly reference or copy-paste from the SDM or |
| 143 | +APM in comments. With few exceptions, KVM *must* honor architectural behavior, |
| 144 | +therefore it's implied that KVM behavior is emulating SDM and/or APM behavior. |
| 145 | +Note, referencing the SDM/APM in changelogs to justify the change and provide |
| 146 | +context is perfectly ok and encouraged. |
| 147 | + |
| 148 | +Shortlog |
| 149 | +~~~~~~~~ |
| 150 | +The preferred prefix format is ``KVM: <topic>:``, where ``<topic>`` is one of:: |
| 151 | + |
| 152 | + - x86 |
| 153 | + - x86/mmu |
| 154 | + - x86/pmu |
| 155 | + - x86/xen |
| 156 | + - selftests |
| 157 | + - SVM |
| 158 | + - nSVM |
| 159 | + - VMX |
| 160 | + - nVMX |
| 161 | + |
| 162 | +**DO NOT use x86/kvm!** ``x86/kvm`` is used exclusively for Linux-as-a-KVM-guest |
| 163 | +changes, i.e. for arch/x86/kernel/kvm.c. Do not use file names or complete file |
| 164 | +paths as the subject/shortlog prefix. |
| 165 | + |
| 166 | +Note, these don't align with the topics branches (the topic branches care much |
| 167 | +more about code conflicts). |
| 168 | + |
| 169 | +All names are case sensitive! ``KVM: x86:`` is good, ``kvm: vmx:`` is not. |
| 170 | + |
| 171 | +Capitalize the first word of the condensed patch description, but omit ending |
| 172 | +punctionation. E.g.:: |
| 173 | + |
| 174 | + KVM: x86: Fix a null pointer dereference in function_xyz() |
| 175 | + |
| 176 | +not:: |
| 177 | + |
| 178 | + kvm: x86: fix a null pointer dereference in function_xyz. |
| 179 | + |
| 180 | +If a patch touches multiple topics, traverse up the conceptual tree to find the |
| 181 | +first common parent (which is often simply ``x86``). When in doubt, |
| 182 | +``git log path/to/file`` should provide a reasonable hint. |
| 183 | + |
| 184 | +New topics do occasionally pop up, but please start an on-list discussion if |
| 185 | +you want to propose introducing a new topic, i.e. don't go rogue. |
| 186 | + |
| 187 | +See :ref:`the_canonical_patch_format` for more information, with one amendment: |
| 188 | +do not treat the 70-75 character limit as an absolute, hard limit. Instead, |
| 189 | +use 75 characters as a firm-but-not-hard limit, and use 80 characters as a hard |
| 190 | +limit. I.e. let the shortlog run a few characters over the standard limit if |
| 191 | +you have good reason to do so. |
| 192 | + |
| 193 | +Changelog |
| 194 | +~~~~~~~~~ |
| 195 | +Most importantly, write changelogs using imperative mood and avoid pronouns. |
| 196 | + |
| 197 | +See :ref:`describe_changes` for more information, with one amendment: lead with |
| 198 | +a short blurb on the actual changes, and then follow up with the context and |
| 199 | +background. Note! This order directly conflicts with the tip tree's preferred |
| 200 | +approach! Please follow the tip tree's preferred style when sending patches |
| 201 | +that primarily target arch/x86 code that is _NOT_ KVM code. |
| 202 | + |
| 203 | +Stating what a patch does before diving into details is preferred by KVM x86 |
| 204 | +for several reasons. First and foremost, what code is actually being changed |
| 205 | +is arguably the most important information, and so that info should be easy to |
| 206 | +find. Changelogs that bury the "what's actually changing" in a one-liner after |
| 207 | +3+ paragraphs of background make it very hard to find that information. |
| 208 | + |
| 209 | +For initial review, one could argue the "what's broken" is more important, but |
| 210 | +for skimming logs and git archaeology, the gory details matter less and less. |
| 211 | +E.g. when doing a series of "git blame", the details of each change along the |
| 212 | +way are useless, the details only matter for the culprit. Providing the "what |
| 213 | +changed" makes it easy to quickly determine whether or not a commit might be of |
| 214 | +interest. |
| 215 | + |
| 216 | +Another benefit of stating "what's changing" first is that it's almost always |
| 217 | +possible to state "what's changing" in a single sentence. Conversely, all but |
| 218 | +the most simple bugs require multiple sentences or paragraphs to fully describe |
| 219 | +the problem. If both the "what's changing" and "what's the bug" are super |
| 220 | +short then the order doesn't matter. But if one is shorter (almost always the |
| 221 | +"what's changing), then covering the shorter one first is advantageous because |
| 222 | +it's less of an inconvenience for readers/reviewers that have a strict ordering |
| 223 | +preference. E.g. having to skip one sentence to get to the context is less |
| 224 | +painful than having to skip three paragraphs to get to "what's changing". |
| 225 | + |
| 226 | +Fixes |
| 227 | +~~~~~ |
| 228 | +If a change fixes a KVM/kernel bug, add a Fixes: tag even if the change doesn't |
| 229 | +need to be backported to stable kernels, and even if the change fixes a bug in |
| 230 | +an older release. |
| 231 | + |
| 232 | +Conversely, if a fix does need to be backported, explicitly tag the patch with |
| 233 | +"Cc: stable@vger.kernel" (though the email itself doesn't need to Cc: stable); |
| 234 | +KVM x86 opts out of backporting Fixes: by default. Some auto-selected patches |
| 235 | +do get backported, but require explicit maintainer approval (search MANUALSEL). |
| 236 | + |
| 237 | +Function References |
| 238 | +~~~~~~~~~~~~~~~~~~~ |
| 239 | +When a function is mentioned in a comment, changelog, or shortlog (or anywhere |
| 240 | +for that matter), use the format ``function_name()``. The parentheses provide |
| 241 | +context and disambiguate the reference. |
| 242 | + |
| 243 | +Testing |
| 244 | +------- |
| 245 | +At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m |
| 246 | +KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs |
| 247 | +isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and |
| 248 | +X86_64 are particularly interesting knobs to turn. |
| 249 | + |
| 250 | +Running KVM selftests and KVM-unit-tests is also mandatory (and stating the |
| 251 | +obvious, the tests need to pass). The only exception is for changes that have |
| 252 | +negligible probability of affecting runtime behavior, e.g. patches that only |
| 253 | +modify comments. When possible and relevant, testing on both Intel and AMD is |
| 254 | +strongly preferred. Booting an actual VM is encouraged, but not mandatory. |
| 255 | + |
| 256 | +For changes that touch KVM's shadow paging code, running with TDP (EPT/NPT) |
| 257 | +disabled is mandatory. For changes that affect common KVM MMU code, running |
| 258 | +with TDP disabled is strongly encouraged. For all other changes, if the code |
| 259 | +being modified depends on and/or interacts with a module param, testing with |
| 260 | +the relevant settings is mandatory. |
| 261 | + |
| 262 | +Note, KVM selftests and KVM-unit-tests do have known failures. If you suspect |
| 263 | +a failure is not due to your changes, verify that the *exact same* failure |
| 264 | +occurs with and without your changes. |
| 265 | + |
| 266 | +Changes that touch reStructured Text documentation, i.e. .rst files, must build |
| 267 | +htmldocs cleanly, i.e. with no new warnings or errors. |
| 268 | + |
| 269 | +If you can't fully test a change, e.g. due to lack of hardware, clearly state |
| 270 | +what level of testing you were able to do, e.g. in the cover letter. |
| 271 | + |
| 272 | +New Features |
| 273 | +~~~~~~~~~~~~ |
| 274 | +With one exception, new features *must* come with test coverage. KVM specific |
| 275 | +tests aren't strictly required, e.g. if coverage is provided by running a |
| 276 | +sufficiently enabled guest VM, or by running a related kernel selftest in a VM, |
| 277 | +but dedicated KVM tests are preferred in all cases. Negative testcases in |
| 278 | +particular are mandatory for enabling of new hardware features as error and |
| 279 | +exception flows are rarely exercised simply by running a VM. |
| 280 | + |
| 281 | +The only exception to this rule is if KVM is simply advertising support for a |
| 282 | +feature via KVM_GET_SUPPORTED_CPUID, i.e. for instructions/features that KVM |
| 283 | +can't prevent a guest from using and for which there is no true enabling. |
| 284 | + |
| 285 | +Note, "new features" does not just mean "new hardware features"! New features |
| 286 | +that can't be well validated using existing KVM selftests and/or KVM-unit-tests |
| 287 | +must come with tests. |
| 288 | + |
| 289 | +Posting new feature development without tests to get early feedback is more |
| 290 | +than welcome, but such submissions should be tagged RFC, and the cover letter |
| 291 | +should clearly state what type of feedback is requested/expected. Do not abuse |
| 292 | +the RFC process; RFCs will typically not receive in-depth review. |
| 293 | + |
| 294 | +Bug Fixes |
| 295 | +~~~~~~~~~ |
| 296 | +Except for "obvious" found-by-inspection bugs, fixes must be accompanied by a |
| 297 | +reproducer for the bug being fixed. In many cases the reproducer is implicit, |
| 298 | +e.g. for build errors and test failures, but it should still be clear to |
| 299 | +readers what is broken and how to verify the fix. Some leeway is given for |
| 300 | +bugs that are found via non-public workloads/tests, but providing regression |
| 301 | +tests for such bugs is strongly preferred. |
| 302 | + |
| 303 | +In general, regression tests are preferred for any bug that is not trivial to |
| 304 | +hit. E.g. even if the bug was originally found by a fuzzer such as syzkaller, |
| 305 | +a targeted regression test may be warranted if the bug requires hitting a |
| 306 | +one-in-a-million type race condition. |
| 307 | + |
| 308 | +Note, KVM bugs are rarely urgent *and* non-trivial to reproduce. Ask yourself |
| 309 | +if a bug is really truly the end of the world before posting a fix without a |
| 310 | +reproducer. |
| 311 | + |
| 312 | +Posting |
| 313 | +------- |
| 314 | + |
| 315 | +Links |
| 316 | +~~~~~ |
| 317 | +Do not explicitly reference bug reports, prior versions of a patch/series, etc. |
| 318 | +via ``In-Reply-To:`` headers. Using ``In-Reply-To:`` becomes an unholy mess |
| 319 | +for large series and/or when the version count gets high, and ``In-Reply-To:`` |
| 320 | +is useless for anyone that doesn't have the original message, e.g. if someone |
| 321 | +wasn't Cc'd on the bug report or if the list of recipients changes between |
| 322 | +versions. |
| 323 | + |
| 324 | +To link to a bug report, previous version, or anything of interest, use lore |
| 325 | +links. For referencing previous version(s), generally speaking do not include |
| 326 | +a Link: in the changelog as there is no need to record the history in git, i.e. |
| 327 | +put the link in the cover letter or in the section git ignores. Do provide a |
| 328 | +formal Link: for bug reports and/or discussions that led to the patch. The |
| 329 | +context of why a change was made is highly valuable for future readers. |
| 330 | + |
| 331 | +Git Base |
| 332 | +~~~~~~~~ |
| 333 | +If you are using git version 2.9.0 or later (Googlers, this is all of you!), |
| 334 | +use ``git format-patch`` with the ``--base`` flag to automatically include the |
| 335 | +base tree information in the generated patches. |
| 336 | + |
| 337 | +Note, ``--base=auto`` works as expected if and only if a branch's upstream is |
| 338 | +set to the base topic branch, e.g. it will do the wrong thing if your upstream |
| 339 | +is set to your personal repository for backup purposes. An alternative "auto" |
| 340 | +solution is to derive the names of your development branches based on their |
| 341 | +KVM x86 topic, and feed that into ``--base``. E.g. ``x86/pmu/my_branch_name``, |
| 342 | +and then write a small wrapper to extract ``pmu`` from the current branch name |
| 343 | +to yield ``--base=x/pmu``, where ``x`` is whatever name your repository uses to |
| 344 | +track the KVM x86 remote. |
| 345 | + |
| 346 | +Co-Posting Tests |
| 347 | +~~~~~~~~~~~~~~~~ |
| 348 | +KVM selftests that are associated with KVM changes, e.g. regression tests for |
| 349 | +bug fixes, should be posted along with the KVM changes as a single series. The |
| 350 | +standard kernel rules for bisection apply, i.e. KVM changes that result in test |
| 351 | +failures should be ordered after the selftests updates, and vice versa, new |
| 352 | +tests that fail due to KVM bugs should be ordered after the KVM fixes. |
| 353 | + |
| 354 | +KVM-unit-tests should *always* be posted separately. Tools, e.g. b4 am, don't |
| 355 | +know that KVM-unit-tests is a separate repository and get confused when patches |
| 356 | +in a series apply on different trees. To tie KVM-unit-tests patches back to |
| 357 | +KVM patches, first post the KVM changes and then provide a lore Link: to the |
| 358 | +KVM patch/series in the KVM-unit-tests patch(es). |
| 359 | + |
| 360 | +Notifications |
| 361 | +------------- |
| 362 | +When a patch/series is officially accepted, a notification email will be sent |
| 363 | +in reply to the original posting (cover letter for multi-patch series). The |
| 364 | +notification will include the tree and topic branch, along with the SHA1s of |
| 365 | +the commits of applied patches. |
| 366 | + |
| 367 | +If a subset of patches is applied, this will be clearly stated in the |
| 368 | +notification. Unless stated otherwise, it's implied that any patches in the |
| 369 | +series that were not accepted need more work and should be submitted in a new |
| 370 | +version. |
| 371 | + |
| 372 | +If for some reason a patch is dropped after officially being accepted, a reply |
| 373 | +will be sent to the notification email explaining why the patch was dropped, as |
| 374 | +well as the next steps. |
| 375 | + |
| 376 | +SHA1 Stability |
| 377 | +~~~~~~~~~~~~~~ |
| 378 | +SHA1s are not 100% guaranteed to be stable until they land in Linus' tree! A |
| 379 | +SHA1 is *usually* stable once a notification has been sent, but things happen. |
| 380 | +In most cases, an update to the notification email be provided if an applied |
| 381 | +patch's SHA1 changes. However, in some scenarios, e.g. if all KVM x86 branches |
| 382 | +need to be rebased, individual notifications will not be given. |
| 383 | + |
| 384 | +Vulnerabilities |
| 385 | +--------------- |
| 386 | +Bugs that can be exploited by the guest to attack the host (kernel or |
| 387 | +userspace), or that can be exploited by a nested VM to *its* host (L2 attacking |
| 388 | +L1), are of particular interest to KVM. Please follow the protocol for |
| 389 | +:ref:`securitybugs` if you suspect a bug can lead to an escape, data leak, etc. |
| 390 | + |
0 commit comments