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
