Re: [PATCH v3 5/8] KVM: nSVM: Always use NextRIP as vmcb02's NextRIP after first L2 VMRUN

[Yosry Ahmed]

[..]
> > > @@ -845,17 +845,24 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
> > >         vmcb02->control.event_inj_err       = svm->nested.ctl.event_inj_err;
> > >
> > >         /*
> > > -        * next_rip is consumed on VMRUN as the return address pushed on the
> > > +        * NextRIP is consumed on VMRUN as the return address pushed on the
> > >          * stack for injected soft exceptions/interrupts.  If nrips is exposed
> > > -        * to L1, take it verbatim from vmcb12.  If nrips is supported in
> > > -        * hardware but not exposed to L1, stuff the actual L2 RIP to emulate
> > > -        * what a nrips=0 CPU would do (L1 is responsible for advancing RIP
> > > -        * prior to injecting the event).
> > > +        * to L1, take it verbatim from vmcb12.
> > > +        *
> > > +        * If nrips is supported in hardware but not exposed to L1, stuff the
> > > +        * actual L2 RIP to emulate what a nrips=0 CPU would do (L1 is
> > > +        * responsible for advancing RIP prior to injecting the event). This is
> > > +        * only the case for the first L2 run after VMRUN. After that (e.g.
> > > +        * during save/restore), NextRIP is updated by the CPU and/or KVM, and
> > > +        * the value of the L2 RIP from vmcb12 should not be used.
> > >          */
> > > -       if (guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS))
> > > -               vmcb02->control.next_rip    = svm->nested.ctl.next_rip;
> > > -       else if (boot_cpu_has(X86_FEATURE_NRIPS))
> > > -               vmcb02->control.next_rip    = vmcb12_rip;
> > > +       if (boot_cpu_has(X86_FEATURE_NRIPS)) {
> > > +               if (guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS) ||
> > > +                   !svm->nested.nested_run_pending)
> > > +                       vmcb02->control.next_rip    = svm->nested.ctl.next_rip;
> > > +               else
> > > +                       vmcb02->control.next_rip    = vmcb12_rip;
> > > +       }
> >
> > This should probably also apply to soft_int_next_rip below the context
> > lines. Otherwise after  patch 7 we keep it uninitialized if the guest
> > doesn't have NRIPs and !nested_run_pending.
>
> That's fine though, isn't it?  Because in that case, doesn't the soft int have to
> comein through svm_update_soft_interrupt_rip()?  Ugh, no because nSVM migrates
> control.event_inj.
>
> IIUC, we want to end up with this?

Yes.

>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 03b201fe9613..d12647080051 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -925,7 +925,8 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>          */
>         if (is_evtinj_soft(vmcb02->control.event_inj)) {
>                 svm->soft_int_injected = true;
> -               if (guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS))
> +               if (guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS) ||
> +                   !svm->nested.nested_run_pending)
>                         svm->soft_int_next_rip = vmcb12_ctrl->next_rip;
>         }