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!
