Date:   Mon, 29 Nov 2021 23:38:33 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>, isaku.yamahata@intel.com,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, erdemaktas@google.com,
        Connor Kuehl <ckuehl@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, isaku.yamahata@gmail.com,
        Xiaoyao Li <xiaoyao.li@intel.com>
Subject: Re: [RFC PATCH v3 54/59] KVM: X86: Introduce initial_tsc_khz in
 struct kvm_arch
Message-ID: <YaVkeZalN5wkd9uL@google.com>
References: <cover.1637799475.git.isaku.yamahata@intel.com>
 <5ba3573c8b82fcbdc3f3994f6d4d2a3c40445be9.1637799475.git.isaku.yamahata@intel.com>
 <875ysghrp8.ffs@tglx>
 <741df444-5cd0-2049-f93a-c2521e4f426d@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <741df444-5cd0-2049-f93a-c2521e4f426d@redhat.com>
Precedence: bulk

On Thu, Nov 25, 2021, Paolo Bonzini wrote:
> 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.

Hmm, I don't think this approach will work as well as it did for SEV when applied
at a per-ioctl granuarity, I suspect several patches will end up quite large.   I
completely agree with the overall idea, but I'd encourage the TDX folks to have a
finer granularity where it makes sense, e.g. things like the x2APIC behavior,
immutable TSC, memory management, etc... can probably be sliced up into separate
patches.

> 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.

Generally speaking, I agree.  For the flag exposion, I 100% agree that setting
the flag in TDX, adding it in x86 is best done in a signal patch, and handling
all side effects is best done in a single patch.  

But for things like letting debug TDs access registers, I would prefer not to
actually squash the two (or more) patches.  I agree that two related patches need
to be contiguous in the series, but I'd prefer that things with non-trivial changes,
especially in common code, are kept separate.

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