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

[Gleb Natapov]

On Wed, Jun 17, 2009 at 01:36:08PM +0200, Filip Navara wrote:
> On Wed, Jun 17, 2009 at 1:06 PM, Gleb Natapov <address@hidden> wrote:
> 
> > 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.
> 
> 
> That's why all the paths where the level in device emulation can change has
> to be covered, so the information in piix3 stays accurate. What I'm saying
> is that the reset callback is not one of the points where you can call
> qemu_set_irq since the state of other devices is unknown at that point (for
> the purpose of simulating interrupt levels). When the reset callbacks are
I understand what you are saying, but the logic that emulates level irqs
from qemu_irq() edge (in piix3) is not a part of device emulation, so it
is not obvious to me that system reset should reset it and bring it to
unknown state. Line state change may reach IRQ chip before or after reset, but
this doesn't matter since IRQ chip will be in knows state in both cases.

> done with their work, all the simulated interrupt levels at all layers have
> to be restored to known state. This state is currently with all the levels
> set to zero. If you call qemu_set_irq during the reset callbacks the
> interrupt levels recorded in the device tree may end up in uncertain state.
That may happen on real HW too. OS will get spurious interrupt, that's it.

> The fact that for PCI devices it works today is just a nice coincidence, if
> the device "tree" was really a graph (imagine a cable from the PCI card to
> some other device as it was in the SB16 cards) then it would suddenly break.
> 
> 
> > > 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.
> 
> 
> To change the level you first have to have the whole device tree topology in
> known state. In the reset callback that's not the case. If there was a
Level change (as seen by other device) can happen only after or before
reset, not during reset. In both cases the device state is known.

> "late" reset callback as mentioned earlier then that would be a place where
> it could work.
> 
> Generally speaking, it is fine to call qemu_set_irq even if the level didn't
> change, but only if the whole device tree topology has up-to-date interrupt
> levels.
> 
> 
> > > > 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.
> 
> 
> I'm suggesting not to call it at all and always reset the recorded
> "interrupt levels" all the way down the device tree on reset. If there is a
By doing this you are setting device irq line without asking device. It
will work since assumption that irq level is zero after reset is usually
correct.

> device which sets the IRQ line high on reset then we should introduce the
> "late" reset/init callback.
> 
Yes, calling set_irq(1) on reset may not achieve this since irq chip may
be reset after set_irq() was called.

> 
> > > > > 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.
> 
> 
> If it's not clear then let's make it clear. Real HW communicates interrupt
> levels and detects edges, QEMU communicates edges and simulates levels, so
> it has to work differently than on real HW in this particular case.
> 
> The system reset callback would reset device registers to a known state as
> specified by the HW documentation. The interrupt levels would be driven low
> at this point on the devices which simulate "interrupt levels" (buses, PIC,
> etc.) - as if "0" was sent to the copper wire ;)
> 
> The "late" reset/init callback would drive the IRQs (and GPIOs) high if
> necessary once the system is in a known state, ie. after all reset callbacks
> have successfully completed.
> 
Basically you want to simulate "dead" period when devices ignore their inputs 
for
some time after reset (like on real HW).

> Hot-unplug callback doesn't need reset the registers of the device, it only
> has to drive the IRQ low on the bus - ie. call *_set_irq(0) for the specific
> bus. In theory that could be done at the bus level.
> 
I should be done at bus level indeed. The device is disconnected at that
point.

> Feel free to disagree or correct me. Suggestions welcome.
> 
Not disagreeing but this still is not enough, for instance there are
devices that have different state after power-up and reset.
 
> 
> > > > "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?
> >
> 
> I don't remember the exact contents of 2/3, sorry.
> 
http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html

--
                        Gleb.