[Qemu-devel] Re: [PATCH 7/7] ahci: work around bug with level interrupts

[Jan Kiszka]

On 2011-02-03 11:38, Alexander Graf wrote:
> 
> On 03.02.2011, at 11:30, Jan Kiszka wrote:
> 
>> On 2011-02-02 15:39, Alexander Graf wrote:
>>> When using level based interrupts, the interrupt is treated the same as an
>>> edge triggered one: leaving the line up does not retrigger the interrupt.
>>>
>>> In fact, when not lowering the line, we won't ever get a new interrupt 
>>> inside
>>> the guest. So let's always retrigger an interrupt as soon as the OS ack'ed
>>> something on the device. This way we're sure the guest doesn't starve on
>>> interrupts until someone fixes the actual interrupt path.
>>>
>>> Signed-off-by: Alexander Graf <address@hidden>
>>>
>>> ---
>>>
>>> v2 -> v3:
>>>
>>>  - add comment about the interrupt hack
>>>
>>> v3 -> v4:
>>>
>>>  - embed non-workaround version in the code
>>> ---
>>> hw/ide/ahci.c |   13 +++++++++++++
>>> 1 files changed, 13 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>> index 98bdf70..10377ca 100644
>>> --- a/hw/ide/ahci.c
>>> +++ b/hw/ide/ahci.c
>>> @@ -152,12 +152,25 @@ static void ahci_check_irq(AHCIState *s)
>>>         }
>>>     }
>>>
>>> +    /* XXX We always lower the interrupt line here because of a bug with
>>> +           interrupt handling in Qemu. When leaving a line up, the 
>>> interrupt
>>> +           does not get retriggered automatically currently. Once that bug 
>>> is
>>> +           fixed, this workaround is not necessary anymore and we only 
>>> need to
>>> +           lower in the else branch. */
>>> +#if 0
>>>     if (s->control_regs.irqstatus &&
>>>         (s->control_regs.ghc & HOST_CTL_IRQ_EN)) {
>>>             ahci_irq_raise(s, NULL);
>>>     } else {
>>>         ahci_irq_lower(s, NULL);
>>>     }
>>> +#else
>>> +    ahci_irq_lower(s, NULL);
>>> +    if (s->control_regs.irqstatus &&
>>> +        (s->control_regs.ghc & HOST_CTL_IRQ_EN)) {
>>> +            ahci_irq_raise(s, NULL);
>>> +    }
>>> +#endif
>>> }
>>>
>>> static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
>>
>> Could you check if this quick-hack obsoletes the above hack?
>>
>> I was hoping it has some influence on our 64-bit Windows issue, but it
>> hasn't, or it's still buggy, or both. However, its intention is to
>> reassert still pending level-triggered IRQs on APIC EOI. This logic is
>> missing in the user space model but exists in KVM's kernel model (I was
>> asking for a test against the latter but unfortunately did not receive
>> an answer - I bet you already don't need your patch over qemu-kvm).
> 
> Oh, sorry for not replying there. I work on qemu.git, so the in-kernel apic 
> is out of question for testing. I tried merging qemu-kvm.git and qemu.git 
> several times and every time just wasted a few hours of my life, so I gave up 
> on testing things on qemu-kvm.git.

Ah, I see. Marcelo recently merged back, resolving the tricky bits, and
now it should be much easier. Anyway.

> 
> The patch works though. If everybody agrees to take it, we can drop patch 7/7 
> of my ahci set.

Cool! I've a few more minor things to clean up here and will sent that
as a series later today, also for 0.14.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux