Re: [PATCH] x86/svm: mandatory update VMCB nextrip for soft interrupts

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/08/2026 4:17 am, Chunjie Zhu wrote:
> Signed-off-by: Chunjie Zhu <chunjie.zhu@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/svm/nestedsvm.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c 
> b/xen/arch/x86/hvm/svm/nestedsvm.c
> index b06124c2c9ed..815713b8b506 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -449,7 +449,14 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, 
> struct cpu_user_regs *regs)
>      n2vmcb->virt_ext.bytes =
>          n1vmcb->virt_ext.bytes | ns_vmcb->virt_ext.bytes;
>  
> -    /* NextRIP - only evaluated on #VMEXIT. */
> +    /* next_rip is consumed on VMRUN as the return address pushed on the
> +     * stack·for·injected·soft·exceptions/interrupts. This assignment
> +     * statement must be enforced, otherwise, it might cause vcpu wedge.
> +     *
> +     * APM Vol.2 Event Injection does not specifies what happens if NEXTRIP
> +     * holds an invalid/garbage value.
> +     */
> +    n2vmcb->nextrip = ns_vmcb->nextrip;

The old comment is indeed wrong.  However, this comment is distinctly
out-of-character for the function too.

As for the text, your subject probably wants to be "x86/svm: Sync
nextrip during virtual VMRUN".

The commit message needs to say that the NextRIP field is consumed
during event injection, with the L1 hypervisor being required to
configure it appropriately.  For a garbage value, the APM does say; it
states that nextrip is always what's used for traps.  Nothing checks
that ->rip and ->nextrip are within 15 bytes, and you can it to
arbitrary values and hardware will put the value on the stack.


Finally, there is a fun problem when running an L1 hypervisor which
can't see NRIPS on hardware which does have NRIPS.  In that case, the
correct sync is actually:

    n2vmcb->nextrip = cp->extd.nrips ? ns_vmcb->nextrip : ns_vmcb->rip;

because for event injection the L1 hypervisor will have had to move
->rip forwards for anything with trap semantics, but the real VMRUN will
still read ->nextrip even if the L1 hypervisor didn't know about it.

Note, we don't actually have the cp->extd.nrips field yet.  I need to
dust off another series to get that fixed.


NRIPs has been around almost forever in SVM terms.  It was also a
breaking change when AMD added it to hardware; while you can ignore the
field for the purposes of instruction length calculations, event
injection still requires it to be set correctly.

We only offer nested virt on NRIPS-capable hardware, but we don't have
any interlock concerning the guest visibility of NRIPS.  Given it's age,
I suggest we don't try and care about guests which can't see NRIPS, so
our safety reasoning becomes:

* Nested Virt is only available on hardware with NRIPS
* NRIPS is required to be advertised [*]
* Therefore the L1 hypervisor is required to set ->nrips correctly
* Therefore we can just do a simple sync with no conditionals

~Andrew