[v4,4/7] x86 / vmx: move teardown from domain_destroy()...
diff mbox series

Message ID 20200124153103.18321-5-pdurrant@amazon.com
State Superseded
Headers show
Series
  • purge free_shared_domheap_page()
Related show

Commit Message

Paul Durrant Jan. 24, 2020, 3:31 p.m. UTC
... 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(-)

Comments

Jan Beulich Jan. 28, 2020, 8:14 a.m. UTC | #1
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,
>
Durrant, Paul Jan. 28, 2020, 8:22 a.m. UTC | #2
> -----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,
> >
Jan Beulich Jan. 28, 2020, 11:41 a.m. UTC | #3
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

Patch
diff mbox series

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,