[PATCH] kvm: initialize kvm_arch_ops in kvm_init()

[PATCH] kvm: initialize kvm_arch_ops in kvm_init()

[Yoshimi Ichiyanagi]

The latest version of kvm doesn't initialize kvm_arch_ops in kvm_init(), 
which causes an error with the following sequence.

1. Load the supported arch's module.
2. Load the unsupported arch's module.　(loading error)
3. Unload the unsupported arch's module.

You'll get the following error message after step 3.
"BUG: unable to handle to handle kernel paging request at virtual 
address xxxxxxxx"

The problem here is that the unsupported arch's module overwrites 
kvm_arch_ops of the supported arch's module at step 2.

This patch initializes kvm_arch_ops upon loading architecture specific 
kvm module, and prevents overwriting kvm_arch_ops when kvm_arch_ops is 
already set correctly.


-------------------------------------------------------------------
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 9f24f22..9287b2d 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -1865,6 +1865,11 @@ int kvm_init_arch(struct kvm_arch_ops *ops, 
struct module *module)
 {
 	int r;
 
+	if ( kvm_arch_ops ) {
+		printk(KERN_ERR "kvm: already loaded the other module\n
");
+		return -EOPNOTSUPP;
+	}
+
 	kvm_arch_ops = ops;
 
 	if (!kvm_arch_ops->cpu_has_kvm_support()) {
@@ -1907,6 +1912,7 @@ void kvm_exit_arch(void)
 	unregister_reboot_notifier(&kvm_reboot_notifier);
 	on_each_cpu(kvm_arch_ops->hardware_disable, 0, 0, 1);
 	kvm_arch_ops->hardware_unsetup();
+	kvm_arch_ops = NULL;
 }
 
 static __init int kvm_init(void)
@@ -1914,6 +1920,7 @@ static __init int kvm_init(void)
 	static struct page *bad_page;
 	int r = 0;
 
+	kvm_arch_ops = NULL;
 	kvm_init_debug();
 
 	kvm_init_msr_list();


--
Yoshimi Ichiyanagi
Open source software computing project, NTT Cyber Space Laboratories
E-mail : ichiyanagi.yoshimi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] kvm: initialize kvm_arch_ops in kvm_init()

[Avi Kivity]

Yoshimi Ichiyanagi wrote:
> The latest version of kvm doesn't initialize kvm_arch_ops in kvm_init(), 
> which causes an error with the following sequence.
>
> 1. Load the supported arch's module.
> 2. Load the unsupported arch's module.　(loading error)
> 3. Unload the unsupported arch's module.
>
> You'll get the following error message after step 3.
> "BUG: unable to handle to handle kernel paging request at virtual 
> address xxxxxxxx"
>
> The problem here is that the unsupported arch's module overwrites 
> kvm_arch_ops of the supported arch's module at step 2.
>
> This patch initializes kvm_arch_ops upon loading architecture specific 
> kvm module, and prevents overwriting kvm_arch_ops when kvm_arch_ops is 
> already set correctly.
>
>   

Applied with edits; thanks.

Please provide a Signed-off-by: line in future patches.

See below:

> -------------------------------------------------------------------
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> index 9f24f22..9287b2d 100644
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -1865,6 +1865,11 @@ int kvm_init_arch(struct kvm_arch_ops *ops, 
> struct module *module)
>  {
>  	int r;
>  
> +	if ( kvm_arch_ops ) {
> +		printk(KERN_ERR "kvm: already loaded the other module\n
> ");
> +		return -EOPNOTSUPP;
> +	}
> +
>   

Changed to -EEXIST.

>  static __init int kvm_init(void)
> @@ -1914,6 +1920,7 @@ static __init int kvm_init(void)
>  	static struct page *bad_page;
>  	int r = 0;
>  
> +	kvm_arch_ops = NULL;
>  	kvm_init_debug();
>  
>  	kvm_init_msr_list();
>
>   

This bit is unnecessary, no? I think kvm_init() will only be called
after the module is loaded, at which point kvm_arch_ops is initialized
from the .bss section.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] kvm: initialize kvm_arch_ops in kvm_init()

[Yoshimi Ichiyanagi]

Hi Avi

Thank you for applying my patch.

By the way, I also made a mistake at the body of my email.

>> 2. Load the unsupported arch's module.　(loading error)
                                         ^^
I put an unreadable character after period.
I would be great if you can remove it before you commit to the 
repository.

-
Yoshimi Ichiyanagi
Open source software computing project, NTT Cyber Space Laboratories
E-mail : ichiyanagi.yoshimi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] kvm: initialize kvm_arch_ops in kvm_init()

[Yoshimi Ichiyanagi]

>>  static __init int kvm_init(void)
>> @@ -1914,6 +1920,7 @@ static __init int kvm_init(void)
>>  	static struct page *bad_page;
>>  	int r = 0;
>>  
>> +	kvm_arch_ops = NULL;
>>  	kvm_init_debug();
>>  
>>  	kvm_init_msr_list();
>>
>>   
>
>This bit is unnecessary, no? I think kvm_init() will only be called
>after the module is loaded, at which point kvm_arch_ops is initialized
>from the .bss section.
>

Logically it's not required, however it's better to initialize 
explicitly in general.


By the way, the previous patch didn't fix the problem completely.
You can't load the supported arch's module
if you load the unsupported arch's module beforehand.

The following patch will fix this problem.
Please take a look.

Signed-off-by: Yoshimi Ichiyanagi <ichiyanagi.yoshimi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>

Index: kvm_main.c
===================================================================
--- kvm_main.c	(revision 4190)
+++ kvm_main.c	(working copy)
@@ -1972,17 +1972,17 @@
 		return -EEXIST;
 	}
 
-	kvm_arch_ops = ops;
-
-	if (!kvm_arch_ops->cpu_has_kvm_support()) {
+	if (!ops->cpu_has_kvm_support()) {
 		printk(KERN_ERR "kvm: no hardware support\n");
 		return -EOPNOTSUPP;
 	}
-	if (kvm_arch_ops->disabled_by_bios()) {
+	if (ops->disabled_by_bios()) {
 		printk(KERN_ERR "kvm: disabled by bios\n");
 		return -EOPNOTSUPP;
 	}
 
+	kvm_arch_ops = ops;
+
 	r = kvm_arch_ops->hardware_setup();
 	if (r < 0)
 	    return r;

--
Yoshimi Ichiyanagi

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] kvm: initialize kvm_arch_ops in kvm_init()

[Avi Kivity]

Yoshimi Ichiyanagi wrote:
>> This bit is unnecessary, no? I think kvm_init() will only be called
>> after the module is loaded, at which point kvm_arch_ops is initialized
>>     
> >from the .bss section.
>   
>
> Logically it's not required, however it's better to initialize 
> explicitly in general.
>
>   

Personally I agree, but Linux coding style does not.

> By the way, the previous patch didn't fix the problem completely.
> You can't load the supported arch's module
> if you load the unsupported arch's module beforehand.
>
> The following patch will fix this problem.
> Please take a look.
>   

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV