Re: [PATCH v1 2/4] KVM: nSVM: Delay stuffing L2's current RIP into NextRIP until vCPU run

[Sean Christopherson <seanjc@google.com>]

On Tue, Feb 24, 2026, Yosry Ahmed wrote:
> On Tue, Feb 24, 2026 at 5:10 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Tue, Feb 24, 2026, Yosry Ahmed wrote:
> > > > > Doing this in svm_prepare_switch_to_guest() is wrong, or at least
> > > > > after the svm->guest_state_loaded check. It's possible to emulate the
> > > > > nested VMRUN without doing a vcpu_put(), which means
> > > > > svm->guest_state_loaded will remain true and this code will be
> > > > > skipped.
> > > > >
> > > > > In fact, this breaks the svm_nested_soft_inject_test test. Funny
> > > > > enough, I was only running it with my repro changes, which papered
> > > > > over the bug because it forced an exit to userspace after VMRUN due to
> > > > > single-stepping, so svm->guest_state_loaded got cleared and the code
> > > > > was executed on the next KVM_RUN, before L2 runs.
> > > > >
> > > > > I can move it above the svm->guest_state_loaded check, but I think I
> > > > > will just put it in pre_svm_run() instead.
> > > >
> > > > I would rather not expand pre_svm_run(), and instead just open code it in
> > > > svm_vcpu_run().  pre_svm_run() probably should never have been added, because
> > > > it's far from a generic "pre run" API.  E.g. if we want to keep the helper around,
> > > > it should probably be named something something ASID.
> > >
> > > I sent a new version before I saw your response.. sorry.
> > >
> > > How strongly do you feel about this? :P
> >
> > Strong enough that I'll fix it up when applying, unless it's a sticking point on
> > your end.
> 
> It's just that 99% of the time someone is reading svm_vcpu_run(), they
> won't care about this code, and it's also cognitive load to filter it
> out. We can add a helper for this code (and the soft IRQ inject),
> something like svm_fixup_nested_rips() or sth.

I don't entirely disagree, but at the same time, why is someome reading svm_vcpu_run()
if they don't want to look at the gory details?

> We discussed a helper before and you didn't like it, but that was in a
> different context (a helper that combined normal and special cases).
> WDYT?

A helper would work.  svm_fixup_nested_rips() is good, the only flaw is the CS.base
chunk, but I'm not sure I care enough about 32-bit to reject the name just because
of that :-)

That would make it easier to reduce indentation, e.g.

static void svm_fixup_nested_rips(struct kvm_vcpu *vcpu)
{
	struct vcpu_svm *svm = to_svm(vcpu);

	/*
	 * 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). Once L2
	 * runs after L1 executes VMRUN, NextRIP is updated by the CPU and/or
	 * KVM, and this is no longer needed.
	 *
	 * This is done here (as opposed to when preparing vmcb02) to use the
	 * most up-to-date value of RIP regardless of the order of restoring
	 * registers and nested state in the vCPU save+restore path.
	 *
	 * Simiarly, initialize svm->soft_int_* fields here to use the most
	 * up-to-date values of RIP and CS base, regardless of restore order.
	 */
	if (!is_guest_mode(vcpu) || !svm->nested.nested_run_pending)
		return;

	if (boot_cpu_has(X86_FEATURE_NRIPS) &&
	    !guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS))
		svm->vmcb->control.next_rip = kvm_rip_read(vcpu);

	if (svm->soft_int_injected) {
		svm->soft_int_csbase = svm->vmcb->save.cs.base;
		svm->soft_int_old_rip = kvm_rip_read(vcpu);
		if (!guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS))
			svm->soft_int_next_rip = kvm_rip_read(vcpu);
	}
}