Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e55a0591a7fc19ab06529dd1e1e6ad94daaa68f0.camel@redhat.com>
Subject: Re: [PATCH v2 06/21] KVM: x86: Treat #DBs from the emulator as
 fault-like (code and DR7.GD=1)
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Oliver Upton <oupton@google.com>,
        Peter Shier <pshier@google.com>
Date:   Wed, 06 Jul 2022 14:57:59 +0300
In-Reply-To: <20220614204730.3359543-7-seanjc@google.com>
References: <20220614204730.3359543-1-seanjc@google.com>
         <20220614204730.3359543-7-seanjc@google.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  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@redhat.com>

Best regards,
	Maxim Levitsky