Patchwork 答复: [PATCH] KVM: fix error handling in svm_hardware_setup

login
register
mail settings
Submitter Li RongQing
Date March 14, 2019, 1:29 a.m.
Message ID <4d7c1a2b1a1d4a69b5915affa8f34c25@baidu.com>
Download mbox | patch
Permalink /patch/748497/
State New
Headers show

Comments

Li RongQing - March 14, 2019, 1:29 a.m.
> -----邮件原件-----

> 发件人: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] 代

> 表 Singh, Brijesh

> 发送时间: 2019年3月13日 23:50

> 收件人: Li,Rongqing <lirongqing@baidu.com>; x86@kernel.org;

> kvm@vger.kernel.org

> 抄送: Singh, Brijesh <brijesh.singh@amd.com>

> 主题: Re: [PATCH] KVM: fix error handling in svm_hardware_setup

> 

...
> > +	if (svm_sev_enabled())

> > +		bitmap_free(sev_asid_bitmap);

> > +

> > +	for_each_possible_cpu(cpu)

> > +		svm_cpu_uninit(cpu);

> > +

> >   	__free_pages(iopm_pages, IOPM_ALLOC_ORDER);

> >   	iopm_base = 0;

> >   	return r;

> >

> 

> 

> Does it make sense to call the svm_hardware_unsetup() instead of duplicating

> the logic in error code path ?

> 


Thanks for your review;

Two thing needs to do if call svm_hardware_unsetup

1. move svm_hardware_unsetup before svm_hardware_setup, to avoid declaration
2. remove __exit attribute for svm_hardware_unsetup , otherwise these is the below warning:

              WARNING: arch/x86/kvm/kvm-amd.o(.init.text+0x4aa): Section mismatch in reference from the function svm_hardware_setup() to         the function .exit.text:svm_hardware_unsetup()
              The function __init svm_hardware_setup() references
              a function __exit svm_hardware_unsetup().
              This is often seen when error handling in the init function
              uses functionality in the exit path.
              The fix is often to remove the __exit annotation of
              svm_hardware_unsetup() so it may be used outside an exit section.


The RFC is below:



-RongQing




> thanks
Brijesh Singh - March 14, 2019, 2:07 p.m.
On 3/13/19 8:29 PM, Li,Rongqing wrote:
...

>>

>>

>> Does it make sense to call the svm_hardware_unsetup() instead of duplicating

>> the logic in error code path ?

>>

> 

> Thanks for your review;

> 

> Two thing needs to do if call svm_hardware_unsetup

> 

> 1. move svm_hardware_unsetup before svm_hardware_setup, to avoid declaration

> 2. remove __exit attribute for svm_hardware_unsetup , otherwise these is the below warning:

> 

>                WARNING: arch/x86/kvm/kvm-amd.o(.init.text+0x4aa): Section mismatch in reference from the function svm_hardware_setup() to         the function .exit.text:svm_hardware_unsetup()

>                The function __init svm_hardware_setup() references

>                a function __exit svm_hardware_unsetup().

>                This is often seen when error handling in the init function

>                uses functionality in the exit path.

>                The fix is often to remove the __exit annotation of

>                svm_hardware_unsetup() so it may be used outside an exit section.

> 

> 



Ah, I didn't realized that svm_hardware_unsetup has __exit attribute.
v1 looks good to me. thanks

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

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;