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?