* [PATCH v3] x86/nhvm: properly clean up after failure to set up all vCPU-s
@ 2013-02-21 13:22 Jan Beulich
2013-02-21 14:09 ` Tim Deegan
2013-02-21 14:42 ` Olaf Hering
0 siblings, 2 replies; 3+ messages in thread
From: Jan Beulich @ 2013-02-21 13:22 UTC (permalink / raw)
To: xen-devel; +Cc: Tim Deegan, Olaf Hering, Eddie Dong, Jun Nakajima
[-- Attachment #1: Type: text/plain, Size: 4396 bytes --]
Otherwise we may leak memory when setting up nHVM fails half way.
This implies that the individual destroy functions will have to remain
capable (in the VMX case they first need to be made so, following
26486:7648ef657fe7 and 26489:83a3fa9c8434) of being called for a vCPU
that the corresponding init function was never run on.
Once at it, also remove a redundant check from the corresponding
parameter validation code.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Make sure we fully tear down nHVM when the parameter gets set to 0.
v2: nVMX fixes required by 26486:7648ef657fe7 and 26489:83a3fa9c8434.
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3918,18 +3918,20 @@ long do_hvm_op(unsigned long op, XEN_GUE
}
if ( a.value > 1 )
rc = -EINVAL;
- if ( !is_hvm_domain(d) )
- rc = -EINVAL;
/* Remove the check below once we have
* shadow-on-shadow.
*/
if ( cpu_has_svm && !paging_mode_hap(d) && a.value )
rc = -EINVAL;
/* Set up NHVM state for any vcpus that are already up */
- if ( !d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] )
+ if ( a.value &&
+ !d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] )
for_each_vcpu(d, v)
if ( rc == 0 )
rc = nestedhvm_vcpu_initialise(v);
+ if ( !a.value || rc )
+ for_each_vcpu(d, v)
+ nestedhvm_vcpu_destroy(v);
break;
case HVM_PARAM_BUFIOREQ_EVTCHN:
rc = -EINVAL;
--- a/xen/arch/x86/hvm/nestedhvm.c
+++ b/xen/arch/x86/hvm/nestedhvm.c
@@ -87,7 +87,7 @@ nestedhvm_vcpu_initialise(struct vcpu *v
void
nestedhvm_vcpu_destroy(struct vcpu *v)
{
- if ( nestedhvm_enabled(v->domain) && hvm_funcs.nhvm_vcpu_destroy )
+ if ( hvm_funcs.nhvm_vcpu_destroy )
hvm_funcs.nhvm_vcpu_destroy(v);
}
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -62,7 +62,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
if ( !nvcpu->nv_n2vmcx )
{
gdprintk(XENLOG_ERR, "nest: allocation for shadow vmcs failed\n");
- goto out;
+ return -ENOMEM;
}
/* non-root VMREAD/VMWRITE bitmap. */
@@ -75,7 +75,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
if ( !vmread_bitmap )
{
gdprintk(XENLOG_ERR, "nest: allocation for vmread bitmap failed\n");
- goto out1;
+ return -ENOMEM;
}
v->arch.hvm_vmx.vmread_bitmap = vmread_bitmap;
@@ -83,7 +83,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
if ( !vmwrite_bitmap )
{
gdprintk(XENLOG_ERR, "nest: allocation for vmwrite bitmap failed\n");
- goto out2;
+ return -ENOMEM;
}
v->arch.hvm_vmx.vmwrite_bitmap = vmwrite_bitmap;
@@ -118,12 +118,6 @@ int nvmx_vcpu_initialise(struct vcpu *v)
nvmx->msrbitmap = NULL;
INIT_LIST_HEAD(&nvmx->launched_list);
return 0;
-out2:
- free_domheap_page(v->arch.hvm_vmx.vmread_bitmap);
-out1:
- free_xenheap_page(nvcpu->nv_n2vmcx);
-out:
- return -ENOMEM;
}
void nvmx_vcpu_destroy(struct vcpu *v)
@@ -147,16 +141,24 @@ void nvmx_vcpu_destroy(struct vcpu *v)
nvcpu->nv_n2vmcx = NULL;
}
- list_for_each_entry_safe(item, n, &nvmx->launched_list, node)
- {
- list_del(&item->node);
- xfree(item);
- }
+ /* Must also cope with nvmx_vcpu_initialise() not having got called. */
+ if ( nvmx->launched_list.next )
+ list_for_each_entry_safe(item, n, &nvmx->launched_list, node)
+ {
+ list_del(&item->node);
+ xfree(item);
+ }
if ( v->arch.hvm_vmx.vmread_bitmap )
+ {
free_domheap_page(v->arch.hvm_vmx.vmread_bitmap);
+ v->arch.hvm_vmx.vmread_bitmap = NULL;
+ }
if ( v->arch.hvm_vmx.vmwrite_bitmap )
+ {
free_domheap_page(v->arch.hvm_vmx.vmwrite_bitmap);
+ v->arch.hvm_vmx.vmwrite_bitmap = NULL;
+ }
}
void nvmx_domain_relinquish_resources(struct domain *d)
[-- Attachment #2: x86-nhvm-enable-failure.patch --]
[-- Type: text/plain, Size: 4458 bytes --]
x86/nhvm: properly clean up after failure to set up all vCPU-s
Otherwise we may leak memory when setting up nHVM fails half way.
This implies that the individual destroy functions will have to remain
capable (in the VMX case they first need to be made so, following
26486:7648ef657fe7 and 26489:83a3fa9c8434) of being called for a vCPU
that the corresponding init function was never run on.
Once at it, also remove a redundant check from the corresponding
parameter validation code.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Make sure we fully tear down nHVM when the parameter gets set to 0.
v2: nVMX fixes required by 26486:7648ef657fe7 and 26489:83a3fa9c8434.
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3918,18 +3918,20 @@ long do_hvm_op(unsigned long op, XEN_GUE
}
if ( a.value > 1 )
rc = -EINVAL;
- if ( !is_hvm_domain(d) )
- rc = -EINVAL;
/* Remove the check below once we have
* shadow-on-shadow.
*/
if ( cpu_has_svm && !paging_mode_hap(d) && a.value )
rc = -EINVAL;
/* Set up NHVM state for any vcpus that are already up */
- if ( !d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] )
+ if ( a.value &&
+ !d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] )
for_each_vcpu(d, v)
if ( rc == 0 )
rc = nestedhvm_vcpu_initialise(v);
+ if ( !a.value || rc )
+ for_each_vcpu(d, v)
+ nestedhvm_vcpu_destroy(v);
break;
case HVM_PARAM_BUFIOREQ_EVTCHN:
rc = -EINVAL;
--- a/xen/arch/x86/hvm/nestedhvm.c
+++ b/xen/arch/x86/hvm/nestedhvm.c
@@ -87,7 +87,7 @@ nestedhvm_vcpu_initialise(struct vcpu *v
void
nestedhvm_vcpu_destroy(struct vcpu *v)
{
- if ( nestedhvm_enabled(v->domain) && hvm_funcs.nhvm_vcpu_destroy )
+ if ( hvm_funcs.nhvm_vcpu_destroy )
hvm_funcs.nhvm_vcpu_destroy(v);
}
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -62,7 +62,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
if ( !nvcpu->nv_n2vmcx )
{
gdprintk(XENLOG_ERR, "nest: allocation for shadow vmcs failed\n");
- goto out;
+ return -ENOMEM;
}
/* non-root VMREAD/VMWRITE bitmap. */
@@ -75,7 +75,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
if ( !vmread_bitmap )
{
gdprintk(XENLOG_ERR, "nest: allocation for vmread bitmap failed\n");
- goto out1;
+ return -ENOMEM;
}
v->arch.hvm_vmx.vmread_bitmap = vmread_bitmap;
@@ -83,7 +83,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
if ( !vmwrite_bitmap )
{
gdprintk(XENLOG_ERR, "nest: allocation for vmwrite bitmap failed\n");
- goto out2;
+ return -ENOMEM;
}
v->arch.hvm_vmx.vmwrite_bitmap = vmwrite_bitmap;
@@ -118,12 +118,6 @@ int nvmx_vcpu_initialise(struct vcpu *v)
nvmx->msrbitmap = NULL;
INIT_LIST_HEAD(&nvmx->launched_list);
return 0;
-out2:
- free_domheap_page(v->arch.hvm_vmx.vmread_bitmap);
-out1:
- free_xenheap_page(nvcpu->nv_n2vmcx);
-out:
- return -ENOMEM;
}
void nvmx_vcpu_destroy(struct vcpu *v)
@@ -147,16 +141,24 @@ void nvmx_vcpu_destroy(struct vcpu *v)
nvcpu->nv_n2vmcx = NULL;
}
- list_for_each_entry_safe(item, n, &nvmx->launched_list, node)
- {
- list_del(&item->node);
- xfree(item);
- }
+ /* Must also cope with nvmx_vcpu_initialise() not having got called. */
+ if ( nvmx->launched_list.next )
+ list_for_each_entry_safe(item, n, &nvmx->launched_list, node)
+ {
+ list_del(&item->node);
+ xfree(item);
+ }
if ( v->arch.hvm_vmx.vmread_bitmap )
+ {
free_domheap_page(v->arch.hvm_vmx.vmread_bitmap);
+ v->arch.hvm_vmx.vmread_bitmap = NULL;
+ }
if ( v->arch.hvm_vmx.vmwrite_bitmap )
+ {
free_domheap_page(v->arch.hvm_vmx.vmwrite_bitmap);
+ v->arch.hvm_vmx.vmwrite_bitmap = NULL;
+ }
}
void nvmx_domain_relinquish_resources(struct domain *d)
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] x86/nhvm: properly clean up after failure to set up all vCPU-s
2013-02-21 13:22 [PATCH v3] x86/nhvm: properly clean up after failure to set up all vCPU-s Jan Beulich
@ 2013-02-21 14:09 ` Tim Deegan
2013-02-21 14:42 ` Olaf Hering
1 sibling, 0 replies; 3+ messages in thread
From: Tim Deegan @ 2013-02-21 14:09 UTC (permalink / raw)
To: Jan Beulich; +Cc: Olaf Hering, Eddie Dong, Jun Nakajima, xen-devel
At 13:22 +0000 on 21 Feb (1361452963), Jan Beulich wrote:
> Otherwise we may leak memory when setting up nHVM fails half way.
>
> This implies that the individual destroy functions will have to remain
> capable (in the VMX case they first need to be made so, following
> 26486:7648ef657fe7 and 26489:83a3fa9c8434) of being called for a vCPU
> that the corresponding init function was never run on.
>
> Once at it, also remove a redundant check from the corresponding
> parameter validation code.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Tim Deegan <tim@xen.org>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] x86/nhvm: properly clean up after failure to set up all vCPU-s
2013-02-21 13:22 [PATCH v3] x86/nhvm: properly clean up after failure to set up all vCPU-s Jan Beulich
2013-02-21 14:09 ` Tim Deegan
@ 2013-02-21 14:42 ` Olaf Hering
1 sibling, 0 replies; 3+ messages in thread
From: Olaf Hering @ 2013-02-21 14:42 UTC (permalink / raw)
To: Jan Beulich; +Cc: Tim Deegan, Eddie Dong, Jun Nakajima, xen-devel
On Thu, Feb 21, Jan Beulich wrote:
> Otherwise we may leak memory when setting up nHVM fails half way.
>
> This implies that the individual destroy functions will have to remain
> capable (in the VMX case they first need to be made so, following
> 26486:7648ef657fe7 and 26489:83a3fa9c8434) of being called for a vCPU
> that the corresponding init function was never run on.
>
> Once at it, also remove a redundant check from the corresponding
> parameter validation code.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Olaf Hering <olaf@aepfle.de>
This one fixes the crash as well. Thanks.
Olaf
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-02-21 14:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-21 13:22 [PATCH v3] x86/nhvm: properly clean up after failure to set up all vCPU-s Jan Beulich
2013-02-21 14:09 ` Tim Deegan
2013-02-21 14:42 ` Olaf Hering
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.