[Qemu-devel] Register uhci_reset() callback.

[Qemu-devel] Register uhci_reset() callback.

[Gleb Natapov]

Update IRQ line on reset.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index 689d40a..2db9850 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -346,6 +346,7 @@ static void uhci_reset(UHCIState *s)
     }
 
     uhci_async_cancel_all(s);
+    uhci_update_irq(s);
 }
 
 static void uhci_save(QEMUFile *f, void *opaque)
@@ -1093,6 +1094,7 @@ void usb_uhci_piix3_init(PCIBus *bus, int devfn)
     }
     s->frame_timer = qemu_new_timer(vm_clock, uhci_frame_timer, s);
 
+    qemu_register_reset(uhci_reset, 0, s);
     uhci_reset(s);
 
     /* Use region 4 for consistency with real hardware.  BSD guests seem
@@ -1127,6 +1129,7 @@ void usb_uhci_piix4_init(PCIBus *bus, int devfn)
     }
     s->frame_timer = qemu_new_timer(vm_clock, uhci_frame_timer, s);
 
+    qemu_register_reset(uhci_reset, 0, s);
     uhci_reset(s);
 
     /* Use region 4 for consistency with real hardware.  BSD guests seem
--
			Gleb.

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

[Paul Brook]

On Thursday 11 June 2009, Gleb Natapov wrote:
> Update IRQ line on reset.

This should not be necessary.

Paul

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

[Gleb Natapov]

On Thu, Jun 11, 2009 at 02:41:33PM +0100, Paul Brook wrote:
> On Thursday 11 June 2009, Gleb Natapov wrote:
> > Update IRQ line on reset.
> 
> This should not be necessary.
> 
You are always so brief. Can you please spent a little bit more time and
afford and explain why is should not be necessary, because it is clear
that I think it is that is why I put it there in the first place.

--
			Gleb.

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

[Paul Brook]

On Thursday 11 June 2009, Gleb Natapov wrote:
> On Thu, Jun 11, 2009 at 02:41:33PM +0100, Paul Brook wrote:
> > On Thursday 11 June 2009, Gleb Natapov wrote:
> > > Update IRQ line on reset.
> >
> > This should not be necessary.
>
> You are always so brief. Can you please spent a little bit more time and
> afford and explain why is should not be necessary, because it is clear
> that I think it is that is why I put it there in the first place.

Ok. Let me put this annother way.
I don't think this is necessary. What bug are you fixing, and why is this the 
right solution?

Paul

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

[Gleb Natapov]

On Thu, Jun 11, 2009 at 02:53:12PM +0100, Paul Brook wrote:
> On Thursday 11 June 2009, Gleb Natapov wrote:
> > On Thu, Jun 11, 2009 at 02:41:33PM +0100, Paul Brook wrote:
> > > On Thursday 11 June 2009, Gleb Natapov wrote:
> > > > Update IRQ line on reset.
> > >
> > > This should not be necessary.
> >
> > You are always so brief. Can you please spent a little bit more time and
> > afford and explain why is should not be necessary, because it is clear
> > that I think it is that is why I put it there in the first place.
> 
> Ok. Let me put this annother way.
You don't think registering reset is not necessary or you don't think
calling uhci_update_irq(s) in the reset handler is necessary?

> I don't think this is necessary. What bug are you fixing, and why is this the 
> right solution?
RHEL4.8 hangs after reboot because usb interrupt is not goes away. This
patch fixes it. Reseting all devices on a system reset is a right thing to do
even if there is no reproducible bug exist. 

--
			Gleb.

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

[Jamie Lokier]

Gleb Natapov wrote:
> RHEL4.8 hangs after reboot because usb interrupt is not goes away. This
> patch fixes it. Reseting all devices on a system reset is a right thing to do
> even if there is no reproducible bug exist. 

I've got real hardware with this problem!  It doesn't reset the USB
hardware (with some kinds of reset), and after reset the kernel runs
about 100x too slow because it's getting a high rate of USB
interrupts, until it loads the USB driver.

In the real hardware the solution is to explicitly shut down the USB
chip prior to manual reset, and fortunately the watchdog/power-on
resets do reset the USB chip anyway.

So I definitely support the idea that all components should be reset
on a system reset event :-)

Now, a CPU-only reset, such as triple fault on x86, that's a bit different.

-- Jamie

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

[Gleb Natapov]

On Thu, Jun 11, 2009 at 05:38:03PM +0100, Jamie Lokier wrote:
> Gleb Natapov wrote:
> > RHEL4.8 hangs after reboot because usb interrupt is not goes away. This
> > patch fixes it. Reseting all devices on a system reset is a right thing to do
> > even if there is no reproducible bug exist. 
> 
> I've got real hardware with this problem!  It doesn't reset the USB
> hardware (with some kinds of reset), and after reset the kernel runs
> about 100x too slow because it's getting a high rate of USB
> interrupts, until it loads the USB driver.
> 
> In the real hardware the solution is to explicitly shut down the USB
> chip prior to manual reset, and fortunately the watchdog/power-on
> resets do reset the USB chip anyway.
> 
> So I definitely support the idea that all components should be reset
> on a system reset event :-)
> 
> Now, a CPU-only reset, such as triple fault on x86, that's a bit different.
> 
On x86 triple fault wired to system reset.

--
			Gleb.

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

[Avi Kivity]

On 06/11/2009 07:40 PM, Gleb Natapov wrote:
>> Now, a CPU-only reset, such as triple fault on x86, that's a bit different.
>>
>>      
> On x86 triple fault wired to system reset.
>    

Some actually wire triple fault (shutdown) to init.  It's pretty broken.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Jamie Lokier]

Avi Kivity wrote:
> On 06/11/2009 07:40 PM, Gleb Natapov wrote:
> >>Now, a CPU-only reset, such as triple fault on x86, that's a bit 
> >>different.
> >>
> >>     
> >On x86 triple fault wired to system reset.
> >   
> 
> Some actually wire triple fault (shutdown) to init.  It's pretty broken.

That sounds useful, actually, for those 286 OSes which use
triple-fault to switch from protected mode to real mode.  No need to
reinitialise all the hardware if it just restarts the CPU.

-- Jamie

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

[Avi Kivity]

On 06/16/2009 06:00 PM, Jamie Lokier wrote:
> Avi Kivity wrote:
>    
>> On 06/11/2009 07:40 PM, Gleb Natapov wrote:
>>      
>>>> Now, a CPU-only reset, such as triple fault on x86, that's a bit
>>>> different.
>>>>
>>>>
>>>>          
>>> On x86 triple fault wired to system reset.
>>>
>>>        
>> Some actually wire triple fault (shutdown) to init.  It's pretty broken.
>>      
>
> That sounds useful, actually, for those 286 OSes which use
> triple-fault to switch from protected mode to real mode.  No need to
> reinitialise all the hardware if it just restarts the CPU.
>    

Ah, I remember now.  But on modern hardware it breaks badly.  Intel 
processors block INIT if vmx is enabled, and the rest of the hardware 
isn't reset so it could be dmaing all over the place.

-- 
error compiling committee.c: too many arguments to function

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

[Jamie Lokier]

Avi Kivity wrote:
> On 06/16/2009 06:00 PM, Jamie Lokier wrote:
> >Avi Kivity wrote:
> >   
> >>On 06/11/2009 07:40 PM, Gleb Natapov wrote:
> >>     
> >>>>Now, a CPU-only reset, such as triple fault on x86, that's a bit
> >>>>different.
> >>>>
> >>>>
> >>>>         
> >>>On x86 triple fault wired to system reset.
> >>>
> >>>       
> >>Some actually wire triple fault (shutdown) to init.  It's pretty broken.
> >>     
> >
> >That sounds useful, actually, for those 286 OSes which use
> >triple-fault to switch from protected mode to real mode.  No need to
> >reinitialise all the hardware if it just restarts the CPU.
> >   
> 
> Ah, I remember now.  But on modern hardware it breaks badly.  Intel 
> processors block INIT if vmx is enabled, and the rest of the hardware 
> isn't reset so it could be dmaing all over the place.

When running 286 code, continuing to DMA is actually correct if using
triple-fault to switch to real mode.  (Yes I still have some 286 code
lying around somewhere).  It's to context switch, not stop devices :-)

Obviously nowadays if you have 286 code that you need to run, you'd
run it in a VM, not real hardware, so that backward compatibility is
quite unnecessary now, and actively unhelpful.

But a VM should offer it, either as an option or always, so you can
run old 286 code in the VM.

Perhaps the ideal thing to do in a VM (i.e. QEMU) is map triple-fault
to CPU reset and BIOS fast restart when in 286 protected mode, and map
triple-fault to full system reset when not in 286 protected mode.

-- Jamie

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

[Avi Kivity]

On 06/11/2009 04:46 PM, Gleb Natapov wrote:
> On Thu, Jun 11, 2009 at 02:41:33PM +0100, Paul Brook wrote:
>    
>> On Thursday 11 June 2009, Gleb Natapov wrote:
>>      
>>> Update IRQ line on reset.
>>>        
>> This should not be necessary.
>>
>>      
> You are always so brief.

To be fair, so are you.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Gleb Natapov]

On Mon, Jun 15, 2009 at 07:21:09PM +0300, Avi Kivity wrote:
> On 06/11/2009 04:46 PM, Gleb Natapov wrote:
>> On Thu, Jun 11, 2009 at 02:41:33PM +0100, Paul Brook wrote:
>>    
>>> On Thursday 11 June 2009, Gleb Natapov wrote:
>>>      
>>>> Update IRQ line on reset.
>>>>        
>>> This should not be necessary.
>>>
>>>      
>> You are always so brief.
>
> To be fair, so are you.
>
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.

--
			Gleb.

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

[Avi Kivity]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

On 06/15/2009 08:02 PM, Gleb Natapov wrote:
> On Mon, Jun 15, 2009 at 07:21:09PM +0300, Avi Kivity wrote:
>    
>> On 06/11/2009 04:46 PM, Gleb Natapov wrote:
>>      
>>> On Thu, Jun 11, 2009 at 02:41:33PM +0100, Paul Brook wrote:
>>>
>>>        
>>>> On Thursday 11 June 2009, Gleb Natapov wrote:
>>>>
>>>>          
>>>>> Update IRQ line on reset.
>>>>>
>>>>>            
>>>> This should not be necessary.
>>>>
>>>>
>>>>          
>>> You are always so brief.
>>>        
>> To be fair, so are you.
>>
>>      
> 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.
>    

Citing which guests are unbroken by the patch, for example, would have 
helped.  As someone who accepts a lot of excellent patches from you, I 
also find the change logs to be laconic.

Of course, replies like "this should not be necessary" provide almost 
zero information and don't improve the situation.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.


[-- Attachment #2: Type: text/html, Size: 1986 bytes --]

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

[Gleb Natapov]

On Mon, Jun 15, 2009 at 08:17:08PM +0300, Avi Kivity wrote:
> On 06/15/2009 08:02 PM, Gleb Natapov wrote:
>> On Mon, Jun 15, 2009 at 07:21:09PM +0300, Avi Kivity wrote:
>>    
>>> On 06/11/2009 04:46 PM, Gleb Natapov wrote:
>>>      
>>>> On Thu, Jun 11, 2009 at 02:41:33PM +0100, Paul Brook wrote:
>>>>
>>>>        
>>>>> On Thursday 11 June 2009, Gleb Natapov wrote:
>>>>>
>>>>>          
>>>>>> Update IRQ line on reset.
>>>>>>
>>>>>>            
>>>>> This should not be necessary.
>>>>>
>>>>>
>>>>>          
>>>> You are always so brief.
>>>>        
>>> To be fair, so are you.
>>>
>>>      
>> 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.
>>    
>
> Citing which guests are unbroken by the patch, for example, would have  
> helped.  As someone who accepts a lot of excellent patches from you, I  
> also find the change logs to be laconic.
>
I agree, I will resend the patch with more detailed changelog. But reset
handler should be added to each device (and PCI devices reset should
lower interrupt line since it may be shared) even if there is no
reproducible test case to show.

I'll try to work on improving my commit logs :)

--
			Gleb.

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

[Paul Brook]

> 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?

Paul

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

[Gleb Natapov]

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?

--
			Gleb.

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

[Blue Swirl]

On 6/15/09, Gleb Natapov <gleb@redhat.com> 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).

Maybe lowering the irq should actually be unnecessary, qemu_irq is not
equal to hardware interrupt line.

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

[Gleb Natapov]

On Mon, Jun 15, 2009 at 09:57:54PM +0300, Blue Swirl wrote:
> On 6/15/09, Gleb Natapov <gleb@redhat.com> 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.

--
			Gleb.

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

[Blue Swirl]

On 6/15/09, Gleb Natapov <gleb@redhat.com> wrote:
> On Mon, Jun 15, 2009 at 09:57:54PM +0300, Blue Swirl wrote:
>  > On 6/15/09, Gleb Natapov <gleb@redhat.com> 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.

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

[Gleb Natapov]

On Mon, Jun 15, 2009 at 10:50:04PM +0300, Blue Swirl wrote:
> On 6/15/09, Gleb Natapov <gleb@redhat.com> wrote:
> > On Mon, Jun 15, 2009 at 09:57:54PM +0300, Blue Swirl wrote:
> >  > On 6/15/09, Gleb Natapov <gleb@redhat.com> 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.

--
			Gleb.

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

[Blue Swirl]

On 6/15/09, Gleb Natapov <gleb@redhat.com> wrote:
> On Mon, Jun 15, 2009 at 10:50:04PM +0300, Blue Swirl wrote:
>  > On 6/15/09, Gleb Natapov <gleb@redhat.com> wrote:
>  > > On Mon, Jun 15, 2009 at 09:57:54PM +0300, Blue Swirl wrote:
>  > >  > On 6/15/09, Gleb Natapov <gleb@redhat.com> 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.

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

[Gleb Natapov]

On Tue, Jun 16, 2009 at 06:14:51PM +0300, Blue Swirl wrote:
> > 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.
The patch that does that was rejected earlier :)
http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html
http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00344.html

--
			Gleb.

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

[Blue Swirl]

On 6/16/09, Gleb Natapov <gleb@redhat.com> wrote:
> On Tue, Jun 16, 2009 at 06:14:51PM +0300, Blue Swirl wrote:
>  > > 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.
>
> The patch that does that was rejected earlier :)
>  http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html
>  http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00344.html

I think patch 2/3 is bogus, but 3/3 should be the correct way. Nobody
commented on that.

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

[Gleb Natapov]

On Tue, Jun 16, 2009 at 07:54:25PM +0300, Blue Swirl wrote:
> On 6/16/09, Gleb Natapov <gleb@redhat.com> wrote:
> > On Tue, Jun 16, 2009 at 06:14:51PM +0300, Blue Swirl wrote:
> >  > > 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.
> >
> > The patch that does that was rejected earlier :)
> >  http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html
> >  http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00344.html
> 
> I think patch 2/3 is bogus, but 3/3 should be the correct way. Nobody
> commented on that.
> 
What is bogus about 2/3? But the comment were not about specific patch,
more about the general approach. And conclusion was that each device should
lower its line. This is how real HW works BTW. IMHO both things should
be done, pci bus should reset its internal state and each device should
lower its own line (bus implementation may be changed in the future to
more be like real HW).

--
			Gleb.

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

[Blue Swirl]

On 6/16/09, Gleb Natapov <gleb@redhat.com> wrote:
> On Tue, Jun 16, 2009 at 07:54:25PM +0300, Blue Swirl wrote:
>  > On 6/16/09, Gleb Natapov <gleb@redhat.com> wrote:
>  > > On Tue, Jun 16, 2009 at 06:14:51PM +0300, Blue Swirl wrote:
>  > >  > > 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.
>  > >
>  > > The patch that does that was rejected earlier :)
>  > >  http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html
>  > >  http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00344.html
>  >
>  > I think patch 2/3 is bogus, but 3/3 should be the correct way. Nobody
>  > commented on that.
>  >
>
> What is bogus about 2/3? But the comment were not about specific patch,
>  more about the general approach. And conclusion was that each device should
>  lower its line. This is how real HW works BTW. IMHO both things should

That was the conclusion then, but it was incorrect. Real HW works like
that, but qemu_irq only handles transition edges. It does not have
state.

>  be done, pci bus should reset its internal state and each device should
>  lower its own line (bus implementation may be changed in the future to
>  more be like real HW).

Bus should reset its own internal state but devices need not do
anything about the qemu_irq. In 2/3 bus would reset also devices,
which is even more wrong.

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

[Gleb Natapov]

On Tue, Jun 16, 2009 at 08:19:00PM +0300, Blue Swirl wrote:
> On 6/16/09, Gleb Natapov <gleb@redhat.com> wrote:
> > On Tue, Jun 16, 2009 at 07:54:25PM +0300, Blue Swirl wrote:
> >  > On 6/16/09, Gleb Natapov <gleb@redhat.com> wrote:
> >  > > On Tue, Jun 16, 2009 at 06:14:51PM +0300, Blue Swirl wrote:
> >  > >  > > 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.
> >  > >
> >  > > The patch that does that was rejected earlier :)
> >  > >  http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html
> >  > >  http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00344.html
> >  >
> >  > I think patch 2/3 is bogus, but 3/3 should be the correct way. Nobody
> >  > commented on that.
> >  >
> >
> > What is bogus about 2/3? But the comment were not about specific patch,
> >  more about the general approach. And conclusion was that each device should
> >  lower its line. This is how real HW works BTW. IMHO both things should
> 
> That was the conclusion then, but it was incorrect. Real HW works like
> that, but qemu_irq only handles transition edges. It does not have
> state.
> 
The state is handled elsewhere. And device on reset has to make sure
that state is correct wherever it is maintained. What about the mantra
that we should be as close to HW as possible? Reseting state only on
a bus level will not work for hot-unplug anyway.

> >  be done, pci bus should reset its internal state and each device should
> >  lower its own line (bus implementation may be changed in the future to
> >  more be like real HW).
> 
> Bus should reset its own internal state but devices need not do
> anything about the qemu_irq. In 2/3 bus would reset also devices,
> which is even more wrong.
It resets part of its internal state that happens to be maintained per
device. It is possible to register reset handler per device that will
clear the state, but then we will have to add qemu_reset_unregister()
for hot-unplug.

--
			Gleb.

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

[Avi Kivity]

On 06/16/2009 08:19 PM, Blue Swirl wrote:
> That was the conclusion then, but it was incorrect. Real HW works like
> that, but qemu_irq only handles transition edges. It does not have
> state.
>    

A hardware irq line does not have state either.  It's a means of 
exposing state within a device to somewhere else.  On reset, the device 
internal state changes, and the qemu_irq must reflect that change.

Internal muxes like the shared-line OR gates don't have state; they can 
always be recomputed from their inputs.

-- 
error compiling committee.c: too many arguments to function

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

[Paul Brook]

On Tuesday 16 June 2009, Gleb Natapov wrote:
> On Tue, Jun 16, 2009 at 06:14:51PM +0300, Blue Swirl wrote:
> > > 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.
>
> The patch that does that was rejected earlier :)
> http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html
> http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00344.html

You're both wrong.

If allow devices to be reset independently then they should probably set theit 
IRQ output on reset.

IRQ muxes (e.g. PCI busses) should handle reseting and save/restore of their 
own internal state.

Devices should not cause IRQ state changes on restore. Commit 3dcd219f is 
incorrect.

Paul

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

[Blue Swirl]

On 6/16/09, Paul Brook <paul@codesourcery.com> wrote:
> On Tuesday 16 June 2009, Gleb Natapov wrote:
>  > On Tue, Jun 16, 2009 at 06:14:51PM +0300, Blue Swirl wrote:
>  > > > 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.
>  >
>  > The patch that does that was rejected earlier :)
>  > http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html
>  > http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00344.html
>
>
> You're both wrong.
>
>  If allow devices to be reset independently then they should probably set theit
>  IRQ output on reset.
>
>  IRQ muxes (e.g. PCI busses) should handle reseting and save/restore of their
>  own internal state.

Fully agree.

>  Devices should not cause IRQ state changes on restore. Commit 3dcd219f is
>  incorrect.

I'm not so sure about this, but I can't think of a restore sequence
where the IRQ state would need to be changed if the IRQs tied together
are handled correctly. But surely if the devices states are restored
in strange order, the state changes could cause problems because the
device receiving the IRQ may still contain old state.

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

[Paul Brook]

> >  Devices should not cause IRQ state changes on restore. Commit 3dcd219f
> > is incorrect.
>
> I'm not so sure about this, but I can't think of a restore sequence
> where the IRQ state would need to be changed if the IRQs tied together
> are handled correctly. But surely if the devices states are restored
> in strange order, the state changes could cause problems because the
> device receiving the IRQ may still contain old state.

It's precisely because devices are restored in unpredictable order that they 
should not be communicating with other devices (e.g. by modifying IRQ lines).

Consider a system with a device (DEV) and a level triggered interrupt 
controller (PIC1) chained to an edge triggered interrupt controller (PIC2).

(DEV) ->  (PIC1) -> (PIC2)

Before restore, DEV output is low, PIC1 has the interrupt unmasked (but low), 
PIC2 has no pending interrupts.

We now restore a state where DEV output is high, PIC1 has masked the 
interrupt, and PIC2 has no pending interrupts. Devices are restored in he 
order PIC2, DEV, PIC1.

If devices toggle their interrupts on restore then we get incorrect state 
after the restore:

PIC2 is restored to the desired no-interrupts-pending state.
DEV is restored. This raises the IRQ, which is passed to PIC1. PIC1 still has 
the old interrupt mask, so passes through to PIC2, which detects the edge 
event and marks the interrupt as pending.
PIC1 is restored, updates the new mask and lowers its output. However this 
does not clear the internal PIC2 pending interrupt flag, so machine state will 
be wrong after resume.

Paul

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

[Blue Swirl]

On 6/16/09, Paul Brook <paul@codesourcery.com> wrote:
> > >  Devices should not cause IRQ state changes on restore. Commit 3dcd219f
>  > > is incorrect.
>  >
>  > I'm not so sure about this, but I can't think of a restore sequence
>  > where the IRQ state would need to be changed if the IRQs tied together
>  > are handled correctly. But surely if the devices states are restored
>  > in strange order, the state changes could cause problems because the
>  > device receiving the IRQ may still contain old state.
>
>
> It's precisely because devices are restored in unpredictable order that they
>  should not be communicating with other devices (e.g. by modifying IRQ lines).
>
>  Consider a system with a device (DEV) and a level triggered interrupt
>  controller (PIC1) chained to an edge triggered interrupt controller (PIC2).
>
>  (DEV) ->  (PIC1) -> (PIC2)
>
>  Before restore, DEV output is low, PIC1 has the interrupt unmasked (but low),
>  PIC2 has no pending interrupts.
>
>  We now restore a state where DEV output is high, PIC1 has masked the
>  interrupt, and PIC2 has no pending interrupts. Devices are restored in he
>  order PIC2, DEV, PIC1.
>
>  If devices toggle their interrupts on restore then we get incorrect state
>  after the restore:
>
>  PIC2 is restored to the desired no-interrupts-pending state.
>  DEV is restored. This raises the IRQ, which is passed to PIC1. PIC1 still has
>  the old interrupt mask, so passes through to PIC2, which detects the edge
>  event and marks the interrupt as pending.
>  PIC1 is restored, updates the new mask and lowers its output. However this
>  does not clear the internal PIC2 pending interrupt flag, so machine state will
>  be wrong after resume.

Yes, this is the "bad" restore scenario that I had in mind. I still
have a nagging feeling that there is a reverse scenario, where in
order to reach good state, you would have to call the IRQ function.
Anyway, 3dcd219f may not be correct in that case either.

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

[Jamie Lokier]

Blue Swirl wrote:
> On 6/16/09, Paul Brook <paul@codesourcery.com> wrote:
> > > >  Devices should not cause IRQ state changes on restore. Commit 3dcd219f
> >  > > is incorrect.
> >  >
> >  > I'm not so sure about this, but I can't think of a restore sequence
> >  > where the IRQ state would need to be changed if the IRQs tied together
> >  > are handled correctly. But surely if the devices states are restored
> >  > in strange order, the state changes could cause problems because the
> >  > device receiving the IRQ may still contain old state.
> >
> >
> > It's precisely because devices are restored in unpredictable order that they
> >  should not be communicating with other devices (e.g. by modifying IRQ lines).
> >
> >  Consider a system with a device (DEV) and a level triggered interrupt
> >  controller (PIC1) chained to an edge triggered interrupt controller (PIC2).
> >
> >  (DEV) ->  (PIC1) -> (PIC2)
> >
> >  Before restore, DEV output is low, PIC1 has the interrupt unmasked (but low),
> >  PIC2 has no pending interrupts.
> >
> >  We now restore a state where DEV output is high, PIC1 has masked the
> >  interrupt, and PIC2 has no pending interrupts. Devices are restored in he
> >  order PIC2, DEV, PIC1.
> >
> >  If devices toggle their interrupts on restore then we get incorrect state
> >  after the restore:
> >
> >  PIC2 is restored to the desired no-interrupts-pending state.
> >  DEV is restored. This raises the IRQ, which is passed to PIC1. PIC1 still has
> >  the old interrupt mask, so passes through to PIC2, which detects the edge
> >  event and marks the interrupt as pending.
> >  PIC1 is restored, updates the new mask and lowers its output. However this
> >  does not clear the internal PIC2 pending interrupt flag, so machine state will
> >  be wrong after resume.
> 
> Yes, this is the "bad" restore scenario that I had in mind. I still
> have a nagging feeling that there is a reverse scenario, where in
> order to reach good state, you would have to call the IRQ function.
> Anyway, 3dcd219f may not be correct in that case either.

If any initialisation order might occur, then you really need all the
states to be restored, including output levels, without any side
effects, and then enable processing in all devices together.

It's analogous to a distributed atomic transaction problem.

So you need two phases:

    - Restoring: All devices states are restored, one by one including
      output levels of interrupt lines and GPIOs, but nothing actually
      _happens_ when those levels are set.

    - Running: All devices start running at the same instant from
      their restored state.  They don't need to reexamine input
      levels, because their internal states are already consistent
      with the input levels.

In the restoring phase, you might still use the normal functions to
set output levels, but they would be prevented from being passed as
changes to other devices.

Anything else might be made to work with particular PICs etc., but
two-phase restore is what you need to work with any wiring (including
cycles) of arbitrary devices with arbitrary states.

-- Jamie

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

[Jamie Lokier]

Jamie Lokier wrote:
> Blue Swirl wrote:
> It's analogous to a distributed atomic transaction problem.
> 
> So you need two phases:
> 
>     - Restoring: All devices states are restored, one by one including
>       output levels of interrupt lines and GPIOs, but nothing actually
>       _happens_ when those levels are set.
> 
>     - Running: All devices start running at the same instant from
>       their restored state.  They don't need to reexamine input
>       levels, because their internal states are already consistent
>       with the input levels.
> 
> In the restoring phase, you might still use the normal functions to
> set output levels, but they would be prevented from being passed as
> changes to other devices.
> 
> Anything else might be made to work with particular PICs etc., but
> two-phase restore is what you need to work with any wiring (including
> cycles) of arbitrary devices with arbitrary states.

I should say, the above applies to both restoring saved state, and
whole system resets.

That's why real hardware has a nice long reset pulse, during which
every input change is ignored until the reset pulse is removed.

-- Jamie

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

[Blue Swirl]

On 6/16/09, Jamie Lokier <jamie@shareable.org> wrote:
> Jamie Lokier wrote:
>
> > Blue Swirl wrote:
>  > It's analogous to a distributed atomic transaction problem.

I did not wrote this, it was you.

>  >
>  > So you need two phases:
>  >
>  >     - Restoring: All devices states are restored, one by one including
>  >       output levels of interrupt lines and GPIOs, but nothing actually
>  >       _happens_ when those levels are set.
>  >
>  >     - Running: All devices start running at the same instant from
>  >       their restored state.  They don't need to reexamine input
>  >       levels, because their internal states are already consistent
>  >       with the input levels.
>  >
>  > In the restoring phase, you might still use the normal functions to
>  > set output levels, but they would be prevented from being passed as
>  > changes to other devices.
>  >
>  > Anything else might be made to work with particular PICs etc., but
>  > two-phase restore is what you need to work with any wiring (including
>  > cycles) of arbitrary devices with arbitrary states.
>
>
> I should say, the above applies to both restoring saved state, and
>  whole system resets.
>
>  That's why real hardware has a nice long reset pulse, during which
>  every input change is ignored until the reset pulse is removed.

qemu_irq does not have state. It's different from real HW IRQ, so QEMU
devices don't need to do anything. I also had this confused earlier.

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

[Gleb Natapov]

On Tue, Jun 16, 2009 at 10:10:59PM +0300, Blue Swirl wrote:
> >  That's why real hardware has a nice long reset pulse, during which
> >  every input change is ignored until the reset pulse is removed.
> 
> qemu_irq does not have state. It's different from real HW IRQ, so QEMU
> devices don't need to do anything. I also had this confused earlier.
> 
Yes, qemu_irq does not have state, but nonetheless the sate exists. It is
maintained inside piix3 (this is just implementation detail that may
or may not go away). But who, if not device itself, should know better
what IRQ level should be in any given time? You can literally call
qemu_irq() after each line of device emulation code and it will be OK,
slow may be, but correct. The more you call qemu_irq() the more closely
you emulate real HW level interrupt.

--
			Gleb.

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

[Paul Brook]

> If any initialisation order might occur, then you really need all the
> states to be restored, including output levels, without any side
> effects, and then enable processing in all devices together.

There is no output or input level state, other than implicitly as part of 
device internal state. qemu_irq is a stateless mechanism for communicating 
level change events. In effect we only model edge triggering, and individual 
devices emulate level triggering on top of that.

Paul

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

[Gleb Natapov]

On Tue, Jun 16, 2009 at 05:54:54PM +0100, Paul Brook wrote:
> On Tuesday 16 June 2009, Gleb Natapov wrote:
> > On Tue, Jun 16, 2009 at 06:14:51PM +0300, Blue Swirl wrote:
> > > > 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.
> >
> > The patch that does that was rejected earlier :)
> > http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html
> > http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00344.html
> 
> You're both wrong.
> 
As always.

> If allow devices to be reset independently then they should probably set theit 
> IRQ output on reset.
It is not "if" it is "when". We have to allow device to be reset independently for
hot-unplug. The reset mechanism of qemu is not sophisticated to do that yet.
This should be a part of a device model. So I take this statement of
yours as if you agree that each device should reset its irq line. Good.

> 
> IRQ muxes (e.g. PCI busses) should handle reseting and save/restore of their 
> own internal state.
Here are patches. You are welcome to commit them:
http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00342.html
http://lists.gnu.org/archive/html/qemu-devel/2009-06/msg00344.html


> Devices should not cause IRQ state changes on restore. Commit 3dcd219f is 
> incorrect.
> 
Don't agree or disagree on this. Haven't thought about it much.

--
			Gleb.

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

[Paul Brook]

> > 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.

I consider this to be a bug in git - or at something you need to be aware of 
and avoid.

IMHO the subject of an email should not contain critical information. 
Obviously it is important to give emails meaningful subject lines, but the 
body of the email should not require the reader to refer back to the subject. 
On pretty much every email client I've ever used the subject and other header 
information is displayed separately to the body of the email.

Paul