Re: [Patch v4 1/4] KVM: SVM: Re-queue events that were never injected

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] KVM: SVM: self-IPI on force_immediate_exit aborts ESMTP VMRUN
  sync in a loop
- [Low] kvm: svm: svm_complete_interrupts() clobbers VMCB exit_int_info
  and event_inj
- [Low] KVM: SVM: inaccurate comment in svm_complete_interrupts() on
  EVENTINJ clear
--

commit 3134c7d995fb21938bef42d0d4bd1ee3a0a75674
Author: Pratik R. Sampat <prsampat@amd.com>

KVM: SVM: Re-queue events that were never injected

This commit updates svm_complete_interrupts() to migrate any remaining
event in event_inj to exit_int_info on #VMEXIT so that events left
uninjected when VMRUN exits early are re-queued, and simplifies
svm_cancel_injection() accordingly.

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 136069dfcd44f..850ebbdefde09 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4463,6 +4463,7 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	u8 vector;
>  	int type;
> +	struct vmcb_control_area *control = &svm->vmcb->control;
>  	u32 exitintinfo = svm->vmcb->control.exit_int_info;
>  	bool nmi_l1_to_l2 = svm->nmi_l1_to_l2;
>  	bool soft_int_injected = svm->soft_int_injected;
> @@ -4470,6 +4471,27 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
>  	svm->nmi_l1_to_l2 = false;
>  	svm->soft_int_injected = false;
>  
> +	/*
> +	 * Hardware clears EVENTINJ field when it injects an event.
> +	 * A non-empty EVENTINJ on #VMEXIT means the vCPU never entered guest
> +	 * mode, and thus that the event was never delivered.  Migrate the event
> +	 * to EXITINTINFO so that it's requeued instead of being dropped.

[Severity: Low]
As reported by Sean Christopherson on v1 [1], is this comment accurate
about when hardware clears EVENTINJ?  Hardware clears EVENTINJ
unconditionally on #VMEXIT when exiting guest mode, whereas pre-entry
Enhanced SMT Protection exits terminate VMRUN at the synchronization point
before guest mode is entered and leave EVENTINJ intact.

> +	 *
> +	 * An undelivered event doesn't imply a fatal VMEXIT_INVALID. With
> +	 * Enhanced SMT Protection, VMRUN may exit with an ordinary #VMEXIT
> +	 * without having injected that event into the guest.
> +	 *
> +	 * Clobbering EXITINTINFO is safe precisely because the vCPU never
> +	 * entered guest mode.
> +	 */
> +	if (control->event_inj) {
> +		control->exit_int_info = control->event_inj;
> +		control->exit_int_info_err = control->event_inj_err;
> +		control->event_inj = 0;
> +
> +		exitintinfo = control->exit_int_info;
> +	}

[Severity: Medium]
As reported by Sean Christopherson on v1 [2], how does re-queuing
control->event_inj interact with KVM_RUN_FORCE_IMMEDIATE_EXIT under
Enhanced SMT Protection?

When kvm_check_and_inject_events() stages an event into control->event_inj
while a second event is pending, vcpu_enter_guest() sets
KVM_RUN_FORCE_IMMEDIATE_EXIT so svm_vcpu_run() sends a physical self-IPI
via smp_send_reschedule() before VMRUN.  If ESMTP stalls VMRUN at the
sibling synchronization point, that pending self-IPI immediately aborts
VMRUN with SVM_EXIT_INTR before guest entry.  svm_complete_interrupts()
then re-queues the first event with injected = true and sets
KVM_REQ_EVENT, causing the next vcpu_enter_guest() iteration to hit -EBUSY
in kvm_check_and_inject_events() and send another self-IPI before VMRUN,
livelocking the vCPU without ever synchronizing with its sibling.

[Severity: Low]
Does modifying control->exit_int_info and control->event_inj in the VMCB
here corrupt subsequent diagnostic output in svm_handle_exit()?

svm_vcpu_run() calls svm_complete_interrupts() before vcpu_enter_guest()
calls svm_handle_exit().  When VMRUN fails with SVM_EXIT_ERR (for example,
due to an invalid control->event_inj), dump_vmcb() in svm_handle_exit()
now prints event_inj as 0 and reports the offending event under
exit_int_info, and trace_kvm_nested_vmexit() logs the undelivered
event_inj as exit_int_info (disagreeing with trace_kvm_exit() called just
before svm_complete_interrupts()).  Using local variables for exitintinfo
and exit_int_info_err while only clearing control->event_inj would leave
control->exit_int_info intact.

[1] https://lore.kernel.org/r/arFYHRzNFT886BME@google.com
[2] https://lore.kernel.org/r/arEuMojZa_kv9VXA@google.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794244450.git.prsampat@amd.com?part=1