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

Commit Message

Li,Rongqing March 14, 2019, 1:29 a.m. UTC

> -----邮件原件-----
> 发件人: [e-mail] [mailto:[e-mail]] 代
> 表 Singh, Brijesh
> 发送时间: 2019年3月13日 23:50
> 收件人: Li,Rongqing <[e-mail]>; [e-mail];
> [e-mail]
> 抄送: Singh, Brijesh <[e-mail]>
> 主题: 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

Comments

Brijesh Singh March 14, 2019, 2:07 p.m. UTC | #1

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 <[e-mail]>

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;