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: <f025784c144c63b05266433756070862aef5f6d5.camel@redhat.com>
Subject: Re: [PATCH v2 04/21] KVM: nVMX: Treat General Detect #DB (DR7.GD=1)
 as fault-like
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:45:38 +0300
In-Reply-To: <20220614204730.3359543-5-seanjc@google.com>
References: <20220614204730.3359543-1-seanjc@google.com>
         <20220614204730.3359543-5-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:
> Exclude General Detect #DBs, which have fault-like behavior but also have
> a non-zero payload (DR6.BD=1), from nVMX's handling of pending debug
> traps.  Opportunistically rewrite the comment to better document what is
> being checked, i.e. "has a non-zero payload" vs. "has a payload", and to
> call out the many caveats surrounding #DBs that KVM dodges one way or
> another.
> 
> Cc: Oliver Upton <oupton@google.com>
> Cc: Peter Shier <pshier@google.com>
> Fixes: 684c0422da71 ("KVM: nVMX: Handle pending #DB when injecting INIT VM-exit")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 33ffc8bcf9cd..61bc80fc4cfa 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3857,16 +3857,29 @@ static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu,
>  }
>  
>  /*
> - * Returns true if a debug trap is pending delivery.
> + * Returns true if a debug trap is (likely) pending delivery.  Infer the class
> + * of a #DB (trap-like vs. fault-like) from the exception payload (to-be-DR6).
> + * Using the payload is flawed because code breakpoints (fault-like) and data
> + * breakpoints (trap-like) set the same bits in DR6 (breakpoint detected), i.e.
> + * this will return false positives if a to-be-injected code breakpoint #DB is
> + * pending (from KVM's perspective, but not "pending" across an instruction
> + * boundary).  ICEBP, a.k.a. INT1, is also not reflected here even though it
> + * too is trap-like.
>   *
> - * In KVM, debug traps bear an exception payload. As such, the class of a #DB
> - * exception may be inferred from the presence of an exception payload.
> + * KVM "works" despite these flaws as ICEBP isn't currently supported by the
> + * emulator, Monitor Trap Flag is not marked pending on intercepted #DBs (the
> + * #DB has already happened), and MTF isn't marked pending on code breakpoints
> + * from the emulator (because such #DBs are fault-like and thus don't trigger
> + * actions that fire on instruction retire).

Makes sense overall, but I am still not 100% sure to be honest I understand that
new description fully.

The patch itself seems to be correct, so,

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky


>   */
> -static inline bool vmx_pending_dbg_trap(struct kvm_vcpu *vcpu)
> +static inline unsigned long vmx_get_pending_dbg_trap(struct kvm_vcpu *vcpu)
>  {
> -	return vcpu->arch.exception.pending &&
> -			vcpu->arch.exception.nr == DB_VECTOR &&
> -			vcpu->arch.exception.payload;
> +	if (!vcpu->arch.exception.pending ||
> +	    vcpu->arch.exception.nr != DB_VECTOR)
> +		return 0;
> +
> +	/* General Detect #DBs are always fault-like. */
> +	return vcpu->arch.exception.payload & ~DR6_BD;
>  }
>  
>  /*
> @@ -3878,9 +3891,10 @@ static inline bool vmx_pending_dbg_trap(struct kvm_vcpu *vcpu)
>   */
>  static void nested_vmx_update_pending_dbg(struct kvm_vcpu *vcpu)
>  {
> -	if (vmx_pending_dbg_trap(vcpu))
> -		vmcs_writel(GUEST_PENDING_DBG_EXCEPTIONS,
> -			    vcpu->arch.exception.payload);
> +	unsigned long pending_dbg = vmx_get_pending_dbg_trap(vcpu);
> +
> +	if (pending_dbg)
> +		vmcs_writel(GUEST_PENDING_DBG_EXCEPTIONS, pending_dbg);
>  }
>  
>  static bool nested_vmx_preemption_timer_pending(struct kvm_vcpu *vcpu)
> @@ -3937,7 +3951,7 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)
>  	 * while delivering the pending exception.
>  	 */
>  
> -	if (vcpu->arch.exception.pending && !vmx_pending_dbg_trap(vcpu)) {
> +	if (vcpu->arch.exception.pending && !vmx_get_pending_dbg_trap(vcpu)) {
>  		if (vmx->nested.nested_run_pending)
>  			return -EBUSY;
>  		if (!nested_vmx_check_exception(vcpu, &exit_qual))