On Tue, Apr 16, 2024 at 11:23:01AM -0700,
Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:
> Hi Isaku,
>
> (In shortlog "tdexit" can be "TD exit" to be consistent with
> documentation.)
>
> On 2/26/2024 12:26 AM, isaku.yamahata@xxxxxxxxx wrote:
> > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> >
> > This corresponds to VMX __vmx_complete_interrupts(). Because TDX
> > virtualize vAPIC, KVM only needs to care NMI injection.
>
> This seems to be the first appearance of NMI and the changelog
> is very brief. How about expending it with:
>
> "This corresponds to VMX __vmx_complete_interrupts(). Because TDX
> virtualize vAPIC, KVM only needs to care about NMI injection.
>
> KVM can request TDX to inject an NMI into a guest TD vCPU when the
> vCPU is not active. TDX will attempt to inject an NMI as soon as
> possible on TD entry. NMI injection is managed by writing to (to
> inject NMI) and reading from (to get status of NMI injection)
> the PEND_NMI field within the TDX vCPU scope metadata (Trust
> Domain Virtual Processor State (TDVPS)).
>
> Update KVM's NMI status on TD exit by checking whether a requested
> NMI has been injected into the TD. Reading the metadata via SEAMCALL
> is expensive so only perform the check if an NMI was injected.
>
> This is the first need to access vCPU scope metadata in the
> "management" class. Ensure that needed accessor is available.
> "
>
> >
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > Reviewed-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Reviewed-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
> > ---
> > v19:
> > - move tdvps_management_check() to this patch
> > - typo: complete -> Complete in short log
> > ---
> > arch/x86/kvm/vmx/tdx.c | 10 ++++++++++
> > arch/x86/kvm/vmx/tdx.h | 4 ++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 83dcaf5b6fbd..b8b168f74dfe 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -535,6 +535,14 @@ void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> > */
> > }
> >
> > +static void tdx_complete_interrupts(struct kvm_vcpu *vcpu)
> > +{
> > + /* Avoid costly SEAMCALL if no nmi was injected */
>
> /* Avoid costly SEAMCALL if no NMI was injected. */
>
> > + if (vcpu->arch.nmi_injected)
> > + vcpu->arch.nmi_injected = td_management_read8(to_tdx(vcpu),
> > + TD_VCPU_PEND_NMI);
> > +}
> > +
> > struct tdx_uret_msr {
> > u32 msr;
> > unsigned int slot;
> > @@ -663,6 +671,8 @@ fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu)
> > vcpu->arch.regs_avail &= ~VMX_REGS_LAZY_LOAD_SET;
> > trace_kvm_exit(vcpu, KVM_ISA_VMX);
> >
> > + tdx_complete_interrupts(vcpu);
> > +
> > return EXIT_FASTPATH_NONE;
> > }
> >
> > diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> > index 44eab734e702..0d8a98feb58e 100644
> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -142,6 +142,8 @@ static __always_inline void tdvps_vmcs_check(u32 field, u8 bits)
> > "Invalid TD VMCS access for 16-bit field");
> > }
> >
> > +static __always_inline void tdvps_management_check(u64 field, u8 bits) {}
>
> Is this intended to be a stub or is it expected to be fleshed out with
> some checks?
It was used to check if field id matches bits. We should make
tdvps_vmcs_check() common for vmcs, management and state_non_arch.
--
Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
