Message ID | 20110321125656.GC9998@valinux.co.jp |
---|---|
State | New |
Headers | show |
On Mon, Mar 21, 2011 at 09:56:56PM +0900, Isaku Yamahata wrote: > 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? Yes. But need to stress-test with migration, windows and linux guests at least. > 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
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)); } }