linux-kernel - Re: [RFC PATCH v3 54/59] KVM: X86: Introduce initial_tsc_khz in struct kvm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Nov 2021 23:13:46 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>, isaku.yamahata@...el.com,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, erdemaktas@...gle.com,
        Connor Kuehl <ckuehl@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc:     isaku.yamahata@...il.com, Xiaoyao Li <xiaoyao.li@...el.com>
Subject: Re: [RFC PATCH v3 54/59] KVM: X86: Introduce initial_tsc_khz in
 struct kvm_arch

On 11/25/21 22:05, Thomas Gleixner wrote:
> You can argue that my request is unreasonable until you are blue in
> your face, it's not going to lift my NAK on this.

There's no need for that.  I'd be saying the same, and I don't think 
it's particularly helpful that you made it almost a personal issue.

While in this series there is a separation of changes to existing code 
vs. new code, what's not clear is _why_ you have all those changes. 
These are not code cleanups or refactorings that can stand on their own 
feet; lots of the early patches are actually part of the new 
functionality.  And being in the form of "add an argument here" or 
"export a function there", it's not really easy (or feasible) to review 
them without seeing how the new functionality is used, which requires a 
constant back and forth between early patches and the final 2000 line file.

In some sense, the poor commit messages at the beginning of the series 
are just a symptom of not having any meat until too late, and then 
dropping it all at once.  There's only so much that you can say about an 
EXPORT_SYMBOL_GPL, the real thing to talk about is probably the thing 
that refers to that symbol.

If there are some patches that are actually independent, go ahead and 
submit them early.  But more practically, for the bulk of the changes 
what you need to do is:

1) incorporate into patch 55 a version of tdx.c that essentially does 
KVM_BUG_ON or WARN_ON for each function.  Temporarily keep the same huge 
patch that adds the remaining 2000 lines of tdx.c

2) squash the tdx.c stub with patch 44.

3) gather a strace of QEMU starting up a TDX domain.

4) figure out which parts of the code are needed to run until the first 
ioctl.  Make that a first patch.

5) repeat step 4 until you have covered all the code

5) Move the new "KVM: VMX: Add 'main.c' to wrap VMX and TDX" (which also 
adds the tdx.c stub) as possible in the series.

6) Move each of the new patches as early as possible in the series.

7) Look for candidates for squashing (e.g. commit messages that say it's 
"used later"; now the use should be very close and the two can be 
merged).  Add to the commit message a note about changes outside VMX.

The resulting series may not be perfect, but it would be a much better 
starting point for review.

Paolo