Xen devel - Sep 2005 - [PATCH][VT] Clear the pending interrupt on shared page when PIC initialized

When PIC is initialized and the irq base changed, the pending interrupt
on shared page should be cleared.

Thanks
Yunhong Jiang

Signed-off-by: Yunhong Jiang <yunhong.jiang@intel.com>
Signed-off-by: Asit Mallick <asit.k.mallick@intel.com>

diff -r b187d5555866 -r 94a3235acaab tools/ioemu/hw/i8259.c
--- a/tools/ioemu/hw/i8259.c	Mon Sep 12 05:09:23 2005
+++ b/tools/ioemu/hw/i8259.c	Mon Sep 12 05:33:10 2005
@@ -127,6 +127,13 @@
 
 /* pic[1] is connected to pin2 of pic[0] */
 #define CASCADE_IRQ 2
+static void clear_shared_page()
+{
+    extern shared_iopage_t *shared_page;
+    unsigned long *intr;
+    intr = (unsigned long *) &(shared_page->sp_global.pic_intr[0]);
+    memset(intr, 0, INTR_LEN);
+}
 
 static void shared_page_update()
 {
@@ -299,6 +306,7 @@
         if (val & 0x10) {
             /* init */
             pic_reset(s);
+            clear_shared_page();
             /* deassert a pending interrupt */
             cpu_reset_interrupt(cpu_single_env, CPU_INTERRUPT_HARD);
 
diff -r b187d5555866 -r 94a3235acaab xen/arch/x86/vmx_intercept.c
--- a/xen/arch/x86/vmx_intercept.c	Mon Sep 12 05:09:23 2005
+++ b/xen/arch/x86/vmx_intercept.c	Mon Sep 12 05:33:10 2005
@@ -227,6 +227,7 @@
     u64 *intr = &(sp->sp_global.pic_intr[0]);
     struct vmx_virpit_t *vpit
&(d->domain->arch.vmx_platform.vmx_pit);
     int rw_mode, reinit = 0;
+    int oldvec = 0;
 
     /* load init count*/
     if (p->state == STATE_IORESP_HOOK) { 
@@ -235,6 +236,7 @@
             VMX_DBG_LOG(DBG_LEVEL_1, "VMX_PIT: guest reset PIT with
channel %lx!\n", (unsigned long) ((p->u.data >> 24) & 0x3) );
             rem_ac_timer(&(vpit->pit_timer));
             reinit = 1;
+            oldvec = vpit->vector;
         }
         else
             init_ac_timer(&vpit->pit_timer, pit_timer_fn, vpit,
d->processor);
@@ -250,6 +252,12 @@
             vpit->period = 1000000;
         }
         vpit->vector = ((p->u.data >> 16) & 0xFF);
+
+        if( reinit && oldvec != vpit->vector){
+            clear_bit(oldvec, intr);
+            vpit->pending_intr_nr = 0;
+        }
+
         vpit->channel = ((p->u.data >> 24) & 0x3);
         vpit->first_injected = 0;
 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 12 Sep 2005, at 11:10, Jiang, Yunhong wrote:
> When PIC is initialized and the irq base changed, the pending interrupt
> on shared page should be cleared.
For the i8259 device-model change -- ought we to be hooking off the 
cpu_interrupt/cpu_reset_interrupt functions rather than adding new 
hooks?

Also, are the patches to the PIT code directly related to the PIC patch 
or should they be split out with a separate explanation?

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser wrote:
> On 12 Sep 2005, at 11:10, Jiang, Yunhong wrote:
> 
>> When PIC is initialized and the irq base changed, the pending
>> interrupt on shared page should be cleared.
> 
> For the i8259 device-model change -- ought we to be hooking off the
> cpu_interrupt/cpu_reset_interrupt functions rather than adding new
> hooks?
> 
> Also, are the patches to the PIT code directly related to the PIC
> patch or should they be split out with a separate explanation?
> 
>   -- Keir
This patch works for following scenerio:
1) The BIOS set the irq base to 0x20, and then interrupt is injected
through shared page.
2) The OS disable interurpt , and reset the PIC with ICW1~ICW4. In this
process, it changes the irq base for PIC from 0x20 to 0x30 through ICW2.

On original code, when OS enable interrupt again after step 2, the
interrupts that was kept on shared page and has no chance to be injected
will be injected into guest. However, at this time, the guest has change
the irq base. So maybe 0x20 will be injected into guest, which cause
wrong vectoring on guest.

The patch on PIC and  PIT works together for this purpose, since the PIT
interrupt is injected on HV directly.


Thanks
Yunhong Jiang

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 12 Sep 2005, at 12:11, Jiang, Yunhong wrote:
> This patch works for following scenerio:
> 1) The BIOS set the irq base to 0x20, and then interrupt is injected
> through shared page.
> 2) The OS disable interurpt , and reset the PIC with ICW1~ICW4. In this
> process, it changes the irq base for PIC from 0x20 to 0x30 through 
> ICW2.
Okay, thanks for the clarification and I see why the PIT bart is needed 
now. But, in the PIC code, wouldn''t hanging the reset off of 
cpu_reset_interrupt() make more sense?

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 12 Sep 2005, at 18:36, Keir Fraser wrote:
>> This patch works for following scenerio:
>> 1) The BIOS set the irq base to 0x20, and then interrupt is injected
>> through shared page.
>> 2) The OS disable interurpt , and reset the PIC with ICW1~ICW4. In 
>> this
>> process, it changes the irq base for PIC from 0x20 to 0x30 through 
>> ICW2.
>
> Okay, thanks for the clarification and I see why the PIT bart is 
> needed now. But, in the PIC code, wouldn''t hanging the reset off
of
> cpu_reset_interrupt() make more sense?
I should add that I think in the end the PIC emulation belongs in Xen, 
or the PIT should be moved out to the qemu device model (but only after 
all that code is moved out of domain0 user space). The current 
architectural split across Xen/dom0 is pretty gross.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser wrote:
> On 12 Sep 2005, at 12:11, Jiang, Yunhong wrote:
> 
>> This patch works for following scenerio:
>> 1) The BIOS set the irq base to 0x20, and then interrupt is injected
>> through shared page. 2) The OS disable interurpt , and reset the PIC
>> with ICW1~ICW4. In this process, it changes the irq base for PIC
>> from 0x20 to 0x30 through ICW2.
> 
> Okay, thanks for the clarification and I see why the PIT bart is
> needed now. But, in the PIC code, wouldn''t hanging the reset off
of
> cpu_reset_interrupt() make more sense?
Yes, you are right that adding a new hook is not a good idea. 
And how about place it on the pic_reset? 
1) I think the  cpu_reset_interrupt() is a common API on qemu for
hardware interrupt/exception etc,while clear the shared page is just for
hardware interrupt. Of course, this function is currently used just for
hardware interrupt since qemu works as a device model :) But change here
may cause following code on cpu_reset_interrupt().
	if ( mask == CPU_INTERRUPT_HARD)
	     ......clear the shared page...

2) I think the pic_intr filed on shared page is something like IRR on
pic device model, and can be logically treated as part of PIC device
model. So it make sense to clear it on pic_reset() just like clear IRR
there.

How do you think about this?


Thanks
Yunhong Jiang
> 
>   -- Keir

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 13 Sep 2005, at 03:40, Jiang, Yunhong wrote:
> 1) I think the  cpu_reset_interrupt() is a common API on qemu for
> hardware interrupt/exception etc,while clear the shared page is just 
> for
> hardware interrupt. Of course, this function is currently used just for
> hardware interrupt since qemu works as a device model :) But change 
> here
> may cause following code on cpu_reset_interrupt().
> 	if ( mask == CPU_INTERRUPT_HARD)
> 	     ......clear the shared page...
Hmm... okay, I see your point here. This would be a bit ugly.

I think placing your  patch under pic_reset() is a better plan.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel