From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46900) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRLpK-0005o4-Bt for qemu-devel@nongnu.org; Tue, 09 Sep 2014 09:51:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XRLpF-0000cY-7z for qemu-devel@nongnu.org; Tue, 09 Sep 2014 09:51:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29894) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRLpE-0000cB-Ve for qemu-devel@nongnu.org; Tue, 09 Sep 2014 09:51:17 -0400 Date: Tue, 9 Sep 2014 16:54:24 +0300 From: "Michael S. Tsirkin" Message-ID: <20140909135424.GA13212@redhat.com> References: <1410265809-27247-1-git-send-email-pbonzini@redhat.com> <1410265809-27247-10-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1410265809-27247-10-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 09/10] piix: do not raise irq while loading vmstate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: amit.shah@redhat.com, quintela@redhat.com, qemu-devel@nongnu.org, Pavel.Dovgaluk@ispras.ru, dgilbert@redhat.com On Tue, Sep 09, 2014 at 02:30:08PM +0200, Paolo Bonzini wrote: > From: Pavel Dovgalyuk > > This patch disables raising an irq while loading the state of PCI bridge. > The aim of this patch is preserving the same behavior while saving and > restoring the VM state. IRQ is not raised while saving the state of the bridge. > That's why the behavior of the restored system will differ from > the original one. Hmm I don't understand. You are removing call to piix3_update_irq_levels this call is supposed to just sync up bus to irq level. How can this change system state? Saved state is supposed to already be in sync. > This patch eliminates raising an irq and just restores > the calculated state fields in post_load function. > > Signed-off-by: Pavel Dovgalyuk > Signed-off-by: Paolo Bonzini > --- > hw/pci-host/piix.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c > index e0e0946..cd50435 100644 > --- a/hw/pci-host/piix.c > +++ b/hw/pci-host/piix.c > @@ -409,7 +409,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq) > (pic_irq * PIIX_NUM_PIRQS)))); > } > > -static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level) > +static void piix3_set_irq_level_internal(PIIX3State *piix3, int pirq, int level) > { > int pic_irq; > uint64_t mask; > @@ -422,6 +422,18 @@ static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level) > mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + pirq); > piix3->pic_levels &= ~mask; > piix3->pic_levels |= mask * !!level; > +} > + > +static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level) > +{ > + int pic_irq; > + > + pic_irq = piix3->dev.config[PIIX_PIRQC + pirq]; > + if (pic_irq >= PIIX_NUM_PIC_IRQS) { > + return; > + } > + > + piix3_set_irq_level_internal(piix3, pirq, level); > > piix3_set_irq_pic(piix3, pic_irq); > } > @@ -527,7 +539,18 @@ static void piix3_reset(void *opaque) > static int piix3_post_load(void *opaque, int version_id) > { > PIIX3State *piix3 = opaque; > - piix3_update_irq_levels(piix3); > + int pirq; > + > + /* Update irq levels without raising an interrupt which could > + * happen in piix3_update_irq_levels. Raising an IRQ will > + * bring the system to a different state than the saved one. I don't like comments like this: don't discuss what could have been if code was different. Explain why code is like it is. > + * Interrupt state is serialized separately through the i8259. > + */ > + piix3->pic_levels = 0; > + for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) { > + piix3_set_irq_level_internal(piix3, pirq, > + pci_bus_get_irq_level(piix3->dev.bus, pirq)); > + } > return 0; > } > > -- > 2.1.0 > >