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

[Paolo Bonzini]

On 11/26/21 00:26, Thomas Gleixner wrote:
> Paolo,
>
> On Thu, Nov 25 2021 at 23:13, Paolo Bonzini wrote:
> > On 11/25/21 22:05, Thomas Gleixner wrote:
> > 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
>
> There is absolutely no reason to populate anything upfront at all.
> Why?
>
> Simply because that whole muck cannot work until all pieces are in place.

It can, sort of.  It cannot run a complete guest, but it could in 
principle run a toy guest with a custom userspace, like the ones that 
make up tools/testing/selftests/kvm.  (Note that KVM_BUG_ON marks the VM 
as bugged but doesn't hang the whole machine).

AMD was working on infrastructure to do this for SEV and SEV-ES.

> So why would you provide:
>
> handle_A(...) { BUG(); }
> ..
> handle_Z(...) { BUG(); }
>
> with all the invocations in the common emulation dispatcher:
>
>    handle_stuff(reason)
>    {
>          switch(reason) {
>          case A: return handle_A();
>          ...
>          case Z: return handle_Z();
>          default: BUG();
>          }
>    };

If it's a switch statement that's good, but the common case is more 
similar to this:

 vmx_handle_A(...) { ... }
+tdx_handle_A(...) { ... }
+
+vt_handle_A(...) {
+    if (is_tdx(vcpu->kvm))
+        tdx_handle_A(...);
+    else
+        vmx_handle_A(...);
+}

...

-     .handle_A = vmx_handle_A,
+     .handle_A = vt_handle_A,

And you could indeed do it in a single patch, without adding the stub 
tdx_handle_A upfront.  But you would have code that is broken and who 
knows what the effects would be of calling vmx_handle_A on a TDX virtual 
machine.  It could be an error, or it could be memory corruption.


> In both scenarious you cannot boot a TDX guest until you reached $Z, but
> in the gradual one you and the reviewers have the pleasure of looking at
> one thing at a time.

I think both of them are gradual.  Not having the stubs might be a 
little more gradual, but it is a very minor issue for the reviewability 
of the whole thing.

Paolo