[PATCH] Fix writing back APIC reset to inkernel APIC

[PATCH] Fix writing back APIC reset to inkernel APIC

[Markus Rechberger]

[-- Attachment #1: Type: text/plain, Size: 300 bytes --]

This patch synchronises the APIC reset with the inkernel implementation 
which fixes the reboot issues which can be seen with Linux and Windows.

Signed-off-by: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
Signed-off-by: Markus Rechberger <markus.rechberger-5C7GfCeVMHo@public.gmane.org>

[-- Attachment #2: apic_reset_fix.diff --]
[-- Type: text/plain, Size: 490 bytes --]

diff --git a/qemu/hw/apic.c b/qemu/hw/apic.c
index 2011247..60d31fa 100644
--- a/qemu/hw/apic.c
+++ b/qemu/hw/apic.c
@@ -930,6 +930,13 @@ static void apic_reset(void *opaque)
      * processor when local APIC is enabled.
      */
     s->lvt[APIC_LVT_LINT0] = 0x700;
+#ifdef USE_KVM
+#ifdef KVM_CAP_IRQCHIP
+    if (kvm_allowed && kvm_irqchip_in_kernel(kvm_context)) {
+        kvm_kernel_lapic_load_from_user(s);
+    }
+#endif
+#endif
 }
 
 static CPUReadMemoryFunc *apic_mem_read[3] = {

[-- Attachment #3: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Fix writing back APIC reset to inkernel APIC

[Dor Laor]

[-- Attachment #1.1: Type: text/plain, Size: 1848 bytes --]

Markus Rechberger wrote:
> This patch synchronises the APIC reset with the inkernel 
> implementation which fixes the reboot issues which can be seen with 
> Linux and Windows.
>
> Signed-off-by: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
> Signed-off-by: Markus Rechberger <markus.rechberger-5C7GfCeVMHo@public.gmane.org>
> ------------------------------------------------------------------------
>
> diff --git a/qemu/hw/apic.c b/qemu/hw/apic.c
> index 2011247..60d31fa 100644
> --- a/qemu/hw/apic.c
> +++ b/qemu/hw/apic.c
> @@ -930,6 +930,13 @@ static void apic_reset(void *opaque)
>       * processor when local APIC is enabled.
>       */
>      s->lvt[APIC_LVT_LINT0] = 0x700;
> +#ifdef USE_KVM
> +#ifdef KVM_CAP_IRQCHIP
> +    if (kvm_allowed && kvm_irqchip_in_kernel(kvm_context)) {
> +        kvm_kernel_lapic_load_from_user(s);
> +    }
> +#endif
> +#endif
>  }
>  
In general your approach might work but there are some inconsistencies - 
there is kvm_apic_reset
in the kernel too. We can either drop it and use your load method or add 
an apic-reset ioctl.
Eddie/Avi which approach do you think is better?
There is also the ioapic who should get reset too.
Thanks,
Dor.
>  static CPUReadMemoryFunc *apic_mem_read[3] = {
>   
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >> http://get.splunk.com/
> ------------------------------------------------------------------------
>
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel


[-- Attachment #1.2: Type: text/html, Size: 2943 bytes --]

[-- Attachment #2: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Fix writing back APIC reset to inkernel APIC

[Dong, Eddie]

[-- Attachment #1.1: Type: text/plain, Size: 2742 bytes --]

Yes, when reseting, we need to reset both APIC/PIC/IOAPIC and pv
driver in future, and also VCPU.
 
But isn't apic_reset a pure user level APIC thing?
 
BTW
 
I posted a patch to support kernel device reset which is still pending.
And I think Avi has some idea to support too and is working on:-)
Eddie
 
 
 


________________________________

	From: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org [mailto:kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of Dor Laor
	Sent: 2007年11月2日 7:02
	To: Markus Rechberger
	Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Joerg.Roedel-5C7GfCeVMHo@public.gmane.org
	Subject: Re: [kvm-devel] [PATCH] Fix writing back APIC reset to inkernel APIC
	
	
	Markus Rechberger wrote: 

		This patch synchronises the APIC reset with the inkernel implementation which fixes the reboot issues which can be seen with Linux and Windows. 
		
		Signed-off-by: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org> <mailto:joerg.roedel-5C7GfCeVMHo@public.gmane.org>  
		Signed-off-by: Markus Rechberger <markus.rechberger-5C7GfCeVMHo@public.gmane.org> <mailto:markus.rechberger-5C7GfCeVMHo@public.gmane.org>  
		
		
		
________________________________


		diff --git a/qemu/hw/apic.c b/qemu/hw/apic.c
		index 2011247..60d31fa 100644
		--- a/qemu/hw/apic.c
		+++ b/qemu/hw/apic.c
		@@ -930,6 +930,13 @@ static void apic_reset(void *opaque)
		      * processor when local APIC is enabled.
		      */
		     s->lvt[APIC_LVT_LINT0] = 0x700;
		+#ifdef USE_KVM
		+#ifdef KVM_CAP_IRQCHIP
		+    if (kvm_allowed && kvm_irqchip_in_kernel(kvm_context)) {
		+        kvm_kernel_lapic_load_from_user(s);
		+    }
		+#endif
		+#endif
		 }
		 

	In general your approach might work but there are some inconsistencies - there is kvm_apic_reset 
	in the kernel too. We can either drop it and use your load method or add an apic-reset ioctl.
	Eddie/Avi which approach do you think is better?
	There is also the ioapic who should get reset too.
	Thanks,
	Dor.
	

		
		 static CPUReadMemoryFunc *apic_mem_read[3] = {
		  
		
		-------------------------------------------------------------------------
		This SF.net email is sponsored by: Splunk Inc.
		Still grepping through log files to find problems?  Stop.
		Now Search log events and configuration files using AJAX and a browser.
		Download your FREE copy of Splunk now >> http://get.splunk.com/
		
________________________________


		_______________________________________________
		kvm-devel mailing list
		kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
		https://lists.sourceforge.net/lists/listinfo/kvm-devel



[-- Attachment #1.2: Type: text/html, Size: 5592 bytes --]

[-- Attachment #2: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Fix writing back APIC reset to inkernel APIC

[Avi Kivity]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

Dong, Eddie wrote:
> Yes, when reseting, we need to reset both APIC/PIC/IOAPIC and pv
> driver in future, and also VCPU.
> But isn't apic_reset a pure user level APIC thing?

The reset callbacks are still called, so the userspace apic is still reset.

> BTW
> I posted a patch to support kernel device reset which is still pending.
> And I think Avi has some idea to support too and is working on:-)

I'm supposed to be working on this, but instead I'm doing binary
patching hacks and travelling. I'll get back on this, though, this is
hurting.

I think the patch is good, will apply after testing it in smp a bit.


-- 
Any sufficiently difficult bug is indistinguishable from a feature.



[-- Attachment #2: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] Fix writing back APIC reset to inkernel APIC

[Dong, Eddie]

Avi Kivity wrote:
> Dong, Eddie wrote:
>> Yes, when reseting, we need to reset both APIC/PIC/IOAPIC and pv
>> driver in future, and also VCPU.
>> But isn't apic_reset a pure user level APIC thing?
> 
> The reset callbacks are still called, so the userspace apic is
> still reset.

OK. But since we have seperate kernel reset, doing double reset here
means
something weir to me.

Maybe you just want to live with a pure apic reset only, then I am ok
too.
Once we established one ABI, probably we won't establish another set, do
we?

Eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH] Fix writing back APIC reset to inkernel APIC

[Avi Kivity]

Dong, Eddie wrote:
> Avi Kivity wrote:
>   
>> Dong, Eddie wrote:
>>     
>>> Yes, when reseting, we need to reset both APIC/PIC/IOAPIC and pv
>>> driver in future, and also VCPU.
>>> But isn't apic_reset a pure user level APIC thing?
>>>       
>> The reset callbacks are still called, so the userspace apic is
>> still reset.
>>     
>
> OK. But since we have seperate kernel reset, doing double reset here
> means
> something weir to me.
>
>   
We can detect KVM_CAP_RESET and avoid it.

> Maybe you just want to live with a pure apic reset only, then I am ok
> too.
> Once we established one ABI, probably we won't establish another set, do
> we?
>
>   

No we have too many ABIs already.  But userspace reset is via an 
existing ABI.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH] Fix writing back APIC reset to inkernel APIC

[Avi Kivity]

Markus Rechberger wrote:
> This patch synchronises the APIC reset with the inkernel 
> implementation which fixes the reboot issues which can be seen with 
> Linux and Windows.

Applied, thanks.  While I'm not sure how SMP guests will behave with 
this (nothing forces the other vcpu to exit), this is better than what 
we have now.

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/