Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for KVM x86

[Maciej S. Szmigiero]

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