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@xxxxxxxxxx>
> Cc: Peter Shier <pshier@xxxxxxxxxx>
> Fixes: 684c0422da71 ("KVM: nVMX: Handle pending #DB when injecting INIT VM-exit")
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
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))
