Re: [Qemu-devel] Register uhci_reset() callback.

[Blue Swirl]

On 6/15/09, Gleb Natapov <address@hidden> wrote:
> On Mon, Jun 15, 2009 at 10:50:04PM +0300, Blue Swirl wrote:
>  > On 6/15/09, Gleb Natapov <address@hidden> wrote:
>  > > On Mon, Jun 15, 2009 at 09:57:54PM +0300, Blue Swirl wrote:
>  > >  > On 6/15/09, Gleb Natapov <address@hidden> wrote:
>  > >  > > On Mon, Jun 15, 2009 at 06:56:26PM +0100, Paul Brook wrote:
>  > >  > >  > > May be, but in this case after previous patch to reset 
> interrupt level
>  > >  > >  > > for each device at PCI bridge level was rejected on the 
> premise that
>  > >  > >  > > device should lower its own irq line on reset and since 
> patches started
>  > >  > >  > > flowing in to do just that, I did not expect that eloquent 
> explanation
>  > >  > >  > > would be needed for such trivial and obviously correct change.
>  > >  > >  >
>  > >  > >  > This argument makes no sense. The fact that you'd recently 
> submitted very
>  > >  > >  > similar looking patches which either got rejected or need 
> modification is a
>  > >  > >  > good argument for providing an explanation. How else are we 
> supposed to know
>  > >  > >  > that you're not just making the same mistake again?
>  > >  > >
>  > >  > > They was not even "similar looking". Have you followed the 
> discussion
>  > >  > >  that those patches generated? Look at this commit and especially 
> its commit
>  > >  > >  message: 32c86e95b. This commit is a direct result of the 
> discussion
>  > >  > >  previous patches generated. Look at my patch now. Looks similar, 
> no? So
>  > >  > >  people with commit permissions can follow lower standards that we 
> mere
>  > >  > >  mortals?
>  > >  >
>  > >  > I find nothing wrong with your commit message (for completeness, it
>  > >  > could mention that it installs a reset handler).
>  > >  >
>  > >
>  > > It does in subject line. Subject line goes to log message with git-am.
>  > >
>  > >
>  > >  > Maybe lowering the irq should actually be unnecessary, qemu_irq is not
>  > >  > equal to hardware interrupt line.
>  > >
>  > > 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 will not
>  > >  be able to lower irq line.
>  >
>  > Maybe, but then the other device is reset also piix3 at some point.
>
> Because interrupt line is stuck a guest can't get to the point where it
>  loads a driver to the second device. For outside observer the guest
>  just hangs.

I see. The problem is in piix_pci interrupt handling, pci_irq_levels[]
should be set to zero on reset.