linux-kernel - Re: [PATCH v19 085/130] KVM: TDX: Complete interrupts after tdexit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 16 Apr 2024 23:56:00 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: isaku.yamahata@...el.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
	Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
	Sean Christopherson <seanjc@...gle.com>,
	Sagi Shahar <sagis@...gle.com>, Kai Huang <kai.huang@...el.com>,
	chen.bo@...el.com, hang.yuan@...el.com, tina.zhang@...el.com,
	Binbin Wu <binbin.wu@...ux.intel.com>,
	isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH v19 085/130] KVM: TDX: Complete interrupts after tdexit

On Tue, Apr 16, 2024 at 11:23:01AM -0700,
Reinette Chatre <reinette.chatre@...el.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@...el.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@...el.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@...el.com>
> > Reviewed-by: Paolo Bonzini <pbonzini@...hat.com>
> > Reviewed-by: Binbin Wu <binbin.wu@...ux.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@...el.com>