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

[Aurelien Jarno]

On Tue, Feb 01, 2011 at 07:35:01PM +0100, 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
> ---
>  hw/ide/ahci.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 98bdf70..95e1cf7 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -152,11 +152,15 @@ static void ahci_check_irq(AHCIState *s)
>          }
>      }
>  
> +    /* XXX We 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. */
> +    ahci_irq_lower(s, NULL);
>      if (s->control_regs.irqstatus &&
>          (s->control_regs.ghc & HOST_CTL_IRQ_EN)) {
>              ahci_irq_raise(s, NULL);
> -    } else {
> -        ahci_irq_lower(s, NULL);
>      }
>  }
>  

It's a lot better that way, however I think we should still keep the
correct code. Also given this problem only concerns the i386 target (ppc
code is actually a bit strange, but at the end does the correct thing),
it's something we should probably mention.

What about something like that?

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 671b4df..0b153f0 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -489,12 +489,25 @@ static void ahci_check_irq(AHCIState *s)
         }
     }
 
+#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
+    /* XXX We lower the interrupt line here because of a bug with interrupt
+           handling in Qemu on the i386 target. 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. */
+    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,


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net