[PATCH v3 0/7] Enhance event delivery error handling

[PATCH v3 0/7] Enhance event delivery error handling

[Ivan Orlov]

Currently, the unhandleable vectoring (e.g. when guest accesses MMIO
during vectoring) is handled differently on VMX and SVM: on VMX KVM
returns internal error, when SVM goes into infinite loop trying to
deliver an event again and again.

This patch series eliminates this difference by returning a KVM internal
error when KVM can't emulate during vectoring for both VMX and SVM.

Also, introduce a selftest test case which covers the error handling
mentioned above.

V1 -> V2:
- Make commit messages more brief, avoid using pronouns
- Extract SVM error handling into a separate commit
- Introduce a new X86EMUL_ return type and detect the unhandleable
vectoring error in vendor-specific check_emulate_instruction instead of
handling it in the common MMU code (which is specific for cached MMIO)

V2 -> V3:
- Make the new X86EMUL_ code more generic
- Prohibit any emulation during vectoring if it is due to an intercepted
#PF
- Add a new patch for checking whether unprotect & retry is possible
before exiting to userspace due to unhandleable vectoring
- Codestyle fixes


Ivan Orlov (7):
  KVM: x86: Add function for vectoring error generation
  KVM: x86: Add emulation status for unhandleable vectoring
  KVM: x86: Unprotect & retry before unhandleable vectoring check
  KVM: VMX: Handle vectoring error in check_emulate_instruction
  KVM: SVM: Handle vectoring error in check_emulate_instruction
  selftests: KVM: extract lidt into helper function
  selftests: KVM: Add test case for MMIO during vectoring

 arch/x86/include/asm/kvm_host.h               | 11 +++-
 arch/x86/kvm/kvm_emulate.h                    |  2 +
 arch/x86/kvm/svm/svm.c                        |  6 +++
 arch/x86/kvm/vmx/vmx.c                        | 30 ++++-------
 arch/x86/kvm/x86.c                            | 31 +++++++++++
 .../selftests/kvm/include/x86_64/processor.h  |  7 +++
 .../selftests/kvm/set_memory_region_test.c    | 53 ++++++++++++++++++-
 .../selftests/kvm/x86_64/sev_smoke_test.c     |  2 +-
 8 files changed, 117 insertions(+), 25 deletions(-)

-- 
2.43.0

[PATCH v3 1/7] KVM: x86: Add function for vectoring error generation

[Ivan Orlov]

Extract VMX code for unhandleable VM-Exit during vectoring into
vendor-agnostic function so that boiler-plate code can be shared by SVM.

Report an actual GPA for EPT misconfig or invalid GPA for any other exit
code in internal.data[3].

Signed-off-by: Ivan Orlov <iorlov@amazon.com>
---
V1 -> V2:
- Return GPA for any exit reason, using reported GPA when it is valid or
INVALID_GPA otherwise.
- Rename the error preparation function
- Fix indentation
V2 -> V3:
- Move 'gpa' declaration into block it is used in
- Use 'if' instead of ternary operator when reporting GPA
- Remove redundant cast

 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/vmx/vmx.c          | 17 +++++------------
 arch/x86/kvm/x86.c              | 22 ++++++++++++++++++++++
 3 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e159e44a6a1b..de8fb1ab230c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2062,6 +2062,8 @@ void __kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu,
 					  u64 *data, u8 ndata);
 void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu);
 
+void kvm_prepare_event_vectoring_exit(struct kvm_vcpu *vcpu, gpa_t gpa);
+
 void kvm_enable_efer_bits(u64);
 bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer);
 int kvm_get_msr_with_filter(struct kvm_vcpu *vcpu, u32 index, u64 *data);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 893366e53732..acc2f0e0a339 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6557,19 +6557,12 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 	     exit_reason.basic != EXIT_REASON_APIC_ACCESS &&
 	     exit_reason.basic != EXIT_REASON_TASK_SWITCH &&
 	     exit_reason.basic != EXIT_REASON_NOTIFY)) {
-		int ndata = 3;
+		gpa_t gpa = INVALID_GPA;
 
-		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
-		vcpu->run->internal.data[0] = vectoring_info;
-		vcpu->run->internal.data[1] = exit_reason.full;
-		vcpu->run->internal.data[2] = vmx_get_exit_qual(vcpu);
-		if (exit_reason.basic == EXIT_REASON_EPT_MISCONFIG) {
-			vcpu->run->internal.data[ndata++] =
-				vmcs_read64(GUEST_PHYSICAL_ADDRESS);
-		}
-		vcpu->run->internal.data[ndata++] = vcpu->arch.last_vmentry_cpu;
-		vcpu->run->internal.ndata = ndata;
+		if (exit_reason.basic == EXIT_REASON_EPT_MISCONFIG)
+			gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
+
+		kvm_prepare_event_vectoring_exit(vcpu, gpa);
 		return 0;
 	}
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2e713480933a..7ce9cdb66f19 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8813,6 +8813,28 @@ void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_prepare_emulation_failure_exit);
 
+void kvm_prepare_event_vectoring_exit(struct kvm_vcpu *vcpu, gpa_t gpa)
+{
+	u32 reason, intr_info, error_code;
+	struct kvm_run *run = vcpu->run;
+	u64 info1, info2;
+	int ndata = 0;
+
+	kvm_x86_call(get_exit_info)(vcpu, &reason, &info1, &info2,
+				    &intr_info, &error_code);
+
+	run->internal.data[ndata++] = info2;
+	run->internal.data[ndata++] = reason;
+	run->internal.data[ndata++] = info1;
+	run->internal.data[ndata++] = gpa;
+	run->internal.data[ndata++] = vcpu->arch.last_vmentry_cpu;
+
+	run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+	run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
+	run->internal.ndata = ndata;
+}
+EXPORT_SYMBOL_GPL(kvm_prepare_event_vectoring_exit);
+
 static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
 {
 	struct kvm *kvm = vcpu->kvm;
-- 
2.43.0

[PATCH v3 2/7] KVM: x86: Add emulation status for unhandleable vectoring

[Ivan Orlov]

Add emulation status for unhandleable vectoring, i.e. when KVM can't
emulate an instruction during vectoring. Such a situation can occur
if guest sets the IDT descriptor base to point to MMIO region, and
triggers an exception after that.

Exit to userspace with event delivery error when KVM can't emulate
an instruction when vectoring an event.

Signed-off-by: Ivan Orlov <iorlov@amazon.com>
---
V1 -> V2:
- This patch wasn't included in V1.
V2 -> V3:
- Make new X86EMUL_ code more generic to allow using it for any type
of unhandleable vectoring

 arch/x86/kvm/kvm_emulate.h | 2 ++
 arch/x86/kvm/x86.c         | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 10495fffb890..5bcf50ffc3de 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -88,6 +88,8 @@ struct x86_instruction_info {
 #define X86EMUL_CMPXCHG_FAILED  4 /* cmpxchg did not see expected value */
 #define X86EMUL_IO_NEEDED       5 /* IO is needed to complete emulation */
 #define X86EMUL_INTERCEPTED     6 /* Intercepted by nested VMCB/VMCS */
+/* Vectroing can't be emulated */
+#define X86EMUL_UNHANDLEABLE_VECTORING	7
 
 /* x86-specific emulation flags */
 #define X86EMUL_F_WRITE			BIT(0)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7ce9cdb66f19..849a6fc364b3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9107,6 +9107,11 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 		if (r == X86EMUL_RETRY_INSTR || r == X86EMUL_PROPAGATE_FAULT)
 			return 1;
 
+		if (r == X86EMUL_UNHANDLEABLE_VECTORING) {
+			kvm_prepare_event_vectoring_exit(vcpu, cr2_or_gpa);
+			return 0;
+		}
+
 		WARN_ON_ONCE(r != X86EMUL_UNHANDLEABLE);
 		return handle_emulation_failure(vcpu, emulation_type);
 	}
-- 
2.43.0

Re: [PATCH v3 2/7] KVM: x86: Add emulation status for unhandleable vectoring

[Sean Christopherson]

On Tue, Dec 17, 2024, Ivan Orlov wrote:
> Add emulation status for unhandleable vectoring, i.e. when KVM can't
> emulate an instruction during vectoring. Such a situation can occur
> if guest sets the IDT descriptor base to point to MMIO region, and
> triggers an exception after that.
> 
> Exit to userspace with event delivery error when KVM can't emulate
> an instruction when vectoring an event.
> 
> Signed-off-by: Ivan Orlov <iorlov@amazon.com>
> ---
> V1 -> V2:
> - This patch wasn't included in V1.
> V2 -> V3:
> - Make new X86EMUL_ code more generic to allow using it for any type
> of unhandleable vectoring
> 
>  arch/x86/kvm/kvm_emulate.h | 2 ++
>  arch/x86/kvm/x86.c         | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> index 10495fffb890..5bcf50ffc3de 100644
> --- a/arch/x86/kvm/kvm_emulate.h
> +++ b/arch/x86/kvm/kvm_emulate.h
> @@ -88,6 +88,8 @@ struct x86_instruction_info {
>  #define X86EMUL_CMPXCHG_FAILED  4 /* cmpxchg did not see expected value */
>  #define X86EMUL_IO_NEEDED       5 /* IO is needed to complete emulation */
>  #define X86EMUL_INTERCEPTED     6 /* Intercepted by nested VMCB/VMCS */
> +/* Vectroing can't be emulated */

Typo.  I think it's also worth elaborating a bit, e.g.

/* Emulation during event vectoring is unsupported */

> +#define X86EMUL_UNHANDLEABLE_VECTORING	7
>  
>  /* x86-specific emulation flags */
>  #define X86EMUL_F_WRITE			BIT(0)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7ce9cdb66f19..849a6fc364b3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9107,6 +9107,11 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  		if (r == X86EMUL_RETRY_INSTR || r == X86EMUL_PROPAGATE_FAULT)
>  			return 1;
>  
> +		if (r == X86EMUL_UNHANDLEABLE_VECTORING) {
> +			kvm_prepare_event_vectoring_exit(vcpu, cr2_or_gpa);
> +			return 0;
> +		}
> +
>  		WARN_ON_ONCE(r != X86EMUL_UNHANDLEABLE);
>  		return handle_emulation_failure(vcpu, emulation_type);
>  	}
> -- 
> 2.43.0
> 

[PATCH v3 3/7] KVM: x86: Unprotect & retry before unhandleable vectoring check

[Ivan Orlov]

Try to unprotect and retry the instruction execution before checking for
unhandleable vectoring. If there is a write to a shadowed page table
when vectoring an event, KVM should be able to unprotect the gfn and
retry the instruction execution without returning an error to userspace.

This ensures that the subsequent patches won't make KVM exit to
userspace when handling an intercepted #PF during vectoring without
checking whether unprotect & retry is possible.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Ivan Orlov <iorlov@amazon.com>
---
V1 -> V2:
- This patch wasn't included in V1.
V2 -> V3:
- This patch wasn't included in V2.

 arch/x86/kvm/x86.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 849a6fc364b3..26faacc99c4c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9107,6 +9107,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 		if (r == X86EMUL_RETRY_INSTR || r == X86EMUL_PROPAGATE_FAULT)
 			return 1;
 
+		if (kvm_unprotect_and_retry_on_failure(vcpu, cr2_or_gpa,
+						       emulation_type))
+			return 1;
+
 		if (r == X86EMUL_UNHANDLEABLE_VECTORING) {
 			kvm_prepare_event_vectoring_exit(vcpu, cr2_or_gpa);
 			return 0;
-- 
2.43.0

[PATCH v3 4/7] KVM: VMX: Handle vectoring error in check_emulate_instruction

[Ivan Orlov]

Move unhandleable vmexit during vectoring error detection
into check_emulate_instruction. Implement the function which prohibits
the emulation if EMULTYPE_PF is set when vectoring, otherwise such a
situation may occur:

1. CPU executes an instruction and hits a #GP
2. While vectoring the #GP, a shadow #PF occurs
3. On vmexit, KVM re-injects #GP
4. KVM emulates because of the write-protected page
5. KVM "successfully" emulates and also detects the #GP
6. KVM synthesizes a #GP, and since #GP has already been injected,
incorrectly escalates to a #DF.

Fix the comment about EMULTYPE_PF as this flag doesn't necessarily
mean MMIO anymore: it can also be set due to the write protection
violation.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Ivan Orlov <iorlov@amazon.com>
---
V1 -> V2:
- Detect the unhandleable vectoring error in vmx_check_emulate_instruction
instead of handling it in the common MMU code (which is specific for
cached MMIO)
V2 -> V3:
- Prohibit any emulation during vectoring if it happens due to an
intercepted #PF.

 arch/x86/include/asm/kvm_host.h |  9 +++++++--
 arch/x86/kvm/vmx/vmx.c          | 23 +++++++++--------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index de8fb1ab230c..f3a1d050e1d6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2019,8 +2019,8 @@ u64 vcpu_tsc_khz(struct kvm_vcpu *vcpu);
  *			VMware backdoor emulation handles select instructions
  *			and reinjects the #GP for all other cases.
  *
- * EMULTYPE_PF - Set when emulating MMIO by way of an intercepted #PF, in which
- *		 case the CR2/GPA value pass on the stack is valid.
+ * EMULTYPE_PF - Set when an intercepted #PF triggers the emulation, in which case
+ *		 the CR2/GPA value pass on the stack is valid.
  *
  * EMULTYPE_COMPLETE_USER_EXIT - Set when the emulator should update interruptibility
  *				 state and inject single-step #DBs after skipping
@@ -2055,6 +2055,11 @@ u64 vcpu_tsc_khz(struct kvm_vcpu *vcpu);
 #define EMULTYPE_COMPLETE_USER_EXIT (1 << 7)
 #define EMULTYPE_WRITE_PF_TO_SP	    (1 << 8)
 
+static inline bool kvm_can_emulate_event_vectoring(int emul_type)
+{
+	return !(emul_type & EMULTYPE_PF);
+}
+
 int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
 int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
 					void *insn, int insn_len);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index acc2f0e0a339..89ddbe1175c7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1705,6 +1705,12 @@ int vmx_check_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type,
 		kvm_queue_exception(vcpu, UD_VECTOR);
 		return X86EMUL_PROPAGATE_FAULT;
 	}
+
+	/* Check that emulation is possible during event vectoring */
+	if ((to_vmx(vcpu)->idt_vectoring_info & VECTORING_INFO_VALID_MASK) &&
+	    !kvm_can_emulate_event_vectoring(emul_type))
+		return X86EMUL_UNHANDLEABLE_VECTORING;
+
 	return X86EMUL_CONTINUE;
 }
 
@@ -6543,26 +6549,15 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 		return 0;
 	}
 
-	/*
-	 * Note:
-	 * Do not try to fix EXIT_REASON_EPT_MISCONFIG if it caused by
-	 * delivery event since it indicates guest is accessing MMIO.
-	 * The vm-exit can be triggered again after return to guest that
-	 * will cause infinite loop.
-	 */
 	if ((vectoring_info & VECTORING_INFO_VALID_MASK) &&
 	    (exit_reason.basic != EXIT_REASON_EXCEPTION_NMI &&
 	     exit_reason.basic != EXIT_REASON_EPT_VIOLATION &&
 	     exit_reason.basic != EXIT_REASON_PML_FULL &&
 	     exit_reason.basic != EXIT_REASON_APIC_ACCESS &&
 	     exit_reason.basic != EXIT_REASON_TASK_SWITCH &&
-	     exit_reason.basic != EXIT_REASON_NOTIFY)) {
-		gpa_t gpa = INVALID_GPA;
-
-		if (exit_reason.basic == EXIT_REASON_EPT_MISCONFIG)
-			gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
-
-		kvm_prepare_event_vectoring_exit(vcpu, gpa);
+	     exit_reason.basic != EXIT_REASON_NOTIFY &&
+	     exit_reason.basic != EXIT_REASON_EPT_MISCONFIG)) {
+		kvm_prepare_event_vectoring_exit(vcpu, INVALID_GPA);
 		return 0;
 	}
 
-- 
2.43.0

Re: [PATCH v3 4/7] KVM: VMX: Handle vectoring error in check_emulate_instruction

[Sean Christopherson]

On Tue, Dec 17, 2024, Ivan Orlov wrote:
> Move unhandleable vmexit during vectoring error detection
> into check_emulate_instruction. Implement the function which prohibits
> the emulation if EMULTYPE_PF is set when vectoring, otherwise such a
> situation may occur:

I definitely think it's worth explaining that moving the detection covers new
emulation cases, and also calling out that handle_ept_misconfig() consults
vmx_check_emulate_instruction(), i.e. that moving the detection shouldn't
affect KVM's overall handlng of EPT Misconfig.

--

Move handling of emulation during event vectoring, which KVM doesn't
support, into VMX's check_emulate_instruction(), so that KVM detects
all unsupported emulation, not just cached emulated MMIO (EPT misconfig).
E.g. on emulated MMIO that isn't cached (EPT Violation) or occurs with
legacy shadow paging (#PF).

Rejecting emulation on other sources of emulation also fixes a largely
theoretical flaw (thanks to the "unprotect and retry" logic), where KVM
could incorrectly inject a #DF:

  1. CPU executes an instruction and hits a #GP
  2. While vectoring the #GP, a shadow #PF occurs
  3. On the #PF VM-Exit, KVM re-injects #GP
  4. KVM emulates because of the write-protected page
  5. KVM "successfully" emulates and also detects the #GP
  6. KVM synthesizes a #GP, and since #GP has already been injected,
     incorrectly escalates to a #DF.

Fix the comment about EMULTYPE_PF as this flag doesn't necessarily
mean MMIO anymore: it can also be set due to the write protection
violation.

Note, handle_ept_misconfig() checks vmx_check_emulate_instruction() before
attempting emulation of any kind.

Re: [PATCH v3 4/7] KVM: VMX: Handle vectoring error in check_emulate_instruction

[Ivan Orlov]

On 12/18/24 18:39, Sean Christopherson wrote:
> I definitely think it's worth explaining that moving the detection covers new
> emulation cases, and also calling out that handle_ept_misconfig() consults
> vmx_check_emulate_instruction(), i.e. that moving the detection shouldn't
> affect KVM's overall handlng of EPT Misconfig.
> 
> --
> 
> Move handling of emulation during event vectoring, which KVM doesn't
> support, into VMX's check_emulate_instruction(), so that KVM detects
> all unsupported emulation, not just cached emulated MMIO (EPT misconfig).
> E.g. on emulated MMIO that isn't cached (EPT Violation) or occurs with
> legacy shadow paging (#PF).
> 
> Rejecting emulation on other sources of emulation also fixes a largely
> theoretical flaw (thanks to the "unprotect and retry" logic), where KVM
> could incorrectly inject a #DF:
> 
>    1. CPU executes an instruction and hits a #GP
>    2. While vectoring the #GP, a shadow #PF occurs
>    3. On the #PF VM-Exit, KVM re-injects #GP
>    4. KVM emulates because of the write-protected page
>    5. KVM "successfully" emulates and also detects the #GP
>    6. KVM synthesizes a #GP, and since #GP has already been injected,
>       incorrectly escalates to a #DF.
> 
> Fix the comment about EMULTYPE_PF as this flag doesn't necessarily
> mean MMIO anymore: it can also be set due to the write protection
> violation.
> 
> Note, handle_ept_misconfig() checks vmx_check_emulate_instruction() before
> attempting emulation of any kind.
> 

Yeah, I thought that covering the change in non-cacheable MMIO / shadow 
paged #PF handling, but forgot to include it into the commit message :( 
Could you please fix the message when applying? The message you 
suggested looks good to me.

Thanks!

-- 
Kind regards,
Ivan Orlov

[PATCH v3 5/7] KVM: SVM: Handle vectoring error in check_emulate_instruction

[Ivan Orlov]

Detect unhandleable vectoring in check_emulate_instruction to prevent
infinite loop on SVM and eliminate the difference in how intercepted #PF
during vectoring is handled on SVM and VMX.

Signed-off-by: Ivan Orlov <iorlov@amazon.com>
---
V1 -> V2:
- Detect the unhandleable vectoring error in svm_check_emulate_instruction
instead of handling it in the common MMU code (which is specific for
cached MMIO)
V2 -> V3:
- Use more generic function to check if emulation is allowed when
vectoring

 arch/x86/kvm/svm/svm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index dd15cc635655..e89c6fc2c4e6 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4802,6 +4802,12 @@ static int svm_check_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type,
 	bool smep, smap, is_user;
 	u64 error_code;
 
+	/* Check that emulation is possible during event vectoring */
+	if ((to_svm(vcpu)->vmcb->control.exit_int_info &
+	    SVM_EXITINTINFO_TYPE_MASK) &&
+	    !kvm_can_emulate_event_vectoring(emul_type))
+		return X86EMUL_UNHANDLEABLE_VECTORING;
+
 	/* Emulation is always possible when KVM has access to all guest state. */
 	if (!sev_guest(vcpu->kvm))
 		return X86EMUL_CONTINUE;
-- 
2.43.0

Re: [PATCH v3 5/7] KVM: SVM: Handle vectoring error in check_emulate_instruction

[Sean Christopherson]

On Tue, Dec 17, 2024, Ivan Orlov wrote:
> Detect unhandleable vectoring in check_emulate_instruction to prevent
> infinite loop on SVM and eliminate the difference in how intercepted #PF
> during vectoring is handled on SVM and VMX.
> 
> Signed-off-by: Ivan Orlov <iorlov@amazon.com>
> ---
> V1 -> V2:
> - Detect the unhandleable vectoring error in svm_check_emulate_instruction
> instead of handling it in the common MMU code (which is specific for
> cached MMIO)
> V2 -> V3:
> - Use more generic function to check if emulation is allowed when
> vectoring
> 
>  arch/x86/kvm/svm/svm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index dd15cc635655..e89c6fc2c4e6 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4802,6 +4802,12 @@ static int svm_check_emulate_instruction(struct kvm_vcpu *vcpu, int emul_type,
>  	bool smep, smap, is_user;
>  	u64 error_code;
>  
> +	/* Check that emulation is possible during event vectoring */
> +	if ((to_svm(vcpu)->vmcb->control.exit_int_info &
> +	    SVM_EXITINTINFO_TYPE_MASK) &&

Let this poke out.  Alternatively, and probably preferably, capture "svm" locally
and it fits nicely on one line (there's an existing user of to_svm() in this
helper).  My objection to a local variable was specifically about a local
"is_event_vectoring", not about local variables in general.

> +	    !kvm_can_emulate_event_vectoring(emul_type))
> +		return X86EMUL_UNHANDLEABLE_VECTORING;
> +
>  	/* Emulation is always possible when KVM has access to all guest state. */
>  	if (!sev_guest(vcpu->kvm))
>  		return X86EMUL_CONTINUE;
> -- 
> 2.43.0
> 

[PATCH v3 6/7] selftests: KVM: extract lidt into helper function

[Ivan Orlov]

Implement a function for setting the IDT descriptor from the guest
code. Replace the existing lidt occurrences with calls to this function
as `lidt` is used in multiple places.

Signed-off-by: Ivan Orlov <iorlov@amazon.com>
---
V1 -> V2:
- This patch wasn't included in V1.
V2 -> V3:
- No changes

 tools/testing/selftests/kvm/include/x86_64/processor.h | 5 +++++
 tools/testing/selftests/kvm/set_memory_region_test.c   | 2 +-
 tools/testing/selftests/kvm/x86_64/sev_smoke_test.c    | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 645200e95f89..69938c649a5e 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -571,6 +571,11 @@ static inline void set_cr4(uint64_t val)
 	__asm__ __volatile__("mov %0, %%cr4" : : "r" (val) : "memory");
 }
 
+static inline void set_idt(const struct desc_ptr *idt_desc)
+{
+	__asm__ __volatile__("lidt %0"::"m"(*idt_desc));
+}
+
 static inline u64 xgetbv(u32 index)
 {
 	u32 eax, edx;
diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
index a8267628e9ed..a1c53cc854a5 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -235,7 +235,7 @@ static void guest_code_delete_memory_region(void)
 	 * in the guest will never succeed, and so isn't an option.
 	 */
 	memset(&idt, 0, sizeof(idt));
-	__asm__ __volatile__("lidt %0" :: "m"(idt));
+	set_idt(&idt);
 
 	GUEST_SYNC(0);
 
diff --git a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
index ae77698e6e97..a1a688e75266 100644
--- a/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sev_smoke_test.c
@@ -155,7 +155,7 @@ static void guest_shutdown_code(void)
 
 	/* Clobber the IDT so that #UD is guaranteed to trigger SHUTDOWN. */
 	memset(&idt, 0, sizeof(idt));
-	__asm__ __volatile__("lidt %0" :: "m"(idt));
+	set_idt(&idt);
 
 	__asm__ __volatile__("ud2");
 }
-- 
2.43.0

Re: [PATCH v3 6/7] selftests: KVM: extract lidt into helper function

[Sean Christopherson]

KVM: selftests: is the preferred scope.

[PATCH v3 7/7] selftests: KVM: Add test case for MMIO during vectoring

[Ivan Orlov]

Extend the 'set_memory_region_test' with a test case which covers the
MMIO during vectoring error handling. The test case

1) Sets an IDT descriptor base to point to an MMIO address
2) Generates a #GP in the guest
3) Verifies that we got a correct exit reason and suberror code
4) Verifies that we got a corrent reported GPA in internal.data[3]

Also, add a definition of non-canonical address to processor.h

Signed-off-by: Ivan Orlov <iorlov@amazon.com>
---
V1 -> V2:
- Get rid of pronouns, redundant comments and incorrect wording
- Define noncanonical address in processor.h
- Fix indentation and wrap lines at 80 columns
V2 -> V3:
- Move "NONCANONICAL" definition to the beginning of the file

 .../selftests/kvm/include/x86_64/processor.h  |  2 +
 .../selftests/kvm/set_memory_region_test.c    | 51 +++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 69938c649a5e..6b8d12f042a8 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -29,6 +29,8 @@ extern uint64_t guest_tsc_khz;
 #define MAX_NR_CPUID_ENTRIES 100
 #endif
 
+#define NONCANONICAL 0xaaaaaaaaaaaaaaaaull
+
 /* Forced emulation prefix, used to invoke the emulator unconditionally. */
 #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
 
diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
index a1c53cc854a5..d65a9f20aa1a 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -553,6 +553,56 @@ static void test_add_overlapping_private_memory_regions(void)
 	close(memfd);
 	kvm_vm_free(vm);
 }
+
+static void guest_code_mmio_during_vectoring(void)
+{
+	const struct desc_ptr idt_desc = {
+		.address = MEM_REGION_GPA,
+		.size = 0xFFF,
+	};
+
+	set_idt(&idt_desc);
+
+	/* Generate a #GP by dereferencing a non-canonical address */
+	*((uint8_t *)NONCANONICAL) = 0x1;
+
+	GUEST_ASSERT(0);
+}
+
+/*
+ * This test points the IDT descriptor base to an MMIO address. It should cause
+ * a KVM internal error when an event occurs in the guest.
+ */
+static void test_mmio_during_vectoring(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_run *run;
+	struct kvm_vm *vm;
+	u64 expected_gpa;
+
+	pr_info("Testing MMIO during vectoring error handling\n");
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code_mmio_during_vectoring);
+	virt_map(vm, MEM_REGION_GPA, MEM_REGION_GPA, 1);
+
+	run = vcpu->run;
+
+	vcpu_run(vcpu);
+	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_INTERNAL_ERROR);
+	TEST_ASSERT(run->internal.suberror == KVM_INTERNAL_ERROR_DELIVERY_EV,
+		    "Unexpected suberror = %d", vcpu->run->internal.suberror);
+	TEST_ASSERT(run->internal.ndata != 4, "Unexpected internal error data array size = %d",
+		    run->internal.ndata);
+
+	/* The reported GPA should be IDT base + offset of the GP vector */
+	expected_gpa = MEM_REGION_GPA + GP_VECTOR * sizeof(struct idt_entry);
+
+	TEST_ASSERT(run->internal.data[3] == expected_gpa,
+		    "Unexpected GPA = %llx (expected %lx)",
+		    vcpu->run->internal.data[3], expected_gpa);
+
+	kvm_vm_free(vm);
+}
 #endif
 
 int main(int argc, char *argv[])
@@ -568,6 +618,7 @@ int main(int argc, char *argv[])
 	 * KVM_RUN fails with ENOEXEC or EFAULT.
 	 */
 	test_zero_memory_regions();
+	test_mmio_during_vectoring();
 #endif
 
 	test_invalid_memory_region_flags();
-- 
2.43.0

Re: [PATCH v3 7/7] selftests: KVM: Add test case for MMIO during vectoring

[Sean Christopherson]

KVM: selftests:

On Tue, Dec 17, 2024, Ivan Orlov wrote:
> Extend the 'set_memory_region_test' with a test case which covers the
> MMIO during vectoring error handling. The test case

Probably a good idea to explicitly state this is x86-only (hard to see that
from the diff alone).

> 
> 1) Sets an IDT descriptor base to point to an MMIO address
> 2) Generates a #GP in the guest
> 3) Verifies that we got a correct exit reason and suberror code

No "we".  It's very specifically userspace that needs to see the exit.
 
> 4) Verifies that we got a corrent reported GPA in internal.data[3]

s/corrent/correct?

And #4 can be combined with #3:

3) Verifies userspace gets the correct exit reason, suberror code, and
   GPA in internal.data[3]

Re: [PATCH v3 0/7] Enhance event delivery error handling

[Sean Christopherson]

On Tue, Dec 17, 2024, Ivan Orlov wrote:
> Currently, the unhandleable vectoring (e.g. when guest accesses MMIO
> during vectoring) is handled differently on VMX and SVM: on VMX KVM
> returns internal error, when SVM goes into infinite loop trying to
> deliver an event again and again.
> 
> This patch series eliminates this difference by returning a KVM internal
> error when KVM can't emulate during vectoring for both VMX and SVM.
> 
> Also, introduce a selftest test case which covers the error handling
> mentioned above.

A few nits throughout, but I'll address them when applying.  Thanks!

Re: [PATCH v3 0/7] Enhance event delivery error handling

[Ivan Orlov]

On 12/18/24 18:44, Sean Christopherson wrote:
> On Tue, Dec 17, 2024, Ivan Orlov wrote:
>> Currently, the unhandleable vectoring (e.g. when guest accesses MMIO
>> during vectoring) is handled differently on VMX and SVM: on VMX KVM
>> returns internal error, when SVM goes into infinite loop trying to
>> deliver an event again and again.
>>
>> This patch series eliminates this difference by returning a KVM internal
>> error when KVM can't emulate during vectoring for both VMX and SVM.
>>
>> Also, introduce a selftest test case which covers the error handling
>> mentioned above.
> 
> A few nits throughout, but I'll address them when applying.  Thanks!
> 

Hi Sean,

Awesome, thanks a lot for fixing the commits and for the review.

-- 
Kind regards,
Ivan Orlov

Re: [PATCH v3 0/7] Enhance event delivery error handling

[Sean Christopherson]

On Tue, 17 Dec 2024 18:14:51 +0000, Ivan Orlov wrote:
> Currently, the unhandleable vectoring (e.g. when guest accesses MMIO
> during vectoring) is handled differently on VMX and SVM: on VMX KVM
> returns internal error, when SVM goes into infinite loop trying to
> deliver an event again and again.
> 
> This patch series eliminates this difference by returning a KVM internal
> error when KVM can't emulate during vectoring for both VMX and SVM.
> 
> [...]

Applied to kvm-x86 misc, thanks!  If you get a chance, please double check that
I didn't fat-finger anything.

[1/7] KVM: x86: Add function for vectoring error generation
      https://github.com/kvm-x86/linux/commit/11c98fa07a79
[2/7] KVM: x86: Add emulation status for unhandleable vectoring
      https://github.com/kvm-x86/linux/commit/5c9cfc486636
[3/7] KVM: x86: Unprotect & retry before unhandleable vectoring check
      https://github.com/kvm-x86/linux/commit/704fc6021b9e
[4/7] KVM: VMX: Handle vectoring error in check_emulate_instruction
      https://github.com/kvm-x86/linux/commit/47ef3ef843c0
[5/7] KVM: SVM: Handle vectoring error in check_emulate_instruction
      https://github.com/kvm-x86/linux/commit/7bd7ff99110a
[6/7] selftests: KVM: extract lidt into helper function
      https://github.com/kvm-x86/linux/commit/4e9427aeb957
[7/7] selftests: KVM: Add test case for MMIO during vectoring
      https://github.com/kvm-x86/linux/commit/62e41f6b4f36

--
https://github.com/kvm-x86/linux/tree/next

Re: [PATCH v3 0/7] Enhance event delivery error handling

[Ivan Orlov]

On Wed, Dec 18, 2024 at 06:40:46PM -0800, Sean Christopherson wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Tue, 17 Dec 2024 18:14:51 +0000, Ivan Orlov wrote:
> > Currently, the unhandleable vectoring (e.g. when guest accesses MMIO
> > during vectoring) is handled differently on VMX and SVM: on VMX KVM
> > returns internal error, when SVM goes into infinite loop trying to
> > deliver an event again and again.
> >
> > This patch series eliminates this difference by returning a KVM internal
> > error when KVM can't emulate during vectoring for both VMX and SVM.
> >
> > [...]
> 
> Applied to kvm-x86 misc, thanks!  If you get a chance, please double check that
> I didn't fat-finger anything.
> 
> [1/7] KVM: x86: Add function for vectoring error generation
>       https://github.com/kvm-x86/linux/commit/11c98fa07a79
> [2/7] KVM: x86: Add emulation status for unhandleable vectoring
>       https://github.com/kvm-x86/linux/commit/5c9cfc486636
> [3/7] KVM: x86: Unprotect & retry before unhandleable vectoring check
>       https://github.com/kvm-x86/linux/commit/704fc6021b9e
> [4/7] KVM: VMX: Handle vectoring error in check_emulate_instruction
>       https://github.com/kvm-x86/linux/commit/47ef3ef843c0
> [5/7] KVM: SVM: Handle vectoring error in check_emulate_instruction
>       https://github.com/kvm-x86/linux/commit/7bd7ff99110a
> [6/7] selftests: KVM: extract lidt into helper function
>       https://github.com/kvm-x86/linux/commit/4e9427aeb957
> [7/7] selftests: KVM: Add test case for MMIO during vectoring
>       https://github.com/kvm-x86/linux/commit/62e41f6b4f36
> 
> --
> https://github.com/kvm-x86/linux/tree/next

Hi Sean,

The commits (and the messages specifically) look good to me, thanks a
lot for making the changelogs better! :)

Also, I ran the selftests for the `next` branch on both Intel and AMD
platforms, and all of them seem to pass.

--
Kind regards,
Ivan Orlov