Re: [PATCH 3/5] KVM: nSVM: Don't forget about L1-injected events

[Maciej S. Szmigiero]

On 1.04.2022 02:08, Sean Christopherson wrote:
> On Fri, Apr 01, 2022, Maciej S. Szmigiero wrote:
> > On 31.03.2022 01:20, Sean Christopherson wrote:
> > > Re-executing the INTn is wrong, the instruction has already completed decode and
> > > execution.  E.g. if there's there's a code breakpoint on the INTn, rewinding will
> > > cause a spurious #DB.
> > >
> > > KVM's INT3 shenanigans are bonkers, but I guess there's no better option given
> > > that the APM says "Software interrupts cannot be properly injected if the processor
> > > does not support the NextRIP field.".  What a mess.
> >
> > Note that KVM currently always tries to re-execute the current instruction
> > when asked to re-inject a #BP or a #OF, even when nrips are enabled.
>
> Yep, and my vote is to fix that.

The only issue I have with making SVM re-inject a #BP or a #OF in the
nrips case is that this might introduce some regression with respect to
host-side guest debugging.

On the other hand, VMX does re-inject these unconditionally so it might
not be a problem after all.

> > Also, #BP (and #OF, too) is returned as type SVM_EXITINTINFO_TYPE_EXEPT,
> > not as SVM_EXITINTINFO_TYPE_SOFT (soft interrupt), so it should be
> > re-injected accordingly.
>
> Ahhh, SVM doesn't differentiate between software exceptions and hardware exceptions.
> Finally found the relevant section in the APM:
>
>    Despite the instruction name, the events raised by the INT1 (also known as ICEBP),
>    INT3 and INTO instructions (opcodes F1h, CCh and CEh) are considered exceptions for
>    the purposes of EXITINTINFO, not software interrupts. Only events raised by the INTn
>    instruction (opcode CDh) are considered software interrupts.
>
> VMX has separate identifiers for software interrupts and for software exceptions,

I guess the sentence above was supposed to read "for *hardware exceptions*
and for software exceptions", just like in the previous paragraph about SVM.

> where as SVM unconditionally treats #BP and #OF as soft:
>
>    Injecting an exception (TYPE = 3) with vectors 3 or 4 behaves like a trap raised by
>    INT3 and INTO instructions
>
> Now I'm curious why Intel doesn't do the same...

Perhaps it's just a result of VMX and SVM being developed independently,
in parallel.

> > > Anyways, for the common nrips=true case, I strongly prefer that we properly fix
> > > the non-nested case and re-inject software interrupts, which should in turn
> > > naturally fix this nested case.
> >
> > This would also need making the #BP or #OF current instruction
> > re-execution conditional on (at least) nrips support.
> >
> > I am not sure, however, whether this won't introduce any regressions.
> > That's why this patch set changed the behavior here only for the
> > L1 -> L2 case.
> >
> > Another issue is whether a L1 hypervisor can legally inject a #VC
> > into its L2 (since these are never re-injected).
>
> I would expect to work, and it's easy to find out.  I know VMX allows injecting
> non-existent exceptions, but the APM is vague as usual and says VMRUN will fail...

I've done a quick test right now and a VMRUN attempt with #VC event
injection does seem to fail.
So we don't need to worry about not re-injecting a #VC.

>    If the VMM attempts to inject an event that is impossible for the guest mode
>
> > We still need L1 -> L2 event injection detection to restore the NextRIP
> > field when re-injecting an event that uses it.
>
> You lost me on this one.  KVM L0 is only (and always!) responsible for saving the
> relevant info into vmcb12, why does it need to detect where the vectoring exception
> came from?

Look at the description of patch 4 of this patch set - after some
L2 VMEXITs handled by L0 (in other words, which do *not* result in
a nested VMEXIT to L1) the VMCB02 NextRIP field will be zero
(APM 15.7.1 "State Saved on Exit" describes when this happens).

If we attempt to re-inject some types of events with the NextRIP field
being zero the return address pushed on stack will also be zero, which
will obviously do bad things to the L2 when it returns from
an (exception|interrupt) handler.

> > > And for nrips=false, my vote is to either punt
> > > and document it as a "KVM erratum", or straight up make nested require nrips.
> >
> > A quick Internet search shows that the first CPUs with NextRIP were the
> > second-generation Family 10h CPUs (Phenom II, Athlon II, etc.).
> > They started being released in early 2009, so we probably don't need to
> > worry about the non-nrips case too much.
> >
> > For the nested case, orthodox reading of the aforementioned APM sentence
> > would mean that a L1 hypervisor is not allowed either to make use of such
> > event injection in the non-nrips case.
>
> Heh, my reading of it is that it's not disallowed, it just won't work correctly,
> i.e. the INTn won't be skipped.

Either way, we probably don't need to worry that this case don't get handled
100% right.

Thanks,
Maciej