On Tue, 2022-06-14 at 20:47 +0000, Sean Christopherson wrote:
> Add a dedicated "exception type" for #DBs, as #DBs can be fault-like or
> trap-like depending the sub-type of #DB, and effectively defer the
> decision of what to do with the #DB to the caller.
>
> For the emulator's two calls to exception_type(), treat the #DB as
> fault-like, as the emulator handles only code breakpoint and general
> detect #DBs, both of which are fault-like.
>
> For event injection, which uses exception_type() to determine whether to
> set EFLAGS.RF=1 on the stack, keep the current behavior of not setting
> RF=1 for #DBs. Intel and AMD explicitly state RF isn't set on code #DBs,
> so exempting by failing the "== EXCPT_FAULT" check is correct. The only
> other fault-like #DB is General Detect, and despite Intel and AMD both
> strongly implying (through omission) that General Detect #DBs should set
> RF=1, hardware (multiple generations of both Intel and AMD), in fact does
> not. Through insider knowledge, extreme foresight, sheer dumb luck, or
> some combination thereof, KVM correctly handled RF for General Detect #DBs.
>
> Fixes: 38827dbd3fb8 ("KVM: x86: Do not update EFLAGS on faulting emulation")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> arch/x86/kvm/x86.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c5db31b4bd6f..7c3ce601bdcc 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -529,6 +529,7 @@ static int exception_class(int vector)
> #define EXCPT_TRAP 1
> #define EXCPT_ABORT 2
> #define EXCPT_INTERRUPT 3
> +#define EXCPT_DB 4
>
> static int exception_type(int vector)
> {
> @@ -539,8 +540,14 @@ static int exception_type(int vector)
>
> mask = 1 << vector;
>
> - /* #DB is trap, as instruction watchpoints are handled elsewhere */
> - if (mask & ((1 << DB_VECTOR) | (1 << BP_VECTOR) | (1 << OF_VECTOR)))
> + /*
> + * #DBs can be trap-like or fault-like, the caller must check other CPU
> + * state, e.g. DR6, to determine whether a #DB is a trap or fault.
> + */
> + if (mask & (1 << DB_VECTOR))
> + return EXCPT_DB;
> +
> + if (mask & ((1 << BP_VECTOR) | (1 << OF_VECTOR)))
> return EXCPT_TRAP;
>
> if (mask & ((1 << DF_VECTOR) | (1 << MC_VECTOR)))
> @@ -8632,6 +8639,12 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> unsigned long rflags = static_call(kvm_x86_get_rflags)(vcpu);
> toggle_interruptibility(vcpu, ctxt->interruptibility);
> vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
> +
> + /*
> + * Note, EXCPT_DB is assumed to be fault-like as the emulator
> + * only supports code breakpoints and general detect #DB, both
> + * of which are fault-like.
> + */
> if (!ctxt->have_exception ||
> exception_type(ctxt->exception.vector) == EXCPT_TRAP) {
> kvm_pmu_trigger_event(vcpu, PERF_COUNT_HW_INSTRUCTIONS);
> @@ -9546,6 +9559,16 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool *req_immediate_exit)
>
> /* try to inject new event if pending */
> if (vcpu->arch.exception.pending) {
> + /*
> + * Fault-class exceptions, except #DBs, set RF=1 in the RFLAGS
> + * value pushed on the stack. Trap-like exception and all #DBs
> + * leave RF as-is (KVM follows Intel's behavior in this regard;
> + * AMD states that code breakpoint #DBs excplitly clear RF=0).
> + *
> + * Note, most versions of Intel's SDM and AMD's APM incorrectly
> + * describe the behavior of General Detect #DBs, which are
> + * fault-like. They do _not_ set RF, a la code breakpoints.
> + */
> if (exception_type(vcpu->arch.exception.nr) == EXCPT_FAULT)
> __kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) |
> X86_EFLAGS_RF);
Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
Best regards,
Maxim Levitsky
