Re: [PATCH] KVM: x86: Preserve KVM_EXIT_DEBUG on emulated HLT with single-step

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The fix to preserve KVM_EXIT_DEBUG during instruction skipping was not applied to kvm_emulate_ap_reset_hold().
- [High] kvm_emulate_halt() restricts early return exclusively to KVM_EXIT_DEBUG, overwriting other critical exit reasons like KVM_EXIT_INTERNAL_ERROR.
--

--- Patch [1]: [PATCH] KVM: x86: Preserve KVM_EXIT_DEBUG on emulated HLT with single-step ---
commit a928dfda27cebab2c60b683589e4cf4ae12635fb
Author: sai madhu <suryasaimadhu369@gmail.com>

KVM: x86: Preserve KVM_EXIT_DEBUG on emulated HLT with single-step
    
kvm_emulate_halt() calls kvm_skip_emulated_instruction() before
kvm_emulate_halt_noskip(). When single-step debugging is active,
the skip path can trigger a debug exception and set exit_reason to
KVM_EXIT_DEBUG, but halt emulation then overwrites it with
KVM_EXIT_HLT on the userspace LAPIC path. Return immediately when
a debug exit is already pending so userspace debuggers receive the
correct event.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ff8812f3a1293..473a03f58e263 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11701,10 +11701,16 @@ EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_emulate_halt_noskip);
>  int kvm_emulate_halt(struct kvm_vcpu *vcpu)
>  {
>  	int ret = kvm_skip_emulated_instruction(vcpu);
> +
>  	/*
> -	 * TODO: we might be squashing a GUESTDBG_SINGLESTEP-triggered
> -	 * KVM_EXIT_DEBUG here.
> +	 * If kvm_skip_emulated_instruction() triggered a singlestep
> +	 * debug exception, return to userspace immediately so the
> +	 * debugger can process the event. Do not let halt emulation
> +	 * overwrite the KVM_EXIT_DEBUG exit reason.
>  	 */
> +	if (vcpu->run->exit_reason == KVM_EXIT_DEBUG)
> +		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does restricting this early
return exclusively to KVM_EXIT_DEBUG overwrite other critical exit reasons?

If kvm_skip_emulated_instruction() fails (e.g., executing HLT inside an
SGX enclave), it queues KVM_EXIT_INTERNAL_ERROR and returns 0.

Because this check only handles KVM_EXIT_DEBUG, emulation failures fall
through to kvm_emulate_halt_noskip(), which then overwrites the exit reason
with KVM_EXIT_HLT for the userspace LAPIC path.

> +
>  	return kvm_emulate_halt_noskip(vcpu) && ret;
>  }
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_emulate_halt);

[Severity: High]
This is a pre-existing issue, but it looks like the same issue is present in
kvm_emulate_ap_reset_hold().

arch/x86/kvm/x86.c:kvm_emulate_ap_reset_hold() {
	...
	int ret = kvm_skip_emulated_instruction(vcpu);

	return __kvm_emulate_halt(vcpu, KVM_MP_STATE_AP_RESET_HOLD,
				  KVM_EXIT_AP_RESET_HOLD) && ret;
}

Because && evaluates left-to-right, __kvm_emulate_halt() executes
unconditionally and overwrites the exit reason with KVM_EXIT_AP_RESET_HOLD,
even if kvm_skip_emulated_instruction() already returned 0 to exit to
userspace with a debug exception or emulation failure.

Does kvm_emulate_ap_reset_hold() need a similar fix?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709053949.211165-1-SaiMadhu.KoyyalaHariVenkata@amd.com?part=1