Fix writing back APIC reset to inkernel APIC

PATCH Applied Markus Rechberger 4 participants 7 messages Nov 1, 2007 – Nov 6, 2007 5 messages not in the archive KVM

Share & tools

Markus Rechberger

author

2007-11-01 15:35 UTC #0 lore raw

PATCH 1/1 Fix writing back APIC reset to inkernel APIC

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 <[email protected]>

Signed-off-by Markus Rechberger <[email protected]>

qemu/hw/apic.c +7 -0

index 2011247..60d31fa 100644
@@ -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] = {

-------------------------------------------------------------------------
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 [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

Dor Laor

2007-11-01 23:02 UTC #2 lore raw

Quoted 13 lines · Markus Rechberger

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 <[email protected]>
Signed-off-by Markus Rechberger <[email protected]>
------------------------------------------------------------------------
qemu/hw/apic.c +0 -0
index 2011247..60d31fa 100644
@@ -930,6 +930,13 @@ static void apic_reset(void *opaque)
      * processor when local APIC is enabled.
      */

qemu/hw/apic.c static void apic_reset(void *opaque) · line 941

Quoted 10 lines

qemu/hw/apic.c +7 -0

     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.

Quoted 9 lines

qemu/hw/apic.c +0 -1
 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/
------------------------------------------------------------------------

Quoted 4 lines

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): 472A5AF4.9040900-atKUWr5tajBWk0Htik3J/[email protected]

Dong, Eddie

2007-11-02 03:07 UTC #4 lore raw

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: [email protected]; [email protected]
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 <[email protected]> <mailto:[email protected]>

Signed-off-by Markus Rechberger <[email protected]> <mailto:[email protected]>

________________________________

		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
		[email protected]
		https://lists.sourceforge.net/lists/listinfo/kvm-devel

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): 10EA09EFD8728347A513008B6B0DA77A0250DC9B-wq7ZOvIWXbNpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org

Avi Kivity

2007-11-04 07:49 UTC #6 lore raw

Quoted 3 lines · Eddie

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.

Quoted 3 lines

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.

Signature · 1 line

Any sufficiently difficult bug is indistinguishable from a feature.

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): 472D7993.8010802-atKUWr5tajBWk0Htik3J/[email protected]

Dong, Eddie

2007-11-05 08:58 UTC #8 lore raw

Quoted 7 lines · Avi Kivity

Quoted 3 lines · Eddie
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

A message that is not in the archive (sent off-list or not yet received): 10EA09EFD8728347A513008B6B0DA77A02557D4B-wq7ZOvIWXbNpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org

Avi Kivity

2007-11-05 14:03 UTC #10 lore raw

Quoted 8 lines · Eddie

Quoted 6 lines · Avi Kivity
Quoted 4 lines · Eddie
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?

Quoted 9 lines

Quoted 3 lines
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.

Quoted 6 lines

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.

Signature · 8 lines

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/

Avi Kivity

Applied

2007-11-06 14:21 UTC #11 lore raw

In reply to a message not in the archive

Quoted 3 lines · Markus Rechberger

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.

Signature · 8 lines

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/