Date:   Wed, 22 Feb 2023 13:25:19 -0800
In-Reply-To: <2dfae61c-6ac7-7686-ebd1-6ad4448b2bf8@maciej.szmigiero.name>
Mime-Version: 1.0
References: <20230217225449.811957-1-seanjc@google.com> <20230217225449.811957-3-seanjc@google.com>
 <2dfae61c-6ac7-7686-ebd1-6ad4448b2bf8@maciej.szmigiero.name>
Message-ID: <Y/aIP4aCxTVOU/ZC@google.com>
Subject: Re: [PATCH 2/2] Documentation/process: Add a maintainer handbook for
 KVM x86
From:   Sean Christopherson <seanjc@google.com>
To:     "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Sagi Shahar <sagis@google.com>,
        Erdem Aktas <erdemaktas@google.com>,
        Peter Shier <pshier@google.com>,
        Anish Ghulati <aghulati@google.com>,
        Oliver Upton <oliver.upton@linux.dev>,
        James Houghton <jthoughton@google.com>,
        Anish Moorthy <amoorthy@google.com>,
        Ben Gardon <bgardon@google.com>,
        David Matlack <dmatlack@google.com>,
        Ricardo Koller <ricarkol@google.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Aaron Lewis <aaronlewis@google.com>,
        Ashish Kalra <ashish.kalra@amd.com>,
        Babu Moger <babu.moger@amd.com>, Chao Gao <chao.gao@intel.com>,
        Chao Peng <chao.p.peng@linux.intel.com>,
        Chenyi Qiang <chenyi.qiang@intel.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        Emanuele Giuseppe Esposito <eesposit@redhat.com>,
        Gavin Shan <gshan@redhat.com>,
        Guang Zeng <guang.zeng@intel.com>,
        Hou Wenlong <houwenlong.hwl@antgroup.com>,
        Jiaxi Chen <jiaxi.chen@linux.intel.com>,
        Jim Mattson <jmattson@google.com>,
        Jing Liu <jing2.liu@intel.com>,
        Junaid Shahid <junaids@google.com>,
        Kai Huang <kai.huang@intel.com>,
        Leonardo Bras <leobras@redhat.com>,
        Like Xu <like.xu.linux@gmail.com>,
        Li RongQing <lirongqing@baidu.com>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        Michael Roth <michael.roth@amd.com>,
        Michal Luczaj <mhal@rbox.co>,
        Mingwei Zhang <mizhang@google.com>,
        Nikunj A Dadhania <nikunj@amd.com>,
        Paul Durrant <pdurrant@amazon.com>,
        Peng Hao <flyingpenghao@gmail.com>,
        Peter Gonda <pgonda@google.com>, Peter Xu <peterx@redhat.com>,
        Robert Hoo <robert.hu@linux.intel.com>,
        Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Vipin Sharma <vipinsh@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Wei Wang <wei.w.wang@intel.com>,
        Xiaoyao Li <xiaoyao.li@intel.com>,
        Yu Zhang <yu.c.zhang@linux.intel.com>,
        Zhenzhong Duan <zhenzhong.duan@intel.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 def=
ined in
> > +Intel's Software Development Manual (SDM) and AMD's Architecture Progr=
ammer=E2=80=99s
> > +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, e=
specially
> > +not in comments.  Instead, copy-paste the relevant snippet (if warrant=
ed), and
> > +reference sections/tables/figures by name.
>=20
> This says do "copy-paste the relevant snippet"...
>=20
> > The layouts of the SDM and APM are
> > +constantly changing, and so the numbers/labels aren't stable/consisten=
t.
> > +
> > +Generally speaking, do not copy-paste SDM or APM snippets into
> > comments.
>=20
> ...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, espe=
cially
  not in comments.  Instead, if necessary (see below), copy-paste the relev=
ant
  snippet and reference sections/tables/figures by name.  The layouts of th=
e SDM
  and APM are constantly changing, and so the numbers/labels aren't stable.
 =20
  Generally speaking, do not explicitly reference or copy-paste from the SD=
M or
  APM in comments.  With few exceptions, KVM *must* honor architectural beh=
avior,
  therefore it's implied that KVM behavior is emulating SDM and/or APM beha=
vior.
  Note, referencing the SDM/APM in changelogs to justify the change and pro=
vide
  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 discuss=
ion 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 p=
refix.
>=20
> 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 KV=
M_INTEL=3Dm
> > +KVM_AMD=3Dm, and KVM_WERROR=3Dy.  Building every possible combination =
of Kconfigs
> > +isn't feasible, but the more the merrier.  KVM_SMM, KVM_XEN, PROVE_LOC=
KING, and
> > +X86_64 are particularly interesting knobs to turn.
> > +
> > +Running KVM selftests and KVM-unit-tests is also mandatory (and statin=
g the
> > +obvious, the tests need to pass).
>=20
> I would add an exception here from mandatory testing for changes that
> obviously have negligible probability of affecting runtime behavior.
>=20
> 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 htmld=
ocs'
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 thi=
ng.

> > When possible and relevant, testing on both
> > +Intel and AMD is strongly preferred.  Booting an actual VM is encourag=
ed, 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, r=
unning
> > +with TDP disabled is strongly encouraged.  For all other changes, if t=
he code
> > +being modified depends on and/or interacts with a module param, testin=
g 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* fai=
lure
> > +occurs with and without your changes.
> > +
> > +If you can't fully test a change, e.g. due to lack of hardware, clearl=
y state
> > +what level of testing you were able to do, e.g. in the cover letter.
> > +
> (...)
>=20
> Thanks for preparing such a detailed handbook Sean.
>=20
> 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.
>=20
> That's especially true for unpaid volunteers that might become
> professional kernel developers one day if their learning curve isn't
> made too steep.
>=20
> Maybe have a paragraph or two that, despite all these rules, KVM x86
> strives to be a welcome community, encouraging newcomers and understandin=
g
> their beginner mistakes (or so)?

I like that idea a lot, I'll add a section at the very top.

Thanks much!