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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 9/23/26 10:42 AM, Ross Lagerwall wrote:
> 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.
Looking further, patches 1, 3, and 4 also contain seemingly unrelated patches
from our internal patchqueue. Can you resubmit this series without these
patches mixed in? Or if they really are required dependencies, (where they
haven't already been posted to xen-devel previously) submit them as separate
patches at the start of this series, maintaining authorship information.

Ross