391 lines · plain
1.. SPDX-License-Identifier: GPL-2.02 3KVM x864=======5 6Foreword7--------8KVM strives to be a welcoming community; contributions from newcomers are9valued and encouraged. Please do not be discouraged or intimidated by the10length of this document and the many rules/guidelines it contains. Everyone11makes mistakes, and everyone was a newbie at some point. So long as you make12an honest effort to follow KVM x86's guidelines, are receptive to feedback,13and learn from any mistakes you make, you will be welcomed with open arms, not14torches and pitchforks.15 16TL;DR17-----18Testing is mandatory. Be consistent with established styles and patterns.19 20Trees21-----22KVM x86 is currently in a transition period from being part of the main KVM23tree, to being "just another KVM arch". As such, KVM x86 is split across the24main KVM tree, ``git.kernel.org/pub/scm/virt/kvm/kvm.git``, and a KVM x8625specific tree, ``github.com/kvm-x86/linux.git``.26 27Generally speaking, fixes for the current cycle are applied directly to the28main KVM tree, while all development for the next cycle is routed through the29KVM x86 tree. In the unlikely event that a fix for the current cycle is routed30through the KVM x86 tree, it will be applied to the ``fixes`` branch before31making its way to the main KVM tree.32 33Note, this transition period is expected to last quite some time, i.e. will be34the status quo for the foreseeable future.35 36Branches37~~~~~~~~38The KVM x86 tree is organized into multiple topic branches. The purpose of39using finer-grained topic branches is to make it easier to keep tabs on an area40of development, and to limit the collateral damage of human errors and/or buggy41commits, e.g. dropping the HEAD commit of a topic branch has no impact on other42in-flight commits' SHA1 hashes, and having to reject a pull request due to bugs43delays only that topic branch.44 45All topic branches, except for ``next`` and ``fixes``, are rolled into ``next``46via a Cthulhu merge on an as-needed basis, i.e. when a topic branch is updated.47As a result, force pushes to ``next`` are common.48 49Lifecycle50~~~~~~~~~51Fixes that target the current release, a.k.a. mainline, are typically applied52directly to the main KVM tree, i.e. do not route through the KVM x86 tree.53 54Changes that target the next release are routed through the KVM x86 tree. Pull55requests (from KVM x86 to main KVM) are sent for each KVM x86 topic branch,56typically the week before Linus' opening of the merge window, e.g. the week57following rc7 for "normal" releases. If all goes well, the topic branches are58rolled into the main KVM pull request sent during Linus' merge window.59 60The KVM x86 tree doesn't have its own official merge window, but there's a soft61close around rc5 for new features, and a soft close around rc6 for fixes (for62the next release; see above for fixes that target the current release).63 64Timeline65~~~~~~~~66Submissions are typically reviewed and applied in FIFO order, with some wiggle67room for the size of a series, patches that are "cache hot", etc. Fixes,68especially for the current release and or stable trees, get to jump the queue.69Patches that will be taken through a non-KVM tree (most often through the tip70tree) and/or have other acks/reviews also jump the queue to some extent.71 72Note, the vast majority of review is done between rc1 and rc6, give or take.73The period between rc6 and the next rc1 is used to catch up on other tasks,74i.e. radio silence during this period isn't unusual.75 76Pings to get a status update are welcome, but keep in mind the timing of the77current release cycle and have realistic expectations. If you are pinging for78acceptance, i.e. not just for feedback or an update, please do everything you79can, within reason, to ensure that your patches are ready to be merged! Pings80on series that break the build or fail tests lead to unhappy maintainers!81 82Development83-----------84 85Base Tree/Branch86~~~~~~~~~~~~~~~~87Fixes that target the current release, a.k.a. mainline, should be based on88``git://git.kernel.org/pub/scm/virt/kvm/kvm.git master``. Note, fixes do not89automatically warrant inclusion in the current release. There is no singular90rule, but typically only fixes for bugs that are urgent, critical, and/or were91introduced in the current release should target the current release.92 93Everything else should be based on ``kvm-x86/next``, i.e. there is no need to94select a specific topic branch as the base. If there are conflicts and/or95dependencies across topic branches, it is the maintainer's job to sort them96out.97 98The only exception to using ``kvm-x86/next`` as the base is if a patch/series99is a multi-arch series, i.e. has non-trivial modifications to common KVM code100and/or has more than superficial changes to other architectures' code. Multi-101arch patch/series should instead be based on a common, stable point in KVM's102history, e.g. the release candidate upon which ``kvm-x86 next`` is based. If103you're unsure whether a patch/series is truly multi-arch, err on the side of104caution and treat it as multi-arch, i.e. use a common base.105 106Coding Style107~~~~~~~~~~~~108When it comes to style, naming, patterns, etc., consistency is the number one109priority in KVM x86. If all else fails, match what already exists.110 111With a few caveats listed below, follow the tip tree maintainers' preferred112:ref:`maintainer-tip-coding-style`, as patches/series often touch both KVM and113non-KVM x86 files, i.e. draw the attention of KVM *and* tip tree maintainers.114 115Using reverse fir tree, a.k.a. reverse Christmas tree or reverse XMAS tree, for116variable declarations isn't strictly required, though it is still preferred.117 118Except for a handful of special snowflakes, do not use kernel-doc comments for119functions. The vast majority of "public" KVM functions aren't truly public as120they are intended only for KVM-internal consumption (there are plans to121privatize KVM's headers and exports to enforce this).122 123Comments124~~~~~~~~125Write comments using imperative mood and avoid pronouns. Use comments to126provide a high level overview of the code, and/or to explain why the code does127what it does. Do not reiterate what the code literally does; let the code128speak for itself. If the code itself is inscrutable, comments will not help.129 130SDM and APM References131~~~~~~~~~~~~~~~~~~~~~~132Much of KVM's code base is directly tied to architectural behavior defined in133Intel's Software Development Manual (SDM) and AMD's Architecture Programmer’s134Manual (APM). Use of "Intel's SDM" and "AMD's APM", or even just "SDM" or135"APM", without additional context is a-ok.136 137Do not reference specific sections, tables, figures, etc. by number, especially138not in comments. Instead, if necessary (see below), copy-paste the relevant139snippet and reference sections/tables/figures by name. The layouts of the SDM140and APM are constantly changing, and so the numbers/labels aren't stable.141 142Generally speaking, do not explicitly reference or copy-paste from the SDM or143APM in comments. With few exceptions, KVM *must* honor architectural behavior,144therefore it's implied that KVM behavior is emulating SDM and/or APM behavior.145Note, referencing the SDM/APM in changelogs to justify the change and provide146context is perfectly ok and encouraged.147 148Shortlog149~~~~~~~~150The preferred prefix format is ``KVM: <topic>:``, where ``<topic>`` is one of::151 152 - x86153 - x86/mmu154 - x86/pmu155 - x86/xen156 - selftests157 - SVM158 - nSVM159 - VMX160 - nVMX161 162**DO NOT use x86/kvm!** ``x86/kvm`` is used exclusively for Linux-as-a-KVM-guest163changes, i.e. for arch/x86/kernel/kvm.c. Do not use file names or complete file164paths as the subject/shortlog prefix.165 166Note, these don't align with the topics branches (the topic branches care much167more about code conflicts).168 169All names are case sensitive! ``KVM: x86:`` is good, ``kvm: vmx:`` is not.170 171Capitalize the first word of the condensed patch description, but omit ending172punctionation. E.g.::173 174 KVM: x86: Fix a null pointer dereference in function_xyz()175 176not::177 178 kvm: x86: fix a null pointer dereference in function_xyz.179 180If a patch touches multiple topics, traverse up the conceptual tree to find the181first common parent (which is often simply ``x86``). When in doubt,182``git log path/to/file`` should provide a reasonable hint.183 184New topics do occasionally pop up, but please start an on-list discussion if185you want to propose introducing a new topic, i.e. don't go rogue.186 187See :ref:`the_canonical_patch_format` for more information, with one amendment:188do not treat the 70-75 character limit as an absolute, hard limit. Instead,189use 75 characters as a firm-but-not-hard limit, and use 80 characters as a hard190limit. I.e. let the shortlog run a few characters over the standard limit if191you have good reason to do so.192 193Changelog194~~~~~~~~~195Most importantly, write changelogs using imperative mood and avoid pronouns.196 197See :ref:`describe_changes` for more information, with one amendment: lead with198a short blurb on the actual changes, and then follow up with the context and199background. Note! This order directly conflicts with the tip tree's preferred200approach! Please follow the tip tree's preferred style when sending patches201that primarily target arch/x86 code that is _NOT_ KVM code.202 203Stating what a patch does before diving into details is preferred by KVM x86204for several reasons. First and foremost, what code is actually being changed205is arguably the most important information, and so that info should be easy to206find. Changelogs that bury the "what's actually changing" in a one-liner after2073+ paragraphs of background make it very hard to find that information.208 209For initial review, one could argue the "what's broken" is more important, but210for skimming logs and git archaeology, the gory details matter less and less.211E.g. when doing a series of "git blame", the details of each change along the212way are useless, the details only matter for the culprit. Providing the "what213changed" makes it easy to quickly determine whether or not a commit might be of214interest.215 216Another benefit of stating "what's changing" first is that it's almost always217possible to state "what's changing" in a single sentence. Conversely, all but218the most simple bugs require multiple sentences or paragraphs to fully describe219the problem. If both the "what's changing" and "what's the bug" are super220short then the order doesn't matter. But if one is shorter (almost always the221"what's changing), then covering the shorter one first is advantageous because222it's less of an inconvenience for readers/reviewers that have a strict ordering223preference. E.g. having to skip one sentence to get to the context is less224painful than having to skip three paragraphs to get to "what's changing".225 226Fixes227~~~~~228If a change fixes a KVM/kernel bug, add a Fixes: tag even if the change doesn't229need to be backported to stable kernels, and even if the change fixes a bug in230an older release.231 232Conversely, if a fix does need to be backported, explicitly tag the patch with233"Cc: stable@vger.kernel" (though the email itself doesn't need to Cc: stable);234KVM x86 opts out of backporting Fixes: by default. Some auto-selected patches235do get backported, but require explicit maintainer approval (search MANUALSEL).236 237Function References238~~~~~~~~~~~~~~~~~~~239When a function is mentioned in a comment, changelog, or shortlog (or anywhere240for that matter), use the format ``function_name()``. The parentheses provide241context and disambiguate the reference.242 243Testing244-------245At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m246KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs247isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and248X86_64 are particularly interesting knobs to turn.249 250Running KVM selftests and KVM-unit-tests is also mandatory (and stating the251obvious, the tests need to pass). The only exception is for changes that have252negligible probability of affecting runtime behavior, e.g. patches that only253modify comments. When possible and relevant, testing on both Intel and AMD is254strongly preferred. Booting an actual VM is encouraged, but not mandatory.255 256For changes that touch KVM's shadow paging code, running with TDP (EPT/NPT)257disabled is mandatory. For changes that affect common KVM MMU code, running258with TDP disabled is strongly encouraged. For all other changes, if the code259being modified depends on and/or interacts with a module param, testing with260the relevant settings is mandatory.261 262Note, KVM selftests and KVM-unit-tests do have known failures. If you suspect263a failure is not due to your changes, verify that the *exact same* failure264occurs with and without your changes.265 266Changes that touch reStructured Text documentation, i.e. .rst files, must build267htmldocs cleanly, i.e. with no new warnings or errors.268 269If you can't fully test a change, e.g. due to lack of hardware, clearly state270what level of testing you were able to do, e.g. in the cover letter.271 272New Features273~~~~~~~~~~~~274With one exception, new features *must* come with test coverage. KVM specific275tests aren't strictly required, e.g. if coverage is provided by running a276sufficiently enabled guest VM, or by running a related kernel selftest in a VM,277but dedicated KVM tests are preferred in all cases. Negative testcases in278particular are mandatory for enabling of new hardware features as error and279exception flows are rarely exercised simply by running a VM.280 281The only exception to this rule is if KVM is simply advertising support for a282feature via KVM_GET_SUPPORTED_CPUID, i.e. for instructions/features that KVM283can't prevent a guest from using and for which there is no true enabling.284 285Note, "new features" does not just mean "new hardware features"! New features286that can't be well validated using existing KVM selftests and/or KVM-unit-tests287must come with tests.288 289Posting new feature development without tests to get early feedback is more290than welcome, but such submissions should be tagged RFC, and the cover letter291should clearly state what type of feedback is requested/expected. Do not abuse292the RFC process; RFCs will typically not receive in-depth review.293 294Bug Fixes295~~~~~~~~~296Except for "obvious" found-by-inspection bugs, fixes must be accompanied by a297reproducer for the bug being fixed. In many cases the reproducer is implicit,298e.g. for build errors and test failures, but it should still be clear to299readers what is broken and how to verify the fix. Some leeway is given for300bugs that are found via non-public workloads/tests, but providing regression301tests for such bugs is strongly preferred.302 303In general, regression tests are preferred for any bug that is not trivial to304hit. E.g. even if the bug was originally found by a fuzzer such as syzkaller,305a targeted regression test may be warranted if the bug requires hitting a306one-in-a-million type race condition.307 308Note, KVM bugs are rarely urgent *and* non-trivial to reproduce. Ask yourself309if a bug is really truly the end of the world before posting a fix without a310reproducer.311 312Posting313-------314 315Links316~~~~~317Do not explicitly reference bug reports, prior versions of a patch/series, etc.318via ``In-Reply-To:`` headers. Using ``In-Reply-To:`` becomes an unholy mess319for large series and/or when the version count gets high, and ``In-Reply-To:``320is useless for anyone that doesn't have the original message, e.g. if someone321wasn't Cc'd on the bug report or if the list of recipients changes between322versions.323 324To link to a bug report, previous version, or anything of interest, use lore325links. For referencing previous version(s), generally speaking do not include326a Link: in the changelog as there is no need to record the history in git, i.e.327put the link in the cover letter or in the section git ignores. Do provide a328formal Link: for bug reports and/or discussions that led to the patch. The329context of why a change was made is highly valuable for future readers.330 331Git Base332~~~~~~~~333If you are using git version 2.9.0 or later (Googlers, this is all of you!),334use ``git format-patch`` with the ``--base`` flag to automatically include the335base tree information in the generated patches.336 337Note, ``--base=auto`` works as expected if and only if a branch's upstream is338set to the base topic branch, e.g. it will do the wrong thing if your upstream339is set to your personal repository for backup purposes. An alternative "auto"340solution is to derive the names of your development branches based on their341KVM x86 topic, and feed that into ``--base``. E.g. ``x86/pmu/my_branch_name``,342and then write a small wrapper to extract ``pmu`` from the current branch name343to yield ``--base=x/pmu``, where ``x`` is whatever name your repository uses to344track the KVM x86 remote.345 346Co-Posting Tests347~~~~~~~~~~~~~~~~348KVM selftests that are associated with KVM changes, e.g. regression tests for349bug fixes, should be posted along with the KVM changes as a single series. The350standard kernel rules for bisection apply, i.e. KVM changes that result in test351failures should be ordered after the selftests updates, and vice versa, new352tests that fail due to KVM bugs should be ordered after the KVM fixes.353 354KVM-unit-tests should *always* be posted separately. Tools, e.g. b4 am, don't355know that KVM-unit-tests is a separate repository and get confused when patches356in a series apply on different trees. To tie KVM-unit-tests patches back to357KVM patches, first post the KVM changes and then provide a lore Link: to the358KVM patch/series in the KVM-unit-tests patch(es).359 360Notifications361-------------362When a patch/series is officially accepted, a notification email will be sent363in reply to the original posting (cover letter for multi-patch series). The364notification will include the tree and topic branch, along with the SHA1s of365the commits of applied patches.366 367If a subset of patches is applied, this will be clearly stated in the368notification. Unless stated otherwise, it's implied that any patches in the369series that were not accepted need more work and should be submitted in a new370version.371 372If for some reason a patch is dropped after officially being accepted, a reply373will be sent to the notification email explaining why the patch was dropped, as374well as the next steps.375 376SHA1 Stability377~~~~~~~~~~~~~~378SHA1s are not 100% guaranteed to be stable until they land in Linus' tree! A379SHA1 is *usually* stable once a notification has been sent, but things happen.380In most cases, an update to the notification email be provided if an applied381patch's SHA1 changes. However, in some scenarios, e.g. if all KVM x86 branches382need to be rebased, individual notifications will not be given.383 384Vulnerabilities385---------------386Bugs that can be exploited by the guest to attack the host (kernel or387userspace), or that can be exploited by a nested VM to *its* host (L2 attacking388L1), are of particular interest to KVM. Please follow the protocol for389:ref:`securitybugs` if you suspect a bug can lead to an escape, data leak, etc.390 391