Re: [Qemu-devel] [PATCH] Register usb-uhci reset function.

[Gleb Natapov]

On Wed, Jun 17, 2009 at 12:17:53PM +0200, Filip Navara wrote:
> On Wed, Jun 17, 2009 at 11:43 AM, Gleb Natapov <address@hidden> 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 <address@hidden> 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.