From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=54916 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q1ek2-0003Al-Az for qemu-devel@nongnu.org; Mon, 21 Mar 2011 09:01:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q1ek1-0006E2-0Q for qemu-devel@nongnu.org; Mon, 21 Mar 2011 09:01:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25051) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q1ek0-0006Cc-O4 for qemu-devel@nongnu.org; Mon, 21 Mar 2011 09:01:48 -0400 Date: Mon, 21 Mar 2011 15:01:31 +0200 From: "Michael S. Tsirkin" Message-ID: <20110321130131.GA30198@redhat.com> References: <533593a679f7c243ddce87c9c3a7c31c6f67acd7.1300540833.git.yamahata@valinux.co.jp> <20110321113707.GC27445@redhat.com> <20110321121032.GA9998@valinux.co.jp> <20110321123111.GA29012@redhat.com> <20110321125656.GC9998@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110321125656.GC9998@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH v3 3/3] piix_pci: optimize set irq path List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org 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