Re: [PATCH v1 5/5] x86/nestedsvm: fix up

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 9/23/26 10:20 AM, chunjie.zhu@xxxxxxxxxx wrote:
> From: Chunjie Zhu <chunjie.zhu@xxxxxxxxxx>
>
> Signed-off-by: Chunjie Zhu <chunjie.zhu@xxxxxxxxxx>
> ---
>   xen/arch/x86/hvm/svm/nestedsvm.c | 19 ++++++++++++++++---
>   xen/arch/x86/hvm/svm/vmcb.c      |  9 +++++----
>   xen/arch/x86/mm/p2m.c            |  5 +++++
>   3 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
> index 82e01e513e69..91872af8aa7b 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -355,7 +355,7 @@ static void nestedsvm_vmcb_set_nestedp2m(struct vcpu *v,
>       vcpu_nestedsvm(v).ns_vmcb_hostcr3 = vvmcb->_h_cr3;
>   
>       p2m = p2m_get_nestedp2m(v);
> -    n2vmcb->_h_cr3 = pagetable_get_paddr(p2m_get_pagetable(p2m));
> +    vmcb_set_h_cr3(n2vmcb, pagetable_get_paddr(p2m_get_pagetable(p2m)));
>   }
>   
>   void nsvm_vcpu_update_nestedp2m(struct vcpu *v)
> @@ -373,6 +373,7 @@ void nsvm_vcpu_update_nestedp2m(struct vcpu *v)
>           return;
>   
>       p2m = p2m_get_nestedp2m(v);
> +    nv->stale_np2m = false;
>   
>       /*
>        * This may happen if we've handled VMEXIT_NPF at the same time as a
> @@ -383,8 +384,6 @@ void nsvm_vcpu_update_nestedp2m(struct vcpu *v)
>            vmcb_get_h_cr3(nv->nv_n2vmcx) !=
>            pagetable_get_paddr(p2m_get_pagetable(p2m)) )
>           nestedsvm_vmcb_set_nestedp2m(v, nv->nv_vvmcx, nv->nv_n2vmcx);
> -
> -    nv->stale_np2m = false;
>   }
>   
>   static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
> @@ -1024,6 +1023,20 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct 
> cpu_user_regs *regs)
>       struct vmcb_struct *ns_vmcb = nv->nv_vvmcx;
>       struct vmcb_struct *n2vmcb = nv->nv_n2vmcx;
>   
> +    ASSERT(v == current);
> +
> +    /*
> +     * The physical VMCB fields covered by VMSAVE/VMLOAD may not be in sync
> +     * with v's vmcb if a context switch happened since the last VMLOAD.
> +     * VMSAVE below would otherwise capture stale/foreign register state
> +     * into the L1 shadow VMCB.
> +     */
> +    if ( v->arch.hvm.svm.vmcb_sync_state == vmcb_needs_vmload )
> +    {
> +        svm_vmload_pa(v->arch.hvm.svm.vmcb_pa);
> +        v->arch.hvm.svm.vmcb_sync_state = vmcb_in_sync;
> +    }
> +
>       svm_vmsave_pa(nv->nv_n1vmcx_pa);
>   
>       /* Cache guest physical address of virtual vmcb
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 5354c4f1b85f..2f7053ed7eec 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -357,10 +357,11 @@ bool svm_vmcb_isvalid(
>           PRINTF("CR0: bits [63:32] are not zero (%#"PRIx64")\n", cr0);
>   
>       if ( (cr0 & X86_CR0_PG) &&
> -         ((cr3 & 7) ||
> -          ((!(cr4 & X86_CR4_PAE) || (efer & EFER_LMA)) && (cr3 & 0xfe0)) ||
> -          ((efer & EFER_LMA) &&
> -           (cr3 >> v->domain->arch.cpuid->extd.maxphysaddr))) )
> +         ((!(cr4 & X86_CR4_PCIDE) &&
> +           ((cr3 & 7) ||
> +            ((!(cr4 & X86_CR4_PAE) || (efer & EFER_LMA)) &&
> +             (cr3 & 0xfe0)))) ||
> +          (cr3 >> v->domain->arch.cpuid->extd.maxphysaddr)) )
>           PRINTF("CR3: MBZ bits are set (%#"PRIx64")\n", cr3);
>   
>       valid = hvm_cr4_guest_valid_bits(v->domain);
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 027b9ae69be3..d1fafc479c78 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1517,7 +1517,12 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
>       np2m_base &= ~(0xfffULL);
>   
>       if ( nv->nv_flushp2m && nv->nv_p2m )
> +    {
> +        p2m = nv->nv_p2m;
> +        if ( p2m )
> +            p2m_flush_table(p2m);
>           nv->nv_p2m = NULL;
> +    }
>   
>       nestedp2m_lock(d);
>       p2m = nv->nv_p2m;
This seems to have combined several bug fixes from our internal patchqueue (I
know because I wrote some of them...), some of which have already been posted
to the list.

It's not clear why they were submitted as part of this series.

Ross