2018-03-23 19:20+0300, Liran Alon:
>
> On 23/03/18 18:26, Christopherson, Sean J wrote:
> > On Thu, Mar 22, 2018, Liran Alon wrote:
> > > The reason that exception.pending should block re-injection of
> > > NMI/interrupt is not described correctly in comment in code.
> > > Instead, it describes why a pending exception should be injected
> > > before a pending NMI/interrupt.
> > >
> > > Therefore, move currently present comment to code-block evaluating
> > > a new pending event which explains why exception.pending is evaluated
> > > first.
> > > In addition, create a new comment describing that exception.pending
> > > blocks re-injection of NMI/interrupt because the exception was
> > > queued by handling vmexit which was due to NMI/interrupt delivery.
> > >
> > > Fixes: 664f8e26b00c ("KVM: X86: Fix loss of exception which has not
> > > yet been injected")
> > >
> > > Signed-off-by: Liran Alon <liran.alon@xxxxxxxxxx>
> > > Reviewed-by: Nikita Leshenko <nikita.leshchenko@xxxxxxxxxx>
> > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxx>
> > > ---
> > > arch/x86/kvm/x86.c | 11 +++++++++--
> > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 9f6b45f2382a..f5587998b57a 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -6645,8 +6645,9 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
> > > }
> > > /*
> > > - * Exceptions must be injected immediately, or the exception
> > > - * frame will have the address of the NMI or interrupt handler.
> > > + * NMI/interrupt must not be injected if an exception is
> > > + * pending, because the exception was queued by handling
> > > + * vmexit which was due to NMI/interrupt delivery.
> > > */
> > Hmm, IMO both the old and new comments describe what can go wrong if
> > we inject an NMI/interrupt, but neither comment addresses the underlying
> > reason of why it's incorrect to inject an NMI or interrupt if there is
> > a pending exception. What about adding a single long-winded comment
> > about event priority to replace both of your new comments? The second
> > comment could be a short reference to the verbose comment, e.g. "see
> > above comment for event priority".
> >
> > /*
> > * Do not inject an NMI or interrupt if there is a pending
> > * exception. Exceptions and interrupts are recognized at
> > * instruction boundaries, i.e. the start of an instruction.
> > * Trap-like exceptions, e.g. #DB, have higher priority than
> > * NMIs and interrupts, i.e. traps are recognized before an
> > * NMI/interrupt that's pending on the same instruction.
> > * Fault-like exceptions, e.g. #GP and #PF, are the lowest
> > * priority, but are only generated (pended) during instruction
> > * execution, i.e. a pending fault-like exception means the
> > * fault occurred on the *previous* instruction and must be
> > * serviced prior to recognizing any new events in order to
> > * fully complete the previous instruction.
> > */
>
> I agree with everything you wrote.
> I am just not sure what is more clear to the average reader.
> I will let Paolo/Radim decide what they prefer.
> I'm fine with both approaches and I think you described it very well in the
> comment above.
I opted for Sean's variant and applied the whole series, thanks.
