Re: [PATCH 5/9] x86/vvmx: Make updating shadow EPTP value more efficient

[Sergey Dyasli <sergey.dyasli@citrix.com>]

On Fri, 2017-09-29 at 16:56 +0100, Andrew Cooper wrote:
> On 29/09/17 16:01, George Dunlap wrote:
> > @@ -4203,13 +4197,17 @@ static void lbr_fixup(void)
> >          bdw_erratum_bdf14_fixup();
> >  }
> >  
> > -void vmx_vmenter_helper(const struct cpu_user_regs *regs)
> > +int vmx_vmenter_helper(const struct cpu_user_regs *regs)
> 
> What are the semantics of this call?  The result looks boolean, and
> indicates that the vmentry should be aborted?

Currently vmx_vmenter_helper() returns !0 if the vmentry must be
restarted.

> 
> >  {
> >      struct vcpu *curr = current;
> >      u32 new_asid, old_asid;
> >      struct hvm_vcpu_asid *p_asid;
> >      bool_t need_flush;
> >  
> > +    /* Shadow EPTP can't be updated here because irqs are disabled */
> > +     if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m )
> > +         return 1;
> > +
> >      if ( curr->domain->arch.hvm_domain.pi_ops.do_resume )
> >          curr->domain->arch.hvm_domain.pi_ops.do_resume(curr);
> >  
> > @@ -4270,6 +4268,8 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs)
> >      __vmwrite(GUEST_RIP,    regs->rip);
> >      __vmwrite(GUEST_RSP,    regs->rsp);
> >      __vmwrite(GUEST_RFLAGS, regs->rflags | X86_EFLAGS_MBS);
> > +
> > +    return 0;
> >  }
> >  
> >  /*
> > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> > index 2f468e6ced..48e37158af 100644
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -1405,12 +1405,32 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
> >      vmsucceed(regs);
> >  }
> >  
> > +static void nvmx_eptp_update(void)
> > +{
> 
> struct vcpu *curr = current; will most likely half the compiled size of
> this function.

Yes, passing a struct vcpu *v to nvmx_eptp_update() removes all
the additional:

    mov    %rsp,%rax
    or     $0x7fff,%rax
    
I wasn't aware of such behavior and will correct the usage of current
for all patches in v3.

> 
> > +    if ( !nestedhvm_vcpu_in_guestmode(current) ||
> > +          vcpu_nestedhvm(current).nv_vmexit_pending ||
> > +         !vcpu_nestedhvm(current).stale_np2m ||
> > +         !nestedhvm_paging_mode_hap(current) )
> > +        return;
> > +
> > +    /*
> > +     * Interrupts are enabled here, so we need to clear stale_np2m
> > +     * before we do the vmwrite.  If we do it in the other order, an
> > +     * and IPI comes in changing the shadow eptp after the vmwrite,
> > +     * we'll complete the vmenter with a stale eptp value.
> > +     */
> > +    vcpu_nestedhvm(current).stale_np2m = false;
> > +    __vmwrite(EPT_POINTER, get_shadow_eptp(current));
> > +}
> > +
> >  void nvmx_switch_guest(void)
> >  {
> >      struct vcpu *v = current;
> >      struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
> >      struct cpu_user_regs *regs = guest_cpu_user_regs();
> >  
> > +    nvmx_eptp_update();
> > +
> >      /*
> >       * A pending IO emulation may still be not finished. In this case, no
> >       * virtual vmswitch is allowed. Or else, the following IO emulation will
> > diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
> > index 6c54773f1c..5cfa4b4aa4 100644
> > --- a/xen/include/asm-x86/hvm/vcpu.h
> > +++ b/xen/include/asm-x86/hvm/vcpu.h
> > @@ -115,6 +115,7 @@ struct nestedvcpu {
> >  
> >      bool_t nv_flushp2m; /* True, when p2m table must be flushed */
> >      struct p2m_domain *nv_p2m; /* used p2m table for this vcpu */
> > +    bool stale_np2m; /* True when p2m_base in VMCX02 is no longer valid */
> 
> VMCx02 ? which helps distinguish the two parts of semantic information
> encoded there, and to avoid looking like we've gained a third acronym.

I like this suggestion. Will update comments and commit messages for all
patches in v3.

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel