From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MGsyl-0003YQ-1w for qemu-devel@nongnu.org; Wed, 17 Jun 2009 07:06:55 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MGsyf-0003TU-Th for qemu-devel@nongnu.org; Wed, 17 Jun 2009 07:06:54 -0400 Received: from [199.232.76.173] (port=52499 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MGsyf-0003T7-NP for qemu-devel@nongnu.org; Wed, 17 Jun 2009 07:06:49 -0400 Received: from mx2.redhat.com ([66.187.237.31]:55986) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MGsyf-0005Su-53 for qemu-devel@nongnu.org; Wed, 17 Jun 2009 07:06:49 -0400 Date: Wed, 17 Jun 2009 14:06:38 +0300 From: Gleb Natapov Subject: Re: [Qemu-devel] [PATCH] Register usb-uhci reset function. Message-ID: <20090617110638.GY19508@redhat.com> References: <20090616124702.GS19508@redhat.com> <5b31733c0906170207u1c553f6by67eb814644f55a10@mail.gmail.com> <20090617094318.GX19508@redhat.com> <5b31733c0906170317m67821bc0o74a656e1b7afef21@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5b31733c0906170317m67821bc0o74a656e1b7afef21@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 12:17:53PM +0200, Filip Navara wrote: > On Wed, Jun 17, 2009 at 11:43 AM, Gleb Natapov wrote: > > > 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. > > > It has to do a lot with that - the qemu_irq abstraction has it's limits. And > there's a certain limit to which you can bend them. qemu_irq simulates > edges, not levels, so levels has to be emulated in the device infrastructure > in a way that doesn't necessarily match what happens in real HW. This also > means that in QEMU you actually have to build infrastructure for anything > that would cause the level change, such as device hot plug/unplug, and > communicate the current level as an edge. > Once again qemu_irq() is used to pass current irq level of the device to the layer that simulate level interrupt: piix3. So level interrupt _is_ simulated, but the only one who nows what level should be at any given moment is a device emulation itself. > 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. > > > In real HW, yes. In QEMU it's not the case with the current abstraction and > adding spurious qemu_set_irq calls won't change that. > Spurious? Irq level changed and you say qemu_set_irq() called to propagate this change is spurious? uhci emulation does not track current irq level it just calls uhci_update_irq() to figure it out and than calls to qemu_set_irq(), so it should be perfectly fine to call qemu_set_irq() even without level change. > > > And if it want to drive irq high on reset it should be able to > > do that. > > > That's a fair argument. Doing so in reset callback is not the way to achieve > it though. With the current abstraction you'd need to add a secondary "late" > reset callback that would be called after all the normal reset callbacks are > processed. Anything else is horribly broken. > Yeah, reset callbacks all the way down. > Consider a device connected to pins of two GPIO controllers. You would need > to ensure the GPIO controllers are in known state before qemu_set_irq is > called, otherwise they can't simulate the interrupt levels from the edge > information. If you did the reset in wrong order, the reset of the GPIO > controllers would discard the information about the pin level from the > device. Calling qemu_irq() on reset should be done only for level interrupt obviously. I am not suggesting it should be done for every device/irq. > > > 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. > > > Agreed. The fix to that is to properly call qemu_set_irq on hot-unplug (or > individual device reset for that matter). > > Instead of fixing problem at > > the level that needs fixing (device reset level) you propose to hack > > solution into piix3 code. > > > That's not what I am proposing! I'm proposing to fix piix3 *system reset* > and implementing the necessary hot-unplug infrastructure for individual > device reset, which is very different thing from system reset. > > So now we have: 1. system reset callback 2. late reset callback (so device can set its line properly after reset) 3. hot-unplug callback. And it is not clear what part of device spec should be implemented in any of them since real spec speaks another language. > > "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. > > > > I didn't oppose patch 3/3 of your previous series. Fixing piix3 code should > definitely be done. > And what about 2/3? Or that part of state can stay intact after reset? -- Gleb.