Dcumentation for contributing to KVM x86, round 2!
v2:
- Fix a KVM_GET_SUPPORTED_CPUID typo. [Yuan]
- Give Cthulhu the respect it deserves. [Like]
- Explicitly state that selftests vs. KVM patches need to maintain
bisection (when possible). [Like]
- Change the recommended base from topic branches to "next" (and plan on
providing stable, persistent git objects via tags). [David W]
- Add a blurb to provide guidance for series that touch non-x86 arch
code. [Yu]
- Clarify when (not) to reference/quote specs (APM and SDM). [Maciej]
- Add preferences for shortlog length. [Maciej]
- Exempt things like comment fixes from testing requirements. [Maciej]
- Add a foreword to try and make the doc less scary to newcomers. [Maciej]
- Add a rule for testing Documentation/ changes.
- Clarify that fixes for the current cycle may be carried in the KVM x86
tree, but are usually taken directly by Paolo. [Robert]
- Tweak the "Changelog" section to call out that using imperative mood
and avoiding pronouns is the most important rule.
v1: https://lore.kernel.org/all/[email protected]
Sean Christopherson (2):
Documentation/process: Add a label for the tip tree handbook's coding
style
Documentation/process: Add a maintainer handbook for KVM x86
.../process/maintainer-handbooks.rst | 1 +
Documentation/process/maintainer-kvm-x86.rst | 391 ++++++++++++++++++
Documentation/process/maintainer-tip.rst | 2 +
MAINTAINERS | 1 +
4 files changed, 395 insertions(+)
create mode 100644 Documentation/process/maintainer-kvm-x86.rst
base-commit: 3ac88fa4605ec98e545fb3ad0154f575fda2de5f
--
2.40.0.rc1.284.g88254d51c5-goog
Add a label for the tip tree's "Coding style notes" so that a forthcoming
KVM x86 handbook can reference/piggyback the tip tree's preferred coding
style.
Signed-off-by: Sean Christopherson <[email protected]>
---
Documentation/process/maintainer-tip.rst | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/process/maintainer-tip.rst b/Documentation/process/maintainer-tip.rst
index 572a3289c9cb..ad0540d6882b 100644
--- a/Documentation/process/maintainer-tip.rst
+++ b/Documentation/process/maintainer-tip.rst
@@ -452,6 +452,8 @@ and can be added to an existing kernel config by running:
Some of these options are x86-specific and can be left out when testing
on other architectures.
+.. _maintainer-tip-coding-style:
+
Coding style notes
------------------
--
2.40.0.rc1.284.g88254d51c5-goog
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 <[email protected]>
---
.../process/maintainer-handbooks.rst | 1 +
Documentation/process/maintainer-kvm-x86.rst | 391 ++++++++++++++++++
MAINTAINERS | 1 +
3 files changed, 393 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..309298fb4822
--- /dev/null
+++ b/Documentation/process/maintainer-kvm-x86.rst
@@ -0,0 +1,391 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+KVM x86
+=======
+
+Foreword
+--------
+KVM strives to be a welcoming community; contributions from newcomers are
+valued and encouraged. Please do not be discouraged or intimidated by the
+length of this document and the many rules/guidelines it contains. Everyone
+makes mistakes, and everyone was a newbie at some point. So long as you make
+an honest effort to follow KVM x86's guidelines, are receptive to feedback,
+and learn from any mistakes you make, you will be welcomed with open arms, not
+torches and pitchforks.
+
+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. In the unlikely event that a fix for the current cycle is routed
+through the KVM x86 tree, it will be applied to the ``fixes`` branch before
+making its way to the main KVM 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
+~~~~~~~~~
+Fixes that target the current release, a.k.a. mainline, are typically applied
+directly to the main KVM tree, i.e. do not route through the KVM x86 tree.
+
+Changes that target the next release are routed through the KVM x86 tree. Pull
+requests (from KVM x86 to main KVM) are sent for each KVM x86 topic branch,
+typically the week before Linus' opening of the merge window, e.g. the week
+following rc7 for "normal" releases. If all goes well, the topic branches are
+rolled into the main KVM pull request sent during Linus' merge window.
+
+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 (for
+the next release; see above for fixes that target the current release).
+
+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 the current release, a.k.a. mainline, should be based on
+``git://git.kernel.org/pub/scm/virt/kvm/kvm.git master``. Note, fixes do not
+automatically warrant inclusion in the current release. There is no singular
+rule, but typically only fixes for bugs that are urgent, critical, and/or were
+introduced in the current release should target the current release.
+
+Everything else should be based on ``kvm-x86/next``, i.e. there is no need to
+select a specific topic branch as the base. If there are conflicts and/or
+dependencies across topic branches, it is the maintainer's job to sort them
+out.
+
+As a general guideline, use ``kvm-x86/next`` even if a patch/series touches
+multiple architectures, i.e. isn't strictly scoped to x86. Using any of the
+branches from the main KVM tree is usually a less good option as they likely
+won't have many, if any, changes for the next release, i.e. using the main KVM
+tree as a base is more likely to yield conflicts. And if there are non-trivial
+conflicts with multiple architectures, coordination between maintainers will be
+required no matter what base is used. Note, this is far from a hard rule, i.e.
+use a different base for multi-arch series if that makes the most sense.
+
+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, 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.
+
+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. Do not use file names or complete file
+paths as the subject/shortlog prefix.
+
+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 dereference in function_xyz()
+
+not::
+
+ kvm: x86: fix a null pointer dereference 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.
+
+See :ref:`the_canonical_patch_format` for more information, with one amendment:
+do not treat the 70-75 character limit as an absolute, hard limit. Instead,
+use 75 characters as a firm-but-not-hard limit, and use 80 characters as a hard
+limit. I.e. let the shortlog run a few characters over the standard limit if
+you have good reason to do so.
+
+Changelog
+~~~~~~~~~
+Most importantly, write changelogs using imperative mood and avoid pronouns.
+
+See :ref:`describe_changes` for more information, with one amendment: lead with
+a short blurb on the actual changes, and then follow up with the context and
+background. Note! This order directly conflicts with the tip tree's preferred
+approach! Please follow the tip tree's preferred style when sending patches
+that primarily target arch/x86 code that is _NOT_ KVM code.
+
+Stating what a patch does before diving into details is preferred by KVM x86
+for several reasons. First and foremost, what code is actually being changed
+is arguably 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: [email protected]" (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). The only exception is for changes that have
+negligible probability of affecting runtime behavior, e.g. patches that only
+modify comments. 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.
+
+Changes that touch reStructured Text documentation, i.e. .rst files, must build
+htmldocs cleanly, i.e. with no new warnings or errors.
+
+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_GET_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. The
+standard kernel rules for bisection apply, i.e. KVM changes that result in test
+failures should be ordered after the selftests updates, and vice versa, new
+tests that fail due to KVM bugs should be ordered after the KVM fixes.
+
+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 <[email protected]>
M: Paolo Bonzini <[email protected]>
L: [email protected]
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.40.0.rc1.284.g88254d51c5-goog
On Wed, Mar 08, 2023 at 05:03:36PM -0800, Sean Christopherson wrote:
> +As a general guideline, use ``kvm-x86/next`` even if a patch/series touches
> +multiple architectures, i.e. isn't strictly scoped to x86. Using any of the
> +branches from the main KVM tree is usually a less good option as they likely
> +won't have many, if any, changes for the next release, i.e. using the main KVM
> +tree as a base is more likely to yield conflicts. And if there are non-trivial
> +conflicts with multiple architectures, coordination between maintainers will be
> +required no matter what base is used. Note, this is far from a hard rule, i.e.
> +use a different base for multi-arch series if that makes the most sense.
That means patches that primarily kvm ARM changes should be based on
kvm-x86/next, right?
> +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.
What do you mean by conceptual tree? Is it Patch subject prefix?
> +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. The
> +standard kernel rules for bisection apply, i.e. KVM changes that result in test
> +failures should be ordered after the selftests updates, and vice versa, new
> +tests that fail due to KVM bugs should be ordered after the KVM fixes.
Did you mean that in a patch series, selftest patches are placed after
their corresponding KVM changes?
Thanks.
--
An old man doll... just what I always wanted! - Clara
On Thu, Mar 09, 2023 at 09:37:45AM +0700, Bagas Sanjaya wrote:
> On Wed, Mar 08, 2023 at 05:03:36PM -0800, Sean Christopherson wrote:
> > +As a general guideline, use ``kvm-x86/next`` even if a patch/series touches
> > +multiple architectures, i.e. isn't strictly scoped to x86. Using any of the
> > +branches from the main KVM tree is usually a less good option as they likely
> > +won't have many, if any, changes for the next release, i.e. using the main KVM
> > +tree as a base is more likely to yield conflicts. And if there are non-trivial
> > +conflicts with multiple architectures, coordination between maintainers will be
> > +required no matter what base is used. Note, this is far from a hard rule, i.e.
> > +use a different base for multi-arch series if that makes the most sense.
I don't think this is the best way to coordinate with other architectures.
Regardless of whether you intended this to be prescriptive, I'm worried most
folks will follow along and just base patches on kvm-x86/next anyway.
It'd be easier to just have multi-arch series use a stable base (i.e. a
release candidate) by default. That'd avoid the undesirable case where merging
a shared branch brings with it some random point in another arch's /next
history.
If a different approach makes sense for a particular series then we can
discuss it on the list and arrive at something agreeable for all parties
involved.
> That means patches that primarily kvm ARM changes should be based on
> kvm-x86/next, right?
No, don't do that.
Patches aimed at KVM/arm64 should be based on a sensible release candidate.
We tend to contstruct the kvmarm/next with an early-ish release candidate
(rc2-rc4). For example the 6.3 pull request was based on 6.2-rc4. We use topic
branches in a slightly different manner than x86, creating ad-hoc branches for
individual patch series grabbed from the list.
If one has a series that conflicts with/depends on another that is in-flight
or already picked up then that should be mentioned in the cover letter.
Ultimately its up to the maintainer(s) to address conflicts, and neither
Marc nor I are afraid to ask for a rebase/respin if it makes our lives
easier to glue it all together.
--
Thanks,
Oliver
On Thu, Mar 09, 2023, Oliver Upton wrote:
> On Thu, Mar 09, 2023 at 09:37:45AM +0700, Bagas Sanjaya wrote:
> > On Wed, Mar 08, 2023 at 05:03:36PM -0800, Sean Christopherson wrote:
> > > +As a general guideline, use ``kvm-x86/next`` even if a patch/series touches
> > > +multiple architectures, i.e. isn't strictly scoped to x86. Using any of the
> > > +branches from the main KVM tree is usually a less good option as they likely
> > > +won't have many, if any, changes for the next release, i.e. using the main KVM
> > > +tree as a base is more likely to yield conflicts. And if there are non-trivial
> > > +conflicts with multiple architectures, coordination between maintainers will be
> > > +required no matter what base is used. Note, this is far from a hard rule, i.e.
> > > +use a different base for multi-arch series if that makes the most sense.
>
> I don't think this is the best way to coordinate with other architectures.
> Regardless of whether you intended this to be prescriptive, I'm worried most
> folks will follow along and just base patches on kvm-x86/next anyway.
Probably, but for the target audience (KVM x86 contributors), that's likely the
least awful base 99% of the time.
> It'd be easier to just have multi-arch series use a stable base (i.e. a
> release candidate) by default. That'd avoid the undesirable case where merging
> a shared branch brings with it some random point in another arch's /next
> history.
You're conflating the base of the patch series with the branch it is applied to.
I'm most definitely not proposing that multi-arch series from x86 contributors
always be routed through kvm-x86. It's ultimately the responsibility of the
maintainers, not the contributors, to avoid funky merges and histories. If a
series warrants a dedicated topic branch, then we need to create said topic branch
off a stable, common base, irrespective of what the contributor based their patches
on.
If a series from an x86 contributor applies cleanly on kvm-x86/next but not on
-rc2 (or whatever), then the reverse would also likely be true (if the contributor
used -rc2 as the base). In other words, for series with non-trivial modifications
to other architectures and/or common KVM code, IMO the base used for the _initial_
posting doesn't matter all that much for us maintainers since such series will
likely require additional attention no matter what base is used.
On the flip side, the vast majority of "multi-arch" series in KVM tend to be focused
on a single architecture, with only incidental contact to other architectures and/or
common KVM code. Those types of series will likely be routed through their "target"
arch tree, and so for x86, using kvm-x86/next as the base is preferrable.
My goal with suggesting/prescribing kvm-x86/next to contributors is to make the
easy things easy. On my end, that means having _predictable_ submissions and
minimizing the number of avoidable conflicts. For contributors, that means having
a very simple rule/guideline. "Use kvm-x86/next unless you know better" satisfies
all those conditions.
> If a different approach makes sense for a particular series then we can
> discuss it on the list and arrive at something agreeable for all parties
> involved.
>
> > That means patches that primarily kvm ARM changes should be based on
> > kvm-x86/next, right?
>
> No, don't do that.
+<infinity symbol>
This doc is specifically for KVM x86.
On Thu, Mar 09, 2023, Bagas Sanjaya wrote:
> On Wed, Mar 08, 2023 at 05:03:36PM -0800, Sean Christopherson wrote:
> > +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.
>
> What do you mean by conceptual tree? Is it Patch subject prefix?
Not really? I'm struggling to describe it in words. What I mean is something
like this
x86
/ | \
VMX pmu SVM
/ \
nVMX nSVM
e.g. if a patch touches VMX and SVM, use "x86". But it's a bit more complex than
that, e.g. a VMX-focused patch that just happens to touch Intel-specific PMU
code should probably be tagged "VMX", not "x86" or "pmu".
Massaging subjects when applying is easy enough, and there will always be some
amount of subjectivity, so unless this is outright confusing (or someone has a
better, succinct, and easy-to-understand description), I'll probably just leave
this section as-is.
> > +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. The
> > +standard kernel rules for bisection apply, i.e. KVM changes that result in test
> > +failures should be ordered after the selftests updates, and vice versa, new
> > +tests that fail due to KVM bugs should be ordered after the KVM fixes.
>
> Did you mean that in a patch series, selftest patches are placed after
> their corresponding KVM changes?
No, I meant what I wrote. If a KVM change will break a test (uncommon, but it
does happen), then the selftest should be patched first so that there are no
unnecessary test failures in the git history. But when adding a regression test,
the KVM bug fix should come first so that again, there are no unnecessary failures
if someone tests an arbitrary commit, e.g. during bisection.
On Wed, 2023-03-08 at 17:03 -0800, Sean Christopherson wrote:
(...)
> +Using reverse fir tree for variable declarations isn't strictly
> required,
> +though it is still preferred.
> +
I googled, but didn't find what's "fir tree". Shed more light?
I saw Maciej also asked.
https://lore.kernel.org/kvm/[email protected]/
On Fri, Mar 10, 2023, Robert Hoo wrote:
> On Wed, 2023-03-08 at 17:03 -0800, Sean Christopherson wrote:
> (...)
> > +Using reverse fir tree for variable declarations isn't strictly
> > required,
> > +though it is still preferred.
> > +
> I googled, but didn't find what's "fir tree". Shed more light?
Y'all need to improve your Google-fu ;-)
Since many people seem to be surprised by this one, I'll reword this section to:
Using reverse fir tree, a.k.a. reverse Christmas tree or reverse XMAS tree, for
variable declarations isn't strictly required, though it is still preferred.
On Thu, Mar 09, 2023 at 09:25:54AM -0800, Sean Christopherson wrote:
> On Thu, Mar 09, 2023, Oliver Upton wrote:
> > On Thu, Mar 09, 2023 at 09:37:45AM +0700, Bagas Sanjaya wrote:
> > > On Wed, Mar 08, 2023 at 05:03:36PM -0800, Sean Christopherson wrote:
> > > > +As a general guideline, use ``kvm-x86/next`` even if a patch/series touches
> > > > +multiple architectures, i.e. isn't strictly scoped to x86. Using any of the
> > > > +branches from the main KVM tree is usually a less good option as they likely
> > > > +won't have many, if any, changes for the next release, i.e. using the main KVM
> > > > +tree as a base is more likely to yield conflicts. And if there are non-trivial
> > > > +conflicts with multiple architectures, coordination between maintainers will be
> > > > +required no matter what base is used. Note, this is far from a hard rule, i.e.
> > > > +use a different base for multi-arch series if that makes the most sense.
> >
> > I don't think this is the best way to coordinate with other architectures.
> > Regardless of whether you intended this to be prescriptive, I'm worried most
> > folks will follow along and just base patches on kvm-x86/next anyway.
>
> Probably, but for the target audience (KVM x86 contributors), that's likely the
> least awful base 99% of the time.
Sorry, I follow this reasoning at all.
If folks are aiming to make a multi-arch contribution then the architecture
they regularly contribute to has absolutely zero relevance on the series
itself.
> > It'd be easier to just have multi-arch series use a stable base (i.e. a
> > release candidate) by default. That'd avoid the undesirable case where merging
> > a shared branch brings with it some random point in another arch's /next
> > history.
>
> You're conflating the base of the patch series with the branch it is applied to.
We cannot pretend the two are in no way related. The dependencies of a series
are not obvious when based on the /next branch of any one architecture.
> I'm most definitely not proposing that multi-arch series from x86 contributors
> always be routed through kvm-x86. It's ultimately the responsibility of the
> maintainers, not the contributors, to avoid funky merges and histories.
Right, but contributors looking to make changes across architectures share
some of the burden of cross-arch coordination as well. Basing patches off of
a random commit not in Linus' tree does not match at least the arm64
workflow.
> If a
> series warrants a dedicated topic branch, then we need to create said topic branch
> off a stable, common base, irrespective of what the contributor based their patches
> on.
The lowest friction way to coordinate such things is to start off with a
common base and go from there. If there is a compelling argument for doing
things differently in the context of one series then let's talk about it on
the list.
> If a series from an x86 contributor applies cleanly on kvm-x86/next but not on
> -rc2 (or whatever), then the reverse would also likely be true (if the contributor
> used -rc2 as the base).
This can be addressed in a merge resolution, thereby offloading the
responsibility to the maintainer.
> In other words, for series with non-trivial modifications
> to other architectures and/or common KVM code, IMO the base used for the _initial_
> posting doesn't matter all that much for us maintainers since such series will
> likely require additional attention no matter what base is used.
In all likelihood, sure the series will be respun. But, you're offloading the
responsibility to ask for a sane base on other arch maintainers which I'm not
cool with.
> On the flip side, the vast majority of "multi-arch" series in KVM tend to be focused
> on a single architecture, with only incidental contact to other architectures and/or
> common KVM code. Those types of series will likely be routed through their "target"
> arch tree, and so for x86, using kvm-x86/next as the base is preferrable.
With long term aspirations to share more code between architectures (e.g.
common MMU) I believe we'll see more changes that have meaningful interaction
with all architecutures.
> My goal with suggesting/prescribing kvm-x86/next to contributors is to make the
> easy things easy. On my end, that means having _predictable_ submissions and
> minimizing the number of avoidable conflicts. For contributors, that means having
> a very simple rule/guideline. "Use kvm-x86/next unless you know better" satisfies
> all those conditions.
I believe "Use a release candidate unless you know better" for multi-arch
changes is just as simple. Better yet, it clues in contributors as to how
changes are coordinated across architectures and might help them know better
next time around.
> > If a different approach makes sense for a particular series then we can
> > discuss it on the list and arrive at something agreeable for all parties
> > involved.
> >
> > > That means patches that primarily kvm ARM changes should be based on
> > > kvm-x86/next, right?
> >
> > No, don't do that.
>
> +<infinity symbol>
>
> This doc is specifically for KVM x86.
You've also made some suggestions about cross-arch development that do not fit
the development model of other architectures. I have no desire to nitpick
about the x86 process but want the multiarch language to actually set folks up
for success working outside of the KVM/x86 tree.
--
Thanks,
Oliver
On Mon, Mar 13, 2023 at 05:32:29PM +0000, Oliver Upton wrote:
> On Thu, Mar 09, 2023 at 09:25:54AM -0800, Sean Christopherson wrote:
> > On Thu, Mar 09, 2023, Oliver Upton wrote:
> > > On Thu, Mar 09, 2023 at 09:37:45AM +0700, Bagas Sanjaya wrote:
> > > > On Wed, Mar 08, 2023 at 05:03:36PM -0800, Sean Christopherson wrote:
> > > > > +As a general guideline, use ``kvm-x86/next`` even if a patch/series touches
> > > > > +multiple architectures, i.e. isn't strictly scoped to x86. Using any of the
> > > > > +branches from the main KVM tree is usually a less good option as they likely
> > > > > +won't have many, if any, changes for the next release, i.e. using the main KVM
> > > > > +tree as a base is more likely to yield conflicts. And if there are non-trivial
> > > > > +conflicts with multiple architectures, coordination between maintainers will be
> > > > > +required no matter what base is used. Note, this is far from a hard rule, i.e.
> > > > > +use a different base for multi-arch series if that makes the most sense.
> > >
> > > I don't think this is the best way to coordinate with other architectures.
> > > Regardless of whether you intended this to be prescriptive, I'm worried most
> > > folks will follow along and just base patches on kvm-x86/next anyway.
> >
> > Probably, but for the target audience (KVM x86 contributors), that's likely the
> > least awful base 99% of the time.
>
> Sorry, I follow this reasoning at all.
I *don't* follow ...
--
Thanks,
Oliver
On Mon, Mar 13, 2023, Oliver Upton wrote:
> On Thu, Mar 09, 2023 at 09:25:54AM -0800, Sean Christopherson wrote:
> > On Thu, Mar 09, 2023, Oliver Upton wrote:
> > > On Thu, Mar 09, 2023 at 09:37:45AM +0700, Bagas Sanjaya wrote:
> > > > On Wed, Mar 08, 2023 at 05:03:36PM -0800, Sean Christopherson wrote:
> > > > > +As a general guideline, use ``kvm-x86/next`` even if a patch/series touches
> > > > > +multiple architectures, i.e. isn't strictly scoped to x86. Using any of the
> > > > > +branches from the main KVM tree is usually a less good option as they likely
> > > > > +won't have many, if any, changes for the next release, i.e. using the main KVM
> > > > > +tree as a base is more likely to yield conflicts. And if there are non-trivial
> > > > > +conflicts with multiple architectures, coordination between maintainers will be
> > > > > +required no matter what base is used. Note, this is far from a hard rule, i.e.
> > > > > +use a different base for multi-arch series if that makes the most sense.
> > >
> > > I don't think this is the best way to coordinate with other architectures.
> > > Regardless of whether you intended this to be prescriptive, I'm worried most
> > > folks will follow along and just base patches on kvm-x86/next anyway.
> >
> > Probably, but for the target audience (KVM x86 contributors), that's likely the
> > least awful base 99% of the time.
>
> Sorry, I follow this reasoning at all.
>
> If folks are aiming to make a multi-arch contribution then the architecture
> they regularly contribute to has absolutely zero relevance on the series
> itself.
There's disconnect between what my brain is thinking and what I wrote.
The intent of the "use kvm-x86/next" guideline is aimed to address series that
are almost entirely x86 specific, and only superficially touch common KVM and/or
other architectures. In my experience, the vast, vast majority of "multi-arch"
contributions from x86 fall into this category, i.e. aren't truly multi-arch in
nature.
If I replace the above paragraph with this, does that address (or at least mitigate
to an acceptable level) your concerns? Inevitably there will still be series that
are wrongly based on kvm-x86, but I am more than happy to do the policing. I
obviously can't guarantee that I will be the first to run afoul of a "bad" series,
but I do think I can be quick enough to avoid shifting the burden to other
maintainers. And if I'm wrong on either front, you get to say "told you so" and
make me submit a patch of shame ;-)
The only exception to using ``kvm-x86/next`` as the base is if a patch/series
is a multi-arch series, i.e. has non-trivial modifications to common KVM code
and/or has more than superficial changes to other architectures's code. Multi-
arch patch/series should instead be based on a common, stable point in KVM's
history, e.g. the release candidate upon which ``kvm-x86 next`` is based. If
you're unsure whether a patch/series is truly multi-arch, err on the side of
caution and treat it as multi-arch, i.e. use a common base.
> > > > That means patches that primarily kvm ARM changes should be based on
> > > > kvm-x86/next, right?
> > >
> > > No, don't do that.
> >
> > +<infinity symbol>
> >
> > This doc is specifically for KVM x86.
>
> You've also made some suggestions about cross-arch development that do not fit
> the development model of other architectures. I have no desire to nitpick
> about the x86 process but want the multiarch language to actually set folks up
> for success working outside of the KVM/x86 tree.
Ah, I see where y'all are coming from. Yeah, I didn't intend for that type of
blanket rule, e.g. my comment about this being specifically for KVM x86 was
intended to clarify that this doc should NOT be used to determine how to handle
non-x86 code.
On Mon, Mar 13, 2023 at 11:20:45AM -0700, Sean Christopherson wrote:
> On Mon, Mar 13, 2023, Oliver Upton wrote:
> > On Thu, Mar 09, 2023 at 09:25:54AM -0800, Sean Christopherson wrote:
> > > On Thu, Mar 09, 2023, Oliver Upton wrote:
> > > > On Thu, Mar 09, 2023 at 09:37:45AM +0700, Bagas Sanjaya wrote:
> > > > > On Wed, Mar 08, 2023 at 05:03:36PM -0800, Sean Christopherson wrote:
> > > > > > +As a general guideline, use ``kvm-x86/next`` even if a patch/series touches
> > > > > > +multiple architectures, i.e. isn't strictly scoped to x86. Using any of the
> > > > > > +branches from the main KVM tree is usually a less good option as they likely
> > > > > > +won't have many, if any, changes for the next release, i.e. using the main KVM
> > > > > > +tree as a base is more likely to yield conflicts. And if there are non-trivial
> > > > > > +conflicts with multiple architectures, coordination between maintainers will be
> > > > > > +required no matter what base is used. Note, this is far from a hard rule, i.e.
> > > > > > +use a different base for multi-arch series if that makes the most sense.
> > > >
> > > > I don't think this is the best way to coordinate with other architectures.
> > > > Regardless of whether you intended this to be prescriptive, I'm worried most
> > > > folks will follow along and just base patches on kvm-x86/next anyway.
> > >
> > > Probably, but for the target audience (KVM x86 contributors), that's likely the
> > > least awful base 99% of the time.
> >
> > Sorry, I follow this reasoning at all.
> >
> > If folks are aiming to make a multi-arch contribution then the architecture
> > they regularly contribute to has absolutely zero relevance on the series
> > itself.
>
> There's disconnect between what my brain is thinking and what I wrote.
>
> The intent of the "use kvm-x86/next" guideline is aimed to address series that
> are almost entirely x86 specific, and only superficially touch common KVM and/or
> other architectures. In my experience, the vast, vast majority of "multi-arch"
> contributions from x86 fall into this category, i.e. aren't truly multi-arch in
> nature.
>
> If I replace the above paragraph with this, does that address (or at least mitigate
> to an acceptable level) your concerns? Inevitably there will still be series that
> are wrongly based on kvm-x86, but I am more than happy to do the policing. I
> obviously can't guarantee that I will be the first to run afoul of a "bad" series,
> but I do think I can be quick enough to avoid shifting the burden to other
> maintainers. And if I'm wrong on either front, you get to say "told you so" and
> make me submit a patch of shame ;-)
>
> The only exception to using ``kvm-x86/next`` as the base is if a patch/series
> is a multi-arch series, i.e. has non-trivial modifications to common KVM code
> and/or has more than superficial changes to other architectures's code. Multi-
nit: Maybe 'to another architecture's code', since English is an annoying
language :)
> arch patch/series should instead be based on a common, stable point in KVM's
> history, e.g. the release candidate upon which ``kvm-x86 next`` is based. If
> you're unsure whether a patch/series is truly multi-arch, err on the side of
> caution and treat it as multi-arch, i.e. use a common base.
LGTM, and sorry for whining without getting across the net effect I was hoping
for in the language.
> > > > > That means patches that primarily kvm ARM changes should be based on
> > > > > kvm-x86/next, right?
> > > >
> > > > No, don't do that.
> > >
> > > +<infinity symbol>
> > >
> > > This doc is specifically for KVM x86.
> >
> > You've also made some suggestions about cross-arch development that do not fit
> > the development model of other architectures. I have no desire to nitpick
> > about the x86 process but want the multiarch language to actually set folks up
> > for success working outside of the KVM/x86 tree.
>
> Ah, I see where y'all are coming from. Yeah, I didn't intend for that type of
> blanket rule, e.g. my comment about this being specifically for KVM x86 was
> intended to clarify that this doc should NOT be used to determine how to handle
> non-x86 code.
My biggest worry was that whether intentional or not, folks will probably take
what you've written out of context. Not as though I could completely blame the
developer in that case, as we have no documented process for arm64 at the
moment.
--
Thanks,
Oliver
On Mon, Mar 13, 2023, Oliver Upton wrote:
> On Mon, Mar 13, 2023 at 11:20:45AM -0700, Sean Christopherson wrote:
> > On Mon, Mar 13, 2023, Oliver Upton wrote:
> > > On Thu, Mar 09, 2023 at 09:25:54AM -0800, Sean Christopherson wrote:
> > > > On Thu, Mar 09, 2023, Oliver Upton wrote:
> > > > > On Thu, Mar 09, 2023 at 09:37:45AM +0700, Bagas Sanjaya wrote:
> > > > > > On Wed, Mar 08, 2023 at 05:03:36PM -0800, Sean Christopherson wrote:
> > > > > > > +As a general guideline, use ``kvm-x86/next`` even if a patch/series touches
> > > > > > > +multiple architectures, i.e. isn't strictly scoped to x86. Using any of the
> > > > > > > +branches from the main KVM tree is usually a less good option as they likely
> > > > > > > +won't have many, if any, changes for the next release, i.e. using the main KVM
> > > > > > > +tree as a base is more likely to yield conflicts. And if there are non-trivial
> > > > > > > +conflicts with multiple architectures, coordination between maintainers will be
> > > > > > > +required no matter what base is used. Note, this is far from a hard rule, i.e.
> > > > > > > +use a different base for multi-arch series if that makes the most sense.
> > > > >
> > > > > I don't think this is the best way to coordinate with other architectures.
> > > > > Regardless of whether you intended this to be prescriptive, I'm worried most
> > > > > folks will follow along and just base patches on kvm-x86/next anyway.
> > > >
> > > > Probably, but for the target audience (KVM x86 contributors), that's likely the
> > > > least awful base 99% of the time.
> > >
> > > Sorry, I follow this reasoning at all.
> > >
> > > If folks are aiming to make a multi-arch contribution then the architecture
> > > they regularly contribute to has absolutely zero relevance on the series
> > > itself.
> >
> > There's disconnect between what my brain is thinking and what I wrote.
> >
> > The intent of the "use kvm-x86/next" guideline is aimed to address series that
> > are almost entirely x86 specific, and only superficially touch common KVM and/or
> > other architectures. In my experience, the vast, vast majority of "multi-arch"
> > contributions from x86 fall into this category, i.e. aren't truly multi-arch in
> > nature.
> >
> > If I replace the above paragraph with this, does that address (or at least mitigate
> > to an acceptable level) your concerns? Inevitably there will still be series that
> > are wrongly based on kvm-x86, but I am more than happy to do the policing. I
> > obviously can't guarantee that I will be the first to run afoul of a "bad" series,
> > but I do think I can be quick enough to avoid shifting the burden to other
> > maintainers. And if I'm wrong on either front, you get to say "told you so" and
> > make me submit a patch of shame ;-)
> >
> > The only exception to using ``kvm-x86/next`` as the base is if a patch/series
> > is a multi-arch series, i.e. has non-trivial modifications to common KVM code
> > and/or has more than superficial changes to other architectures's code. Multi-
>
> nit: Maybe 'to another architecture's code', since English is an annoying
> language :)
Gah, was supposed to be just "architectures'". I don't want to limit the wording
to just one other architecture, because then I'll get nitpicked on what to do if
a series touches _two_ architectures.
> > arch patch/series should instead be based on a common, stable point in KVM's
> > history, e.g. the release candidate upon which ``kvm-x86 next`` is based. If
> > you're unsure whether a patch/series is truly multi-arch, err on the side of
> > caution and treat it as multi-arch, i.e. use a common base.
>
> LGTM, and sorry for whining without getting across the net effect I was hoping
> for in the language.
No need to apologize, the whole point of this doc is to try to make everyone's
lives easier, not just my own. But mostly my own :-D