Re: [RFC PATCH v3 54/59] KVM: X86: Introduce initial_tsc_khz in struct kvm_arch

Paolo Bonzini
2021-11-25T22:15:57Z

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