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

Paolo Bonzini
2021-11-26T07:59:09Z

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