Re: [PATCH v2 2/5] KVM: nVMX: Handle pending #DB when injecting INIT VM-exit

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Mon, Feb 03, 2020 at 11:13:30AM -0800, Sean Christopherson wrote:
> On Tue, Jan 28, 2020 at 01:27:12AM -0800, Oliver Upton wrote:
> > SDM 27.3.4 states that the 'pending debug exceptions' VMCS field will
> > be populated if a VM-exit caused by an INIT signal takes priority over a
> > debug-trap. Emulate this behavior when synthesizing an INIT signal
> > VM-exit into L1.
> > 
> > Fixes: 558b8d50dbff ("KVM: x86: Fix INIT signal handling in various CPU states")
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 95b3f4306ac2..aba16599ca69 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -3572,6 +3572,27 @@ static void nested_vmx_inject_exception_vmexit(struct kvm_vcpu *vcpu,
> >  	nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI, intr_info, exit_qual);
> >  }
> >  
> > +static inline bool nested_vmx_check_pending_dbg(struct kvm_vcpu *vcpu)
> 
> Really dislike the name, partially because the code checks @has_payload and
> partially because the part, nested_vmx_set_pending_dbg() "sets" completely
> different state than this checks.
> 
> Checking has_payload may also be wrong, e.g. wouldn't it make sense to
> update GUEST_PENDING_DBG_EXCEPTIONS, even if we crush it with '0'?
> 
> > +{
> > +	return vcpu->arch.exception.nr == DB_VECTOR &&
> > +			vcpu->arch.exception.pending &&
> > +			vcpu->arch.exception.has_payload;
> > +}
> > +
> > +/*
> > + * If a higher priority VM-exit is delivered before a debug-trap, hardware will
> > + * set the 'pending debug exceptions' field appropriately for reinjection on the
> > + * next VM-entry.
> > + */
> > +static void nested_vmx_set_pending_dbg(struct kvm_vcpu *vcpu)
> > +{
> > +	vmcs_writel(GUEST_PENDING_DBG_EXCEPTIONS, vcpu->arch.exception.payload);
> > +	vcpu->arch.exception.has_payload = false;
> > +	vcpu->arch.exception.payload = 0;
> > +	vcpu->arch.exception.pending = false;
> > +	vcpu->arch.exception.injected = true;
> 
> This looks wrong.  The #DB hasn't been injected, KVM is simply emulating
> the side effect of the VMCS field being updated.  E.g. KVM will have
> different architecturally visible behavior depending on @has_payload.

My head is spinning trying to work through the #DB/MTF interactions.  I
think this ends up being a moot point because prepare_vmcs12() will purge
the pending exceptions.  If it is a moot point, then I'd prefer to not do
the explicit arch.exception updates so as to keep this similar to other
exceptions.