Re: [PATCH 0/7] KVM: TDX: TD vcpu enter/exit

[Binbin Wu]

On 11/26/2024 6:51 AM, Sean Christopherson wrote:

[...]
> When an NMI happens in non-root, the NMI is acknowledged by the CPU prior to
> performing VM-Exit.  In regular VMX, NMIs are blocked after such VM-Exits.  With
> TDX, that blocking happens for SEAM root, but the SEAMRET back to VMX root will
> load interruptibility from the SEAMCALL VMCS, and I don't see any code in the
> TDX-Module that propagates that blocking to SEAMCALL VMCS.
I see, thanks for the explanation!

> Hmm, actually, this means that TDX has a causality inversion, which may become
> visible with FRED's NMI source reporting.  E.g. NMI X arrives in SEAM non-root
> and triggers a VM-Exit.  NMI X+1 becomes pending while SEAM root is active.
> TDX-Module SEAMRETs to VMX root, NMIs are unblocked, and so NMI X+1 is delivered
> and handled before NMI X.

This example can also cause an issue without FRED.
1. NMI X arrives in SEAM non-root and triggers a VM-Exit.
2. NMI X+1 becomes pending while SEAM root is active.
3. TDX-Module SEAMRETs to VMX root, NMIs are unblocked.
4. NMI X+1 is delivered and handled before NMI X.
   (NMI handler could handle all NMI source events, including the source
    triggered NMI X)
5. KVM calls exc_nmi() to handle the VM Exit caused by NMI X
In step 5, because the source event caused NMI X has been handled, and NMI X
will not be detected as a second half of back-to-back NMIs, according to
Linux NMI handler, it will be considered as an unknown NMI.

Actually, the issue could happen if NMI X+1 occurs after exiting to SEAM root
mode and before KVM handling the VM-Exit caused by NMI X.


> So the TDX-Module needs something like this:
>
> diff --git a/src/td_transitions/td_exit.c b/src/td_transitions/td_exit.c
> index eecfb2e..b5c17c3 100644
> --- a/src/td_transitions/td_exit.c
> +++ b/src/td_transitions/td_exit.c
> @@ -527,6 +527,11 @@ void td_vmexit_to_vmm(uint8_t vcpu_state, uint8_t last_td_exit, uint64_t scrub_m
>           load_xmms_by_mask(tdvps_ptr, xmm_select);
>       }
>   
> +    if (<is NMI VM-Exit => SEAMRET)
> +    {
> +        set_guest_inter_blocking_by_nmi();
> +    }
> +
>       // 7.   Run the common SEAMRET routine.
>       tdx_vmm_post_dispatching();
>
>
> and then KVM should indeed handle NMI exits prior to leaving the noinstr section.
>   
> > > TDX is also different because KVM isn't responsible for context switching guest
> > > state.  Specifically, CR2 is managed by the TDX Module, and so there is no window
> > > where KVM runs with guest CR2, and thus there is no risk of clobbering guest CR2
> > > with a host value, e.g. due to take a #PF due instrumentation triggering something.
> > >
> > > All that said, I did forget that code that runs between guest_state_enter_irqoff()
> > > and guest_state_exit_irqoff() can't be instrumeneted.  And at least as of patch 2
> > > in this series, the simplest way to make that happen is to tag tdx_vcpu_enter_exit()
> > > as noinstr.  Just please make sure nothing else is added in the noinstr section
> > > unless it absolutely needs to be there.
> > If NMI is not a concern, is below also an option?
> No, because instrumentation needs to be prohibited for the entire time between
> guest_state_enter_irqoff() and guest_state_exit_irqoff().
>
> > 	guest_state_enter_irqoff();
> >
> > 	instructmentation_begin();
> > 	tdh_vp_enter();
> > 	instructmentation_end();
> >
> > 	guest_state_exit_irqoff();