| Message ID | 20200124153103.18321-5-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | purge free_shared_domheap_page() | expand |
On 24.01.2020 16:31, Paul Durrant wrote: > ... to domain_relinquish_resources(). > > The teardown code frees the APICv page. This does not need to be done late > so do it in domain_relinquish_resources() rather than domain_destroy(). For the normal domain destruction path this is fine. For the error path of domain_create(), however, this will leak the page, as in this case hvm_domain_relinquish_resources() won't be called. I'm afraid there already is a similar issue with e.g. viridian_domain_deinit(). I guess I'll make a patch. Jan > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > --- > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > Cc: George Dunlap <george.dunlap@citrix.com> > > v4: > - New in v4 (disaggregated from v3 patch #3) > --- > xen/arch/x86/hvm/vmx/vmx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index b262d38a7c..606f3dc2eb 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -419,7 +419,7 @@ static int vmx_domain_initialise(struct domain *d) > return 0; > } > > -static void vmx_domain_destroy(struct domain *d) > +static void vmx_domain_relinquish_resources(struct domain *d) > { > if ( !has_vlapic(d) ) > return; > @@ -2240,7 +2240,7 @@ static struct hvm_function_table __initdata vmx_function_table = { > .cpu_up_prepare = vmx_cpu_up_prepare, > .cpu_dead = vmx_cpu_dead, > .domain_initialise = vmx_domain_initialise, > - .domain_destroy = vmx_domain_destroy, > + .domain_relinquish_resources = vmx_domain_relinquish_resources, > .vcpu_initialise = vmx_vcpu_initialise, > .vcpu_destroy = vmx_vcpu_destroy, > .save_cpu_ctxt = vmx_save_vmcs_ctxt, >
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 28 January 2020 08:15 > To: Durrant, Paul <pdurrant@amazon.co.uk> > Cc: xen-devel@lists.xenproject.org; Jun Nakajima <jun.nakajima@intel.com>; > Kevin Tian <kevin.tian@intel.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné > <roger.pau@citrix.com>; George Dunlap <george.dunlap@citrix.com> > Subject: Re: [PATCH v4 4/7] x86 / vmx: move teardown from > domain_destroy()... > > On 24.01.2020 16:31, Paul Durrant wrote: > > ... to domain_relinquish_resources(). > > > > The teardown code frees the APICv page. This does not need to be done > late > > so do it in domain_relinquish_resources() rather than domain_destroy(). > > For the normal domain destruction path this is fine. For the error path > of domain_create(), however, this will leak the page, as in this case > hvm_domain_relinquish_resources() won't be called. Well it's really arch_domain_create() that's at fault but, yes that needs fixing. > I'm afraid there > already is a similar issue with e.g. viridian_domain_deinit(). I guess > I'll make a patch. > Ok, thanks. Paul > Jan > > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > > --- > > Cc: Jun Nakajima <jun.nakajima@intel.com> > > Cc: Kevin Tian <kevin.tian@intel.com> > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: Wei Liu <wl@xen.org> > > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > > Cc: George Dunlap <george.dunlap@citrix.com> > > > > v4: > > - New in v4 (disaggregated from v3 patch #3) > > --- > > xen/arch/x86/hvm/vmx/vmx.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > > index b262d38a7c..606f3dc2eb 100644 > > --- a/xen/arch/x86/hvm/vmx/vmx.c > > +++ b/xen/arch/x86/hvm/vmx/vmx.c > > @@ -419,7 +419,7 @@ static int vmx_domain_initialise(struct domain *d) > > return 0; > > } > > > > -static void vmx_domain_destroy(struct domain *d) > > +static void vmx_domain_relinquish_resources(struct domain *d) > > { > > if ( !has_vlapic(d) ) > > return; > > @@ -2240,7 +2240,7 @@ static struct hvm_function_table __initdata > vmx_function_table = { > > .cpu_up_prepare = vmx_cpu_up_prepare, > > .cpu_dead = vmx_cpu_dead, > > .domain_initialise = vmx_domain_initialise, > > - .domain_destroy = vmx_domain_destroy, > > + .domain_relinquish_resources = vmx_domain_relinquish_resources, > > .vcpu_initialise = vmx_vcpu_initialise, > > .vcpu_destroy = vmx_vcpu_destroy, > > .save_cpu_ctxt = vmx_save_vmcs_ctxt, > >
On 28.01.2020 09:22, Durrant, Paul wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 28 January 2020 08:15 >> To: Durrant, Paul <pdurrant@amazon.co.uk> >> Cc: xen-devel@lists.xenproject.org; Jun Nakajima <jun.nakajima@intel.com>; >> Kevin Tian <kevin.tian@intel.com>; Andrew Cooper >> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné >> <roger.pau@citrix.com>; George Dunlap <george.dunlap@citrix.com> >> Subject: Re: [PATCH v4 4/7] x86 / vmx: move teardown from >> domain_destroy()... >> >> On 24.01.2020 16:31, Paul Durrant wrote: >>> ... to domain_relinquish_resources(). >>> >>> The teardown code frees the APICv page. This does not need to be done >> late >>> so do it in domain_relinquish_resources() rather than domain_destroy(). >> >> For the normal domain destruction path this is fine. For the error path >> of domain_create(), however, this will leak the page, as in this case >> hvm_domain_relinquish_resources() won't be called. > > Well it's really arch_domain_create() that's at fault but, yes that needs fixing. Why arch_domain_create()? For HVM domains hvm_domain_initialise() is the last thing tried, and hence no further cleanup is needed (assuming hvm_domain_initialise() cleans up in case of failure after itself). It's failures encountered by domain_create() after arch_domain_create() has succeeded which are a problem here. In this case arch_domain_destroy() will be called, but nothing down from there has called hvm_domain_relinquish_resources() so far. >> I'm afraid there >> already is a similar issue with e.g. viridian_domain_deinit(). I guess >> I'll make a patch. > > Ok, thanks. Which turned out to take more time because of other issues I've found in the course, but I think I now have something I can actually test. Jan
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index b262d38a7c..606f3dc2eb 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -419,7 +419,7 @@ static int vmx_domain_initialise(struct domain *d) return 0; } -static void vmx_domain_destroy(struct domain *d) +static void vmx_domain_relinquish_resources(struct domain *d) { if ( !has_vlapic(d) ) return; @@ -2240,7 +2240,7 @@ static struct hvm_function_table __initdata vmx_function_table = { .cpu_up_prepare = vmx_cpu_up_prepare, .cpu_dead = vmx_cpu_dead, .domain_initialise = vmx_domain_initialise, - .domain_destroy = vmx_domain_destroy, + .domain_relinquish_resources = vmx_domain_relinquish_resources, .vcpu_initialise = vmx_vcpu_initialise, .vcpu_destroy = vmx_vcpu_destroy, .save_cpu_ctxt = vmx_save_vmcs_ctxt,
... to domain_relinquish_resources(). The teardown code frees the APICv page. This does not need to be done late so do it in domain_relinquish_resources() rather than domain_destroy(). Signed-off-by: Paul Durrant <pdurrant@amazon.com> --- Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@intel.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Wei Liu <wl@xen.org> Cc: "Roger Pau Monné" <roger.pau@citrix.com> Cc: George Dunlap <george.dunlap@citrix.com> v4: - New in v4 (disaggregated from v3 patch #3) --- xen/arch/x86/hvm/vmx/vmx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)