* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-17 22:54 ` [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86 Sean Christopherson
@ 2023-02-18 1:52 ` Mingwei Zhang
2023-02-22 0:29 ` Sean Christopherson
2023-02-20 8:10 ` Yuan Yao
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Mingwei Zhang @ 2023-02-18 1:52 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, David Woodhouse, Emanuele Giuseppe Esposito,
Gavin Shan, Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson,
Jing Liu, Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu,
Li RongQing, Maciej S . Szmigiero, Maxim Levitsky, Michael Roth,
Michal Luczaj, Nikunj A Dadhania, Paul Durrant, Peng Hao,
Peter Gonda, Peter Xu, Robert Hoo, Suravee Suthikulpanit,
Tom Lendacky, Vipin Sharma, Vitaly Kuznetsov, Wanpeng Li,
Wei Wang, Xiaoyao Li, Yu Zhang, Zhenzhong Duan, kvm,
linux-kernel
On Fri, Feb 17, 2023, Sean Christopherson wrote:
> Add a KVM x86 doc to the subsystem/maintainer handbook section to explain
> how KVM x86 (currently) operates as a sub-subsystem, and to soapbox on
> the rules and expectations for contributing to KVM x86.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> .../process/maintainer-handbooks.rst | 1 +
> Documentation/process/maintainer-kvm-x86.rst | 347 ++++++++++++++++++
> MAINTAINERS | 1 +
> 3 files changed, 349 insertions(+)
> create mode 100644 Documentation/process/maintainer-kvm-x86.rst
>
> diff --git a/Documentation/process/maintainer-handbooks.rst b/Documentation/process/maintainer-handbooks.rst
> index d783060b4cc6..d12cbbe2b7df 100644
> --- a/Documentation/process/maintainer-handbooks.rst
> +++ b/Documentation/process/maintainer-handbooks.rst
> @@ -17,3 +17,4 @@ Contents:
>
> maintainer-tip
> maintainer-netdev
> + maintainer-kvm-x86
> diff --git a/Documentation/process/maintainer-kvm-x86.rst b/Documentation/process/maintainer-kvm-x86.rst
> new file mode 100644
> index 000000000000..ac81a42a32a3
> --- /dev/null
> +++ b/Documentation/process/maintainer-kvm-x86.rst
> @@ -0,0 +1,347 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +KVM x86
> +=======
> +
> +TL;DR
> +-----
> +Testing is mandatory. Be consistent with established styles and patterns.
> +
> +Trees
> +-----
> +KVM x86 is currently in a transition period from being part of the main KVM
> +tree, to being "just another KVM arch". As such, KVM x86 is split across the
> +main KVM tree, ``git.kernel.org/pub/scm/virt/kvm/kvm.git``, and a KVM x86
> +specific tree, ``github.com/kvm-x86/linux.git``.
> +
> +Generally speaking, fixes for the current cycle are applied directly to the
> +main KVM tree, while all development for the next cycle is routed through the
> +KVM x86 tree.
> +
> +Note, this transition period is expected to last quite some time, i.e. will be
> +the status quo for the foreseeable future.
> +
> +Branches
> +~~~~~~~~
> +The KVM x86 tree is organized into multiple topic branches. The purpose of
> +using finer-grained topic branches is to make it easier to keep tabs on an area
> +of development, and to limit the collateral damage of human errors and/or buggy
> +commits, e.g. dropping the HEAD commit of a topic branch has no impact on other
> +in-flight commits' SHA1 hashes, and having to reject a pull request due to bugs
> +delays only that topic branch.
> +
> +All topic branches, except for ``next`` and ``fixes``, are rolled into ``next``
> +via a cthulu merge on an as-needed basis, i.e. when a topic branch is updated.
> +As a result, force pushes to ``next`` are common.
> +
> +Lifecycle
> +~~~~~~~~~
> +Pull requests for the next release cycle are sent to the main KVM tree, one
> +for each KVM x86 topic branch. If all goes well, the topic branches are rolled
> +into the main KVM pull request sent during Linus' merge window. Pull requests
> +for KVM x86 branches are typically made the week before Linus' opening of the
> +merge window, e.g. the week following rc7 for "normal" releases.
> +
> +The KVM x86 tree doesn't have its own official merge window, but there's a soft
> +close around rc5 for new features, and a soft close around rc6 for fixes.
> +
> +Timeline
> +~~~~~~~~
> +Submissions are typically reviewed and applied in FIFO order, with some wiggle
> +room for the size of a series, patches that are "cache hot", etc. Fixes,
> +especially for the current release and or stable trees, get to jump the queue.
> +Patches that will be taken through a non-KVM tree (most often through the tip
> +tree) and/or have other acks/reviews also jump the queue to some extent.
> +
> +Note, the vast majority of review is done between rc1 and rc6, give or take.
> +The period between rc6 and the next rc1 is used to catch up on other tasks,
> +i.e. radio silence during this period isn't unusual.
> +
> +Pings to get a status update are welcome, but keep in mind the timing of the
> +current release cycle and have realistic expectations. If you are pinging for
> +acceptance, i.e. not just for feedback or an update, please do everything you
> +can, within reason, to ensure that your patches are ready to be merged! Pings
> +on series that break the build or fail tests lead to unhappy maintainers!
> +
> +Development
> +-----------
> +
> +Base Tree/Branch
> +~~~~~~~~~~~~~~~~
> +Fixes that target mainline, i.e. the current release, should be based on
> +``git://git.kernel.org/pub/scm/virt/kvm/kvm.git master``.
> +
> +Everything else should be based on a kvm-x86 topic branch. If there is no
> +obvious fit, use ``misc``. Unless a patch/series depends on and/or conflicts
> +with multiple topic branches, do not use ``next`` as a base. Because ``next``
> +is force-pushed on a regular basis, depending on when others fetch ``next``,
> +they may or may not have the relevant objects in their local git tree.
> +
> +Coding Style
> +~~~~~~~~~~~~
> +When it comes to style, naming, patterns, etc., consistency is the number one
> +priority in KVM x86. If all else fails, match what already exists.
> +
> +With a few caveats listed below, follow the tip tree maintainers' preferred
> +:ref:`maintainer-tip-coding-style`, as patches/series often touch both KVM and
> +non-KVM x86 files, i.e. draw the attention of KVM *and* tip tree maintainers.
> +
> +Using reverse fir tree for variable declarations isn't strictly required,
> +though it is still preferred.
What is the 'reverse fir tree'? Maybe, "Reverse Christmas Tree" is
better to understand.
> +
> +Except for a handful of special snowflakes, do not use kernel-doc comments for
> +functions. The vast majority of "public" KVM functions aren't truly public as
> +they are intended only for KVM-internal consumption (there are plans to
> +privatize KVM's headers and exports to enforce this).
> +
> +Comments
> +~~~~~~~~
> +Write comments using imperative mood and avoid pronouns. Use comments to
> +provide a high level overview of the code, and/or to explain why the code does
> +what it does. Do not reiterate what the code literally does; let the code
> +speak for itself. If the code itself is inscrutable, comments will not help.
> +
> +SDM and APM References
> +~~~~~~~~~~~~~~~~~~~~~~
> +Much of KVM's code base is directly tied to architectural behavior defined in
> +Intel's Software Development Manual (SDM) and AMD's Architecture Programmer’s
> +Manual (APM). Use of "Intel's SDM" and "AMD's APM", or even just "SDM" or
> +"APM", without additional context is a-ok.
> +
> +Do not reference specific sections, tables, figures, etc. by number, especially
> +not in comments. Instead, copy-paste the relevant snippet (if warranted), and
> +reference sections/tables/figures by name. The layouts of the SDM and APM are
> +constantly changing, and so the numbers/labels aren't stable/consistent.
> +
> +Generally speaking, do not copy-paste SDM or APM snippets into comments. With
> +few exceptions, KVM *must* honor architectural behavior, therefore it's implied
> +that KVM behavior is emulating SDM and/or APM behavior.
> +
> +Shortlog
> +~~~~~~~~
> +The preferred prefix format is ``KVM: <topic>:``, where ``<topic>`` is one of::
> +
> + - x86
> + - x86/mmu
> + - x86/pmu
> + - x86/xen
> + - selftests
> + - SVM
> + - nSVM
> + - VMX
> + - nVMX
> +
> +**DO NOT use x86/kvm!** ``x86/kvm`` is used exclusively for Linux-as-a-KVM-guest
> +changes, i.e. for arch/x86/kernel/kvm.c.
> +
> +Note, these don't align with the topics branches (the topic branches care much
> +more about code conflicts).
> +
> +All names are case sensitive! ``KVM: x86:`` is good, ``kvm: vmx:`` is not.
> +
> +Capitalize the first word of the condensed patch description, but omit ending
> +punctionation. E.g.::
> +
> + KVM: x86: Fix a null pointer dererence in function_xyz()
> +
> +not::
> +
> + kvm: x86: fix a null pointer dererence in function_xyz.
> +
> +If a patch touches multiple topics, traverse up the conceptual tree to find the
> +first common parent (which is often simply ``x86``). When in doubt,
> +``git log path/to/file`` should provide a reasonable hint.
> +
> +New topics do occasionally pop up, but please start an on-list discussion if
> +you want to propose introducing a new topic, i.e. don't go rogue.
> +
> +Do not use file names or complete file paths as the subject/shortlog prefix.
> +
> +Changelog
> +~~~~~~~~~
> +Write changelogs using imperative mood and avoid pronouns. Lead with a short
> +blurb on what is changing, and then follow up with the context and background.
> +Note! This order directly conflicts with the tip tree's preferred approach!
> +
> +Beyond personal preference, there are less subjective reasons for stating what
> +a patch does before diving into details. First and foremost, what code is
> +actually being changed is the most important information, and so that info
> +should be easy to find. Changelogs that bury the "what's actually changing" in
> +a one-liner after 3+ paragraphs of background make it very hard to find that
> +information.
> +
> +For initial review, one could argue the "what's broken" is more important, but
> +for skimming logs and git archaeology, the gory details matter less and less.
> +E.g. when doing a series of "git blame", the details of each change along the
> +way are useless, the details only matter for the culprit. Providing the "what
> +changed" makes it easy to quickly determine whether or not a commit might be of
> +interest.
> +
> +Another benefit of stating "what's changing" first is that it's almost always
> +possible to state "what's changing" in a single sentence. Conversely, all but
> +the most simple bugs require multiple sentences or paragraphs to fully describe
> +the problem. If both the "what's changing" and "what's the bug" are super
> +short then the order doesn't matter. But if one is shorter (almost always the
> +"what's changing), then covering the shorter one first is advantageous because
> +it's less of an inconvenience for readers/reviewers that have a strict ordering
> +preference. E.g. having to skip one sentence to get to the context is less
> +painful than having to skip three paragraphs to get to "what's changing".
> +
> +Fixes
> +~~~~~
> +If a change fixes a KVM/kernel bug, add a Fixes: tag even if the change doesn't
> +need to be backported to stable kernels, and even if the change fixes a bug in
> +an older release.
> +
> +Conversely, if a fix does need to be backported, explicitly tag the patch with
> +"Cc: stable@vger.kernel" (though the email itself doesn't need to Cc: stable);
> +KVM x86 opts out of backporting Fixes: by default. Some auto-selected patches
> +do get backported, but require explicit maintainer approval (search MANUALSEL).
> +
> +Function References
> +~~~~~~~~~~~~~~~~~~~
> +When a function is mentioned in a comment, changelog, or shortlog (or anywhere
> +for that matter), use the format ``function_name()``. The parentheses provide
> +context and disambiguate the reference.
> +
> +Testing
> +-------
> +At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m
> +KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs
> +isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and
> +X86_64 are particularly interesting knobs to turn.
> +
> +Running KVM selftests and KVM-unit-tests is also mandatory (and stating the
> +obvious, the tests need to pass). When possible and relevant, testing on both
> +Intel and AMD is strongly preferred. Booting an actual VM is encouraged, but
> +not mandatory.
> +
> +For changes that touch KVM's shadow paging code, running with TDP (EPT/NPT)
> +disabled is mandatory. For changes that affect common KVM MMU code, running
> +with TDP disabled is strongly encouraged. For all other changes, if the code
> +being modified depends on and/or interacts with a module param, testing with
> +the relevant settings is mandatory.
> +
> +Note, KVM selftests and KVM-unit-tests do have known failures. If you suspect
> +a failure is not due to your changes, verify that the *exact same* failure
> +occurs with and without your changes.
> +
> +If you can't fully test a change, e.g. due to lack of hardware, clearly state
> +what level of testing you were able to do, e.g. in the cover letter.
> +
> +New Features
> +~~~~~~~~~~~~
> +With one exception, new features *must* come with test coverage. KVM specific
> +tests aren't strictly required, e.g. if coverage is provided by running a
> +sufficiently enabled guest VM, or by running a related kernel selftest in a VM,
> +but dedicated KVM tests are preferred in all cases. Negative testcases in
> +particular are mandatory for enabling of new hardware features as error and
> +exception flows are rarely exercised simply by running a VM.
> +
> +The only exception to this rule is if KVM is simply advertising support for a
> +feature via KVM_SET_SUPPORTED_CPUID, i.e. for instructions/features that KVM
> +can't prevent a guest from using and for which there is no true enabling.
> +
> +Note, "new features" does not just mean "new hardware features"! New features
> +that can't be well validated using existing KVM selftests and/or KVM-unit-tests
> +must come with tests.
> +
> +Posting new feature development without tests to get early feedback is more
> +than welcome, but such submissions should be tagged RFC, and the cover letter
> +should clearly state what type of feedback is requested/expected. Do not abuse
> +the RFC process; RFCs will typically not receive in-depth review.
> +
> +Bug Fixes
> +~~~~~~~~~
> +Except for "obvious" found-by-inspection bugs, fixes must be accompanied by a
> +reproducer for the bug being fixed. In many cases the reproducer is implicit,
> +e.g. for build errors and test failures, but it should still be clear to
> +readers what is broken and how to verify the fix. Some leeway is given for
> +bugs that are found via non-public workloads/tests, but providing regression
> +tests for such bugs is strongly preferred.
> +
> +In general, regression tests are preferred for any bug that is not trivial to
> +hit. E.g. even if the bug was originally found by a fuzzer such as syzkaller,
> +a targeted regression test may be warranted if the bug requires hitting a
> +one-in-a-million type race condition.
> +
> +Note, KVM bugs are rarely urgent *and* non-trivial to reproduce. Ask yourself
> +if a bug is really truly the end of the world before posting a fix without a
> +reproducer.
> +
> +Posting
> +-------
> +
> +Links
> +~~~~~
> +Do not explicitly reference bug reports, prior versions of a patch/series, etc.
> +via ``In-Reply-To:`` headers. Using ``In-Reply-To:`` becomes an unholy mess
> +for large series and/or when the version count gets high, and ``In-Reply-To:``
> +is useless for anyone that doesn't have the original message, e.g. if someone
> +wasn't Cc'd on the bug report or if the list of recipients changes between
> +versions.
> +
> +To link to a bug report, previous version, or anything of interest, use lore
> +links. For referencing previous version(s), generally speaking do not include
> +a Link: in the changelog as there is no need to record the history in git, i.e.
> +put the link in the cover letter or in the section git ignores. Do provide a
> +formal Link: for bug reports and/or discussions that led to the patch. The
> +context of why a change was made is highly valuable for future readers.
> +
> +Git Base
> +~~~~~~~~
> +If you are using git version 2.9.0 or later (Googlers, this is all of you!),
> +use ``git format-patch`` with the ``--base`` flag to automatically include the
> +base tree information in the generated patches.
> +
> +Note, ``--base=auto`` works as expected if and only if a branch's upstream is
> +set to the base topic branch, e.g. it will do the wrong thing if your upstream
> +is set to your personal repository for backup purposes. An alternative "auto"
> +solution is to derive the names of your development branches based on their
> +KVM x86 topic, and feed that into ``--base``. E.g. ``x86/pmu/my_branch_name``,
> +and then write a small wrapper to extract ``pmu`` from the current branch name
> +to yield ``--base=x/pmu``, where ``x`` is whatever name your repository uses to
> +track the KVM x86 remote.
> +
> +Co-Posting Tests
> +~~~~~~~~~~~~~~~~
> +KVM selftests that are associated with KVM changes, e.g. regression tests for
> +bug fixes, should be posted along with the KVM changes as a single series.
> +
> +KVM-unit-tests should *always* be posted separately. Tools, e.g. b4 am, don't
> +know that KVM-unit-tests is a separate repository and get confused when patches
> +in a series apply on different trees. To tie KVM-unit-tests patches back to
> +KVM patches, first post the KVM changes and then provide a lore Link: to the
> +KVM patch/series in the KVM-unit-tests patch(es).
> +
> +Notifications
> +-------------
> +When a patch/series is officially accepted, a notification email will be sent
> +in reply to the original posting (cover letter for multi-patch series). The
> +notification will include the tree and topic branch, along with the SHA1s of
> +the commits of applied patches.
> +
> +If a subset of patches is applied, this will be clearly stated in the
> +notification. Unless stated otherwise, it's implied that any patches in the
> +series that were not accepted need more work and should be submitted in a new
> +version.
> +
> +If for some reason a patch is dropped after officially being accepted, a reply
> +will be sent to the notification email explaining why the patch was dropped, as
> +well as the next steps.
> +
> +SHA1 Stability
> +~~~~~~~~~~~~~~
> +SHA1s are not 100% guaranteed to be stable until they land in Linus' tree! A
> +SHA1 is *usually* stable once a notification has been sent, but things happen.
> +In most cases, an update to the notification email be provided if an applied
> +patch's SHA1 changes. However, in some scenarios, e.g. if all KVM x86 branches
> +need to be rebased, individual notifications will not be given.
> +
> +Vulnerabilities
> +---------------
> +Bugs that can be exploited by the guest to attack the host (kernel or
> +userspace), or that can be exploited by a nested VM to *its* host (L2 attacking
> +L1), are of particular interest to KVM. Please follow the protocol for
> +:ref:`securitybugs` if you suspect a bug can lead to an escape, data leak, etc.
> +
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6a47510d1592..13e67a8b4827 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11436,6 +11436,7 @@ M: Sean Christopherson <seanjc@google.com>
> M: Paolo Bonzini <pbonzini@redhat.com>
> L: kvm@vger.kernel.org
> S: Supported
> +P: Documentation/process/maintainer-kvm-x86.rst
> T: git git://git.kernel.org/pub/scm/virt/kvm/kvm.git
> F: arch/x86/include/asm/kvm*
> F: arch/x86/include/asm/svm.h
> --
> 2.39.2.637.g21b0678d19-goog
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-18 1:52 ` Mingwei Zhang
@ 2023-02-22 0:29 ` Sean Christopherson
2023-03-02 18:46 ` Mingwei Zhang
0 siblings, 1 reply; 19+ messages in thread
From: Sean Christopherson @ 2023-02-22 0:29 UTC (permalink / raw)
To: Mingwei Zhang
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, David Woodhouse, Emanuele Giuseppe Esposito,
Gavin Shan, Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson,
Jing Liu, Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu,
Li RongQing, Maciej S . Szmigiero, Maxim Levitsky, Michael Roth,
Michal Luczaj, Nikunj A Dadhania, Paul Durrant, Peng Hao,
Peter Gonda, Peter Xu, Robert Hoo, Suravee Suthikulpanit,
Tom Lendacky, Vipin Sharma, Vitaly Kuznetsov, Wanpeng Li,
Wei Wang, Xiaoyao Li, Yu Zhang, Zhenzhong Duan, kvm,
linux-kernel
On Sat, Feb 18, 2023, Mingwei Zhang wrote:
> On Fri, Feb 17, 2023, Sean Christopherson wrote:
> > +Coding Style
> > +~~~~~~~~~~~~
> > +When it comes to style, naming, patterns, etc., consistency is the number one
> > +priority in KVM x86. If all else fails, match what already exists.
> > +
> > +With a few caveats listed below, follow the tip tree maintainers' preferred
> > +:ref:`maintainer-tip-coding-style`, as patches/series often touch both KVM and
> > +non-KVM x86 files, i.e. draw the attention of KVM *and* tip tree maintainers.
> > +
> > +Using reverse fir tree for variable declarations isn't strictly required,
> > +though it is still preferred.
>
> What is the 'reverse fir tree'? Maybe, "Reverse Christmas Tree" is
> better to understand.
For some parts of the world, but not all. For this, I want to follow whatever
description the tip tree uses, which as of today is "reverse fir tree", as this
is really a qualifier on the tip tree rules.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-22 0:29 ` Sean Christopherson
@ 2023-03-02 18:46 ` Mingwei Zhang
0 siblings, 0 replies; 19+ messages in thread
From: Mingwei Zhang @ 2023-03-02 18:46 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, David Woodhouse, Emanuele Giuseppe Esposito,
Gavin Shan, Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson,
Jing Liu, Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu,
Li RongQing, Maciej S . Szmigiero, Maxim Levitsky, Michael Roth,
Michal Luczaj, Nikunj A Dadhania, Paul Durrant, Peng Hao,
Peter Gonda, Peter Xu, Robert Hoo, Suravee Suthikulpanit,
Tom Lendacky, Vipin Sharma, Vitaly Kuznetsov, Wanpeng Li,
Wei Wang, Xiaoyao Li, Yu Zhang, Zhenzhong Duan, kvm,
linux-kernel
On Tue, Feb 21, 2023, Sean Christopherson wrote:
> On Sat, Feb 18, 2023, Mingwei Zhang wrote:
> > On Fri, Feb 17, 2023, Sean Christopherson wrote:
> > > +Coding Style
> > > +~~~~~~~~~~~~
> > > +When it comes to style, naming, patterns, etc., consistency is the number one
> > > +priority in KVM x86. If all else fails, match what already exists.
> > > +
> > > +With a few caveats listed below, follow the tip tree maintainers' preferred
> > > +:ref:`maintainer-tip-coding-style`, as patches/series often touch both KVM and
> > > +non-KVM x86 files, i.e. draw the attention of KVM *and* tip tree maintainers.
> > > +
> > > +Using reverse fir tree for variable declarations isn't strictly required,
> > > +though it is still preferred.
> >
> > What is the 'reverse fir tree'? Maybe, "Reverse Christmas Tree" is
> > better to understand.
>
> For some parts of the world, but not all. For this, I want to follow whatever
> description the tip tree uses, which as of today is "reverse fir tree", as this
> is really a qualifier on the tip tree rules.
Some parts of the world is correct. In fact, in our world, we use
'reverse Christmas Tree' more than the other. Check lore.kernel.org:
https://lore.kernel.org/all/?q=reverse+christmas+tree
https://lore.kernel.org/all/?q=reverse+fir+tree
You will find the former is used 10x more frequent than the latter.
Overall, I don't hold a strong opinion immediately after I understand
the meaning of 'reverse fir tree' and I do agree that it is safer to
follow the Linux Tip Tree Handbook.
Also, thanks for the whole guideline.
-Mingwei
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-17 22:54 ` [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86 Sean Christopherson
2023-02-18 1:52 ` Mingwei Zhang
@ 2023-02-20 8:10 ` Yuan Yao
2023-02-20 10:07 ` Like Xu
` (4 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Yuan Yao @ 2023-02-20 8:10 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, David Woodhouse, Emanuele Giuseppe Esposito,
Gavin Shan, Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson,
Jing Liu, Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu,
Li RongQing, Maciej S . Szmigiero, Maxim Levitsky, Michael Roth,
Michal Luczaj, Mingwei Zhang, Nikunj A Dadhania, Paul Durrant,
Peng Hao, Peter Gonda, Peter Xu, Robert Hoo,
Suravee Suthikulpanit, Tom Lendacky, Vipin Sharma,
Vitaly Kuznetsov, Wanpeng Li, Wei Wang, Xiaoyao Li, Yu Zhang,
Zhenzhong Duan, kvm, linux-kernel
On Fri, Feb 17, 2023 at 02:54:49PM -0800, Sean Christopherson wrote:
> Add a KVM x86 doc to the subsystem/maintainer handbook section to explain
> how KVM x86 (currently) operates as a sub-subsystem, and to soapbox on
> the rules and expectations for contributing to KVM x86.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> .../process/maintainer-handbooks.rst | 1 +
> Documentation/process/maintainer-kvm-x86.rst | 347 ++++++++++++++++++
> MAINTAINERS | 1 +
> 3 files changed, 349 insertions(+)
> create mode 100644 Documentation/process/maintainer-kvm-x86.rst
>
> diff --git a/Documentation/process/maintainer-handbooks.rst b/Documentation/process/maintainer-handbooks.rst
> index d783060b4cc6..d12cbbe2b7df 100644
> --- a/Documentation/process/maintainer-handbooks.rst
> +++ b/Documentation/process/maintainer-handbooks.rst
> @@ -17,3 +17,4 @@ Contents:
>
> maintainer-tip
> maintainer-netdev
> + maintainer-kvm-x86
> diff --git a/Documentation/process/maintainer-kvm-x86.rst b/Documentation/process/maintainer-kvm-x86.rst
> new file mode 100644
> index 000000000000..ac81a42a32a3
> --- /dev/null
> +++ b/Documentation/process/maintainer-kvm-x86.rst
> @@ -0,0 +1,347 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +KVM x86
> +=======
> +
> +TL;DR
> +-----
> +Testing is mandatory. Be consistent with established styles and patterns.
> +
> +Trees
> +-----
> +KVM x86 is currently in a transition period from being part of the main KVM
> +tree, to being "just another KVM arch". As such, KVM x86 is split across the
> +main KVM tree, ``git.kernel.org/pub/scm/virt/kvm/kvm.git``, and a KVM x86
> +specific tree, ``github.com/kvm-x86/linux.git``.
> +
> +Generally speaking, fixes for the current cycle are applied directly to the
> +main KVM tree, while all development for the next cycle is routed through the
> +KVM x86 tree.
> +
> +Note, this transition period is expected to last quite some time, i.e. will be
> +the status quo for the foreseeable future.
> +
> +Branches
> +~~~~~~~~
> +The KVM x86 tree is organized into multiple topic branches. The purpose of
> +using finer-grained topic branches is to make it easier to keep tabs on an area
> +of development, and to limit the collateral damage of human errors and/or buggy
> +commits, e.g. dropping the HEAD commit of a topic branch has no impact on other
> +in-flight commits' SHA1 hashes, and having to reject a pull request due to bugs
> +delays only that topic branch.
> +
> +All topic branches, except for ``next`` and ``fixes``, are rolled into ``next``
> +via a cthulu merge on an as-needed basis, i.e. when a topic branch is updated.
> +As a result, force pushes to ``next`` are common.
> +
> +Lifecycle
> +~~~~~~~~~
> +Pull requests for the next release cycle are sent to the main KVM tree, one
> +for each KVM x86 topic branch. If all goes well, the topic branches are rolled
> +into the main KVM pull request sent during Linus' merge window. Pull requests
> +for KVM x86 branches are typically made the week before Linus' opening of the
> +merge window, e.g. the week following rc7 for "normal" releases.
> +
> +The KVM x86 tree doesn't have its own official merge window, but there's a soft
> +close around rc5 for new features, and a soft close around rc6 for fixes.
> +
> +Timeline
> +~~~~~~~~
> +Submissions are typically reviewed and applied in FIFO order, with some wiggle
> +room for the size of a series, patches that are "cache hot", etc. Fixes,
> +especially for the current release and or stable trees, get to jump the queue.
> +Patches that will be taken through a non-KVM tree (most often through the tip
> +tree) and/or have other acks/reviews also jump the queue to some extent.
> +
> +Note, the vast majority of review is done between rc1 and rc6, give or take.
> +The period between rc6 and the next rc1 is used to catch up on other tasks,
> +i.e. radio silence during this period isn't unusual.
> +
> +Pings to get a status update are welcome, but keep in mind the timing of the
> +current release cycle and have realistic expectations. If you are pinging for
> +acceptance, i.e. not just for feedback or an update, please do everything you
> +can, within reason, to ensure that your patches are ready to be merged! Pings
> +on series that break the build or fail tests lead to unhappy maintainers!
> +
> +Development
> +-----------
> +
> +Base Tree/Branch
> +~~~~~~~~~~~~~~~~
> +Fixes that target mainline, i.e. the current release, should be based on
> +``git://git.kernel.org/pub/scm/virt/kvm/kvm.git master``.
> +
> +Everything else should be based on a kvm-x86 topic branch. If there is no
> +obvious fit, use ``misc``. Unless a patch/series depends on and/or conflicts
> +with multiple topic branches, do not use ``next`` as a base. Because ``next``
> +is force-pushed on a regular basis, depending on when others fetch ``next``,
> +they may or may not have the relevant objects in their local git tree.
> +
> +Coding Style
> +~~~~~~~~~~~~
> +When it comes to style, naming, patterns, etc., consistency is the number one
> +priority in KVM x86. If all else fails, match what already exists.
> +
> +With a few caveats listed below, follow the tip tree maintainers' preferred
> +:ref:`maintainer-tip-coding-style`, as patches/series often touch both KVM and
> +non-KVM x86 files, i.e. draw the attention of KVM *and* tip tree maintainers.
> +
> +Using reverse fir tree for variable declarations isn't strictly required,
> +though it is still preferred.
> +
> +Except for a handful of special snowflakes, do not use kernel-doc comments for
> +functions. The vast majority of "public" KVM functions aren't truly public as
> +they are intended only for KVM-internal consumption (there are plans to
> +privatize KVM's headers and exports to enforce this).
> +
> +Comments
> +~~~~~~~~
> +Write comments using imperative mood and avoid pronouns. Use comments to
> +provide a high level overview of the code, and/or to explain why the code does
> +what it does. Do not reiterate what the code literally does; let the code
> +speak for itself. If the code itself is inscrutable, comments will not help.
> +
> +SDM and APM References
> +~~~~~~~~~~~~~~~~~~~~~~
> +Much of KVM's code base is directly tied to architectural behavior defined in
> +Intel's Software Development Manual (SDM) and AMD's Architecture Programmer’s
> +Manual (APM). Use of "Intel's SDM" and "AMD's APM", or even just "SDM" or
> +"APM", without additional context is a-ok.
> +
> +Do not reference specific sections, tables, figures, etc. by number, especially
> +not in comments. Instead, copy-paste the relevant snippet (if warranted), and
> +reference sections/tables/figures by name. The layouts of the SDM and APM are
> +constantly changing, and so the numbers/labels aren't stable/consistent.
> +
> +Generally speaking, do not copy-paste SDM or APM snippets into comments. With
> +few exceptions, KVM *must* honor architectural behavior, therefore it's implied
> +that KVM behavior is emulating SDM and/or APM behavior.
> +
> +Shortlog
> +~~~~~~~~
> +The preferred prefix format is ``KVM: <topic>:``, where ``<topic>`` is one of::
> +
> + - x86
> + - x86/mmu
> + - x86/pmu
> + - x86/xen
> + - selftests
> + - SVM
> + - nSVM
> + - VMX
> + - nVMX
> +
> +**DO NOT use x86/kvm!** ``x86/kvm`` is used exclusively for Linux-as-a-KVM-guest
> +changes, i.e. for arch/x86/kernel/kvm.c.
> +
> +Note, these don't align with the topics branches (the topic branches care much
> +more about code conflicts).
> +
> +All names are case sensitive! ``KVM: x86:`` is good, ``kvm: vmx:`` is not.
> +
> +Capitalize the first word of the condensed patch description, but omit ending
> +punctionation. E.g.::
> +
> + KVM: x86: Fix a null pointer dererence in function_xyz()
> +
> +not::
> +
> + kvm: x86: fix a null pointer dererence in function_xyz.
> +
> +If a patch touches multiple topics, traverse up the conceptual tree to find the
> +first common parent (which is often simply ``x86``). When in doubt,
> +``git log path/to/file`` should provide a reasonable hint.
> +
> +New topics do occasionally pop up, but please start an on-list discussion if
> +you want to propose introducing a new topic, i.e. don't go rogue.
> +
> +Do not use file names or complete file paths as the subject/shortlog prefix.
> +
> +Changelog
> +~~~~~~~~~
> +Write changelogs using imperative mood and avoid pronouns. Lead with a short
> +blurb on what is changing, and then follow up with the context and background.
> +Note! This order directly conflicts with the tip tree's preferred approach!
> +
> +Beyond personal preference, there are less subjective reasons for stating what
> +a patch does before diving into details. First and foremost, what code is
> +actually being changed is the most important information, and so that info
> +should be easy to find. Changelogs that bury the "what's actually changing" in
> +a one-liner after 3+ paragraphs of background make it very hard to find that
> +information.
> +
> +For initial review, one could argue the "what's broken" is more important, but
> +for skimming logs and git archaeology, the gory details matter less and less.
> +E.g. when doing a series of "git blame", the details of each change along the
> +way are useless, the details only matter for the culprit. Providing the "what
> +changed" makes it easy to quickly determine whether or not a commit might be of
> +interest.
> +
> +Another benefit of stating "what's changing" first is that it's almost always
> +possible to state "what's changing" in a single sentence. Conversely, all but
> +the most simple bugs require multiple sentences or paragraphs to fully describe
> +the problem. If both the "what's changing" and "what's the bug" are super
> +short then the order doesn't matter. But if one is shorter (almost always the
> +"what's changing), then covering the shorter one first is advantageous because
> +it's less of an inconvenience for readers/reviewers that have a strict ordering
> +preference. E.g. having to skip one sentence to get to the context is less
> +painful than having to skip three paragraphs to get to "what's changing".
> +
> +Fixes
> +~~~~~
> +If a change fixes a KVM/kernel bug, add a Fixes: tag even if the change doesn't
> +need to be backported to stable kernels, and even if the change fixes a bug in
> +an older release.
> +
> +Conversely, if a fix does need to be backported, explicitly tag the patch with
> +"Cc: stable@vger.kernel" (though the email itself doesn't need to Cc: stable);
> +KVM x86 opts out of backporting Fixes: by default. Some auto-selected patches
> +do get backported, but require explicit maintainer approval (search MANUALSEL).
> +
> +Function References
> +~~~~~~~~~~~~~~~~~~~
> +When a function is mentioned in a comment, changelog, or shortlog (or anywhere
> +for that matter), use the format ``function_name()``. The parentheses provide
> +context and disambiguate the reference.
> +
> +Testing
> +-------
> +At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m
> +KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs
> +isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and
> +X86_64 are particularly interesting knobs to turn.
> +
> +Running KVM selftests and KVM-unit-tests is also mandatory (and stating the
> +obvious, the tests need to pass). When possible and relevant, testing on both
> +Intel and AMD is strongly preferred. Booting an actual VM is encouraged, but
> +not mandatory.
> +
> +For changes that touch KVM's shadow paging code, running with TDP (EPT/NPT)
> +disabled is mandatory. For changes that affect common KVM MMU code, running
> +with TDP disabled is strongly encouraged. For all other changes, if the code
> +being modified depends on and/or interacts with a module param, testing with
> +the relevant settings is mandatory.
> +
> +Note, KVM selftests and KVM-unit-tests do have known failures. If you suspect
> +a failure is not due to your changes, verify that the *exact same* failure
> +occurs with and without your changes.
> +
> +If you can't fully test a change, e.g. due to lack of hardware, clearly state
> +what level of testing you were able to do, e.g. in the cover letter.
> +
> +New Features
> +~~~~~~~~~~~~
> +With one exception, new features *must* come with test coverage. KVM specific
> +tests aren't strictly required, e.g. if coverage is provided by running a
> +sufficiently enabled guest VM, or by running a related kernel selftest in a VM,
> +but dedicated KVM tests are preferred in all cases. Negative testcases in
> +particular are mandatory for enabling of new hardware features as error and
> +exception flows are rarely exercised simply by running a VM.
> +
> +The only exception to this rule is if KVM is simply advertising support for a
> +feature via KVM_SET_SUPPORTED_CPUID, i.e. for instructions/features that KVM
^^^^^^^^^^^^
Should be KVM_GET_SUPPORTED_CPUID.
It's GOOD documentation to me!
> +can't prevent a guest from using and for which there is no true enabling.
> +
> +Note, "new features" does not just mean "new hardware features"! New features
> +that can't be well validated using existing KVM selftests and/or KVM-unit-tests
> +must come with tests.
> +
> +Posting new feature development without tests to get early feedback is more
> +than welcome, but such submissions should be tagged RFC, and the cover letter
> +should clearly state what type of feedback is requested/expected. Do not abuse
> +the RFC process; RFCs will typically not receive in-depth review.
> +
> +Bug Fixes
> +~~~~~~~~~
> +Except for "obvious" found-by-inspection bugs, fixes must be accompanied by a
> +reproducer for the bug being fixed. In many cases the reproducer is implicit,
> +e.g. for build errors and test failures, but it should still be clear to
> +readers what is broken and how to verify the fix. Some leeway is given for
> +bugs that are found via non-public workloads/tests, but providing regression
> +tests for such bugs is strongly preferred.
> +
> +In general, regression tests are preferred for any bug that is not trivial to
> +hit. E.g. even if the bug was originally found by a fuzzer such as syzkaller,
> +a targeted regression test may be warranted if the bug requires hitting a
> +one-in-a-million type race condition.
> +
> +Note, KVM bugs are rarely urgent *and* non-trivial to reproduce. Ask yourself
> +if a bug is really truly the end of the world before posting a fix without a
> +reproducer.
> +
> +Posting
> +-------
> +
> +Links
> +~~~~~
> +Do not explicitly reference bug reports, prior versions of a patch/series, etc.
> +via ``In-Reply-To:`` headers. Using ``In-Reply-To:`` becomes an unholy mess
> +for large series and/or when the version count gets high, and ``In-Reply-To:``
> +is useless for anyone that doesn't have the original message, e.g. if someone
> +wasn't Cc'd on the bug report or if the list of recipients changes between
> +versions.
> +
> +To link to a bug report, previous version, or anything of interest, use lore
> +links. For referencing previous version(s), generally speaking do not include
> +a Link: in the changelog as there is no need to record the history in git, i.e.
> +put the link in the cover letter or in the section git ignores. Do provide a
> +formal Link: for bug reports and/or discussions that led to the patch. The
> +context of why a change was made is highly valuable for future readers.
> +
> +Git Base
> +~~~~~~~~
> +If you are using git version 2.9.0 or later (Googlers, this is all of you!),
> +use ``git format-patch`` with the ``--base`` flag to automatically include the
> +base tree information in the generated patches.
> +
> +Note, ``--base=auto`` works as expected if and only if a branch's upstream is
> +set to the base topic branch, e.g. it will do the wrong thing if your upstream
> +is set to your personal repository for backup purposes. An alternative "auto"
> +solution is to derive the names of your development branches based on their
> +KVM x86 topic, and feed that into ``--base``. E.g. ``x86/pmu/my_branch_name``,
> +and then write a small wrapper to extract ``pmu`` from the current branch name
> +to yield ``--base=x/pmu``, where ``x`` is whatever name your repository uses to
> +track the KVM x86 remote.
> +
> +Co-Posting Tests
> +~~~~~~~~~~~~~~~~
> +KVM selftests that are associated with KVM changes, e.g. regression tests for
> +bug fixes, should be posted along with the KVM changes as a single series.
> +
> +KVM-unit-tests should *always* be posted separately. Tools, e.g. b4 am, don't
> +know that KVM-unit-tests is a separate repository and get confused when patches
> +in a series apply on different trees. To tie KVM-unit-tests patches back to
> +KVM patches, first post the KVM changes and then provide a lore Link: to the
> +KVM patch/series in the KVM-unit-tests patch(es).
> +
> +Notifications
> +-------------
> +When a patch/series is officially accepted, a notification email will be sent
> +in reply to the original posting (cover letter for multi-patch series). The
> +notification will include the tree and topic branch, along with the SHA1s of
> +the commits of applied patches.
> +
> +If a subset of patches is applied, this will be clearly stated in the
> +notification. Unless stated otherwise, it's implied that any patches in the
> +series that were not accepted need more work and should be submitted in a new
> +version.
> +
> +If for some reason a patch is dropped after officially being accepted, a reply
> +will be sent to the notification email explaining why the patch was dropped, as
> +well as the next steps.
> +
> +SHA1 Stability
> +~~~~~~~~~~~~~~
> +SHA1s are not 100% guaranteed to be stable until they land in Linus' tree! A
> +SHA1 is *usually* stable once a notification has been sent, but things happen.
> +In most cases, an update to the notification email be provided if an applied
> +patch's SHA1 changes. However, in some scenarios, e.g. if all KVM x86 branches
> +need to be rebased, individual notifications will not be given.
> +
> +Vulnerabilities
> +---------------
> +Bugs that can be exploited by the guest to attack the host (kernel or
> +userspace), or that can be exploited by a nested VM to *its* host (L2 attacking
> +L1), are of particular interest to KVM. Please follow the protocol for
> +:ref:`securitybugs` if you suspect a bug can lead to an escape, data leak, etc.
> +
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6a47510d1592..13e67a8b4827 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11436,6 +11436,7 @@ M: Sean Christopherson <seanjc@google.com>
> M: Paolo Bonzini <pbonzini@redhat.com>
> L: kvm@vger.kernel.org
> S: Supported
> +P: Documentation/process/maintainer-kvm-x86.rst
> T: git git://git.kernel.org/pub/scm/virt/kvm/kvm.git
> F: arch/x86/include/asm/kvm*
> F: arch/x86/include/asm/svm.h
> --
> 2.39.2.637.g21b0678d19-goog
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-17 22:54 ` [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86 Sean Christopherson
2023-02-18 1:52 ` Mingwei Zhang
2023-02-20 8:10 ` Yuan Yao
@ 2023-02-20 10:07 ` Like Xu
2023-02-22 1:55 ` Sean Christopherson
2023-02-20 23:17 ` David Woodhouse
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Like Xu @ 2023-02-20 10:07 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, linux-kernel, Paolo Bonzini
On 18/2/2023 6:54 am, Sean Christopherson wrote:
> Add a KVM x86 doc to the subsystem/maintainer handbook section to explain
> how KVM x86 (currently) operates as a sub-subsystem, and to soapbox on
> the rules and expectations for contributing to KVM x86.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> .../process/maintainer-handbooks.rst | 1 +
> Documentation/process/maintainer-kvm-x86.rst | 347 ++++++++++++++++++
> MAINTAINERS | 1 +
> 3 files changed, 349 insertions(+)
> create mode 100644 Documentation/process/maintainer-kvm-x86.rst
>
> diff --git a/Documentation/process/maintainer-handbooks.rst b/Documentation/process/maintainer-handbooks.rst
> index d783060b4cc6..d12cbbe2b7df 100644
> --- a/Documentation/process/maintainer-handbooks.rst
> +++ b/Documentation/process/maintainer-handbooks.rst
> @@ -17,3 +17,4 @@ Contents:
>
> maintainer-tip
> maintainer-netdev
> + maintainer-kvm-x86
> diff --git a/Documentation/process/maintainer-kvm-x86.rst b/Documentation/process/maintainer-kvm-x86.rst
> new file mode 100644
> index 000000000000..ac81a42a32a3
> --- /dev/null
> +++ b/Documentation/process/maintainer-kvm-x86.rst
> @@ -0,0 +1,347 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +KVM x86
KERNEL VIRTUAL MACHINE FOR X86 (KVM/x86)
> +=======
> +
> +TL;DR
> +-----
> +Testing is mandatory. Be consistent with established styles and patterns.
> +
> +Trees
> +-----
> +KVM x86 is currently in a transition period from being part of the main KVM
> +tree, to being "just another KVM arch". As such, KVM x86 is split across the
> +main KVM tree, ``git.kernel.org/pub/scm/virt/kvm/kvm.git``, and a KVM x86
> +specific tree, ``github.com/kvm-x86/linux.git``.
> +
> +Generally speaking, fixes for the current cycle are applied directly to the
> +main KVM tree, while all development for the next cycle is routed through the
> +KVM x86 tree.
> +
> +Note, this transition period is expected to last quite some time, i.e. will be
> +the status quo for the foreseeable future.
> +
> +Branches
> +~~~~~~~~
> +The KVM x86 tree is organized into multiple topic branches. The purpose of
> +using finer-grained topic branches is to make it easier to keep tabs on an area
> +of development, and to limit the collateral damage of human errors and/or buggy
> +commits, e.g. dropping the HEAD commit of a topic branch has no impact on other
> +in-flight commits' SHA1 hashes, and having to reject a pull request due to bugs
> +delays only that topic branch.
> +
> +All topic branches, except for ``next`` and ``fixes``, are rolled into ``next``
> +via a cthulu merge on an as-needed basis, i.e. when a topic branch is updated.
s/cthulu/Cthulhu
> +As a result, force pushes to ``next`` are common.
> +
> +Lifecycle
> +~~~~~~~~~
> +Pull requests for the next release cycle are sent to the main KVM tree, one
> +for each KVM x86 topic branch. If all goes well, the topic branches are rolled
> +into the main KVM pull request sent during Linus' merge window. Pull requests
> +for KVM x86 branches are typically made the week before Linus' opening of the
> +merge window, e.g. the week following rc7 for "normal" releases.
> +
> +The KVM x86 tree doesn't have its own official merge window, but there's a soft
> +close around rc5 for new features, and a soft close around rc6 for fixes.
Any urgent AND critical fixes are exempt. No?
> +
> +Timeline
> +~~~~~~~~
> +Submissions are typically reviewed and applied in FIFO order, with some wiggle
> +room for the size of a series, patches that are "cache hot", etc. Fixes,
> +especially for the current release and or stable trees, get to jump the queue.
> +Patches that will be taken through a non-KVM tree (most often through the tip
> +tree) and/or have other acks/reviews also jump the queue to some extent.
> +
> +Note, the vast majority of review is done between rc1 and rc6, give or take.
> +The period between rc6 and the next rc1 is used to catch up on other tasks,
> +i.e. radio silence during this period isn't unusual.
> +
> +Pings to get a status update are welcome, but keep in mind the timing of the
> +current release cycle and have realistic expectations. If you are pinging for
> +acceptance, i.e. not just for feedback or an update, please do everything you
> +can, within reason, to ensure that your patches are ready to be merged! Pings
> +on series that break the build or fail tests lead to unhappy maintainers!
> +
> +Development
> +-----------
> +
> +Base Tree/Branch
> +~~~~~~~~~~~~~~~~
> +Fixes that target mainline, i.e. the current release, should be based on
> +``git://git.kernel.org/pub/scm/virt/kvm/kvm.git master``.
> +
> +Everything else should be based on a kvm-x86 topic branch. If there is no
> +obvious fit, use ``misc``. Unless a patch/series depends on and/or conflicts
> +with multiple topic branches, do not use ``next`` as a base. Because ``next``
> +is force-pushed on a regular basis, depending on when others fetch ``next``,
> +they may or may not have the relevant objects in their local git tree.
> +
> +Coding Style
> +~~~~~~~~~~~~
> +When it comes to style, naming, patterns, etc., consistency is the number one
> +priority in KVM x86. If all else fails, match what already exists.
priority in KVM x86 (including selftests).
> +
> +With a few caveats listed below, follow the tip tree maintainers' preferred
> +:ref:`maintainer-tip-coding-style`, as patches/series often touch both KVM and
> +non-KVM x86 files, i.e. draw the attention of KVM *and* tip tree maintainers.
> +
> +Using reverse fir tree for variable declarations isn't strictly required,
> +though it is still preferred.
> +
> +Except for a handful of special snowflakes, do not use kernel-doc comments for
> +functions. The vast majority of "public" KVM functions aren't truly public as
> +they are intended only for KVM-internal consumption (there are plans to
> +privatize KVM's headers and exports to enforce this).
> +
> +Comments
> +~~~~~~~~
> +Write comments using imperative mood and avoid pronouns. Use comments to
> +provide a high level overview of the code, and/or to explain why the code does
> +what it does. Do not reiterate what the code literally does; let the code
> +speak for itself. If the code itself is inscrutable, comments will not help.
> +
> +SDM and APM References
> +~~~~~~~~~~~~~~~~~~~~~~
> +Much of KVM's code base is directly tied to architectural behavior defined in
> +Intel's Software Development Manual (SDM) and AMD's Architecture Programmer’s
> +Manual (APM). Use of "Intel's SDM" and "AMD's APM", or even just "SDM" or
> +"APM", without additional context is a-ok.
ISE: Intel® Architecture Instruction Set Extensions and Future Features
PPR: Processor Programming Reference (PPR) for specific AMD Model
> +
> +Do not reference specific sections, tables, figures, etc. by number, especially
> +not in comments. Instead, copy-paste the relevant snippet (if warranted), and
> +reference sections/tables/figures by name. The layouts of the SDM and APM are
> +constantly changing, and so the numbers/labels aren't stable/consistent.
> +
> +Generally speaking, do not copy-paste SDM or APM snippets into comments. With
> +few exceptions, KVM *must* honor architectural behavior, therefore it's implied
> +that KVM behavior is emulating SDM and/or APM behavior.
All undefined behaviors (if any) need to be clarified.
> +
> +Shortlog
> +~~~~~~~~
> +The preferred prefix format is ``KVM: <topic>:``, where ``<topic>`` is one of::
> +
> + - x86
> + - x86/mmu
> + - x86/pmu
> + - x86/xen
Any conflict w/ "KVM X86 Xen (KVM/Xen)" ? Then "KVM X86 HYPER-V (KVM/hyper-v)" ?
> + - selftests > + - SVM
> + - nSVM
> + - VMX
> + - nVMX
emulator ? lapic ?
> +
> +**DO NOT use x86/kvm!** ``x86/kvm`` is used exclusively for Linux-as-a-KVM-guest
> +changes, i.e. for arch/x86/kernel/kvm.c.
> +
> +Note, these don't align with the topics branches (the topic branches care much
> +more about code conflicts).
> +
> +All names are case sensitive! ``KVM: x86:`` is good, ``kvm: vmx:`` is not.
> +
> +Capitalize the first word of the condensed patch description, but omit ending
> +punctionation. E.g.::
s/punctionation/punctuation
> +
> + KVM: x86: Fix a null pointer dererence in function_xyz()
s/dererence/dereference
> +
> +not::
> +
> + kvm: x86: fix a null pointer dererence in function_xyz.
> +
> +If a patch touches multiple topics, traverse up the conceptual tree to find the
> +first common parent (which is often simply ``x86``). When in doubt,
> +``git log path/to/file`` should provide a reasonable hint.
> +
> +New topics do occasionally pop up, but please start an on-list discussion if
> +you want to propose introducing a new topic, i.e. don't go rogue.
> +
> +Do not use file names or complete file paths as the subject/shortlog prefix.
> +
> +Changelog
> +~~~~~~~~~
> +Write changelogs using imperative mood and avoid pronouns. Lead with a short
> +blurb on what is changing, and then follow up with the context and background.
> +Note! This order directly conflicts with the tip tree's preferred approach!
Emm, could this be considered/clarified as an incentive option ?
> +
> +Beyond personal preference, there are less subjective reasons for stating what
> +a patch does before diving into details. First and foremost, what code is
> +actually being changed is the most important information, and so that info
> +should be easy to find. Changelogs that bury the "what's actually changing" in
> +a one-liner after 3+ paragraphs of background make it very hard to find that
> +information.
> +
> +For initial review, one could argue the "what's broken" is more important, but
> +for skimming logs and git archaeology, the gory details matter less and less.
> +E.g. when doing a series of "git blame", the details of each change along the
> +way are useless, the details only matter for the culprit. Providing the "what
> +changed" makes it easy to quickly determine whether or not a commit might be of
> +interest.
> +
> +Another benefit of stating "what's changing" first is that it's almost always
> +possible to state "what's changing" in a single sentence. Conversely, all but
> +the most simple bugs require multiple sentences or paragraphs to fully describe
> +the problem. If both the "what's changing" and "what's the bug" are super
> +short then the order doesn't matter. But if one is shorter (almost always the
> +"what's changing), then covering the shorter one first is advantageous because
> +it's less of an inconvenience for readers/reviewers that have a strict ordering
> +preference. E.g. having to skip one sentence to get to the context is less
> +painful than having to skip three paragraphs to get to "what's changing".
I'm sure more than one kernel friends will be concerned about this requirement,
especially those who like to read novels in our git logs.
> +
> +Fixes
> +~~~~~
> +If a change fixes a KVM/kernel bug, add a Fixes: tag even if the change doesn't
> +need to be backported to stable kernels, and even if the change fixes a bug in
> +an older release.
> +
> +Conversely, if a fix does need to be backported, explicitly tag the patch with
> +"Cc: stable@vger.kernel" (though the email itself doesn't need to Cc: stable);
> +KVM x86 opts out of backporting Fixes: by default. Some auto-selected patches
> +do get backported, but require explicit maintainer approval (search MANUALSEL).
> +
> +Function References
> +~~~~~~~~~~~~~~~~~~~
> +When a function is mentioned in a comment, changelog, or shortlog (or anywhere
> +for that matter), use the format ``function_name()``. The parentheses provide
> +context and disambiguate the reference.
> +
> +Testing
> +-------
> +At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m
> +KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs
> +isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and
> +X86_64 are particularly interesting knobs to turn.
> +
> +Running KVM selftests and KVM-unit-tests is also mandatory (and stating the
> +obvious, the tests need to pass). When possible and relevant, testing on both
> +Intel and AMD is strongly preferred. Booting an actual VM is encouraged, but
> +not mandatory.
Testing L2 guest available features inside L1 is also encouraged.
> +
> +For changes that touch KVM's shadow paging code, running with TDP (EPT/NPT)
> +disabled is mandatory. For changes that affect common KVM MMU code, running
> +with TDP disabled is strongly encouraged. For all other changes, if the code
> +being modified depends on and/or interacts with a module param, testing with
> +the relevant settings is mandatory.
> +
> +Note, KVM selftests and KVM-unit-tests do have known failures. If you suspect
> +a failure is not due to your changes, verify that the *exact same* failure
> +occurs with and without your changes.
> +
> +If you can't fully test a change, e.g. due to lack of hardware, clearly state
> +what level of testing you were able to do, e.g. in the cover letter.
Add an RFT (request for test) tag.
> +
> +New Features
> +~~~~~~~~~~~~
> +With one exception, new features *must* come with test coverage. KVM specific
> +tests aren't strictly required, e.g. if coverage is provided by running a
> +sufficiently enabled guest VM, or by running a related kernel selftest in a VM,
> +but dedicated KVM tests are preferred in all cases. Negative testcases in
> +particular are mandatory for enabling of new hardware features as error and
> +exception flows are rarely exercised simply by running a VM.
> +
> +The only exception to this rule is if KVM is simply advertising support for a
> +feature via KVM_SET_SUPPORTED_CPUID, i.e. for instructions/features that KVM
> +can't prevent a guest from using and for which there is no true enabling.
> +
> +Note, "new features" does not just mean "new hardware features"! New features
> +that can't be well validated using existing KVM selftests and/or KVM-unit-tests
> +must come with tests.
must come with tests to ensure good code coverage.
> +
> +Posting new feature development without tests to get early feedback is more
> +than welcome, but such submissions should be tagged RFC, and the cover letter
> +should clearly state what type of feedback is requested/expected. Do not abuse
> +the RFC process; RFCs will typically not receive in-depth review.
> +
> +Bug Fixes
> +~~~~~~~~~
> +Except for "obvious" found-by-inspection bugs, fixes must be accompanied by a
> +reproducer for the bug being fixed. In many cases the reproducer is implicit,
> +e.g. for build errors and test failures, but it should still be clear to
> +readers what is broken and how to verify the fix. Some leeway is given for
> +bugs that are found via non-public workloads/tests, but providing regression
> +tests for such bugs is strongly preferred.
tests or detailed reproduction sequence for such bugs is strongly preferred.
> +
> +In general, regression tests are preferred for any bug that is not trivial to
> +hit. E.g. even if the bug was originally found by a fuzzer such as syzkaller,
> +a targeted regression test may be warranted if the bug requires hitting a
> +one-in-a-million type race condition.
> +
> +Note, KVM bugs are rarely urgent *and* non-trivial to reproduce. Ask yourself
> +if a bug is really truly the end of the world before posting a fix without a
> +reproducer.
> +
> +Posting
> +-------
> +
> +Links
> +~~~~~
> +Do not explicitly reference bug reports, prior versions of a patch/series, etc.
> +via ``In-Reply-To:`` headers. Using ``In-Reply-To:`` becomes an unholy mess
> +for large series and/or when the version count gets high, and ``In-Reply-To:``
> +is useless for anyone that doesn't have the original message, e.g. if someone
> +wasn't Cc'd on the bug report or if the list of recipients changes between
> +versions.
> +
> +To link to a bug report, previous version, or anything of interest, use lore
> +links. For referencing previous version(s), generally speaking do not include
> +a Link: in the changelog as there is no need to record the history in git, i.e.
> +put the link in the cover letter or in the section git ignores. Do provide a
> +formal Link: for bug reports and/or discussions that led to the patch. The
> +context of why a change was made is highly valuable for future readers.
> +
> +Git Base
> +~~~~~~~~
> +If you are using git version 2.9.0 or later (Googlers, this is all of you!),
Please do not mention specific developers or groups in this type of document.
> +use ``git format-patch`` with the ``--base`` flag to automatically include the
> +base tree information in the generated patches.
> +
> +Note, ``--base=auto`` works as expected if and only if a branch's upstream is
> +set to the base topic branch, e.g. it will do the wrong thing if your upstream
> +is set to your personal repository for backup purposes. An alternative "auto"
> +solution is to derive the names of your development branches based on their
> +KVM x86 topic, and feed that into ``--base``. E.g. ``x86/pmu/my_branch_name``,
> +and then write a small wrapper to extract ``pmu`` from the current branch name
> +to yield ``--base=x/pmu``, where ``x`` is whatever name your repository uses to
> +track the KVM x86 remote.
> +
> +Co-Posting Tests
> +~~~~~~~~~~~~~~~~
> +KVM selftests that are associated with KVM changes, e.g. regression tests for
> +bug fixes, should be posted along with the KVM changes as a single series.
Keeping git-bisect is mandatory.
> +
> +KVM-unit-tests should *always* be posted separately. Tools, e.g. b4 am, don't
> +know that KVM-unit-tests is a separate repository and get confused when patches
> +in a series apply on different trees. To tie KVM-unit-tests patches back to
> +KVM patches, first post the KVM changes and then provide a lore Link: to the
> +KVM patch/series in the KVM-unit-tests patch(es).
> +
> +Notifications
> +-------------
> +When a patch/series is officially accepted, a notification email will be sent
> +in reply to the original posting (cover letter for multi-patch series). The
> +notification will include the tree and topic branch, along with the SHA1s of
> +the commits of applied patches.
> +
> +If a subset of patches is applied, this will be clearly stated in the
> +notification. Unless stated otherwise, it's implied that any patches in the
> +series that were not accepted need more work and should be submitted in a new
> +version.
in a new version or a separate topic thread.
> +
> +If for some reason a patch is dropped after officially being accepted, a reply
> +will be sent to the notification email explaining why the patch was dropped, as
> +well as the next steps.
> +
> +SHA1 Stability
> +~~~~~~~~~~~~~~
> +SHA1s are not 100% guaranteed to be stable until they land in Linus' tree! A
> +SHA1 is *usually* stable once a notification has been sent, but things happen.
> +In most cases, an update to the notification email be provided if an applied
> +patch's SHA1 changes. However, in some scenarios, e.g. if all KVM x86 branches
> +need to be rebased, individual notifications will not be given.
> +
> +Vulnerabilities
> +---------------
> +Bugs that can be exploited by the guest to attack the host (kernel or
> +userspace), or that can be exploited by a nested VM to *its* host (L2 attacking
> +L1), are of particular interest to KVM. Please follow the protocol for
L1, even L0 host),
> +:ref:`securitybugs` if you suspect a bug can lead to an escape, data leak, etc.
> +
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6a47510d1592..13e67a8b4827 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11436,6 +11436,7 @@ M: Sean Christopherson <seanjc@google.com>
> M: Paolo Bonzini <pbonzini@redhat.com>
> L: kvm@vger.kernel.org
> S: Supported
> +P: Documentation/process/maintainer-kvm-x86.rst
> T: git git://git.kernel.org/pub/scm/virt/kvm/kvm.git
> F: arch/x86/include/asm/kvm*
> F: arch/x86/include/asm/svm.h
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-20 10:07 ` Like Xu
@ 2023-02-22 1:55 ` Sean Christopherson
0 siblings, 0 replies; 19+ messages in thread
From: Sean Christopherson @ 2023-02-22 1:55 UTC (permalink / raw)
To: Like Xu; +Cc: kvm, linux-kernel, Paolo Bonzini
On Mon, Feb 20, 2023, Like Xu wrote:
> On 18/2/2023 6:54 am, Sean Christopherson wrote:
> > Add a KVM x86 doc to the subsystem/maintainer handbook section to explain
> > how KVM x86 (currently) operates as a sub-subsystem, and to soapbox on
> > the rules and expectations for contributing to KVM x86.
> >
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> > .../process/maintainer-handbooks.rst | 1 +
> > Documentation/process/maintainer-kvm-x86.rst | 347 ++++++++++++++++++
> > MAINTAINERS | 1 +
> > 3 files changed, 349 insertions(+)
> > create mode 100644 Documentation/process/maintainer-kvm-x86.rst
> >
> > diff --git a/Documentation/process/maintainer-handbooks.rst b/Documentation/process/maintainer-handbooks.rst
> > index d783060b4cc6..d12cbbe2b7df 100644
> > --- a/Documentation/process/maintainer-handbooks.rst
> > +++ b/Documentation/process/maintainer-handbooks.rst
> > @@ -17,3 +17,4 @@ Contents:
> > maintainer-tip
> > maintainer-netdev
> > + maintainer-kvm-x86
> > diff --git a/Documentation/process/maintainer-kvm-x86.rst b/Documentation/process/maintainer-kvm-x86.rst
> > new file mode 100644
> > index 000000000000..ac81a42a32a3
> > --- /dev/null
> > +++ b/Documentation/process/maintainer-kvm-x86.rst
> > @@ -0,0 +1,347 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +KVM x86
>
> KERNEL VIRTUAL MACHINE FOR X86 (KVM/x86)
My preference is "KVM x86", but I don't have a super strong opinion. I definitely
don't think it's necessary to strictly match the MAINTAINERS file.
> > +Lifecycle
> > +~~~~~~~~~
> > +Pull requests for the next release cycle are sent to the main KVM tree, one
> > +for each KVM x86 topic branch. If all goes well, the topic branches are rolled
> > +into the main KVM pull request sent during Linus' merge window. Pull requests
> > +for KVM x86 branches are typically made the week before Linus' opening of the
> > +merge window, e.g. the week following rc7 for "normal" releases.
> > +
> > +The KVM x86 tree doesn't have its own official merge window, but there's a soft
> > +close around rc5 for new features, and a soft close around rc6 for fixes.
>
> Any urgent AND critical fixes are exempt. No?
More or less. The majority of urgent and critical fixes should target the current
release, and the "soft" qualifier on "soft close" covers the rest. Ah, but this
section doesn't say anything about fixes that target mainline. I'll add a paragraph
to explicitly talk about fixes.
> > +SDM and APM References
> > +~~~~~~~~~~~~~~~~~~~~~~
> > +Much of KVM's code base is directly tied to architectural behavior defined in
> > +Intel's Software Development Manual (SDM) and AMD's Architecture Programmer’s
> > +Manual (APM). Use of "Intel's SDM" and "AMD's APM", or even just "SDM" or
> > +"APM", without additional context is a-ok.
>
> ISE: Intel® Architecture Instruction Set Extensions and Future Features
> PPR: Processor Programming Reference (PPR) for specific AMD Model
Hmm, I think we should explicitly spell those out in changelogs. ISE releases
should _never_ be referenced in code, and I would prefer
> > +
> > +Do not reference specific sections, tables, figures, etc. by number, especially
> > +not in comments. Instead, copy-paste the relevant snippet (if warranted), and
> > +reference sections/tables/figures by name. The layouts of the SDM and APM are
> > +constantly changing, and so the numbers/labels aren't stable/consistent.
> > +
> > +Generally speaking, do not copy-paste SDM or APM snippets into comments. With
> > +few exceptions, KVM *must* honor architectural behavior, therefore it's implied
> > +that KVM behavior is emulating SDM and/or APM behavior.
>
> All undefined behaviors (if any) need to be clarified.
>
> > +
> > +Shortlog
> > +~~~~~~~~
> > +The preferred prefix format is ``KVM: <topic>:``, where ``<topic>`` is one of::
> > +
> > + - x86
> > + - x86/mmu
> > + - x86/pmu
> > + - x86/xen
>
> Any conflict w/ "KVM X86 Xen (KVM/Xen)" ? Then "KVM X86 HYPER-V (KVM/hyper-v)" ?
I didn't follow this question. Are you asking if we should have "KVM: x86/hyperv:"?
If so, my answer is "probably not". The Hyper-V code isn't as isolated as the Xen
code and much of it is vendor specific, e.g. I would rather Hyper-V changes that
primarily touch vmx.c be tagged "KVM: VMX" as it's possible, however unlikely, that
non-Hyper-V users could be affected.
> > + - selftests > + - SVM
> > + - nSVM
> > + - VMX
> > + - nVMX
>
> emulator ? lapic ?
I don't want to introduce "emulator" as I don't think there will be a high enough
volume of patches to benefit from the extra qualifier, the emulator isn't as isolated
as we like to think it is, and because KVM does a lot of emulation outside of the
emulator. E.g. enter_smm() doesn't flow through emulate.c, but emulator_leave_smm()
_only_ gets invoked via the emulator.
Similar to Hyper-V, I'm hesitant to introduce x86/apic as many "APIC" changes are
actually in vendor specific code, e.g. much of APICv, AVIC, IPI virtualization, etc.
I'm definitely not opposed to using x86/apic for local and and maybe even I/O APIC
changes though.
> > +**DO NOT use x86/kvm!** ``x86/kvm`` is used exclusively for Linux-as-a-KVM-guest
> > +changes, i.e. for arch/x86/kernel/kvm.c.
> > +
> > +Note, these don't align with the topics branches (the topic branches care much
> > +more about code conflicts).
> > +
> > +All names are case sensitive! ``KVM: x86:`` is good, ``kvm: vmx:`` is not.
> > +
> > +Capitalize the first word of the condensed patch description, but omit ending
> > +punctionation. E.g.::
>
> s/punctionation/punctuation
>
> > +
> > + KVM: x86: Fix a null pointer dererence in function_xyz()
>
> s/dererence/dereference
Heh, at least I'm consistent.
> > + kvm: x86: fix a null pointer dererence in function_xyz.
> > +
> > +If a patch touches multiple topics, traverse up the conceptual tree to find the
> > +first common parent (which is often simply ``x86``). When in doubt,
> > +``git log path/to/file`` should provide a reasonable hint.
> > +
> > +New topics do occasionally pop up, but please start an on-list discussion if
> > +you want to propose introducing a new topic, i.e. don't go rogue.
> > +
> > +Do not use file names or complete file paths as the subject/shortlog prefix.
> > +
> > +Changelog
> > +~~~~~~~~~
> > +Write changelogs using imperative mood and avoid pronouns. Lead with a short
> > +blurb on what is changing, and then follow up with the context and background.
> > +Note! This order directly conflicts with the tip tree's preferred approach!
>
> Emm, could this be considered/clarified as an incentive option ?
Can you elaborate? I don't understand what you're asking.
> > +Testing
> > +-------
> > +At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m
> > +KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs
> > +isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and
> > +X86_64 are particularly interesting knobs to turn.
> > +
> > +Running KVM selftests and KVM-unit-tests is also mandatory (and stating the
> > +obvious, the tests need to pass). When possible and relevant, testing on both
> > +Intel and AMD is strongly preferred. Booting an actual VM is encouraged, but
> > +not mandatory.
>
> Testing L2 guest available features inside L1 is also encouraged.
Hmm. Sort of. In a perfect world where everyone has unlimited time, then testing
a inside a nested VM would be encouraged. But practically speaking, doing basic
testing of a feature in a nested VM doesn't buy all that much meaningful coverage,
and can even give a false sense of security.
The things we tend to get wrong are the edge cases, error handling, etc., and those
are unlikely to be excercised by basic testing, i.e. really need dedicated tests.
And if we have dedicated tests, then nested is covered by "Running selftests and KUT
are mandatory".
At least for official documentation, I think I would prefer not to explicitly
encourage people to use their time booting and testing in nested VMs.
> > +For changes that touch KVM's shadow paging code, running with TDP (EPT/NPT)
> > +disabled is mandatory. For changes that affect common KVM MMU code, running
> > +with TDP disabled is strongly encouraged. For all other changes, if the code
> > +being modified depends on and/or interacts with a module param, testing with
> > +the relevant settings is mandatory.
> > +
> > +Note, KVM selftests and KVM-unit-tests do have known failures. If you suspect
> > +a failure is not due to your changes, verify that the *exact same* failure
> > +occurs with and without your changes.
> > +
> > +If you can't fully test a change, e.g. due to lack of hardware, clearly state
> > +what level of testing you were able to do, e.g. in the cover letter.
>
> Add an RFT (request for test) tag.
I'm not necessarily opposed to the idea of "RFT", but until it's a ubiquitous
and/or formally documented tag I would prefer to avoid using RFT KVM x86.
For the overwhelming majority of changes where "I couldn't test this" is legitimate
_and_ the series/patch isn't tagged RFC, I have access to the necessary hardware.
For the few cases where I don't have hardware, I don't mind hunting down someone
who does. I.e. blasting "RFT to everyone isn't necessary.
In other words, I think we'll spend more time explaining and trying to understand
the use of RFT than we would benefit from using it to identify when a series needs
extra testing.
> > +New Features
> > +~~~~~~~~~~~~
> > +With one exception, new features *must* come with test coverage. KVM specific
> > +tests aren't strictly required, e.g. if coverage is provided by running a
> > +sufficiently enabled guest VM, or by running a related kernel selftest in a VM,
> > +but dedicated KVM tests are preferred in all cases. Negative testcases in
> > +particular are mandatory for enabling of new hardware features as error and
> > +exception flows are rarely exercised simply by running a VM.
> > +
> > +The only exception to this rule is if KVM is simply advertising support for a
> > +feature via KVM_SET_SUPPORTED_CPUID, i.e. for instructions/features that KVM
> > +can't prevent a guest from using and for which there is no true enabling.
> > +
> > +Note, "new features" does not just mean "new hardware features"! New features
> > +that can't be well validated using existing KVM selftests and/or KVM-unit-tests
> > +must come with tests.
>
> must come with tests to ensure good code coverage.
I _really_ don't want to add that qualifier. Code coverage is but one aspect of
testing, and it's not even #1 priority, e.g. exercising code that violates architectural
behavior means nothing if the test doesn't detect the error. I.e. the purpose of
tests isn't _just_ to ensure good code coverage.
> > +Posting new feature development without tests to get early feedback is more
> > +than welcome, but such submissions should be tagged RFC, and the cover letter
> > +should clearly state what type of feedback is requested/expected. Do not abuse
> > +the RFC process; RFCs will typically not receive in-depth review.
> > +
> > +Bug Fixes
> > +~~~~~~~~~
> > +Except for "obvious" found-by-inspection bugs, fixes must be accompanied by a
> > +reproducer for the bug being fixed. In many cases the reproducer is implicit,
> > +e.g. for build errors and test failures, but it should still be clear to
> > +readers what is broken and how to verify the fix. Some leeway is given for
> > +bugs that are found via non-public workloads/tests, but providing regression
> > +tests for such bugs is strongly preferred.
>
> tests or detailed reproduction sequence for such bugs is strongly preferred.
No, just tests. This is specifically talking about non-public workloads. If a
detailed reproduction sequence _doesn't_ involve writing code, i.e. a test, then
the fix should simply provider the reproducer.
If you're talking about the sequence of events in KVM that result in the bug,
then that info belongs in the changelog and is largely covered by the tip tree
handbook.
> > +Git Base
> > +~~~~~~~~
> > +If you are using git version 2.9.0 or later (Googlers, this is all of you!),
>
> Please do not mention specific developers or groups in this type of document.
Honest question, why not?
Calling out a specific developer is one thing, but I don't see the harm in adding
what essentially amounts to an extra requirement for Google employees.
> > +use ``git format-patch`` with the ``--base`` flag to automatically include the
> > +base tree information in the generated patches.
> > +
> > +Note, ``--base=auto`` works as expected if and only if a branch's upstream is
> > +set to the base topic branch, e.g. it will do the wrong thing if your upstream
> > +is set to your personal repository for backup purposes. An alternative "auto"
> > +solution is to derive the names of your development branches based on their
> > +KVM x86 topic, and feed that into ``--base``. E.g. ``x86/pmu/my_branch_name``,
> > +and then write a small wrapper to extract ``pmu`` from the current branch name
> > +to yield ``--base=x/pmu``, where ``x`` is whatever name your repository uses to
> > +track the KVM x86 remote.
> > +
> > +Co-Posting Tests
> > +~~~~~~~~~~~~~~~~
> > +KVM selftests that are associated with KVM changes, e.g. regression tests for
> > +bug fixes, should be posted along with the KVM changes as a single series.
>
> Keeping git-bisect is mandatory.
Ah, yeah, I'll explicitly call out the order between tests and KVM.
> > +Vulnerabilities
> > +---------------
> > +Bugs that can be exploited by the guest to attack the host (kernel or
> > +userspace), or that can be exploited by a nested VM to *its* host (L2 attacking
> > +L1), are of particular interest to KVM. Please follow the protocol for
>
> L1, even L0 host),
No, because at that point it's just the guest attacking the host. This snippet
is purely translating "nested VM to *its* host* into familiar terminology.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-17 22:54 ` [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86 Sean Christopherson
` (2 preceding siblings ...)
2023-02-20 10:07 ` Like Xu
@ 2023-02-20 23:17 ` David Woodhouse
2023-02-21 19:54 ` Sean Christopherson
2023-02-21 11:06 ` Yu Zhang
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: David Woodhouse @ 2023-02-20 23:17 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
Sagi Shahar, Erdem Aktas, Peter Shier, Anish Ghulati,
Oliver Upton, James Houghton, Anish Moorthy, Ben Gardon,
David Matlack, Ricardo Koller, Axel Rasmussen, Aaron Lewis,
Ashish Kalra, Babu Moger, Chao Gao, Chao Peng, Chenyi Qiang,
Emanuele Giuseppe Esposito, Gavin Shan, Guang Zeng, Hou Wenlong,
Jiaxi Chen, Jim Mattson, Jing Liu, Junaid Shahid, Kai Huang,
Leonardo Bras, Like Xu, Li RongQing, Maciej S . Szmigiero,
Maxim Levitsky, Michael Roth, Michal Luczaj, Mingwei Zhang,
Nikunj A Dadhania, Paul Durrant, Peng Hao, Peter Gonda, Peter Xu,
Robert Hoo, Suravee Suthikulpanit, Tom Lendacky, Vipin Sharma,
Vitaly Kuznetsov, Wanpeng Li, Wei Wang, Xiaoyao Li, Yu Zhang,
Zhenzhong Duan, kvm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 588 bytes --]
On Fri, 2023-02-17 at 14:54 -0800, Sean Christopherson wrote:
>
> +All topic branches, except for ``next`` and ``fixes``, are rolled into ``next``
> +via a cthulu merge on an as-needed basis, i.e. when a topic branch is updated.
> +As a result, force pushes to ``next`` are common.
> +
This makes 'next' an unfortunate name, doesn't it? Since branches
destined for "linux-next", which has been using that name for far
longer, have exactly the opposite expectation — that they have stable
commit IDs.
Would 'staging' not be more conventional for the branch you describe?
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-20 23:17 ` David Woodhouse
@ 2023-02-21 19:54 ` Sean Christopherson
0 siblings, 0 replies; 19+ messages in thread
From: Sean Christopherson @ 2023-02-21 19:54 UTC (permalink / raw)
To: David Woodhouse
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, Emanuele Giuseppe Esposito, Gavin Shan, Guang Zeng,
Hou Wenlong, Jiaxi Chen, Jim Mattson, Jing Liu, Junaid Shahid,
Kai Huang, Leonardo Bras, Like Xu, Li RongQing,
Maciej S . Szmigiero, Maxim Levitsky, Michael Roth,
Michal Luczaj, Mingwei Zhang, Nikunj A Dadhania, Paul Durrant,
Peng Hao, Peter Gonda, Peter Xu, Robert Hoo,
Suravee Suthikulpanit, Tom Lendacky, Vipin Sharma,
Vitaly Kuznetsov, Wanpeng Li, Wei Wang, Xiaoyao Li, Yu Zhang,
Zhenzhong Duan, kvm, linux-kernel
On Mon, Feb 20, 2023, David Woodhouse wrote:
> On Fri, 2023-02-17 at 14:54 -0800, Sean Christopherson wrote:
> >
> > +All topic branches, except for ``next`` and ``fixes``, are rolled into ``next``
> > +via a cthulu merge on an as-needed basis, i.e. when a topic branch is updated.
> > +As a result, force pushes to ``next`` are common.
> > +
>
> This makes 'next' an unfortunate name, doesn't it? Since branches
> destined for "linux-next", which has been using that name for far
> longer, have exactly the opposite expectation — that they have stable
> commit IDs.
I was coming at it from the viewpoint of linux-next itself, where HEAD is rebuilt
nightly and thus is not stable. The inputs are stable, just not the merge commit.
> Would 'staging' not be more conventional for the branch you describe?
Not really? It's not a staging area, it really is the branch that contains the
changes for the "next" kernel.
What if I drop the above guidance and instead push a date-stamped tag when pushing
to 'next'? That should ensure the base is reachable for everyone, and would also
provide a paper trail for what I've done, which is probably a good idea regardless.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-17 22:54 ` [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86 Sean Christopherson
` (3 preceding siblings ...)
2023-02-20 23:17 ` David Woodhouse
@ 2023-02-21 11:06 ` Yu Zhang
2023-02-22 0:25 ` Sean Christopherson
2023-02-22 19:26 ` Maciej S. Szmigiero
2023-02-28 14:45 ` Robert Hoo
6 siblings, 1 reply; 19+ messages in thread
From: Yu Zhang @ 2023-02-21 11:06 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, David Woodhouse, Emanuele Giuseppe Esposito,
Gavin Shan, Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson,
Jing Liu, Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu,
Li RongQing, Maciej S . Szmigiero, Maxim Levitsky, Michael Roth,
Michal Luczaj, Mingwei Zhang, Nikunj A Dadhania, Paul Durrant,
Peng Hao, Peter Gonda, Peter Xu, Robert Hoo,
Suravee Suthikulpanit, Tom Lendacky, Vipin Sharma,
Vitaly Kuznetsov, Wanpeng Li, Wei Wang, Xiaoyao Li,
Zhenzhong Duan, kvm, linux-kernel
Thank you so much, Sean, for such a detailed guidance!
Some questions below:
On Fri, Feb 17, 2023 at 02:54:49PM -0800, Sean Christopherson wrote:
> Add a KVM x86 doc to the subsystem/maintainer handbook section to explain
> how KVM x86 (currently) operates as a sub-subsystem, and to soapbox on
> the rules and expectations for contributing to KVM x86.
It's a fantastic doc! Also, many good requirements can be common in KVM, not
just KVM x86(e.g. the comment, changelog format, the testing requirement
etc.). Could we be greedier to ask our KVM maintainers for a generic handbook
of KVM, and maybe different sections for specific arches, which describe their
specific requirements(the base trees and branches, the maintaining processes
etc.)? :)
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> .../process/maintainer-handbooks.rst | 1 +
> Documentation/process/maintainer-kvm-x86.rst | 347 ++++++++++++++++++
> MAINTAINERS | 1 +
> 3 files changed, 349 insertions(+)
> create mode 100644 Documentation/process/maintainer-kvm-x86.rst
>
> diff --git a/Documentation/process/maintainer-handbooks.rst b/Documentation/process/maintainer-handbooks.rst
> index d783060b4cc6..d12cbbe2b7df 100644
> --- a/Documentation/process/maintainer-handbooks.rst
> +++ b/Documentation/process/maintainer-handbooks.rst
> @@ -17,3 +17,4 @@ Contents:
>
> maintainer-tip
> maintainer-netdev
> + maintainer-kvm-x86
> diff --git a/Documentation/process/maintainer-kvm-x86.rst b/Documentation/process/maintainer-kvm-x86.rst
> new file mode 100644
> index 000000000000..ac81a42a32a3
> --- /dev/null
> +++ b/Documentation/process/maintainer-kvm-x86.rst
> @@ -0,0 +1,347 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +KVM x86
> +=======
> +
> +TL;DR
> +-----
> +Testing is mandatory. Be consistent with established styles and patterns.
> +
> +Trees
> +-----
> +KVM x86 is currently in a transition period from being part of the main KVM
> +tree, to being "just another KVM arch". As such, KVM x86 is split across the
> +main KVM tree, ``git.kernel.org/pub/scm/virt/kvm/kvm.git``, and a KVM x86
> +specific tree, ``github.com/kvm-x86/linux.git``.
Does other arch also have a specific tree? If a patch series touches multiple
archs(though the chance could be very low), I guess that patch set should still
be based on the main KVM tree? The master branch or the next branch?
...
> +Co-Posting Tests
> +~~~~~~~~~~~~~~~~
> +KVM selftests that are associated with KVM changes, e.g. regression tests for
> +bug fixes, should be posted along with the KVM changes as a single series.
> +
> +KVM-unit-tests should *always* be posted separately. Tools, e.g. b4 am, don't
> +know that KVM-unit-tests is a separate repository and get confused when patches
> +in a series apply on different trees. To tie KVM-unit-tests patches back to
> +KVM patches, first post the KVM changes and then provide a lore Link: to the
> +KVM patch/series in the KVM-unit-tests patch(es).
I wonder, for KVM bugzilla to report a bug, or for our QAs to perform regular
tests using KVM selftests/KVM-unit-tests, which tree/branch is more reasonable
to be based on?
E.g., I saw some bugzilla issues earlier, reporting failures of some unit tests,
did some investigation, yet to find those failures were just because the corresponding
KVM patches had not been merged yet.
Maybe we also should take care of the timings of the merging of KVM patches and
the test patches? Two examples(I'm sure there're more :)):
1> https://bugzilla.kernel.org/show_bug.cgi?id=216812
2> https://bugzilla.kernel.org/show_bug.cgi?id=216725
B.R.
Yu
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-21 11:06 ` Yu Zhang
@ 2023-02-22 0:25 ` Sean Christopherson
2023-02-24 9:44 ` Yu Zhang
0 siblings, 1 reply; 19+ messages in thread
From: Sean Christopherson @ 2023-02-22 0:25 UTC (permalink / raw)
To: Yu Zhang
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, David Woodhouse, Emanuele Giuseppe Esposito,
Gavin Shan, Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson,
Jing Liu, Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu,
Li RongQing, Maciej S . Szmigiero, Maxim Levitsky, Michael Roth,
Michal Luczaj, Mingwei Zhang, Nikunj A Dadhania, Paul Durrant,
Peng Hao, Peter Gonda, Peter Xu, Robert Hoo,
Suravee Suthikulpanit, Tom Lendacky, Vipin Sharma,
Vitaly Kuznetsov, Wanpeng Li, Wei Wang, Xiaoyao Li,
Zhenzhong Duan, kvm, linux-kernel
On Tue, Feb 21, 2023, Yu Zhang wrote:
> Thank you so much, Sean, for such a detailed guidance!
>
> Some questions below:
>
> On Fri, Feb 17, 2023 at 02:54:49PM -0800, Sean Christopherson wrote:
> > Add a KVM x86 doc to the subsystem/maintainer handbook section to explain
> > how KVM x86 (currently) operates as a sub-subsystem, and to soapbox on
> > the rules and expectations for contributing to KVM x86.
>
> It's a fantastic doc! Also, many good requirements can be common in KVM, not
> just KVM x86(e.g. the comment, changelog format, the testing requirement
> etc.). Could we be greedier to ask our KVM maintainers for a generic handbook
> of KVM, and maybe different sections for specific arches, which describe their
> specific requirements(the base trees and branches, the maintaining processes
> etc.)? :)
At some point, yes, but my strong preference is to document the x86 side of things
and then work from there. For KVM x86, I can mostly just say "these are the rules".
Same goes for the other KVM arch maintainers (for their areas).
Incorporating all of KVM would require a much more collaborative effort, which isn't
a bad thing, but it will take more time and effort. And IMO, KVM x86 needs this
typ eof documentation a lot more than the other KVM architectures, i.e. pushing out
KVM x86 documentation in order to go for more comprehensive documentation is not a
good tradeoff.
> > +Trees
> > +-----
> > +KVM x86 is currently in a transition period from being part of the main KVM
> > +tree, to being "just another KVM arch". As such, KVM x86 is split across the
> > +main KVM tree, ``git.kernel.org/pub/scm/virt/kvm/kvm.git``, and a KVM x86
> > +specific tree, ``github.com/kvm-x86/linux.git``.
>
> Does other arch also have a specific tree?
Yes.
> If a patch series touches multiple archs(though the chance could be very
> low), I guess that patch set should still be based on the main KVM tree? The
> master branch or the next branch?
Hmm, good question. Using kvm-86/next is likely the best answer in most cases.
kvm/master is usually a bad choice because it won't have _any_ changes for the next
release, i.e. using it as a base is more likely to yield conflicts. Similarly,
kvm/queue and kvm/next are unlikely to have more relevant changes than kvm-x86/next.
If there are non-trivial conflicts with multiple architectures then coordination
between maintainers will be required no matter what base is used. And I would
expect people sending those types of series to have enough experience to be able
to make a judgment call and/or engage with maintainers to figure out the best solution.
I'll rework the "Base Tree/Branch" to explicitly state that any series that primarily
targets x86 should be based on kvm-x86/next, but with a "use common sense" qualifier.
> > +Co-Posting Tests
> > +~~~~~~~~~~~~~~~~
> > +KVM selftests that are associated with KVM changes, e.g. regression tests for
> > +bug fixes, should be posted along with the KVM changes as a single series.
> > +
> > +KVM-unit-tests should *always* be posted separately. Tools, e.g. b4 am, don't
> > +know that KVM-unit-tests is a separate repository and get confused when patches
> > +in a series apply on different trees. To tie KVM-unit-tests patches back to
> > +KVM patches, first post the KVM changes and then provide a lore Link: to the
> > +KVM patch/series in the KVM-unit-tests patch(es).
>
> I wonder, for KVM bugzilla to report a bug, or for our QAs to perform regular
> tests using KVM selftests/KVM-unit-tests, which tree/branch is more reasonable
> to be based on?
>
> E.g., I saw some bugzilla issues earlier, reporting failures of some unit tests,
> did some investigation, yet to find those failures were just because the corresponding
> KVM patches had not been merged yet.
>
> Maybe we also should take care of the timings of the merging of KVM patches and
> the test patches?
I really don't want to hold up KVM-unit-test patches waiting for KVM fixes to be
merged. KUT is already woefully under-maintained, artificially holding up patches
will only make things worse. And simply waiting for patches to land in KVM doesn't
necessarily solve things either, e.g. if the fixes land in kvm/master mid-cycle
then running against kvm/next will continue to fail. Waiting also doesn't help
people running KUT against older kernels, e.g. for qualifying stable kernels.
I completely understand the pain, but unfortunately no one has come up with an
elegant, low-maintenance solution (this problem has been discussed multiple times
in the past).
> Two examples(I'm sure there're more :)):
> 1> https://bugzilla.kernel.org/show_bug.cgi?id=216812
> 2> https://bugzilla.kernel.org/show_bug.cgi?id=216725
>
>
> B.R.
> Yu
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-22 0:25 ` Sean Christopherson
@ 2023-02-24 9:44 ` Yu Zhang
0 siblings, 0 replies; 19+ messages in thread
From: Yu Zhang @ 2023-02-24 9:44 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, David Woodhouse, Emanuele Giuseppe Esposito,
Gavin Shan, Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson,
Jing Liu, Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu,
Li RongQing, Maciej S . Szmigiero, Maxim Levitsky, Michael Roth,
Michal Luczaj, Mingwei Zhang, Nikunj A Dadhania, Paul Durrant,
Peng Hao, Peter Gonda, Peter Xu, Robert Hoo,
Suravee Suthikulpanit, Tom Lendacky, Vipin Sharma,
Vitaly Kuznetsov, Wanpeng Li, Wei Wang, Xiaoyao Li,
Zhenzhong Duan, kvm, linux-kernel
On Tue, Feb 21, 2023 at 04:25:42PM -0800, Sean Christopherson wrote:
> On Tue, Feb 21, 2023, Yu Zhang wrote:
> > Thank you so much, Sean, for such a detailed guidance!
> >
> > Some questions below:
> >
> > On Fri, Feb 17, 2023 at 02:54:49PM -0800, Sean Christopherson wrote:
> > > Add a KVM x86 doc to the subsystem/maintainer handbook section to explain
> > > how KVM x86 (currently) operates as a sub-subsystem, and to soapbox on
> > > the rules and expectations for contributing to KVM x86.
> >
> > It's a fantastic doc! Also, many good requirements can be common in KVM, not
> > just KVM x86(e.g. the comment, changelog format, the testing requirement
> > etc.). Could we be greedier to ask our KVM maintainers for a generic handbook
> > of KVM, and maybe different sections for specific arches, which describe their
> > specific requirements(the base trees and branches, the maintaining processes
> > etc.)? :)
>
> At some point, yes, but my strong preference is to document the x86 side of things
> and then work from there. For KVM x86, I can mostly just say "these are the rules".
> Same goes for the other KVM arch maintainers (for their areas).
>
> Incorporating all of KVM would require a much more collaborative effort, which isn't
> a bad thing, but it will take more time and effort. And IMO, KVM x86 needs this
> typ eof documentation a lot more than the other KVM architectures, i.e. pushing out
> KVM x86 documentation in order to go for more comprehensive documentation is not a
> good tradeoff.
Sure. No reason to push out this doc.
>
> > > +Trees
> > > +-----
> > > +KVM x86 is currently in a transition period from being part of the main KVM
> > > +tree, to being "just another KVM arch". As such, KVM x86 is split across the
> > > +main KVM tree, ``git.kernel.org/pub/scm/virt/kvm/kvm.git``, and a KVM x86
> > > +specific tree, ``github.com/kvm-x86/linux.git``.
> >
> > Does other arch also have a specific tree?
>
> Yes.
>
> > If a patch series touches multiple archs(though the chance could be very
> > low), I guess that patch set should still be based on the main KVM tree? The
> > master branch or the next branch?
>
> Hmm, good question. Using kvm-86/next is likely the best answer in most cases.
> kvm/master is usually a bad choice because it won't have _any_ changes for the next
> release, i.e. using it as a base is more likely to yield conflicts. Similarly,
> kvm/queue and kvm/next are unlikely to have more relevant changes than kvm-x86/next.
Thanks. Let's try.
>
> If there are non-trivial conflicts with multiple architectures then coordination
> between maintainers will be required no matter what base is used. And I would
> expect people sending those types of series to have enough experience to be able
> to make a judgment call and/or engage with maintainers to figure out the best solution.
>
> I'll rework the "Base Tree/Branch" to explicitly state that any series that primarily
> targets x86 should be based on kvm-x86/next, but with a "use common sense" qualifier.
>
> > > +Co-Posting Tests
> > > +~~~~~~~~~~~~~~~~
> > > +KVM selftests that are associated with KVM changes, e.g. regression tests for
> > > +bug fixes, should be posted along with the KVM changes as a single series.
> > > +
> > > +KVM-unit-tests should *always* be posted separately. Tools, e.g. b4 am, don't
> > > +know that KVM-unit-tests is a separate repository and get confused when patches
> > > +in a series apply on different trees. To tie KVM-unit-tests patches back to
> > > +KVM patches, first post the KVM changes and then provide a lore Link: to the
> > > +KVM patch/series in the KVM-unit-tests patch(es).
> >
> > I wonder, for KVM bugzilla to report a bug, or for our QAs to perform regular
> > tests using KVM selftests/KVM-unit-tests, which tree/branch is more reasonable
> > to be based on?
> >
> > E.g., I saw some bugzilla issues earlier, reporting failures of some unit tests,
> > did some investigation, yet to find those failures were just because the corresponding
> > KVM patches had not been merged yet.
> >
> > Maybe we also should take care of the timings of the merging of KVM patches and
> > the test patches?
>
> I really don't want to hold up KVM-unit-test patches waiting for KVM fixes to be
> merged. KUT is already woefully under-maintained, artificially holding up patches
> will only make things worse. And simply waiting for patches to land in KVM doesn't
> necessarily solve things either, e.g. if the fixes land in kvm/master mid-cycle
> then running against kvm/next will continue to fail. Waiting also doesn't help
> people running KUT against older kernels, e.g. for qualifying stable kernels.
>
> I completely understand the pain, but unfortunately no one has come up with an
> elegant, low-maintenance solution (this problem has been discussed multiple times
> in the past).
Got it. The pain is still tolerable.
B.R.
Yu
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-17 22:54 ` [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86 Sean Christopherson
` (4 preceding siblings ...)
2023-02-21 11:06 ` Yu Zhang
@ 2023-02-22 19:26 ` Maciej S. Szmigiero
2023-02-22 21:25 ` Sean Christopherson
2023-02-28 14:45 ` Robert Hoo
6 siblings, 1 reply; 19+ messages in thread
From: Maciej S. Szmigiero @ 2023-02-22 19:26 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
Sagi Shahar, Erdem Aktas, Peter Shier, Anish Ghulati,
Oliver Upton, James Houghton, Anish Moorthy, Ben Gardon,
David Matlack, Ricardo Koller, Axel Rasmussen, Aaron Lewis,
Ashish Kalra, Babu Moger, Chao Gao, Chao Peng, Chenyi Qiang,
David Woodhouse, Emanuele Giuseppe Esposito, Gavin Shan,
Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson, Jing Liu,
Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu, Li RongQing,
Maxim Levitsky, Michael Roth, Michal Luczaj, Mingwei Zhang,
Nikunj A Dadhania, Paul Durrant, Peng Hao, Peter Gonda, Peter Xu,
Robert Hoo, Suravee Suthikulpanit, Tom Lendacky, Vipin Sharma,
Vitaly Kuznetsov, Wanpeng Li, Wei Wang, Xiaoyao Li, Yu Zhang,
Zhenzhong Duan, kvm, linux-kernel
On 17.02.2023 23:54, Sean Christopherson wrote:
> Add a KVM x86 doc to the subsystem/maintainer handbook section to explain
> how KVM x86 (currently) operates as a sub-subsystem, and to soapbox on
> the rules and expectations for contributing to KVM x86.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> .../process/maintainer-handbooks.rst | 1 +
> Documentation/process/maintainer-kvm-x86.rst | 347 ++++++++++++++++++
> MAINTAINERS | 1 +
> 3 files changed, 349 insertions(+)
> create mode 100644 Documentation/process/maintainer-kvm-x86.rst
>
> diff --git a/Documentation/process/maintainer-handbooks.rst b/Documentation/process/maintainer-handbooks.rst
> index d783060b4cc6..d12cbbe2b7df 100644
> --- a/Documentation/process/maintainer-handbooks.rst
> +++ b/Documentation/process/maintainer-handbooks.rst
> @@ -17,3 +17,4 @@ Contents:
>
> maintainer-tip
> maintainer-netdev
> + maintainer-kvm-x86
> diff --git a/Documentation/process/maintainer-kvm-x86.rst b/Documentation/process/maintainer-kvm-x86.rst
> new file mode 100644
> index 000000000000..ac81a42a32a3
> --- /dev/null
> +++ b/Documentation/process/maintainer-kvm-x86.rst
(...)
> +
> +Coding Style
> +~~~~~~~~~~~~
> +When it comes to style, naming, patterns, etc., consistency is the number one
> +priority in KVM x86. If all else fails, match what already exists.
> +
> +With a few caveats listed below, follow the tip tree maintainers' preferred
> +:ref:`maintainer-tip-coding-style`, as patches/series often touch both KVM and
> +non-KVM x86 files, i.e. draw the attention of KVM *and* tip tree maintainers.
> +
> +Using reverse fir tree for variable declarations isn't strictly required,
> +though it is still preferred.
That's the first time I hear about "reverse fir tree" variable order,
no wonder other people were surprised too.
AFAIK the most common variable order seems to be the order in which these
variables actually get first used inside that code block.
> +
> +Except for a handful of special snowflakes, do not use kernel-doc comments for
> +functions. The vast majority of "public" KVM functions aren't truly public as
> +they are intended only for KVM-internal consumption (there are plans to
> +privatize KVM's headers and exports to enforce this).
> +
> +Comments
> +~~~~~~~~
> +Write comments using imperative mood and avoid pronouns. Use comments to
> +provide a high level overview of the code, and/or to explain why the code does
> +what it does. Do not reiterate what the code literally does; let the code
> +speak for itself. If the code itself is inscrutable, comments will not help.
> +
> +SDM and APM References
> +~~~~~~~~~~~~~~~~~~~~~~
> +Much of KVM's code base is directly tied to architectural behavior defined in
> +Intel's Software Development Manual (SDM) and AMD's Architecture Programmer’s
> +Manual (APM). Use of "Intel's SDM" and "AMD's APM", or even just "SDM" or
> +"APM", without additional context is a-ok.
> +
> +Do not reference specific sections, tables, figures, etc. by number, especially
> +not in comments. Instead, copy-paste the relevant snippet (if warranted), and
> +reference sections/tables/figures by name.
This says do "copy-paste the relevant snippet"...
> The layouts of the SDM and APM are
> +constantly changing, and so the numbers/labels aren't stable/consistent.
> +
> +Generally speaking, do not copy-paste SDM or APM snippets into comments.
...but this seems to say "don't do that".
More specific guidance would probably help here.
> With
> +few exceptions, KVM *must* honor architectural behavior, therefore it's implied
> +that KVM behavior is emulating SDM and/or APM behavior.
> +
> +Shortlog
> +~~~~~~~~
> +The preferred prefix format is ``KVM: <topic>:``, where ``<topic>`` is one of::
> +
> + - x86
> + - x86/mmu
> + - x86/pmu
> + - x86/xen
> + - selftests
> + - SVM
> + - nSVM
> + - VMX
> + - nVMX
> +
> +**DO NOT use x86/kvm!** ``x86/kvm`` is used exclusively for Linux-as-a-KVM-guest
> +changes, i.e. for arch/x86/kernel/kvm.c.
> +
> +Note, these don't align with the topics branches (the topic branches care much
> +more about code conflicts).
> +
> +All names are case sensitive! ``KVM: x86:`` is good, ``kvm: vmx:`` is not.
> +
> +Capitalize the first word of the condensed patch description, but omit ending
> +punctionation. E.g.::
> +
> + KVM: x86: Fix a null pointer dererence in function_xyz()
> +
> +not::
> +
> + kvm: x86: fix a null pointer dererence in function_xyz.
> +
> +If a patch touches multiple topics, traverse up the conceptual tree to find the
> +first common parent (which is often simply ``x86``). When in doubt,
> +``git log path/to/file`` should provide a reasonable hint.
> +
> +New topics do occasionally pop up, but please start an on-list discussion if
> +you want to propose introducing a new topic, i.e. don't go rogue.
> +
> +Do not use file names or complete file paths as the subject/shortlog prefix.
Do we strictly obey the "75 characters max" rule for the subject/shortlog
or do we prefer to be more flexible here if it results in a more
descriptive patch subject?
(...)
> +Testing
> +-------
> +At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m
> +KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs
> +isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and
> +X86_64 are particularly interesting knobs to turn.
> +
> +Running KVM selftests and KVM-unit-tests is also mandatory (and stating the
> +obvious, the tests need to pass).
I would add an exception here from mandatory testing for changes that
obviously have negligible probability of affecting runtime behavior.
For example: patches that modify just code comments or documentation.
> When possible and relevant, testing on both
> +Intel and AMD is strongly preferred. Booting an actual VM is encouraged, but
> +not mandatory.
> +
> +For changes that touch KVM's shadow paging code, running with TDP (EPT/NPT)
> +disabled is mandatory. For changes that affect common KVM MMU code, running
> +with TDP disabled is strongly encouraged. For all other changes, if the code
> +being modified depends on and/or interacts with a module param, testing with
> +the relevant settings is mandatory.
> +
> +Note, KVM selftests and KVM-unit-tests do have known failures. If you suspect
> +a failure is not due to your changes, verify that the *exact same* failure
> +occurs with and without your changes.
> +
> +If you can't fully test a change, e.g. due to lack of hardware, clearly state
> +what level of testing you were able to do, e.g. in the cover letter.
> +
(...)
Thanks for preparing such a detailed handbook Sean.
However, having so many rules might seem intimidating for newcomers, and
deter them from contributing out of fear that they'll be screamed at for
accidentally breaking some obscure rule.
That's especially true for unpaid volunteers that might become
professional kernel developers one day if their learning curve isn't
made too steep.
Maybe have a paragraph or two that, despite all these rules, KVM x86
strives to be a welcome community, encouraging newcomers and understanding
their beginner mistakes (or so)?
Thanks,
Maciej
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-22 19:26 ` Maciej S. Szmigiero
@ 2023-02-22 21:25 ` Sean Christopherson
2023-02-22 22:09 ` Maciej S. Szmigiero
0 siblings, 1 reply; 19+ messages in thread
From: Sean Christopherson @ 2023-02-22 21:25 UTC (permalink / raw)
To: Maciej S. Szmigiero
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, David Woodhouse, Emanuele Giuseppe Esposito,
Gavin Shan, Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson,
Jing Liu, Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu,
Li RongQing, Maxim Levitsky, Michael Roth, Michal Luczaj,
Mingwei Zhang, Nikunj A Dadhania, Paul Durrant, Peng Hao,
Peter Gonda, Peter Xu, Robert Hoo, Suravee Suthikulpanit,
Tom Lendacky, Vipin Sharma, Vitaly Kuznetsov, Wanpeng Li,
Wei Wang, Xiaoyao Li, Yu Zhang, Zhenzhong Duan, kvm,
linux-kernel
On Wed, Feb 22, 2023, Maciej S. Szmigiero wrote:
> On 17.02.2023 23:54, Sean Christopherson wrote:
> > +SDM and APM References
> > +~~~~~~~~~~~~~~~~~~~~~~
> > +Much of KVM's code base is directly tied to architectural behavior defined in
> > +Intel's Software Development Manual (SDM) and AMD's Architecture Programmer’s
> > +Manual (APM). Use of "Intel's SDM" and "AMD's APM", or even just "SDM" or
> > +"APM", without additional context is a-ok.
> > +
> > +Do not reference specific sections, tables, figures, etc. by number, especially
> > +not in comments. Instead, copy-paste the relevant snippet (if warranted), and
> > +reference sections/tables/figures by name.
>
> This says do "copy-paste the relevant snippet"...
>
> > The layouts of the SDM and APM are
> > +constantly changing, and so the numbers/labels aren't stable/consistent.
> > +
> > +Generally speaking, do not copy-paste SDM or APM snippets into
> > comments.
>
> ...but this seems to say "don't do that".
Yeah, that didn't come out right.
> More specific guidance would probably help here.
Is this better?
Do not reference specific sections, tables, figures, etc. by number, especially
not in comments. Instead, if necessary (see below), copy-paste the relevant
snippet and reference sections/tables/figures by name. The layouts of the SDM
and APM are constantly changing, and so the numbers/labels aren't stable.
Generally speaking, do not explicitly reference or copy-paste from the SDM or
APM in comments. With few exceptions, KVM *must* honor architectural behavior,
therefore it's implied that KVM behavior is emulating SDM and/or APM behavior.
Note, referencing the SDM/APM in changelogs to justify the change and provide
context is perfectly ok and encouraged.
> > +If a patch touches multiple topics, traverse up the conceptual tree to find the
> > +first common parent (which is often simply ``x86``). When in doubt,
> > +``git log path/to/file`` should provide a reasonable hint.
> > +
> > +New topics do occasionally pop up, but please start an on-list discussion if
> > +you want to propose introducing a new topic, i.e. don't go rogue.
> > +
> > +Do not use file names or complete file paths as the subject/shortlog prefix.
>
> Do we strictly obey the "75 characters max" rule for the subject/shortlog
> or do we prefer to be more flexible here if it results in a more
> descriptive patch subject?
I prefer to be a little flexible, I'll expand this section to clarify that.
> (...)
> > +Testing
> > +-------
> > +At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m
> > +KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs
> > +isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and
> > +X86_64 are particularly interesting knobs to turn.
> > +
> > +Running KVM selftests and KVM-unit-tests is also mandatory (and stating the
> > +obvious, the tests need to pass).
>
> I would add an exception here from mandatory testing for changes that
> obviously have negligible probability of affecting runtime behavior.
>
> For example: patches that modify just code comments or documentation.
Agreed, will add.
Regarding documentation, I think I'll also add a requirement of 'make htmldocs'
without warnings for non-trivial docs changes. It's all too easy to write buggy
ReST "code" that looks correct as raw text, e.g. the whole double-colon thing.
> > When possible and relevant, testing on both
> > +Intel and AMD is strongly preferred. Booting an actual VM is encouraged, but
> > +not mandatory.
> > +
> > +For changes that touch KVM's shadow paging code, running with TDP (EPT/NPT)
> > +disabled is mandatory. For changes that affect common KVM MMU code, running
> > +with TDP disabled is strongly encouraged. For all other changes, if the code
> > +being modified depends on and/or interacts with a module param, testing with
> > +the relevant settings is mandatory.
> > +
> > +Note, KVM selftests and KVM-unit-tests do have known failures. If you suspect
> > +a failure is not due to your changes, verify that the *exact same* failure
> > +occurs with and without your changes.
> > +
> > +If you can't fully test a change, e.g. due to lack of hardware, clearly state
> > +what level of testing you were able to do, e.g. in the cover letter.
> > +
> (...)
>
> Thanks for preparing such a detailed handbook Sean.
>
> However, having so many rules might seem intimidating for newcomers, and
> deter them from contributing out of fear that they'll be screamed at for
> accidentally breaking some obscure rule.
>
> That's especially true for unpaid volunteers that might become
> professional kernel developers one day if their learning curve isn't
> made too steep.
>
> Maybe have a paragraph or two that, despite all these rules, KVM x86
> strives to be a welcome community, encouraging newcomers and understanding
> their beginner mistakes (or so)?
I like that idea a lot, I'll add a section at the very top.
Thanks much!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-22 21:25 ` Sean Christopherson
@ 2023-02-22 22:09 ` Maciej S. Szmigiero
0 siblings, 0 replies; 19+ messages in thread
From: Maciej S. Szmigiero @ 2023-02-22 22:09 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, David Woodhouse, Emanuele Giuseppe Esposito,
Gavin Shan, Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson,
Jing Liu, Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu,
Li RongQing, Maxim Levitsky, Michael Roth, Michal Luczaj,
Mingwei Zhang, Nikunj A Dadhania, Paul Durrant, Peng Hao,
Peter Gonda, Peter Xu, Robert Hoo, Suravee Suthikulpanit,
Tom Lendacky, Vipin Sharma, Vitaly Kuznetsov, Wanpeng Li,
Wei Wang, Xiaoyao Li, Yu Zhang, Zhenzhong Duan, kvm,
linux-kernel
On 22.02.2023 22:25, Sean Christopherson wrote:
> On Wed, Feb 22, 2023, Maciej S. Szmigiero wrote:
>> On 17.02.2023 23:54, Sean Christopherson wrote:
>>> +SDM and APM References
>>> +~~~~~~~~~~~~~~~~~~~~~~
>>> +Much of KVM's code base is directly tied to architectural behavior defined in
>>> +Intel's Software Development Manual (SDM) and AMD's Architecture Programmer’s
>>> +Manual (APM). Use of "Intel's SDM" and "AMD's APM", or even just "SDM" or
>>> +"APM", without additional context is a-ok.
>>> +
>>> +Do not reference specific sections, tables, figures, etc. by number, especially
>>> +not in comments. Instead, copy-paste the relevant snippet (if warranted), and
>>> +reference sections/tables/figures by name.
>>
>> This says do "copy-paste the relevant snippet"...
>>
>>> The layouts of the SDM and APM are
>>> +constantly changing, and so the numbers/labels aren't stable/consistent.
>>> +
>>> +Generally speaking, do not copy-paste SDM or APM snippets into
>>> comments.
>>
>> ...but this seems to say "don't do that".
>
> Yeah, that didn't come out right.
>
>> More specific guidance would probably help here.
>
> Is this better?
>
> Do not reference specific sections, tables, figures, etc. by number, especially
> not in comments. Instead, if necessary (see below), copy-paste the relevant
> snippet and reference sections/tables/figures by name. The layouts of the SDM
> and APM are constantly changing, and so the numbers/labels aren't stable.
>
> Generally speaking, do not explicitly reference or copy-paste from the SDM or
> APM in comments. With few exceptions, KVM *must* honor architectural behavior,
> therefore it's implied that KVM behavior is emulating SDM and/or APM behavior.
> Note, referencing the SDM/APM in changelogs to justify the change and provide
> context is perfectly ok and encouraged.
Yes, I think the new wording conveys the underlying idea better, thanks.
>>> +Testing
>>> +-------
>>> +At a bare minimum, *all* patches in a series must build cleanly for KVM_INTEL=m
>>> +KVM_AMD=m, and KVM_WERROR=y. Building every possible combination of Kconfigs
>>> +isn't feasible, but the more the merrier. KVM_SMM, KVM_XEN, PROVE_LOCKING, and
>>> +X86_64 are particularly interesting knobs to turn.
>>> +
>>> +Running KVM selftests and KVM-unit-tests is also mandatory (and stating the
>>> +obvious, the tests need to pass).
>>
>> I would add an exception here from mandatory testing for changes that
>> obviously have negligible probability of affecting runtime behavior.
>>
>> For example: patches that modify just code comments or documentation.
>
> Agreed, will add.
>
> Regarding documentation, I think I'll also add a requirement of 'make htmldocs'
> without warnings for non-trivial docs changes. It's all too easy to write buggy
> ReST "code" that looks correct as raw text, e.g. the whole double-colon thing.
Good idea to mention that, I totally forgot that ReST docs need "compiling", too.
>>> When possible and relevant, testing on both
>>> +Intel and AMD is strongly preferred. Booting an actual VM is encouraged, but
>>> +not mandatory.
>>> +
>>> +For changes that touch KVM's shadow paging code, running with TDP (EPT/NPT)
>>> +disabled is mandatory. For changes that affect common KVM MMU code, running
>>> +with TDP disabled is strongly encouraged. For all other changes, if the code
>>> +being modified depends on and/or interacts with a module param, testing with
>>> +the relevant settings is mandatory.
>>> +
>>> +Note, KVM selftests and KVM-unit-tests do have known failures. If you suspect
>>> +a failure is not due to your changes, verify that the *exact same* failure
>>> +occurs with and without your changes.
>>> +
>>> +If you can't fully test a change, e.g. due to lack of hardware, clearly state
>>> +what level of testing you were able to do, e.g. in the cover letter.
>>> +
>> (...)
>>
>> Thanks for preparing such a detailed handbook Sean.
>>
>> However, having so many rules might seem intimidating for newcomers, and
>> deter them from contributing out of fear that they'll be screamed at for
>> accidentally breaking some obscure rule.
>>
>> That's especially true for unpaid volunteers that might become
>> professional kernel developers one day if their learning curve isn't
>> made too steep.
>>
>> Maybe have a paragraph or two that, despite all these rules, KVM x86
>> strives to be a welcome community, encouraging newcomers and understanding
>> their beginner mistakes (or so)?
>
> I like that idea a lot, I'll add a section at the very top.
>
> Thanks much!
Thanks,
Maciej
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-17 22:54 ` [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86 Sean Christopherson
` (5 preceding siblings ...)
2023-02-22 19:26 ` Maciej S. Szmigiero
@ 2023-02-28 14:45 ` Robert Hoo
2023-03-07 17:53 ` Sean Christopherson
6 siblings, 1 reply; 19+ messages in thread
From: Robert Hoo @ 2023-02-28 14:45 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
Sagi Shahar, Erdem Aktas, Peter Shier, Anish Ghulati,
Oliver Upton, James Houghton, Anish Moorthy, Ben Gardon,
David Matlack, Ricardo Koller, Axel Rasmussen, Aaron Lewis,
Ashish Kalra, Babu Moger, Chao Gao, Chao Peng, Chenyi Qiang,
David Woodhouse, Emanuele Giuseppe Esposito, Gavin Shan,
Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson, Jing Liu,
Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu, Li RongQing,
Maciej S . Szmigiero, Maxim Levitsky, Michael Roth,
Michal Luczaj, Mingwei Zhang, Nikunj A Dadhania, Paul Durrant,
Peng Hao, Peter Gonda, Peter Xu, Suravee Suthikulpanit,
Tom Lendacky, Vipin Sharma, Vitaly Kuznetsov, Wanpeng Li,
Wei Wang, Xiaoyao Li, Yu Zhang, Zhenzhong Duan, kvm,
linux-kernel
On Fri, 2023-02-17 at 14:54 -0800, Sean Christopherson wrote:
> +Branches
> +~~~~~~~~
> +The KVM x86 tree is organized into multiple topic branches. The
> purpose of
> +using finer-grained topic branches is to make it easier to keep tabs
> on an area
> +of development, and to limit the collateral damage of human errors
> and/or buggy
> +commits, e.g. dropping the HEAD commit of a topic branch has no
> impact on other
> +in-flight commits' SHA1 hashes, and having to reject a pull request
> due to bugs
> +delays only that topic branch.
> +
> +All topic branches, except for ``next`` and ``fixes``
What's this "fixes" branch for?
If fixes for current cycle, will apply to main KVM tree; if fixes for
next cycle, why not directly to its topic branch or next branch (kvm-
x86 tree)?
I see in main KVM tree, its "fixes" branch is very inactive.
Too many functional branches will add your maintenance burden.
> , are rolled into ``next``
> +via a cthulu merge on an as-needed basis, i.e. when a topic branch
> is updated.
> +As a result, force pushes to ``next`` are common.
> +
> +Lifecycle
> +~~~~~~~~~
> +Pull requests for the next release cycle are sent to the main KVM
> tree, one
> +for each KVM x86 topic branch.
Will each KVM x86 topic branch has a mapping topic branch in main KVM
tree? I mean where is their pull target(s), next branch in main KVM
tree? or their counterpart branches in main KVM tree?
> If all goes well, the topic branches are rolled
> +into the main KVM pull request sent during Linus' merge
> window. Pull requests
> +for KVM x86 branches are typically made the week before Linus'
> opening of the
> +merge window, e.g. the week following rc7 for "normal" releases.
> +
> +The KVM x86 tree doesn't have its own official merge window, but
> there's a soft
> +close around rc5 for new features, and a soft close around rc6 for
> fixes.
> +
>
> +Comments
> +~~~~~~~~
> +Write comments using imperative mood and avoid pronouns. Use
> comments to
> +provide a high level overview of the code, and/or to explain why the
> code does
> +what it does. Do not reiterate what the code literally does; let
> the code
> +speak for itself. If the code itself is inscrutable, comments will
> not help.
Welcome comments that state preconditions for calling this function?
e.g. some lock held.
> +
> +SDM and APM References
> +~~~~~~~~~~~~~~~~~~~~~~
> +Much of KVM's code base is directly tied to architectural behavior
> defined in
> +Intel's Software Development Manual (SDM) and AMD's Architecture
> Programmer’s
> +Manual (APM). Use of "Intel's SDM" and "AMD's APM", or even just
> "SDM" or
> +"APM", without additional context is a-ok.
> +
> +Do not reference specific sections, tables, figures, etc. by number,
> especially
> +not in comments. Instead, copy-paste the relevant snippet (if
> warranted), and
> +reference sections/tables/figures by name. The layouts of the SDM
> and APM are
> +constantly changing, and so the numbers/labels aren't
> stable/consistent.
Ack
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86
2023-02-28 14:45 ` Robert Hoo
@ 2023-03-07 17:53 ` Sean Christopherson
0 siblings, 0 replies; 19+ messages in thread
From: Sean Christopherson @ 2023-03-07 17:53 UTC (permalink / raw)
To: Robert Hoo
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Sagi Shahar, Erdem Aktas, Peter Shier,
Anish Ghulati, Oliver Upton, James Houghton, Anish Moorthy,
Ben Gardon, David Matlack, Ricardo Koller, Axel Rasmussen,
Aaron Lewis, Ashish Kalra, Babu Moger, Chao Gao, Chao Peng,
Chenyi Qiang, David Woodhouse, Emanuele Giuseppe Esposito,
Gavin Shan, Guang Zeng, Hou Wenlong, Jiaxi Chen, Jim Mattson,
Jing Liu, Junaid Shahid, Kai Huang, Leonardo Bras, Like Xu,
Li RongQing, Maciej S . Szmigiero, Maxim Levitsky, Michael Roth,
Michal Luczaj, Mingwei Zhang, Nikunj A Dadhania, Paul Durrant,
Peng Hao, Peter Gonda, Peter Xu, Suravee Suthikulpanit,
Tom Lendacky, Vipin Sharma, Vitaly Kuznetsov, Wanpeng Li,
Wei Wang, Xiaoyao Li, Yu Zhang, Zhenzhong Duan, kvm,
linux-kernel
On Tue, Feb 28, 2023, Robert Hoo wrote:
> On Fri, 2023-02-17 at 14:54 -0800, Sean Christopherson wrote:
> > +Branches
> > +~~~~~~~~
> > +The KVM x86 tree is organized into multiple topic branches. The
> > purpose of
> > +using finer-grained topic branches is to make it easier to keep tabs
> > on an area
> > +of development, and to limit the collateral damage of human errors
> > and/or buggy
> > +commits, e.g. dropping the HEAD commit of a topic branch has no
> > impact on other
> > +in-flight commits' SHA1 hashes, and having to reject a pull request
> > due to bugs
> > +delays only that topic branch.
> > +
> > +All topic branches, except for ``next`` and ``fixes``
>
> What's this "fixes" branch for?
> If fixes for current cycle, will apply to main KVM tree; if fixes for
> next cycle, why not directly to its topic branch or next branch (kvm-
> x86 tree)?
kvm-x86/fixes is a placeholder for carrying fixes for the current cycle. I
deliberately hedged in the previous section by saying "Generally speaking". I.e.
the vast majority of fixes will be applied to the main tree, but there may be
situations where I gather fixes in kvm-x86 and send a pull request to Paolo, e.g.
that may be easier if Paolo is on holiday for an extended period.
: Generally speaking, fixes for the current cycle are applied directly to the
: main KVM tree, while all development for the next cycle is routed through the
: KVM x86 tree.
I'll add a blurb to the above paragraph to clarify that fixes _may_ be carried in
kvm-x86/fixes.
> I see in main KVM tree, its "fixes" branch is very inactive.
AFAIK, Paolo doesn't use it at all.
> Too many functional branches will add your maintenance burden.
There's definitely a point where more branches would be a bad thing, but I don't
think having a "fixes" branch moves the needle on that front.
> > , are rolled into ``next``
> > +via a cthulu merge on an as-needed basis, i.e. when a topic branch
> > is updated.
> > +As a result, force pushes to ``next`` are common.
> > +
> > +Lifecycle
> > +~~~~~~~~~
> > +Pull requests for the next release cycle are sent to the main KVM
> > tree, one
> > +for each KVM x86 topic branch.
>
> Will each KVM x86 topic branch has a mapping topic branch in main KVM
> tree? I mean where is their pull target(s), next branch in main KVM
> tree? or their counterpart branches in main KVM tree?
Barring some esoteric edge case, the target will be kvm/next or kvm/queue. Merging
to a topic branch and then merging again would add no value. I'd prefer not to add
anything to clarify the target because (a) it's technically outside the scope of KVM
x86 and (b) it simply doesn't matter as far as KVM x86 is concerned. E.g. if Paolo
wants/needs to merge a topic branch somewhere else for whatever reason, that doesn't
impact the KVM x86 lifecycle in any way.
> > If all goes well, the topic branches are rolled
> > +into the main KVM pull request sent during Linus' merge
> > window. Pull requests
> > +for KVM x86 branches are typically made the week before Linus'
> > opening of the
> > +merge window, e.g. the week following rc7 for "normal" releases.
> > +
> > +The KVM x86 tree doesn't have its own official merge window, but
> > there's a soft
> > +close around rc5 for new features, and a soft close around rc6 for
> > fixes.
> > +
> >
> > +Comments
> > +~~~~~~~~
> > +Write comments using imperative mood and avoid pronouns. Use
> > comments to
> > +provide a high level overview of the code, and/or to explain why the
> > code does
> > +what it does. Do not reiterate what the code literally does; let
> > the code
> > +speak for itself. If the code itself is inscrutable, comments will
> > not help.
>
> Welcome comments that state preconditions for calling this function?
> e.g. some lock held.
Hard "no". Any non-obvious preconditions should be conveyed through lockdep
assertions, WARNs, etc...
^ permalink raw reply [flat|nested] 19+ messages in thread