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 11:23:01 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: <isaku.yamahata@...el.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
CC: <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>
Subject: Re: [PATCH v19 085/130] KVM: TDX: Complete interrupts after tdexit

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?

> +
>  #define TDX_BUILD_TDVPS_ACCESSORS(bits, uclass, lclass)				\
>  static __always_inline u##bits td_##lclass##_read##bits(struct vcpu_tdx *tdx,	\
>  							u32 field)		\
> @@ -200,6 +202,8 @@ TDX_BUILD_TDVPS_ACCESSORS(16, VMCS, vmcs);
>  TDX_BUILD_TDVPS_ACCESSORS(32, VMCS, vmcs);
>  TDX_BUILD_TDVPS_ACCESSORS(64, VMCS, vmcs);
>  
> +TDX_BUILD_TDVPS_ACCESSORS(8, MANAGEMENT, management);
> +
>  static __always_inline u64 td_tdcs_exec_read64(struct kvm_tdx *kvm_tdx, u32 field)
>  {
>  	struct tdx_module_args out;

Reinette