Re: [RFC PATCH v5 16/29] KVM: selftests: TDX: Add TDX HLT exit test

[Sagi Shahar]

On Tue, Mar 5, 2024 at 12:10 AM Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote:
>
> On Sat, Mar 02, 2024 at 03:31:07PM +0800, Binbin Wu wrote:
> > On 12/13/2023 4:46 AM, Sagi Shahar wrote:
> > > The test verifies that the guest runs TDVMCALL<INSTRUCTION.HLT> and the
> > > guest vCPU enters to the halted state.
> > >
> > > Signed-off-by: Erdem Aktas <erdemaktas@xxxxxxxxxx>
> > > Signed-off-by: Sagi Shahar <sagis@xxxxxxxxxx>
> > > Signed-off-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> > > Signed-off-by: Ryan Afranji <afranji@xxxxxxxxxx>
> > > ---
> > >   .../selftests/kvm/include/x86_64/tdx/tdx.h    |  2 +
> > >   .../selftests/kvm/lib/x86_64/tdx/tdx.c        | 10 +++
> > >   .../selftests/kvm/x86_64/tdx_vm_tests.c       | 78 +++++++++++++++++++
> > >   3 files changed, 90 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/kvm/include/x86_64/tdx/tdx.h b/tools/testing/selftests/kvm/include/x86_64/tdx/tdx.h
> > > index 85ba6aab79a7..b18e39d20498 100644
> > > --- a/tools/testing/selftests/kvm/include/x86_64/tdx/tdx.h
> > > +++ b/tools/testing/selftests/kvm/include/x86_64/tdx/tdx.h
> > > @@ -8,6 +8,7 @@
> > >   #define TDG_VP_VMCALL_GET_TD_VM_CALL_INFO 0x10000
> > >   #define TDG_VP_VMCALL_REPORT_FATAL_ERROR 0x10003
> > > +#define TDG_VP_VMCALL_INSTRUCTION_HLT 12
> > >   #define TDG_VP_VMCALL_INSTRUCTION_IO 30
> > >   #define TDG_VP_VMCALL_INSTRUCTION_RDMSR 31
> > >   #define TDG_VP_VMCALL_INSTRUCTION_WRMSR 32
> > > @@ -20,5 +21,6 @@ uint64_t tdg_vp_vmcall_get_td_vmcall_info(uint64_t *r11, uint64_t *r12,
> > >                                     uint64_t *r13, uint64_t *r14);
> > >   uint64_t tdg_vp_vmcall_instruction_rdmsr(uint64_t index, uint64_t *ret_value);
> > >   uint64_t tdg_vp_vmcall_instruction_wrmsr(uint64_t index, uint64_t value);
> > > +uint64_t tdg_vp_vmcall_instruction_hlt(uint64_t interrupt_blocked_flag);
> > >   #endif // SELFTEST_TDX_TDX_H
> > > diff --git a/tools/testing/selftests/kvm/lib/x86_64/tdx/tdx.c b/tools/testing/selftests/kvm/lib/x86_64/tdx/tdx.c
> > > index 88ea6f2a6469..9485bafedc38 100644
> > > --- a/tools/testing/selftests/kvm/lib/x86_64/tdx/tdx.c
> > > +++ b/tools/testing/selftests/kvm/lib/x86_64/tdx/tdx.c
> > > @@ -114,3 +114,13 @@ uint64_t tdg_vp_vmcall_instruction_wrmsr(uint64_t index, uint64_t value)
> > >     return __tdx_hypercall(&args, 0);
> > >   }
> > > +
> > > +uint64_t tdg_vp_vmcall_instruction_hlt(uint64_t interrupt_blocked_flag)
> > > +{
> > > +   struct tdx_hypercall_args args = {
> > > +           .r11 = TDG_VP_VMCALL_INSTRUCTION_HLT,
> > > +           .r12 = interrupt_blocked_flag,
> > > +   };
> > > +
> > > +   return __tdx_hypercall(&args, 0);
> > > +}
> > > diff --git a/tools/testing/selftests/kvm/x86_64/tdx_vm_tests.c b/tools/testing/selftests/kvm/x86_64/tdx_vm_tests.c
> > > index 5db3701cc6d9..5fae4c6e5f95 100644
> > > --- a/tools/testing/selftests/kvm/x86_64/tdx_vm_tests.c
> > > +++ b/tools/testing/selftests/kvm/x86_64/tdx_vm_tests.c
> > > @@ -721,6 +721,83 @@ void verify_guest_msr_writes(void)
> > >     printf("\t ... PASSED\n");
> > >   }
> > > +/*
> > > + * Verifies HLT functionality.
> > > + */
> > > +void guest_hlt(void)
> > > +{
> > > +   uint64_t ret;
> > > +   uint64_t interrupt_blocked_flag;
> > > +
> > > +   interrupt_blocked_flag = 0;
> > > +   ret = tdg_vp_vmcall_instruction_hlt(interrupt_blocked_flag);
> > > +   if (ret)
> > > +           tdx_test_fatal(ret);
> > > +
> > > +   tdx_test_success();
> > > +}
> > > +
> > > +void _verify_guest_hlt(int signum);
> > > +
> > > +void wake_me(int interval)
> > > +{
> > > +   struct sigaction action;
> > > +
> > > +   action.sa_handler = _verify_guest_hlt;
> > > +   sigemptyset(&action.sa_mask);
> > > +   action.sa_flags = 0;
> > > +
> > > +   TEST_ASSERT(sigaction(SIGALRM, &action, NULL) == 0,
> > > +               "Could not set the alarm handler!");
> > > +
> > > +   alarm(interval);
> > > +}
> > > +
> > > +void _verify_guest_hlt(int signum)
> > > +{
> > > +   struct kvm_vm *vm;
> > > +   static struct kvm_vcpu *vcpu;
> > > +
> > > +   /*
> > > +    * This function will also be called by SIGALRM handler to check the
> > > +    * vCPU MP State. If vm has been initialized, then we are in the signal
> > > +    * handler. Check the MP state and let the guest run again.
> > > +    */
> > > +   if (vcpu != NULL) {
> >
> > What if the following case if there is a bug in KVM so that:
> >
> > In guest, execution of tdg_vp_vmcall_instruction_hlt() return 0, but the
> > vcpu is not actually halted. Then guest will call tdx_test_success().
> >
> > And the first call of _verify_guest_hlt() will call kvm_vm_free(vm) to free
> > the vm, which also frees the vcpu, and 1 second later, in this path vcpu
> > will
> > be accessed after free.
> >
> Right. Another possibility is that if buggy KVM returns success to guest
> without putting guest to halted state, the selftest will still print
> "PASSED" because the second _verify_guest_hlt() (after waiting for 1s)
> has no chance to get executed before the process exits.
>
It sounds like in both cases we're going to hit an assert at some
point. If the VM was already freed then vcpu_mp_state_get will fail
the ioctl and assert internally or crash the process. If the guest
never halts and vcpu is still valid then the mp state assert will
fire. Either way it would be pretty obvious that something is wrong.


> > > +           struct kvm_mp_state mp_state;
> > > +
> > > +           vcpu_mp_state_get(vcpu, &mp_state);
> > > +           TEST_ASSERT_EQ(mp_state.mp_state, KVM_MP_STATE_HALTED);
> > > +
> > > +           /* Let the guest to run and finish the test.*/
> > > +           mp_state.mp_state = KVM_MP_STATE_RUNNABLE;
> > > +           vcpu_mp_state_set(vcpu, &mp_state);
> > > +           return;
> > > +   }
> > > +
> > > +   vm = td_create();
> > > +   td_initialize(vm, VM_MEM_SRC_ANONYMOUS, 0);
> > > +   vcpu = td_vcpu_add(vm, 0, guest_hlt);
> > > +   td_finalize(vm);
> > > +
> > > +   printf("Verifying HLT:\n");
> > > +
> > > +   printf("\t ... Running guest\n");
> > > +
> > > +   /* Wait 1 second for guest to execute HLT */
> > > +   wake_me(1);
> > > +   td_vcpu_run(vcpu);
> > > +
> > > +   TDX_TEST_ASSERT_SUCCESS(vcpu);
> > > +
> > > +   kvm_vm_free(vm);
> > > +   printf("\t ... PASSED\n");
> > > +}
> > > +
> > > +void verify_guest_hlt(void)
> > > +{
> > > +   _verify_guest_hlt(0);
> > > +}
> > >   int main(int argc, char **argv)
> > >   {
> > > @@ -740,6 +817,7 @@ int main(int argc, char **argv)
> > >     run_in_new_process(&verify_guest_reads);
> > >     run_in_new_process(&verify_guest_msr_writes);
> > >     run_in_new_process(&verify_guest_msr_reads);
> > > +   run_in_new_process(&verify_guest_hlt);
> > >     return 0;
> > >   }
> >
> >