Received-SPF: pass (google.com: domain of linux-kernel+bounces-148015-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 16 Apr 2024 23:56:00 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: isaku.yamahata@intel.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
	Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
	Sean Christopherson <seanjc@google.com>,
	Sagi Shahar <sagis@google.com>, Kai Huang <kai.huang@intel.com>,
	chen.bo@intel.com, hang.yuan@intel.com, tina.zhang@intel.com,
	Binbin Wu <binbin.wu@linux.intel.com>,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 085/130] KVM: TDX: Complete interrupts after tdexit
Message-ID: <20240417065600.GE3039520@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <aa6a927214a5d29d5591a0079f4374b05a82a03f.1708933498.git.isaku.yamahata@intel.com>
 <7d19f693-d8e9-4a9d-8cfa-3ec9c388622f@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <7d19f693-d8e9-4a9d-8cfa-3ec9c388622f@intel.com>

On Tue, Apr 16, 2024 at 11:23:01AM -0700,
Reinette Chatre <reinette.chatre@intel.com> wrote:

> Hi Isaku,
> 
> (In shortlog "tdexit" can be "TD exit" to be consistent with
> documentation.)
> 
> On 2/26/2024 12:26 AM, isaku.yamahata@intel.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > 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@intel.com>
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
> > ---
> > 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@intel.com>