Date:   Wed, 22 Feb 2023 23:09:44 +0100
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sagi Shahar <sagis@...gle.com>,
        Erdem Aktas <erdemaktas@...gle.com>,
        Peter Shier <pshier@...gle.com>,
        Anish Ghulati <aghulati@...gle.com>,
        Oliver Upton <oliver.upton@...ux.dev>,
        James Houghton <jthoughton@...gle.com>,
        Anish Moorthy <amoorthy@...gle.com>,
        Ben Gardon <bgardon@...gle.com>,
        David Matlack <dmatlack@...gle.com>,
        Ricardo Koller <ricarkol@...gle.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Aaron Lewis <aaronlewis@...gle.com>,
        Ashish Kalra <ashish.kalra@....com>,
        Babu Moger <babu.moger@....com>, Chao Gao <chao.gao@...el.com>,
        Chao Peng <chao.p.peng@...ux.intel.com>,
        Chenyi Qiang <chenyi.qiang@...el.com>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Emanuele Giuseppe Esposito <eesposit@...hat.com>,
        Gavin Shan <gshan@...hat.com>,
        Guang Zeng <guang.zeng@...el.com>,
        Hou Wenlong <houwenlong.hwl@...group.com>,
        Jiaxi Chen <jiaxi.chen@...ux.intel.com>,
        Jim Mattson <jmattson@...gle.com>,
        Jing Liu <jing2.liu@...el.com>,
        Junaid Shahid <junaids@...gle.com>,
        Kai Huang <kai.huang@...el.com>,
        Leonardo Bras <leobras@...hat.com>,
        Like Xu <like.xu.linux@...il.com>,
        Li RongQing <lirongqing@...du.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        Michael Roth <michael.roth@....com>,
        Michal Luczaj <mhal@...x.co>,
        Mingwei Zhang <mizhang@...gle.com>,
        Nikunj A Dadhania <nikunj@....com>,
        Paul Durrant <pdurrant@...zon.com>,
        Peng Hao <flyingpenghao@...il.com>,
        Peter Gonda <pgonda@...gle.com>, Peter Xu <peterx@...hat.com>,
        Robert Hoo <robert.hu@...ux.intel.com>,
        Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Vipin Sharma <vipinsh@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Wei Wang <wei.w.wang@...el.com>,
        Xiaoyao Li <xiaoyao.li@...el.com>,
        Yu Zhang <yu.c.zhang@...ux.intel.com>,
        Zhenzhong Duan <zhenzhong.duan@...el.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
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

Powered by blists - more mailing lists