| Message ID | 20110201195824.GB17924@volta.aurel32.net |
|---|---|
| State | New |
| Headers | show |
On 01.02.2011, at 20:58, Aurelien Jarno wrote: > 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 <agraf@suse.de> >> >> --- >> >> 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? While I dislike #if 0s in released code in general, it's fine with me. I know what I meant based on the comment, but for others this might make it more explicit. How would we go about committing this? Kevin, will you just change the code inside your tree? Alex
Am 01.02.2011 21:10, schrieb Alexander Graf: > > On 01.02.2011, at 20:58, Aurelien Jarno wrote: > >> 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 <agraf@suse.de> >>> >>> --- >>> >>> 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? > > While I dislike #if 0s in released code in general, it's fine with me. I know what I meant based on the comment, but for others this might make it more explicit. How would we go about committing this? Kevin, will you just change the code inside your tree? I would prefer if you sent a new version of this patch. Kevin
Kevin Wolf wrote: > Am 01.02.2011 21:10, schrieb Alexander Graf: > >> On 01.02.2011, at 20:58, Aurelien Jarno wrote: >> >> >>> 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 <agraf@suse.de> >>>> >>>> --- >>>> >>>> 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? >>> >> While I dislike #if 0s in released code in general, it's fine with me. I know what I meant based on the comment, but for others this might make it more explicit. How would we go about committing this? Kevin, will you just change the code inside your tree? >> > > I would prefer if you sent a new version of this patch. > > Ok, done :) Alex
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,