[v19,101/130] KVM: TDX: handle ept violation/misconfig exit

Message ID	f05b978021522d70a259472337e0b53658d47636.1708933498.git.isaku.yamahata@intel.com (mailing list archive)
State	New, archived
Headers	show Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B87F37E784; Mon, 26 Feb 2024 08:28:58 +0000 (UTC) From: isaku.yamahata@intel.com To: kvm@vger.kernel.org, linux-kernel@vger.kernel.org Cc: isaku.yamahata@intel.com, 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 Subject: [PATCH v19 101/130] KVM: TDX: handle ept violation/misconfig exit Date: Mon, 26 Feb 2024 00:26:43 -0800 Message-Id: <f05b978021522d70a259472337e0b53658d47636.1708933498.git.isaku.yamahata@intel.com> In-Reply-To: <cover.1708933498.git.isaku.yamahata@intel.com> References: <cover.1708933498.git.isaku.yamahata@intel.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit
Series	[v19,001/130] x86/virt/tdx: Rename _offset to _member for TD_SYSINFO_MAP() macro \| expand [v19,001/130] x86/virt/tdx: Rename _offset to _member for TD_SYSINFO_MAP() macro [v19,002/130] x86/virt/tdx: Move TDMR metadata fields map table to local variable [v19,003/130] x86/virt/tdx: Unbind global metadata read with 'struct tdx_tdmr_sysinfo' [v19,004/130] x86/virt/tdx: Support global metadata read for all element sizes [v19,005/130] x86/virt/tdx: Export global metadata read infrastructure [v19,006/130] x86/virt/tdx: Export TDX KeyID information [v19,007/130] x86/virt/tdx: Export SEAMCALL functions [v19,008/130] x86/tdx: Warning with 32bit build shift-count-overflow [v19,009/130] KVM: x86: Add gmem hook for determining max NPT mapping level [v19,010/130] KVM: x86: Pass is_private to gmem hook of gmem_max_level [v19,011/130] KVM: Add new members to struct kvm_gfn_range to operate on [v19,012/130] KVM: x86/mmu: Pass around full 64-bit error code for the KVM page fault [v19,013/130] KVM: x86: Use PFERR_GUEST_ENC_MASK to indicate fault is private [v19,014/130] KVM: Add KVM vcpu ioctl to pre-populate guest memory [v19,015/130] KVM: Document KVM_MEMORY_MAPPING ioctl [v19,016/130] KVM: x86/mmu: Introduce kvm_mmu_map_tdp_page() for use by TDX [v19,017/130] KVM: x86: Implement kvm_arch_{, pre_}vcpu_memory_mapping() [v19,018/130] KVM: x86/mmu: Assume guest MMIOs are shared [v19,019/130] KVM: x86: Add is_vm_type_supported callback [v19,020/130] KVM: VMX: Move out vmx_x86_ops to 'main.c' to wrap VMX and TDX [v19,021/130] KVM: x86/vmx: initialize loaded_vmcss_on_cpu in vmx_init() [v19,022/130] KVM: x86/vmx: Refactor KVM VMX module init/exit functions [v19,023/130] KVM: TDX: Initialize the TDX module when loading the KVM intel kernel module [v19,024/130] KVM: TDX: Add placeholders for TDX VM/vcpu structure [v19,025/130] KVM: TDX: Make TDX VM type supported [v19,026/130,MARKER] The start of TDX KVM patch series: TDX architectural definitions [v19,027/130] KVM: TDX: Define TDX architectural definitions [v19,028/130] KVM: TDX: Add TDX "architectural" error codes [v19,029/130] KVM: TDX: Add C wrapper functions for SEAMCALLs to the TDX module [v19,030/130] KVM: TDX: Add helper functions to print TDX SEAMCALL error [v19,031/130,MARKER] The start of TDX KVM patch series: TD VM creation/destruction [v19,032/130] KVM: TDX: Add helper functions to allocate/free TDX private host key id [v19,033/130] KVM: TDX: Add helper function to read TDX metadata in array [v19,034/130] KVM: TDX: Get system-wide info about TDX module on initialization [v19,035/130] KVM: TDX: Add place holder for TDX VM specific mem_enc_op ioctl [v19,036/130] KVM: TDX: x86: Add ioctl to get TDX systemwide parameters [v19,037/130] KVM: TDX: Make KVM_CAP_MAX_VCPUS backend specific [v19,038/130] KVM: TDX: create/destroy VM structure [v19,039/130] KVM: TDX: initialize VM with TDX specific parameters [v19,040/130] KVM: TDX: Make pmu_intel.c ignore guest TD case [v19,041/130] KVM: TDX: Refuse to unplug the last cpu on the package [v19,042/130,MARKER] The start of TDX KVM patch series: TD vcpu creation/destruction [v19,043/130] KVM: TDX: create/free TDX vcpu structure [v19,044/130] KVM: TDX: Do TDX specific vcpu initialization [v19,045/130,MARKER] The start of TDX KVM patch series: KVM MMU GPA shared bits [v19,046/130] KVM: x86/mmu: Add address conversion functions for TDX shared bit of GPA [v19,047/130,MARKER] The start of TDX KVM patch series: KVM TDP refactoring for TDX [v19,048/130] KVM: Allow page-sized MMU caches to be initialized with custom 64-bit values [v19,049/130] KVM: x86/mmu: Replace hardcoded value 0 for the initial value for SPTE [v19,050/130] KVM: x86/mmu: Allow non-zero value for non-present SPTE and removed SPTE [v19,051/130] KVM: x86/mmu: Add Suppress VE bit to shadow_mmio_mask/shadow_present_mask [v19,052/130] KVM: x86/mmu: Track shadow MMIO value on a per-VM basis [v19,053/130] KVM: x86/mmu: Disallow fast page fault on private GPA [v19,054/130] KVM: VMX: Introduce test mode related to EPT violation VE [v19,055/130,MARKER] The start of TDX KVM patch series: KVM TDP MMU hooks [v19,056/130] KVM: x86/tdp_mmu: Init role member of struct kvm_mmu_page at allocation [v19,057/130] KVM: x86/mmu: Add a new is_private member for union kvm_mmu_page_role [v19,058/130] KVM: x86/mmu: Add a private pointer to struct kvm_mmu_page [v19,059/130] KVM: x86/tdp_mmu: Don't zap private pages for unsupported cases [v19,060/130] KVM: x86/tdp_mmu: Apply mmu notifier callback to only shared GPA [v19,061/130] KVM: x86/tdp_mmu: Sprinkle __must_check [v19,062/130] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU [v19,063/130,MARKER] The start of TDX KVM patch series: TDX EPT violation [v19,064/130] KVM: x86/mmu: Do not enable page track for TD guest [v19,065/130] KVM: VMX: Split out guts of EPT violation to common/exposed function [v19,066/130] KVM: TDX: Add accessors VMX VMCS helpers [v19,067/130] KVM: TDX: Add load_mmu_pgd method for TDX [v19,068/130] KVM: TDX: Retry seamcall when TDX_OPERAND_BUSY with operand SEPT [v19,069/130] KVM: TDX: Require TDP MMU and mmio caching for TDX [v19,070/130] KVM: TDX: TDP MMU TDX support [v19,071/130] KVM: TDX: MTRR: implement get_mt_mask() for TDX [v19,072/130,MARKER] The start of TDX KVM patch series: TD finalization [v19,073/130] KVM: x86: Add hooks in kvm_arch_vcpu_memory_mapping() [v19,074/130] KVM: TDX: Create initial guest memory [v19,075/130] KVM: TDX: Extend memory measurement with initial guest memory [v19,076/130] KVM: TDX: Finalize VM initialization [v19,077/130,MARKER] The start of TDX KVM patch series: TD vcpu enter/exit [v19,078/130] KVM: TDX: Implement TDX vcpu enter/exit path [v19,079/130] KVM: TDX: vcpu_run: save/restore host state(host kernel gs) [v19,080/130] KVM: TDX: restore host xsave state when exit from the guest TD [v19,081/130] KVM: x86: Allow to update cached values in kvm_user_return_msrs w/o wrmsr [v19,082/130] KVM: TDX: restore user ret MSRs [v19,083/130] KVM: TDX: Add TSX_CTRL msr into uret_msrs list [v19,084/130,MARKER] The start of TDX KVM patch series: TD vcpu exits/interrupts/hypercalls [v19,085/130] KVM: TDX: Complete interrupts after tdexit [v19,086/130] KVM: TDX: restore debug store when TD exit [v19,087/130] KVM: TDX: handle vcpu migration over logical processor [v19,088/130] KVM: x86: Add a switch_db_regs flag to handle TDX's auto-switched behavior [v19,089/130] KVM: TDX: Add support for find pending IRQ in a protected local APIC [v19,090/130] KVM: x86: Assume timer IRQ was injected if APIC state is proteced [v19,091/130] KVM: TDX: remove use of struct vcpu_vmx from posted_interrupt.c [v19,092/130] KVM: TDX: Implement interrupt injection [v19,093/130] KVM: TDX: Implements vcpu request_immediate_exit [v19,094/130] KVM: TDX: Implement methods to inject NMI [v19,095/130] KVM: VMX: Modify NMI and INTR handlers to take intr_info as function argument [v19,096/130] KVM: VMX: Move NMI/exception handler to common helper [v19,097/130] KVM: x86: Split core of hypercall emulation to helper function [v19,098/130] KVM: TDX: Add a place holder to handle TDX VM exit [v19,099/130] KVM: TDX: Handle vmentry failure for INTEL TD guest [v19,100/130] KVM: TDX: handle EXIT_REASON_OTHER_SMI [v19,101/130] KVM: TDX: handle ept violation/misconfig exit [v19,102/130] KVM: TDX: handle EXCEPTION_NMI and EXTERNAL_INTERRUPT [v19,103/130] KVM: TDX: Handle EXIT_REASON_OTHER_SMI with MSMI [v19,104/130] KVM: TDX: Add a place holder for handler of TDX hypercalls (TDG.VP.VMCALL) [v19,105/130] KVM: TDX: handle KVM hypercall with TDG.VP.VMCALL [v19,106/130] KVM: TDX: Add KVM Exit for TDX TDG.VP.VMCALL [v19,107/130] KVM: TDX: Handle TDX PV CPUID hypercall [v19,108/130] KVM: TDX: Handle TDX PV HLT hypercall [v19,109/130] KVM: TDX: Handle TDX PV port io hypercall [v19,110/130] KVM: TDX: Handle TDX PV MMIO hypercall [v19,111/130] KVM: TDX: Implement callbacks for MSR operations for TDX [v19,112/130] KVM: TDX: Handle TDX PV rdmsr/wrmsr hypercall [v19,113/130] KVM: TDX: Handle MSR MTRRCap and MTRRDefType access [v19,114/130] KVM: TDX: Handle MSR IA32_FEAT_CTL MSR and IA32_MCG_EXT_CTL [v19,115/130] KVM: TDX: Handle TDG.VP.VMCALL<GetTdVmCallInfo> hypercall [v19,116/130] KVM: TDX: Silently discard SMI request [v19,117/130] KVM: TDX: Silently ignore INIT/SIPI [v19,118/130] KVM: TDX: Add methods to ignore accesses to CPU state [v19,119/130] KVM: TDX: Add methods to ignore guest instruction emulation [v19,120/130] KVM: TDX: Add a method to ignore dirty logging [v19,121/130] KVM: TDX: Add methods to ignore VMX preemption timer [v19,122/130] KVM: TDX: Add methods to ignore accesses to TSC [v19,123/130] KVM: TDX: Ignore setting up mce [v19,124/130] KVM: TDX: Add a method to ignore for TDX to ignore hypercall patch [v19,125/130] KVM: TDX: Add methods to ignore virtual apic related operation [v19,126/130] KVM: TDX: Inhibit APICv for TDX guest [v19,127/130] Documentation/virt/kvm: Document on Trust Domain Extensions(TDX) [v19,128/130] KVM: x86: design documentation on TDX support of x86 KVM TDP MMU [v19,129/130] RFC: KVM: x86: Add x86 callback to check cpuid [v19,130/130] RFC: KVM: x86, TDX: Add check for KVM_SET_CPUID2

Isaku Yamahata Feb. 26, 2024, 8:26 a.m. UTC

From: Isaku Yamahata <isaku.yamahata@intel.com>

On EPT violation, call a common function, __vmx_handle_ept_violation() to
trigger x86 MMU code.  On EPT misconfiguration, exit to ring 3 with
KVM_EXIT_UNKNOWN.  because EPT misconfiguration can't happen as MMIO is
trigged by TDG.VP.VMCALL. No point to set a misconfiguration value for the
fast path.

Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>

---
v14 -> v15:
- use PFERR_GUEST_ENC_MASK to tell the fault is private

Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
---
 arch/x86/kvm/vmx/common.h |  3 +++
 arch/x86/kvm/vmx/tdx.c    | 49 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

Chenyi Qiang March 4, 2024, 7:39 a.m. UTC | #1

On 2/26/2024 4:26 PM, isaku.yamahata@intel.com wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> On EPT violation, call a common function, __vmx_handle_ept_violation() to
> trigger x86 MMU code.  On EPT misconfiguration, exit to ring 3 with
> KVM_EXIT_UNKNOWN.  because EPT misconfiguration can't happen as MMIO is
> trigged by TDG.VP.VMCALL. No point to set a misconfiguration value for the

s/trigged/triggered

> fast path.
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> ---
> v14 -> v15:
> - use PFERR_GUEST_ENC_MASK to tell the fault is private
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>

duplicated SOB

> ---
>  arch/x86/kvm/vmx/common.h |  3 +++
>  arch/x86/kvm/vmx/tdx.c    | 49 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
> index 632af7a76d0a..027aa4175d2c 100644
> --- a/arch/x86/kvm/vmx/common.h
> +++ b/arch/x86/kvm/vmx/common.h
> @@ -87,6 +87,9 @@ static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
>  	error_code |= (exit_qualification & EPT_VIOLATION_GVA_TRANSLATED) != 0 ?
>  	       PFERR_GUEST_FINAL_MASK : PFERR_GUEST_PAGE_MASK;
>  
> +	if (kvm_is_private_gpa(vcpu->kvm, gpa))
> +		error_code |= PFERR_GUEST_ENC_MASK;
> +
>  	return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
>  }
>  
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 2f68e6f2b53a..0db80fa020d2 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1285,6 +1285,51 @@ void tdx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,
>  	__vmx_deliver_posted_interrupt(vcpu, &tdx->pi_desc, vector);
>  }
>  
> +static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
> +{
> +	unsigned long exit_qual;
> +
> +	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
> +		/*
> +		 * Always treat SEPT violations as write faults.  Ignore the
> +		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
> +		 * TD private pages are always RWX in the SEPT tables,
> +		 * i.e. they're always mapped writable.  Just as importantly,
> +		 * treating SEPT violations as write faults is necessary to
> +		 * avoid COW allocations, which will cause TDAUGPAGE failures
> +		 * due to aliasing a single HPA to multiple GPAs.
> +		 */
> +#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
> +		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
> +	} else {
> +		exit_qual = tdexit_exit_qual(vcpu);
> +		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
> +			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
> +				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
> +			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
> +			vcpu->run->ex.exception = PF_VECTOR;
> +			vcpu->run->ex.error_code = exit_qual;
> +			return 0;
> +		}
> +	}
> +
> +	trace_kvm_page_fault(vcpu, tdexit_gpa(vcpu), exit_qual);
> +	return __vmx_handle_ept_violation(vcpu, tdexit_gpa(vcpu), exit_qual);
> +}
> +
> +static int tdx_handle_ept_misconfig(struct kvm_vcpu *vcpu)
> +{
> +	WARN_ON_ONCE(1);
> +
> +	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
> +	vcpu->run->internal.ndata = 2;
> +	vcpu->run->internal.data[0] = EXIT_REASON_EPT_MISCONFIG;
> +	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
> +
> +	return 0;
> +}
> +
>  int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
>  {
>  	union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;
> @@ -1345,6 +1390,10 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
>  	WARN_ON_ONCE(fastpath != EXIT_FASTPATH_NONE);
>  
>  	switch (exit_reason.basic) {
> +	case EXIT_REASON_EPT_VIOLATION:
> +		return tdx_handle_ept_violation(vcpu);
> +	case EXIT_REASON_EPT_MISCONFIG:
> +		return tdx_handle_ept_misconfig(vcpu);
>  	case EXIT_REASON_OTHER_SMI:
>  		/*
>  		 * If reach here, it's not a Machine Check System Management

Chao Gao April 1, 2024, 4:10 a.m. UTC | #2

>+static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
>+{
>+	unsigned long exit_qual;
>+
>+	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
>+		/*
>+		 * Always treat SEPT violations as write faults.  Ignore the
>+		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
>+		 * TD private pages are always RWX in the SEPT tables,
>+		 * i.e. they're always mapped writable.  Just as importantly,
>+		 * treating SEPT violations as write faults is necessary to
>+		 * avoid COW allocations, which will cause TDAUGPAGE failures
>+		 * due to aliasing a single HPA to multiple GPAs.
>+		 */
>+#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
>+		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
>+	} else {
>+		exit_qual = tdexit_exit_qual(vcpu);
>+		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {

Unless the CPU has a bug, instruction fetch in TD from shared memory causes a
#PF. I think you can add a comment for this.

Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.

>+			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
>+				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
>+			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
>+			vcpu->run->ex.exception = PF_VECTOR;
>+			vcpu->run->ex.error_code = exit_qual;
>+			return 0;
>+		}
>+	}
>+
>+	trace_kvm_page_fault(vcpu, tdexit_gpa(vcpu), exit_qual);
>+	return __vmx_handle_ept_violation(vcpu, tdexit_gpa(vcpu), exit_qual);
>+}
>+
>+static int tdx_handle_ept_misconfig(struct kvm_vcpu *vcpu)
>+{
>+	WARN_ON_ONCE(1);
>+
>+	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>+	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
>+	vcpu->run->internal.ndata = 2;
>+	vcpu->run->internal.data[0] = EXIT_REASON_EPT_MISCONFIG;
>+	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
>+
>+	return 0;
>+}
>+
> int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
> {
> 	union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;
>@@ -1345,6 +1390,10 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
> 	WARN_ON_ONCE(fastpath != EXIT_FASTPATH_NONE);
> 
> 	switch (exit_reason.basic) {
>+	case EXIT_REASON_EPT_VIOLATION:
>+		return tdx_handle_ept_violation(vcpu);
>+	case EXIT_REASON_EPT_MISCONFIG:
>+		return tdx_handle_ept_misconfig(vcpu);

Handling EPT misconfiguration can be dropped because the "default" case handles
all unexpected exits in the same way


> 	case EXIT_REASON_OTHER_SMI:
> 		/*
> 		 * If reach here, it's not a Machine Check System Management
>-- 
>2.25.1
>
>

Isaku Yamahata April 3, 2024, 6:42 p.m. UTC | #3

On Mon, Apr 01, 2024 at 12:10:58PM +0800,
Chao Gao <chao.gao@intel.com> wrote:

> >+static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
> >+{
> >+	unsigned long exit_qual;
> >+
> >+	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
> >+		/*
> >+		 * Always treat SEPT violations as write faults.  Ignore the
> >+		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
> >+		 * TD private pages are always RWX in the SEPT tables,
> >+		 * i.e. they're always mapped writable.  Just as importantly,
> >+		 * treating SEPT violations as write faults is necessary to
> >+		 * avoid COW allocations, which will cause TDAUGPAGE failures
> >+		 * due to aliasing a single HPA to multiple GPAs.
> >+		 */
> >+#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
> >+		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
> >+	} else {
> >+		exit_qual = tdexit_exit_qual(vcpu);
> >+		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
> 
> Unless the CPU has a bug, instruction fetch in TD from shared memory causes a
> #PF. I think you can add a comment for this.

Yes.


> Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.

Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR +
KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.


> >+			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
> >+				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
> >+			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
> >+			vcpu->run->ex.exception = PF_VECTOR;
> >+			vcpu->run->ex.error_code = exit_qual;
> >+			return 0;
> >+		}
> >+	}
> >+
> >+	trace_kvm_page_fault(vcpu, tdexit_gpa(vcpu), exit_qual);
> >+	return __vmx_handle_ept_violation(vcpu, tdexit_gpa(vcpu), exit_qual);
> >+}
> >+
> >+static int tdx_handle_ept_misconfig(struct kvm_vcpu *vcpu)
> >+{
> >+	WARN_ON_ONCE(1);
> >+
> >+	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> >+	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
> >+	vcpu->run->internal.ndata = 2;
> >+	vcpu->run->internal.data[0] = EXIT_REASON_EPT_MISCONFIG;
> >+	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
> >+
> >+	return 0;
> >+}
> >+
> > int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
> > {
> > 	union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;
> >@@ -1345,6 +1390,10 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
> > 	WARN_ON_ONCE(fastpath != EXIT_FASTPATH_NONE);
> > 
> > 	switch (exit_reason.basic) {
> >+	case EXIT_REASON_EPT_VIOLATION:
> >+		return tdx_handle_ept_violation(vcpu);
> >+	case EXIT_REASON_EPT_MISCONFIG:
> >+		return tdx_handle_ept_misconfig(vcpu);
> 
> Handling EPT misconfiguration can be dropped because the "default" case handles
> all unexpected exits in the same way

Ah, right. Will update it.

Reinette Chatre April 30, 2024, 8:47 p.m. UTC | #4

Hi Isaku,

On 4/3/2024 11:42 AM, Isaku Yamahata wrote:
> On Mon, Apr 01, 2024 at 12:10:58PM +0800,
> Chao Gao <chao.gao@intel.com> wrote:
> 
>>> +static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
>>> +{
>>> +	unsigned long exit_qual;
>>> +
>>> +	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
>>> +		/*
>>> +		 * Always treat SEPT violations as write faults.  Ignore the
>>> +		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
>>> +		 * TD private pages are always RWX in the SEPT tables,
>>> +		 * i.e. they're always mapped writable.  Just as importantly,
>>> +		 * treating SEPT violations as write faults is necessary to
>>> +		 * avoid COW allocations, which will cause TDAUGPAGE failures
>>> +		 * due to aliasing a single HPA to multiple GPAs.
>>> +		 */
>>> +#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
>>> +		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
>>> +	} else {
>>> +		exit_qual = tdexit_exit_qual(vcpu);
>>> +		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
>>
>> Unless the CPU has a bug, instruction fetch in TD from shared memory causes a
>> #PF. I think you can add a comment for this.
> 
> Yes.
> 
> 
>> Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.
> 
> Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR +
> KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.
> 

Is below what you have in mind?

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 499c6cd9633f..bd30b4c4d710 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1305,11 +1305,18 @@ static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
 	} else {
 		exit_qual = tdexit_exit_qual(vcpu);
 		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
+			/*
+			 * Instruction fetch in TD from shared memory
+			 * causes a #PF.
+			 */
 			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
 				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
-			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
-			vcpu->run->ex.exception = PF_VECTOR;
-			vcpu->run->ex.error_code = exit_qual;
+			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+			vcpu->run->internal.suberror =
+				KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
+			vcpu->run->internal.ndata = 2;
+			vcpu->run->internal.data[0] = EXIT_REASON_EPT_VIOLATION;
+			vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
 			return 0;
 		}
 	}

Thank you

Reinette

Isaku Yamahata May 1, 2024, 3:56 p.m. UTC | #5

On Tue, Apr 30, 2024 at 01:47:07PM -0700,
Reinette Chatre <reinette.chatre@intel.com> wrote:

> Hi Isaku,
> 
> On 4/3/2024 11:42 AM, Isaku Yamahata wrote:
> > On Mon, Apr 01, 2024 at 12:10:58PM +0800,
> > Chao Gao <chao.gao@intel.com> wrote:
> > 
> >>> +static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
> >>> +{
> >>> +	unsigned long exit_qual;
> >>> +
> >>> +	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
> >>> +		/*
> >>> +		 * Always treat SEPT violations as write faults.  Ignore the
> >>> +		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
> >>> +		 * TD private pages are always RWX in the SEPT tables,
> >>> +		 * i.e. they're always mapped writable.  Just as importantly,
> >>> +		 * treating SEPT violations as write faults is necessary to
> >>> +		 * avoid COW allocations, which will cause TDAUGPAGE failures
> >>> +		 * due to aliasing a single HPA to multiple GPAs.
> >>> +		 */
> >>> +#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
> >>> +		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
> >>> +	} else {
> >>> +		exit_qual = tdexit_exit_qual(vcpu);
> >>> +		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
> >>
> >> Unless the CPU has a bug, instruction fetch in TD from shared memory causes a
> >> #PF. I think you can add a comment for this.
> > 
> > Yes.
> > 
> > 
> >> Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.
> > 
> > Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR +
> > KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.
> > 
> 
> Is below what you have in mind?

Yes. data[0] should be the raw value of exit reason if possible.
data[2] should be exit_qual.  Hmm, I don't find document on data[] for
KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON.
Qemu doesn't assumt ndata = 2. Just report all data within ndata.


> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 499c6cd9633f..bd30b4c4d710 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1305,11 +1305,18 @@ static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
>  	} else {
>  		exit_qual = tdexit_exit_qual(vcpu);
>  		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
> +			/*
> +			 * Instruction fetch in TD from shared memory
> +			 * causes a #PF.
> +			 */
>  			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
>  				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
> -			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
> -			vcpu->run->ex.exception = PF_VECTOR;
> -			vcpu->run->ex.error_code = exit_qual;
> +			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +			vcpu->run->internal.suberror =
> +				KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
> +			vcpu->run->internal.ndata = 2;
> +			vcpu->run->internal.data[0] = EXIT_REASON_EPT_VIOLATION;
> +			vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
>  			return 0;
>  		}
>  	}
> 
> Thank you
> 
> Reinette
> 
> 
>

Reinette Chatre May 1, 2024, 4:54 p.m. UTC | #6

Hi Isaku,

On 5/1/2024 8:56 AM, Isaku Yamahata wrote:
> On Tue, Apr 30, 2024 at 01:47:07PM -0700,
> Reinette Chatre <reinette.chatre@intel.com> wrote:
>> On 4/3/2024 11:42 AM, Isaku Yamahata wrote:
>>> On Mon, Apr 01, 2024 at 12:10:58PM +0800,
>>> Chao Gao <chao.gao@intel.com> wrote:
>>>
>>>>> +static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	unsigned long exit_qual;
>>>>> +
>>>>> +	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
>>>>> +		/*
>>>>> +		 * Always treat SEPT violations as write faults.  Ignore the
>>>>> +		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
>>>>> +		 * TD private pages are always RWX in the SEPT tables,
>>>>> +		 * i.e. they're always mapped writable.  Just as importantly,
>>>>> +		 * treating SEPT violations as write faults is necessary to
>>>>> +		 * avoid COW allocations, which will cause TDAUGPAGE failures
>>>>> +		 * due to aliasing a single HPA to multiple GPAs.
>>>>> +		 */
>>>>> +#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
>>>>> +		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
>>>>> +	} else {
>>>>> +		exit_qual = tdexit_exit_qual(vcpu);
>>>>> +		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
>>>>
>>>> Unless the CPU has a bug, instruction fetch in TD from shared memory causes a
>>>> #PF. I think you can add a comment for this.
>>>
>>> Yes.
>>>
>>>
>>>> Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.
>>>
>>> Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR +
>>> KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.
>>>
>>
>> Is below what you have in mind?
> 
> Yes. data[0] should be the raw value of exit reason if possible.
> data[2] should be exit_qual.  Hmm, I don't find document on data[] for

Did you perhaps intend to write "data[1] should be exit_qual" or would you
like to see ndata = 3? I followed existing usages, for example [1] and [2],
that have ndata = 2 with "data[1] = vcpu->arch.last_vmentry_cpu".

> KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON.
> Qemu doesn't assumt ndata = 2. Just report all data within ndata.

I am not sure I interpreted your response correctly so I share one possible
snippet below as I interpret it. Could you please check where I misinterpreted
you? I could also make ndata = 3 to break the existing custom and add
"data[2] = vcpu->arch.last_vmentry_cpu" to match existing pattern. What do you
think?

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 499c6cd9633f..ba81e6f68c97 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1305,11 +1305,20 @@ static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
 	} else {
 		exit_qual = tdexit_exit_qual(vcpu);
 		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
+			union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;
+
+			/*
+			 * Instruction fetch in TD from shared memory
+			 * causes a #PF.
+			 */
 			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
 				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
-			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
-			vcpu->run->ex.exception = PF_VECTOR;
-			vcpu->run->ex.error_code = exit_qual;
+			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+			vcpu->run->internal.suberror =
+				KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
+			vcpu->run->internal.ndata = 2;
+			vcpu->run->internal.data[0] = exit_reason.full;
+			vcpu->run->internal.data[1] = exit_qual;
 			return 0;
 		}
 	}

Reinette

[1] https://github.com/kvm-x86/linux/blob/next/arch/x86/kvm/vmx/vmx.c#L6587
[2] https://github.com/kvm-x86/linux/blob/next/arch/x86/kvm/svm/svm.c#L3436

Isaku Yamahata May 1, 2024, 6:19 p.m. UTC | #7

On Wed, May 01, 2024 at 09:54:07AM -0700,
Reinette Chatre <reinette.chatre@intel.com> wrote:

> Hi Isaku,
> 
> On 5/1/2024 8:56 AM, Isaku Yamahata wrote:
> > On Tue, Apr 30, 2024 at 01:47:07PM -0700,
> > Reinette Chatre <reinette.chatre@intel.com> wrote:
> >> On 4/3/2024 11:42 AM, Isaku Yamahata wrote:
> >>> On Mon, Apr 01, 2024 at 12:10:58PM +0800,
> >>> Chao Gao <chao.gao@intel.com> wrote:
> >>>
> >>>>> +static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
> >>>>> +{
> >>>>> +	unsigned long exit_qual;
> >>>>> +
> >>>>> +	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
> >>>>> +		/*
> >>>>> +		 * Always treat SEPT violations as write faults.  Ignore the
> >>>>> +		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
> >>>>> +		 * TD private pages are always RWX in the SEPT tables,
> >>>>> +		 * i.e. they're always mapped writable.  Just as importantly,
> >>>>> +		 * treating SEPT violations as write faults is necessary to
> >>>>> +		 * avoid COW allocations, which will cause TDAUGPAGE failures
> >>>>> +		 * due to aliasing a single HPA to multiple GPAs.
> >>>>> +		 */
> >>>>> +#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
> >>>>> +		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
> >>>>> +	} else {
> >>>>> +		exit_qual = tdexit_exit_qual(vcpu);
> >>>>> +		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
> >>>>
> >>>> Unless the CPU has a bug, instruction fetch in TD from shared memory causes a
> >>>> #PF. I think you can add a comment for this.
> >>>
> >>> Yes.
> >>>
> >>>
> >>>> Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.
> >>>
> >>> Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR +
> >>> KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.
> >>>
> >>
> >> Is below what you have in mind?
> > 
> > Yes. data[0] should be the raw value of exit reason if possible.
> > data[2] should be exit_qual.  Hmm, I don't find document on data[] for
> 
> Did you perhaps intend to write "data[1] should be exit_qual" or would you
> like to see ndata = 3? I followed existing usages, for example [1] and [2],
> that have ndata = 2 with "data[1] = vcpu->arch.last_vmentry_cpu".
> 
> > KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON.
> > Qemu doesn't assumt ndata = 2. Just report all data within ndata.
> 
> I am not sure I interpreted your response correctly so I share one possible
> snippet below as I interpret it. Could you please check where I misinterpreted
> you? I could also make ndata = 3 to break the existing custom and add
> "data[2] = vcpu->arch.last_vmentry_cpu" to match existing pattern. What do you
> think?
> 
Sorry, I wasn't clear enough. I meant
  ndata = 3;
  data[0] = exit_reason.full;
  data[1] = vcpu->arch.last_vmentry_cpu;
  data[2] = exit_qual;

Because I hesitate to change the meaning of data[1] from other usage, I
appended exit_qual as data[2].

Reinette Chatre May 1, 2024, 6:22 p.m. UTC | #8

Hi Isaku,

On 5/1/2024 11:19 AM, Isaku Yamahata wrote:
> On Wed, May 01, 2024 at 09:54:07AM -0700,
> Reinette Chatre <reinette.chatre@intel.com> wrote:
> 
>> Hi Isaku,
>>
>> On 5/1/2024 8:56 AM, Isaku Yamahata wrote:
>>> On Tue, Apr 30, 2024 at 01:47:07PM -0700,
>>> Reinette Chatre <reinette.chatre@intel.com> wrote:
>>>> On 4/3/2024 11:42 AM, Isaku Yamahata wrote:
>>>>> On Mon, Apr 01, 2024 at 12:10:58PM +0800,
>>>>> Chao Gao <chao.gao@intel.com> wrote:
>>>>>
>>>>>>> +static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
>>>>>>> +{
>>>>>>> +	unsigned long exit_qual;
>>>>>>> +
>>>>>>> +	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
>>>>>>> +		/*
>>>>>>> +		 * Always treat SEPT violations as write faults.  Ignore the
>>>>>>> +		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
>>>>>>> +		 * TD private pages are always RWX in the SEPT tables,
>>>>>>> +		 * i.e. they're always mapped writable.  Just as importantly,
>>>>>>> +		 * treating SEPT violations as write faults is necessary to
>>>>>>> +		 * avoid COW allocations, which will cause TDAUGPAGE failures
>>>>>>> +		 * due to aliasing a single HPA to multiple GPAs.
>>>>>>> +		 */
>>>>>>> +#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
>>>>>>> +		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
>>>>>>> +	} else {
>>>>>>> +		exit_qual = tdexit_exit_qual(vcpu);
>>>>>>> +		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
>>>>>>
>>>>>> Unless the CPU has a bug, instruction fetch in TD from shared memory causes a
>>>>>> #PF. I think you can add a comment for this.
>>>>>
>>>>> Yes.
>>>>>
>>>>>
>>>>>> Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.
>>>>>
>>>>> Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR +
>>>>> KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.
>>>>>
>>>>
>>>> Is below what you have in mind?
>>>
>>> Yes. data[0] should be the raw value of exit reason if possible.
>>> data[2] should be exit_qual.  Hmm, I don't find document on data[] for
>>
>> Did you perhaps intend to write "data[1] should be exit_qual" or would you
>> like to see ndata = 3? I followed existing usages, for example [1] and [2],
>> that have ndata = 2 with "data[1] = vcpu->arch.last_vmentry_cpu".
>>
>>> KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON.
>>> Qemu doesn't assumt ndata = 2. Just report all data within ndata.
>>
>> I am not sure I interpreted your response correctly so I share one possible
>> snippet below as I interpret it. Could you please check where I misinterpreted
>> you? I could also make ndata = 3 to break the existing custom and add
>> "data[2] = vcpu->arch.last_vmentry_cpu" to match existing pattern. What do you
>> think?
>>
> Sorry, I wasn't clear enough. I meant
>   ndata = 3;
>   data[0] = exit_reason.full;
>   data[1] = vcpu->arch.last_vmentry_cpu;
>   data[2] = exit_qual;
> 
> Because I hesitate to change the meaning of data[1] from other usage, I
> appended exit_qual as data[2].

I understand it now. Thank you very much Isaku.

Reinette

Chao Gao May 6, 2024, 7:21 a.m. UTC | #9

On Wed, May 01, 2024 at 09:54:07AM -0700, Reinette Chatre wrote:
>diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>index 499c6cd9633f..ba81e6f68c97 100644
>--- a/arch/x86/kvm/vmx/tdx.c
>+++ b/arch/x86/kvm/vmx/tdx.c
>@@ -1305,11 +1305,20 @@ static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
> 	} else {
> 		exit_qual = tdexit_exit_qual(vcpu);
> 		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
>+			union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;
>+
>+			/*
>+			 * Instruction fetch in TD from shared memory
>+			 * causes a #PF.
>+			 */

It is better to hoist this comment above the if-statement.

		/*
		 * Instruction fetch in TD from shared memory never causes EPT
		 * violation. Warn if such an EPT violation occurs as the CPU
		 * probably is buggy.
		 */
 		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
		...
		}


but, I am wondering why KVM needs to perform this check. I prefer to drop this
check if KVM doesn't rely on it to handle EPT violation correctly.

> 			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
> 				tdexit_gpa(vcpu), kvm_rip_read(vcpu));

how about using vcpu_unimpl()? it is a wrapper for printing such a message.

>-			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
>-			vcpu->run->ex.exception = PF_VECTOR;
>-			vcpu->run->ex.error_code = exit_qual;
>+			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>+			vcpu->run->internal.suberror =
>+				KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
>+			vcpu->run->internal.ndata = 2;
>+			vcpu->run->internal.data[0] = exit_reason.full;
>+			vcpu->run->internal.data[1] = exit_qual;
> 			return 0;
> 		}
> 	}
>
>Reinette
>
>[1] https://github.com/kvm-x86/linux/blob/next/arch/x86/kvm/vmx/vmx.c#L6587
>[2] https://github.com/kvm-x86/linux/blob/next/arch/x86/kvm/svm/svm.c#L3436

Sean Christopherson May 6, 2024, 2:21 p.m. UTC | #10

On Mon, May 06, 2024, Chao Gao wrote:
> On Wed, May 01, 2024 at 09:54:07AM -0700, Reinette Chatre wrote:
> >diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> >index 499c6cd9633f..ba81e6f68c97 100644
> >--- a/arch/x86/kvm/vmx/tdx.c
> >+++ b/arch/x86/kvm/vmx/tdx.c
> >@@ -1305,11 +1305,20 @@ static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
> > 	} else {
> > 		exit_qual = tdexit_exit_qual(vcpu);
> > 		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
> >+			union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;
> >+
> >+			/*
> >+			 * Instruction fetch in TD from shared memory
> >+			 * causes a #PF.
> >+			 */
> 
> It is better to hoist this comment above the if-statement.
> 
> 		/*
> 		 * Instruction fetch in TD from shared memory never causes EPT
> 		 * violation. Warn if such an EPT violation occurs as the CPU
> 		 * probably is buggy.
> 		 */
>  		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
> 		...
> 		}
> 
> 
> but, I am wondering why KVM needs to perform this check. I prefer to drop this
> check if KVM doesn't rely on it to handle EPT violation correctly.
> 
> > 			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
> > 				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
> 
> how about using vcpu_unimpl()? it is a wrapper for printing such a message.

This isn't an "unimplemented" scenario in KVM, this is a "hardware is broken"
scenario.  Just WARN_ON_ONCE() and move on.  Or I suppose since EPT misconfig
needs to do something as well:

		if (KVM_BUG_ON(exit_qual & EPT_VIOLATION_ACC_INSTR, vcpu->kvm))
			return -EIO;

Sean Christopherson May 6, 2024, 2:22 p.m. UTC | #11

On Mon, Feb 26, 2024, isaku.yamahata@intel.com wrote:
> +static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
> +{
> +	unsigned long exit_qual;
> +
> +	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
> +		/*
> +		 * Always treat SEPT violations as write faults.  Ignore the
> +		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
> +		 * TD private pages are always RWX in the SEPT tables,
> +		 * i.e. they're always mapped writable.  Just as importantly,
> +		 * treating SEPT violations as write faults is necessary to
> +		 * avoid COW allocations, which will cause TDAUGPAGE failures
> +		 * due to aliasing a single HPA to multiple GPAs.
> +		 */
> +#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE

This does not needd a #define.  It's use in literally one place, one line below.

[v19,101/130] KVM: TDX: handle ept violation/misconfig exit

Commit Message

Comments

Patch