From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MGrg0-0002fC-0j for qemu-devel@nongnu.org; Wed, 17 Jun 2009 05:43:28 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MGrfv-0002ef-Fk for qemu-devel@nongnu.org; Wed, 17 Jun 2009 05:43:27 -0400 Received: from [199.232.76.173] (port=53707 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MGrfv-0002eW-59 for qemu-devel@nongnu.org; Wed, 17 Jun 2009 05:43:23 -0400 Received: from mx2.redhat.com ([66.187.237.31]:43002) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MGrfu-0006gz-M4 for qemu-devel@nongnu.org; Wed, 17 Jun 2009 05:43:22 -0400 Date: Wed, 17 Jun 2009 12:43:18 +0300 From: Gleb Natapov Subject: Re: [Qemu-devel] [PATCH] Register usb-uhci reset function. Message-ID: <20090617094318.GX19508@redhat.com> References: <20090616124702.GS19508@redhat.com> <5b31733c0906170207u1c553f6by67eb814644f55a10@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5b31733c0906170207u1c553f6by67eb814644f55a10@mail.gmail.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Filip Navara Cc: qemu-devel@nongnu.org On Wed, Jun 17, 2009 at 11:07:24AM +0200, Filip Navara wrote: > On Tue, Jun 16, 2009 at 2:47 PM, Gleb Natapov wrote: > > > Update irq line on reset. Reseting irq line is required because > > racing irq from pci device will call piix3_set_irq(). piix3_set_irq() > > will remember current level in pci_irq_levels[]. The PIC line will be > > triggered if one of pci_irq_levels[] is set (depends on piix3 config). > > If for instance pci_irq_levels[0] and pci_irq_levels[1] are mapped to > > the same PIC irq and during reset pci_irq_levels[1] == 1, but device > > that drives pci_irq_levels[0] is initialized first the device driver > > will not be able to lower irq line. > > > > I have been trying to stay away from the discussion for a long while, but I > can't keep it anymore. The patch is wrong. Since qemu_irq doesn't hold any > state, the information on reset has to be cleared on the places where the The fact that qemu_irq() doesn't hold any state has nothing to do with what should be done on device reset. Nothing at all, nada, zilch. You can repeat this many times more and it will not became more relevant. What is important is that only device knows what irq level should be at any given moment, and qemu_irq() is the way to communicate this to the system. And if it want to drive irq high on reset it should be able to do that. > state is maintained. Under no circumstances should any *_set_irq() function > should be called from reset handlers! Especially since the order of reset > handlers is not guaranteed. The reseting of the interrupt state in practice > means that interrupt status registers of individual devices should be > cleared, the PCI bus interrupt levels should be cleared - *in the PCI reset > handler* and so on. Eventually you will end up with reset handlers that > clear the state at every level, so there won't be any "hanging interrupts" > after reset. > This will not work for reseting individual device (needed by hot-unplug) since pci chipset reset is not called. Instead of fixing problem at the level that needs fixing (device reset level) you propose to hack solution into piix3 code. "Yaeh, gdb shows we have a wrong value in some random array, why is it there? Who cares, lest zero this thing and forget about it." And BTW _I_ send patch to do just that a week or so ago, and I think it should be applied along with reseting irq line in device reset handler just to prevent buggy devices from hanging a guest. -- Gleb.