[Qemu-devel] Re: [PATCH v3 3/3] piix_pci: optimize set irq path

[Isaku Yamahata <yamahata@valinux.co.jp>]

On Mon, Mar 21, 2011 at 02:31:11PM +0200, Michael S. Tsirkin wrote:
> On Mon, Mar 21, 2011 at 09:10:32PM +0900, Isaku Yamahata wrote:
> > On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote:
> > > > +static int piix3_post_load(void *opaque, int version_id)
> > > > +{
> > > > +    PIIX3State *piix3 = opaque;
> > > > +    piix3_update_irq_levels(piix3);
> > > 
> > > Couldn't figure out why would we not want to
> > > propagate the interrupts here.
> > > Could you explain please?
> > > What happens if we do propagate them?
> > > Nothing bad, right?
> > 
> > I wanted to be just conservative.
> > If you are brave enough to change the behavior, I'm fine with propagating
> > interrupts.
> > 
> > If we propagate the interrupts, guest OS may see interrupts
> > unnecessarily/spuriously injected after load.
> > Probably such interrupts doesn't harm OSes, so there is nothing
> > bad in theory as you said.
> > On the other hand, I hesitated to change the existing behavior because
> > it would be very difficult to debug it and to test many OSes.
> 
> I expect it won't change the behaviour because the interrupts
> are level: at the moment e.g. pci devices already reassert
> interrupts on load.
> 
> But I agree it better be a separate patch, and needs a lot of testing.

Like this?

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index f07e19d..8052c1e 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -280,8 +280,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
                     ((PIIX_NUM_PIRQS - 1) << (pic_irq * PIIX_NUM_PIRQS))));
 }
 
-static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
-                                bool propagate)
+static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level)
 {
     int pic_irq;
     uint64_t mask;
@@ -295,15 +294,13 @@ static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
     piix3->pic_levels &= ~mask;
     piix3->pic_levels |= mask * !!level;
 
-    if (propagate) {
-        piix3_set_irq_pic(piix3, pic_irq);
-    }
+    piix3_set_irq_pic(piix3, pic_irq);
 }
 
 static void piix3_set_irq(void *opaque, int irq_num, int level)
 {
     PIIX3State *piix3 = opaque;
-    piix3_set_irq_level(piix3, irq_num, level, true);
+    piix3_set_irq_level(piix3, irq_num, level);
 }
 
 /* irq routing is changed. so rebuild bitmap */
@@ -314,8 +311,7 @@ static void piix3_update_irq_levels(PIIX3State *piix3)
     piix3->pic_levels = 0;
     for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
         piix3_set_irq_level(piix3, pirq,
-                            pci_bus_get_irq_level(piix3->dev.bus, pirq),
-                            false);
+                            pci_bus_get_irq_level(piix3->dev.bus, pirq));
     }
 }
 


-- 
yamahata