Re: [PATCH] x86/svm: require VMSAVEvirt for nested virt

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

> From: Stephen Cheng （程小枫） <stephen.cheng@xxxxxxxxxx>
> Sent: Monday, July 27, 2026 8:17 AM
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Ross Lagerwall; Stephen Cheng （程小枫）; Andrew Cooper; Anthony PERARD; 
> Michal Orzel; Jan Beulich; Julien Grall; Roger Pau Monné; Stefano Stabellini; 
> Teddy Astie; Jason Andryuk
> Subject: [PATCH] x86/svm: require VMSAVEvirt for nested virt
> 
> Virtual VMLOAD/VMSAVE lets an L1 guest execute VMLOAD and VMSAVE
> without intercepts.  Without it, Xen has to map the L1-provided VMCB
> and re-execute each instruction in L0, complicating security-sensitive
> state handling.
> 
> Make VMSAVEvirt a hard requirement for nested SVM.  Advertise it to
> L1, enable it when L1 enables SVM, and remove the nested VMLOAD/VMSAVE
> emulation path.  Any remaining intercept therefore comes from a guest
> which has not enabled SVM and receives #UD.
> 
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Stephen Cheng <stephen.cheng@xxxxxxxxxx>
> ---
>  docs/designs/nested-svm-cpu-features.md |  11 +++
>  xen/arch/x86/cpu-policy.c               |   3 +-
>  xen/arch/x86/hvm/svm/nestedsvm.c        |  24 ++++--
>  xen/arch/x86/hvm/svm/svm.c              | 100 +++---------------------
>  xen/arch/x86/hvm/svm/svm.h              |   2 -
>  5 files changed, 41 insertions(+), 99 deletions(-)
> 
> diff --git a/docs/designs/nested-svm-cpu-features.md 
> b/docs/designs/nested-svm-cpu-features.md
> index ce168e68e1..798480b259 100644
> --- a/docs/designs/nested-svm-cpu-features.md
> +++ b/docs/designs/nested-svm-cpu-features.md
> @@ -109,3 +109,14 @@ leaf 8000000A:edx
>    Using it in L0 reduces the chance that we'll make some sort of error
>    in the decode path.  And if hardware supports it, it's easy enough
>    to provide to the L1.
> +
> +- 15 `VLoadSave` *Virtual VMLOAD/VMSAVE*: Require for L0, provide to L1
> +
> +  Without this feature Xen has to intercept the L1 hypervisor's VMLOAD
> +  and VMSAVE instructions and emulate them by re-executing the real
> +  instruction on a mapped copy of the L1-supplied VMCB.  That path
> +  handles a complex, security-sensitive subset of state (the hidden
> +  segment descriptors for FS/GS/TR/LDTR plus the SYSCALL/SYSENTER
> +  MSRs), so on faithfulness grounds we'd much rather let the hardware
> +  do it.  When present, the instructions execute natively in the guest
> +  without a #VMEXIT, which is both simpler and faster.
> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
> index 5273fe0ae4..f151623f5e 100644
> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -837,7 +837,8 @@ static void __init calculate_hvm_max_policy(void)
>                                 (1u << SVM_FEATURE_LBRV) |
>                                 (1u << SVM_FEATURE_NRIPS) |
>                                 (1u << SVM_FEATURE_PAUSEFILTER) |
> -                               (1u << SVM_FEATURE_DECODEASSISTS));
> +                               (1u << SVM_FEATURE_DECODEASSISTS) |
> +                               (1u << SVM_FEATURE_VLOADSAVE));

Exposing this feature to L1 is separate from requiring it in L0 and IMO should
be a separate commit. If it's exposed to L1, there is a lot more to think
about. E.g. whether L2 should follow L1's setting or L0's for vloadsave_enable
and if it is not enabled what happens if L1 doesn't intercept VMLOAD/VMSAVE.

Also note we discovered previously that contrary to what the APM documents:

If the VMSAVE/VMLOAD intercept bits are clear, and virtual
VMLOAD/VMSAVE is not enabled, then a VMSAVE instruction executed in
guest mode executes as if it were in non-root mode, and operates on host
physical addresses.

I think there is also a separate bug that what Xen advertises doesn't
necessarily control what the guest can use. E.g. previously we didn't advertise
SVM_FEATURE_VLOADSAVE but Xen would always merge in tthe virt_ext settings from
VMCB(L1-L2).

>          /* Enable features which are always emulated. */
>          p->extd.raw[0xa].d |= (1u << SVM_FEATURE_VMCBCLEAN);
>      }
> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c 
> b/xen/arch/x86/hvm/svm/nestedsvm.c
> index b06124c2c9..794fb6e589 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -562,7 +562,10 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, 
> struct cpu_user_regs *regs)
> 
>      /* Keep the host values of the fs, gs, ldtr, tr, kerngsbase,
>       * star, lstar, cstar, sfmask, sysenter_cs, sysenter_esp,
> -     * sysenter_eip. These are handled via VMSAVE/VMLOAD emulation.
> +     * sysenter_eip. These are not transferred by VMRUN/#VMEXIT; they
> +     * are moved directly to/from the L1-provided VMCB by the guest's
> +     * own VMSAVE/VMLOAD, which run natively (VMSAVEvirt is required
> +     * for nested virt).
>       */
> 
>      /* PAT */
> @@ -1097,7 +1100,10 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct 
> cpu_user_regs *regs)
> 
>      /* Keep the l2 guest values of the fs, gs, ldtr, tr, kerngsbase,
>       * star, lstar, cstar, sfmask, sysenter_cs, sysenter_esp,
> -     * sysenter_eip. These are handled via VMSAVE/VMLOAD emulation.
> +     * sysenter_eip. These are not transferred by VMRUN/#VMEXIT; they
> +     * are moved directly to/from the L1-provided VMCB by the guest's
> +     * own VMSAVE/VMLOAD, which run natively (VMSAVEvirt is required
> +     * for nested virt).
>       */
> 
>      /* CR2 */
> @@ -1547,9 +1553,14 @@ void svm_nested_features_on_efer_update(struct vcpu *v)
>       */
>      if ( nsvm_efer_svm_enabled(v) )
>      {
> +        /*
> +         * VMSAVEvirt is a hard requirement for nested virt (see
> +         * start_nested_svm()), so it is guaranteed present here.
> +         */
> +        ASSERT(cpu_has_svm_vloadsave);
> +

Maybe just personal taste but I think this assert is unnecessary.
cpu_has_svm_vloadsave is constant since boot and also checked at boot so
further checks are unnecessary.

>          if ( !vmcb->virt_ext.fields.vloadsave_enable &&
> -             paging_mode_hap(v->domain) &&
> -             cpu_has_svm_vloadsave )
> +             paging_mode_hap(v->domain) )
>          {
>              vmcb->virt_ext.fields.vloadsave_enable = 1;
>              general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> @@ -1603,9 +1614,10 @@ void __init start_nested_svm(struct hvm_function_table 
> *hvm_function_table)
>       * docs/designs/nested-svm-cpu-features.md for rationale.
>       */
>      hvm_function_table->caps.nested_virt =
> -        hvm_function_table->caps.hap &&
> +        hvm_function_table->caps.hap &&
>          cpu_has_svm_lbrv &&
>          cpu_has_svm_nrips &&
>          cpu_has_svm_flushbyasid &&
> -        cpu_has_svm_decode;
> +        cpu_has_svm_decode &&
> +        cpu_has_svm_vloadsave;
>  }
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 38c61db1d7..637c29daf7 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2134,95 +2134,17 @@ svm_vmexit_do_vmrun(struct cpu_user_regs *regs,
>      return;
>  }
> 
> -static struct page_info *
> -nsvm_get_nvmcb_page(struct vcpu *v, uint64_t vmcbaddr)
> -{
> -    p2m_type_t p2mt;
> -    struct page_info *page;
> -    struct nestedvcpu *nv = &vcpu_nestedhvm(v);
> -
> -    if ( !nestedsvm_vmcb_map(v, vmcbaddr) )
> -        return NULL;
> -
> -    /* Need to translate L1-GPA to MPA */
> -    page = get_page_from_gfn(v->domain, nv->nv_vvmcxaddr >> PAGE_SHIFT,
> -                             &p2mt, P2M_ALLOC | P2M_UNSHARE);
> -    if ( !page )
> -        return NULL;
> -
> -    if ( !p2m_is_ram(p2mt) || p2m_is_readonly(p2mt) )
> -    {
> -        put_page(page);
> -        return NULL;
> -    }
> -
> -    return  page;
> -}
> -
> -static void
> -svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
> -                     struct cpu_user_regs *regs,
> -                     struct vcpu *v, uint64_t vmcbaddr)
> -{
> -    unsigned int inst_len;
> -    struct page_info *page;
> -
> -    if ( (inst_len = svm_get_insn_len(v, INSTR_VMLOAD)) == 0 )
> -        return;
> -
> -    if ( !nsvm_efer_svm_enabled(v) )
> -    {
> -        hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
> -        return;
> -    }
> -
> -    page = nsvm_get_nvmcb_page(v, vmcbaddr);
> -    if ( !page )
> -    {
> -        gdprintk(XENLOG_ERR,
> -            "VMLOAD: mapping failed, injecting #GP\n");
> -        hvm_inject_hw_exception(X86_EXC_GP, 0);
> -        return;
> -    }
> -
> -    svm_vmload_pa(page_to_maddr(page));
> -    put_page(page);
> -
> -    /* State in L1 VMCB is stale now */
> -    v->arch.hvm.svm.vmcb_sync_state = vmcb_needs_vmsave;
> -
> -    __update_guest_eip(regs, inst_len);
> -}
> -
>  static void
> -svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
> -                     struct cpu_user_regs *regs,
> -                     struct vcpu *v, uint64_t vmcbaddr)
> +svm_vmexit_do_vmload_vmsave(struct vcpu *v)
>  {
> -    unsigned int inst_len;
> -    struct page_info *page;
> -
> -    if ( (inst_len = svm_get_insn_len(v, INSTR_VMSAVE)) == 0 )
> -        return;
> -
> -    if ( !nsvm_efer_svm_enabled(v) )
> -    {
> -        hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
> -        return;
> -    }
> -
> -    page = nsvm_get_nvmcb_page(v, vmcbaddr);
> -    if ( !page )
> -    {
> -        gdprintk(XENLOG_ERR,
> -            "VMSAVE: mapping vmcb failed, injecting #GP\n");
> -        hvm_inject_hw_exception(X86_EXC_GP, 0);
> -        return;
> -    }
> -
> -    svm_vmsave_pa(page_to_maddr(page));
> -    put_page(page);
> -    __update_guest_eip(regs, inst_len);
> +    /*
> +     * VMSAVEvirt is required for nested virt, so once a guest enables SVM 
> its
> +     * VMLOAD/VMSAVE execute natively and are no longer intercepted.  An
> +     * intercepted VMLOAD/VMSAVE therefore means the guest has not enabled 
> SVM:
> +     * inject #UD.
> +     */
> +    ASSERT(!nsvm_efer_svm_enabled(v));
> +    hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC);
>  }
> 
>  static int svm_is_erratum_383(struct cpu_user_regs *regs)
> @@ -2950,10 +2872,8 @@ void asmlinkage svm_vmexit_handler(void)
>          svm_vmexit_do_vmrun(regs, v, regs->rax);
>          break;
>      case VMEXIT_VMLOAD:
> -        svm_vmexit_do_vmload(vmcb, regs, v, regs->rax);
> -        break;
>      case VMEXIT_VMSAVE:
> -        svm_vmexit_do_vmsave(vmcb, regs, v, regs->rax);
> +        svm_vmexit_do_vmload_vmsave(v);
>          break;
>      case VMEXIT_STGI:
>          svm_vmexit_do_stgi(regs, v);

I think it would be simpler to just move these case statements up to the
VMEXIT_RDPRU section.

> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
> index cfa411ad5a..56a1a4840b 100644
> --- a/xen/arch/x86/hvm/svm/svm.h
> +++ b/xen/arch/x86/hvm/svm/svm.h
> @@ -52,8 +52,6 @@ static inline void svm_invlpga(unsigned long linear, 
> uint32_t asid)
>  #define INSTR_XSETBV     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321) /* 
> octal-ok */
>  #define INSTR_VMRUN      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330) /* 
> octal-ok */
>  #define INSTR_VMCALL     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331) /* 
> octal-ok */
> -#define INSTR_VMLOAD     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332) /* 
> octal-ok */
> -#define INSTR_VMSAVE     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333) /* 
> octal-ok */
>  #define INSTR_STGI       INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334) /* 
> octal-ok */
>  #define INSTR_CLGI       INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335) /* 
> octal-ok */
>  #define INSTR_INVLPGA    INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337) /* 
> octal-ok */
> 
> base-commit: 644ea6f84510af27ee1847e981df945a94068479
> --
> 2.49.0

Thanks,
Ross