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
