Patchwork [v2] KVM: fix error handling in svm_hardware_setup

login
register
mail settings
Submitter Li RongQing
Date March 14, 2019, 2:12 a.m.
Message ID <1552529524-14668-1-git-send-email-lirongqing@baidu.com>
Download mbox | patch
Permalink /patch/748499/
State New
Headers show

Comments

Li RongQing - March 14, 2019, 2:12 a.m.
From: Li Rongqing <lirongqing@baidu.com>

move svm_hardware_unsetup before svm_hardware_setup, and call
it to free all memory if fail to setup in svm_hardware_setup,
otherwise memory will be leaked

remove __exit attribute for svm_hardware_unsetup since it is
called in __init function

Signed-off-by: Li Rongqing <lirongqing@baidu.com>
Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
---
 arch/x86/kvm/svm.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)
Vitaly Kuznetsov - March 14, 2019, 1:35 p.m.
Li RongQing <lirongqing@baidu.com> writes:

> From: Li Rongqing <lirongqing@baidu.com>
>
> move svm_hardware_unsetup before svm_hardware_setup, and call
> it to free all memory if fail to setup in svm_hardware_setup,
> otherwise memory will be leaked
>
> remove __exit attribute for svm_hardware_unsetup since it is
> called in __init function
>
> Signed-off-by: Li Rongqing <lirongqing@baidu.com>
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> ---
>  arch/x86/kvm/svm.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 1787a484d21c..276ab8ab6c95 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1290,6 +1290,20 @@ static void shrink_ple_window(struct kvm_vcpu *vcpu)
>  				    control->pause_filter_count, old);
>  }
>  
> +static void svm_hardware_unsetup(void)
> +{

Unrelated to your patch but "unsetup" makes me feel sad. Why can't we
use e.g. "teardown" instead? (KVM is among just four users of 'unsetup'
word in kernel).

> +	int cpu;
> +
> +	if (svm_sev_enabled())
> +		bitmap_free(sev_asid_bitmap);
> +
> +	for_each_possible_cpu(cpu)
> +		svm_cpu_uninit(cpu);
> +
> +	__free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT), IOPM_ALLOC_ORDER);
> +	iopm_base = 0;
> +}
> +
>  static __init int svm_hardware_setup(void)
>  {
>  	int cpu;
> @@ -1396,25 +1410,10 @@ static __init int svm_hardware_setup(void)
>  	return 0;
>  
>  err:
> -	__free_pages(iopm_pages, IOPM_ALLOC_ORDER);
> -	iopm_base = 0;
> +	svm_hardware_unsetup();
>  	return r;
>  }
>  
> -static __exit void svm_hardware_unsetup(void)
> -{
> -	int cpu;
> -
> -	if (svm_sev_enabled())
> -		bitmap_free(sev_asid_bitmap);
> -
> -	for_each_possible_cpu(cpu)
> -		svm_cpu_uninit(cpu);
> -
> -	__free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT), IOPM_ALLOC_ORDER);
> -	iopm_base = 0;
> -}
> -
>  static void init_seg(struct vmcb_seg *seg)
>  {
>  	seg->selector = 0;

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Li RongQing - April 17, 2019, 2:36 a.m.
> > From: Li Rongqing <lirongqing@baidu.com>

> >

> > move svm_hardware_unsetup before svm_hardware_setup, and call it to

> > free all memory if fail to setup in svm_hardware_setup, otherwise

> > memory will be leaked

> >

> > remove __exit attribute for svm_hardware_unsetup since it is called in

> > __init function

> >

> > Signed-off-by: Li Rongqing <lirongqing@baidu.com>

> > Signed-off-by: Zhang Yu <zhangyu31@baidu.com>

> > ---

> >  arch/x86/kvm/svm.c | 31 +++++++++++++++----------------

> >  1 file changed, 15 insertions(+), 16 deletions(-)

> >

> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index

> > 1787a484d21c..276ab8ab6c95 100644

> > --- a/arch/x86/kvm/svm.c

> > +++ b/arch/x86/kvm/svm.c

> > @@ -1290,6 +1290,20 @@ static void shrink_ple_window(struct kvm_vcpu

> *vcpu)

> >  				    control->pause_filter_count, old);  }

> >

> > +static void svm_hardware_unsetup(void) {

> 

> Unrelated to your patch but "unsetup" makes me feel sad. Why can't we use

> e.g. "teardown" instead? (KVM is among just four users of 'unsetup'

> word in kernel).

> 

> > +	int cpu;

> > +

> > +	if (svm_sev_enabled())

> > +		bitmap_free(sev_asid_bitmap);

> > +

> > +	for_each_possible_cpu(cpu)

> > +		svm_cpu_uninit(cpu);

> > +

> > +	__free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT),

> IOPM_ALLOC_ORDER);

> > +	iopm_base = 0;

> > +}

> > +

> >  static __init int svm_hardware_setup(void)  {

> >  	int cpu;

> > @@ -1396,25 +1410,10 @@ static __init int svm_hardware_setup(void)

> >  	return 0;

> >

> >  err:

> > -	__free_pages(iopm_pages, IOPM_ALLOC_ORDER);

> > -	iopm_base = 0;

> > +	svm_hardware_unsetup();

> >  	return r;

> >  }

> >

> > -static __exit void svm_hardware_unsetup(void) -{

> > -	int cpu;

> > -

> > -	if (svm_sev_enabled())

> > -		bitmap_free(sev_asid_bitmap);

> > -

> > -	for_each_possible_cpu(cpu)

> > -		svm_cpu_uninit(cpu);

> > -

> > -	__free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT),

> IOPM_ALLOC_ORDER);

> > -	iopm_base = 0;

> > -}

> > -

> >  static void init_seg(struct vmcb_seg *seg)  {

> >  	seg->selector = 0;

> 

> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

> 


Ping

-RongQing

> --

> Vitaly

Patch

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 1787a484d21c..276ab8ab6c95 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1290,6 +1290,20 @@  static void shrink_ple_window(struct kvm_vcpu *vcpu)
 				    control->pause_filter_count, old);
 }
 
+static void svm_hardware_unsetup(void)
+{
+	int cpu;
+
+	if (svm_sev_enabled())
+		bitmap_free(sev_asid_bitmap);
+
+	for_each_possible_cpu(cpu)
+		svm_cpu_uninit(cpu);
+
+	__free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT), IOPM_ALLOC_ORDER);
+	iopm_base = 0;
+}
+
 static __init int svm_hardware_setup(void)
 {
 	int cpu;
@@ -1396,25 +1410,10 @@  static __init int svm_hardware_setup(void)
 	return 0;
 
 err:
-	__free_pages(iopm_pages, IOPM_ALLOC_ORDER);
-	iopm_base = 0;
+	svm_hardware_unsetup();
 	return r;
 }
 
-static __exit void svm_hardware_unsetup(void)
-{
-	int cpu;
-
-	if (svm_sev_enabled())
-		bitmap_free(sev_asid_bitmap);
-
-	for_each_possible_cpu(cpu)
-		svm_cpu_uninit(cpu);
-
-	__free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT), IOPM_ALLOC_ORDER);
-	iopm_base = 0;
-}
-
 static void init_seg(struct vmcb_seg *seg)
 {
 	seg->selector = 0;