Messages in this thread | | | Date | Wed, 22 Feb 2023 23:09:44 +0100 | From | "Maciej S. Szmigiero" <> | Subject | Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86 |
| |
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
| |