Re: [Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
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
---
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
[Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
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
---
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,
--
1.6.0.2
Re: [Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
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
>>>
>>> ---
>>>
>>> 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
Re: [Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
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
>>
>> ---
>>
>> 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
Re: [Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
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
>
> ---
>
> 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
aurel...@aurel32.net http://www.aurel32.net
[Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
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
---
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);
}
}
--
1.6.0.2
Re: [Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
On Tue, Feb 01, 2011 at 06:10:56PM +0100, Alexander Graf wrote: > > On 01.02.2011, at 18:06, Aurelien Jarno wrote: > > > On Tue, Feb 01, 2011 at 05:53:43PM +0100, Alexander Graf wrote: > >> > >> On 01.02.2011, at 17:34, Aurelien Jarno wrote: > >> > >>> On Tue, Feb 01, 2011 at 03:51:32PM +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. > >>> > >>> Given this issue mostly concerns x86 and not other architectures where > >>> the SATA emulation can probably be used, what about putting the two > >>> versions of the codes like in i8259.c: > >>> > >>> | * all targets should do this rather than acking the IRQ in the cpu */ > >>> | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA) > >>> > >>> The list of architectures here is reduced given the few architectures > >>> that actually use the i8259, so for ahci.c it should probably be #if not > >>> defined(TARGET_I386) instead. > >> > >> Because then we'd have to build the ahci code conditionally on the > >> architecture. Right now it builds into libhw :) > > > > If we want to keep it in libhw, it's probably better to disable it on > > other architectures than i386. > > How so? The workaround simply triggers a superfluous interrupt event, but > shouldn't break anything for other architectures. In fact, last time I > checked it worked just fine on ppc. > Superfluous interrupt events can have a lot of consequences, it's often a kernel panic (see recent zilog serial port issues for example), and when it comes to disk controllers, it might mean data loss. Anyway the way to go there is cleary to fix the x86 interrupt model, it's the only one broken among all targets. If we can't find anyone to do that, we can also declare x86 unsupported :) -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
On 01.02.2011, at 17:34, Aurelien Jarno wrote: > On Tue, Feb 01, 2011 at 03:51:32PM +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. > > Given this issue mostly concerns x86 and not other architectures where > the SATA emulation can probably be used, what about putting the two > versions of the codes like in i8259.c: > > | * all targets should do this rather than acking the IRQ in the cpu */ > | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA) > > The list of architectures here is reduced given the few architectures > that actually use the i8259, so for ahci.c it should probably be #if not > defined(TARGET_I386) instead. Because then we'd have to build the ahci code conditionally on the architecture. Right now it builds into libhw :) Alex
Re: [Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
On 01.02.2011, at 18:06, Aurelien Jarno wrote: > On Tue, Feb 01, 2011 at 05:53:43PM +0100, Alexander Graf wrote: >> >> On 01.02.2011, at 17:34, Aurelien Jarno wrote: >> >>> On Tue, Feb 01, 2011 at 03:51:32PM +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. >>> >>> Given this issue mostly concerns x86 and not other architectures where >>> the SATA emulation can probably be used, what about putting the two >>> versions of the codes like in i8259.c: >>> >>> | * all targets should do this rather than acking the IRQ in the cpu */ >>> | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA) >>> >>> The list of architectures here is reduced given the few architectures >>> that actually use the i8259, so for ahci.c it should probably be #if not >>> defined(TARGET_I386) instead. >> >> Because then we'd have to build the ahci code conditionally on the >> architecture. Right now it builds into libhw :) > > If we want to keep it in libhw, it's probably better to disable it on > other architectures than i386. How so? The workaround simply triggers a superfluous interrupt event, but shouldn't break anything for other architectures. In fact, last time I checked it worked just fine on ppc. Alex
Re: [Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
On Tue, Feb 01, 2011 at 05:53:43PM +0100, Alexander Graf wrote: > > On 01.02.2011, at 17:34, Aurelien Jarno wrote: > > > On Tue, Feb 01, 2011 at 03:51:32PM +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. > > > > Given this issue mostly concerns x86 and not other architectures where > > the SATA emulation can probably be used, what about putting the two > > versions of the codes like in i8259.c: > > > > | * all targets should do this rather than acking the IRQ in the cpu */ > > | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA) > > > > The list of architectures here is reduced given the few architectures > > that actually use the i8259, so for ahci.c it should probably be #if not > > defined(TARGET_I386) instead. > > Because then we'd have to build the ahci code conditionally on the > architecture. Right now it builds into libhw :) If we want to keep it in libhw, it's probably better to disable it on other architectures than i386. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
On Tue, Feb 01, 2011 at 03:51:32PM +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. Given this issue mostly concerns x86 and not other architectures where the SATA emulation can probably be used, what about putting the two versions of the codes like in i8259.c: | * all targets should do this rather than acking the IRQ in the cpu */ | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA) The list of architectures here is reduced given the few architectures that actually use the i8259, so for ahci.c it should probably be #if not defined(TARGET_I386) instead. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
[Qemu-devel] [PATCH 7/7] ahci: work around bug with level interrupts
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
---
hw/ide/ahci.c |3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 98bdf70..bce7fba 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -152,11 +152,10 @@ static void ahci_check_irq(AHCIState *s)
}
}
+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);
}
}
--
1.6.0.2
