Re: [PATCH 1/2] KVM: vmx, pmu: accept 0 for absent MSRs when host-initiated

[Paolo Bonzini <pbonzini@redhat.com>]

On 6/15/22 20:52, Sean Christopherson wrote:
> On Thu, Jun 02, 2022, Like Xu wrote:
>> I actually agree and understand the situation of maintainers/reviewers.
>> No one wants to maintain flawed code, especially in this community
>> where the majority of previous contributors disappeared after the code
>> was merged in. The existing heavy maintenance burden is already visible.

I don't think this is true.  I think it's relatively rare for 
contributors to disappear.

>> Thus we may have a maintainer/reviewers scalability issue. Due to missing
>> trust, competence or mastery of rules, most of the patches sent to the list
>> have no one to point out their flaws.
> 
> Then write tests and run the ones that already exist.  Relying purely on reviewers
> to detect flaws does not and cannot scale.  I agree that we currently have a
> scalability issue, but I have different views on how to improve things.
> 
>> I have privately received many complaints about the indifference of our
>> community, which is distressing.

You're welcome to expand on these complaints.  But I suspect that a lot 
of these would come from people that have been told "review other 
people's work", "write tests" and/or "you submitted a broken patch" before.

"Let's try to accept" is basically what I did for PEBS and LBR, both of 
which I merged basically out of guilt after a little-more-than-cursory 
review.  It turns out that both of them were broken in ways that weren't 
subtle at all; and as a result, other work already queued to 5.19 had to 
be bumped to 5.20.

Honestly I should have complained and un-merged them right after seeing 
the msr.flat failure.  Or perhaps I should have just said "write tests 
and then I'll consider the series", but I "tried to accept" and we can 
already see it was a failure.

>> Obviously, "try to accept" is not a 100% commitment and it will fail with high
>> probability, but such a stance (along with standard clarifications and requirements)
>> from reviewers and maintainers will make the contributors more concerned,
>> attract potential volunteers, and focus the efforts of our nominated reviewers.

If it "fails with high probability", all that happened was a waste of 
time for everyone involved.  Including the submitter who has waited for 
weeks for a reviews only to be told "test X fails".

> I completely agree on needing better transparency for the lifecycle of patches
> going through the KVM tree.  First and foremost, there need to be formal, documented
> rules for the "official" kvm/* branches, e.g. everything in kvm/queue passes ABC
> tests, everything in kvm/next also passes XYZ tests.  That would also be a good
> place to document expectations, how things works, etc...

Agreed.  I think this is a more general problem with Linux development 
and I will propose this for maintainer summit.

But again, the relationship between contributors and maintainers should 
be of mutual benefit.  Rules help contributors, but contributors should 
themselves behave and not throw broken patches at maintainers.  And 
again, guess what the best way is to tell maintainers your patch is not 
broken?  Include a test.  It shows that you are paying attention.

> I fully realize that writing tests is not glamorous, and that some of KVM's tooling
> and infrastructure is lacking,

I wouldn't say lacking.  Sure it's complicated, but between selftests 
and kvm-unit-tests the tools *are* there.  selftests that allow you to 
test migration at an instruction boundary, for example, are not that 
hard to write and were very important for features such as nested state 
and AMX.  They're not perfect, but they go a long way towards giving 
confidence in the code; and it's easier to catch weird ioctl policies 
from reviewing comprehensive tests than from reviewing the actual KVM code.

We're not talking of something like SEV or TDX here, we're talking about 
very boring MSR emulation and only slightly less boring PMU passthrough.

Paolo