Re: [Xen-devel] [PATCH] nvmx: fix resource relinquish for nested VMX

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@xxxxxxxxx] On Behalf Of Keir Fraser
> Sent: Friday, August 24, 2012 4:46 PM
> To: Xu, Dongxiao; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH] nvmx: fix resource relinquish for nested VMX
> 
> On 23/08/2012 04:11, "Dongxiao Xu" <dongxiao.xu@xxxxxxxxx> wrote:
> 
> > The previous order of relinquish resource is:
> > relinquish_domain_resources() -> vcpu_destroy() -> nvmx_vcpu_destroy().
> > However some L1 resources like nv_vvmcx and io_bitmaps are free in
> > nvmx_vcpu_destroy(), therefore the relinquish_domain_resources() will
> > not reduce the refcnt of the domain to 0, therefore the latter vcpu
> > release functions will not be called.
> >
> > To fix this issue, we need to release the nv_vvmcx and io_bitmaps in
> > relinquish_domain_resources().
> >
> > Besides, after destroy the nested vcpu, we need to switch the
> > vmx->vmcs back to the L1 and let the vcpu_destroy() logic to free the L1
> VMCS page.
> >
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> 
> Couple of comments below.
> 
> > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> > index 2e0b79d..1f610eb 100644
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -57,6 +57,9 @@ void nvmx_vcpu_destroy(struct vcpu *v)  {
> >      struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
> >
> > +    if ( nvcpu->nv_n1vmcx )
> > +        v->arch.hvm_vmx.vmcs = nvcpu->nv_n1vmcx;
> 
> Okay, this undoes the fork in nvmx_handle_vmxon()? A small code comment to
> explain that would be handy.

Consider the following case:
When the vcpu is representing the L2 guest, therefore the v->arch.hvm_vmx.vmcs 
points to the L2's VMCS (as known as the shadow VMCS, nvcpu->nv_n2vmcx), and at 
this time, user destroy the L1 guest by "xl destroy", we need to set the 
v->arch.hvm_vmx.vmcs back to L1's VMCS, otherwise, L2's VMCS will be free twice 
and keep L1's VMCS un-freed.
I will add a comment the code.

> 
> >      nvmx_purge_vvmcs(v);
> 
> This call of nvmx_purge_vvmcs() is no longer needed, and should be removed?
Yes, this could be removed. I will send out a new version.

Thanks,
Dongxiao


> 
>  -- Keir
> 
> >      if ( nvcpu->nv_n2vmcx ) {
> >          __vmpclear(virt_to_maddr(nvcpu->nv_n2vmcx));
> > @@ -65,6 +68,14 @@ void nvmx_vcpu_destroy(struct vcpu *v)
> >      }
> >  }
> >
> > +void nvmx_domain_relinquish_resources(struct domain *d) {
> > +    struct vcpu *v;
> > +
> > +    for_each_vcpu ( d, v )
> > +        nvmx_purge_vvmcs(v);
> > +}
> > +
> >  int nvmx_vcpu_reset(struct vcpu *v)
> >  {
> >      return 0;
> > diff --git a/xen/include/asm-x86/hvm/hvm.h
> > b/xen/include/asm-x86/hvm/hvm.h index 7243c4e..3592a8c 100644
> > --- a/xen/include/asm-x86/hvm/hvm.h
> > +++ b/xen/include/asm-x86/hvm/hvm.h
> > @@ -179,6 +179,7 @@ struct hvm_function_table {
> >      bool_t (*nhvm_vmcx_hap_enabled)(struct vcpu *v);
> >
> >      enum hvm_intblk (*nhvm_intr_blocked)(struct vcpu *v);
> > +    void (*nhvm_domain_relinquish_resources)(struct domain *d);
> >  };
> >
> >  extern struct hvm_function_table hvm_funcs; diff --git
> > a/xen/include/asm-x86/hvm/vmx/vvmx.h
> > b/xen/include/asm-x86/hvm/vmx/vvmx.h
> > index 995f9f4..bbc34e7 100644
> > --- a/xen/include/asm-x86/hvm/vmx/vvmx.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
> > @@ -96,6 +96,7 @@ uint32_t nvmx_vcpu_asid(struct vcpu *v);  enum
> > hvm_intblk nvmx_intr_blocked(struct vcpu *v);  int
> > nvmx_intercepts_exception(struct vcpu *v,
> >                                unsigned int trap, int error_code);
> > +void nvmx_domain_relinquish_resources(struct domain *d);
> >
> >  int nvmx_handle_vmxon(struct cpu_user_regs *regs);  int
> > nvmx_handle_vmxoff(struct cpu_user_regs *regs);
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel