Patchwork sparc32 esp fix spurious interrupts in chip reset

login
register
mail settings
Submitter Artyom Tarasenko
Date May 30, 2010, 10:35 p.m.
Message ID <1275258946-15739-1-git-send-email-atar4qemu@gmail.com>
Download mbox | patch
Permalink /patch/54007/
State New
Headers show

Comments

Artyom Tarasenko - May 30, 2010, 10:35 p.m.
lower interrupt during chip reset. Otherwise the ESP_RSTAT register
may get out of sync with the IRQ line status. This effect became
visible after commit 65899fe3

Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
---
 hw/esp.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Blue Swirl - June 1, 2010, 5:42 p.m.
On Sun, May 30, 2010 at 10:35 PM, Artyom Tarasenko
<atar4qemu@googlemail.com> wrote:
> lower interrupt during chip reset. Otherwise the ESP_RSTAT register
> may get out of sync with the IRQ line status. This effect became
> visible after commit 65899fe3

Hard reset handlers should not touch qemu_irqs, because on cold start,
the receiving end may be unprepared to handle the signal. See
0d0a7e69e853639b123798877e019c3c7ee6634a,
bc26e55a6615dc594be425d293db40d5cdcdb84b and
42f1ced228c9b616cfa2b69846025271618e4ef5.

For ESP there are two other sources of reset: signal from DMA and chip
reset command. On those cases, lowering IRQ makes sense.

So the correct fix is to refactor the reset handling a bit. Does this
patch also fix your test case?
Artyom Tarasenko - June 1, 2010, 7:56 p.m.
2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
> On Sun, May 30, 2010 at 10:35 PM, Artyom Tarasenko
> <atar4qemu@googlemail.com> wrote:
>> lower interrupt during chip reset. Otherwise the ESP_RSTAT register
>> may get out of sync with the IRQ line status. This effect became
>> visible after commit 65899fe3
>
> Hard reset handlers should not touch qemu_irqs, because on cold start,
> the receiving end may be unprepared to handle the signal.

Wouldn't the real hardware lower irq on the hardware reset?
And if it would not, would it still clear the corresponding bit in
the ESP_RSTAT register?

> See
> 0d0a7e69e853639b123798877e019c3c7ee6634a,
> bc26e55a6615dc594be425d293db40d5cdcdb84b and
> 42f1ced228c9b616cfa2b69846025271618e4ef5.
>
> For ESP there are two other sources of reset: signal from DMA and chip
> reset command. On those cases, lowering IRQ makes sense.
>
> So the correct fix is to refactor the reset handling a bit. Does this
> patch also fix your test case?

It does, but

+static void esp_soft_reset(DeviceState *d)
+{
+    ESPState *s = container_of(d, ESPState, busdev.qdev);
+
+    qemu_irq_lower(s->irq);

Shouldn't it be esp_lower_irq(s)? What's going to happen to the
DMA_INTR bit if dma was the source of the irq?

+    esp_hard_reset(d);
+}
+
Blue Swirl - June 1, 2010, 8:09 p.m.
On Tue, Jun 1, 2010 at 7:56 PM, Artyom Tarasenko
<atar4qemu@googlemail.com> wrote:
> 2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
>> On Sun, May 30, 2010 at 10:35 PM, Artyom Tarasenko
>> <atar4qemu@googlemail.com> wrote:
>>> lower interrupt during chip reset. Otherwise the ESP_RSTAT register
>>> may get out of sync with the IRQ line status. This effect became
>>> visible after commit 65899fe3
>>
>> Hard reset handlers should not touch qemu_irqs, because on cold start,
>> the receiving end may be unprepared to handle the signal.
>
> Wouldn't the real hardware lower irq on the hardware reset?

Yes, but since qemu_irqs have no state, and on a cold start or system
reset all other devices are guaranteed to be reset, the callback would
be useless.

> And if it would not, would it still clear the corresponding bit in
> the ESP_RSTAT register?

All registers are set to zero in the lines below.

>
>> See
>> 0d0a7e69e853639b123798877e019c3c7ee6634a,
>> bc26e55a6615dc594be425d293db40d5cdcdb84b and
>> 42f1ced228c9b616cfa2b69846025271618e4ef5.
>>
>> For ESP there are two other sources of reset: signal from DMA and chip
>> reset command. On those cases, lowering IRQ makes sense.
>>
>> So the correct fix is to refactor the reset handling a bit. Does this
>> patch also fix your test case?
>
> It does, but
>
> +static void esp_soft_reset(DeviceState *d)
> +{
> +    ESPState *s = container_of(d, ESPState, busdev.qdev);
> +
> +    qemu_irq_lower(s->irq);
>
> Shouldn't it be esp_lower_irq(s)? What's going to happen to the
> DMA_INTR bit if dma was the source of the irq?

Again, the registers are zeroed in esp_hard_reset().

Thanks for testing.
Artyom Tarasenko - June 1, 2010, 8:16 p.m.
2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
> On Tue, Jun 1, 2010 at 7:56 PM, Artyom Tarasenko
> <atar4qemu@googlemail.com> wrote:
>> 2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
>>> On Sun, May 30, 2010 at 10:35 PM, Artyom Tarasenko
>>> <atar4qemu@googlemail.com> wrote:
>>>> lower interrupt during chip reset. Otherwise the ESP_RSTAT register
>>>> may get out of sync with the IRQ line status. This effect became
>>>> visible after commit 65899fe3
>>>
>>> Hard reset handlers should not touch qemu_irqs, because on cold start,
>>> the receiving end may be unprepared to handle the signal.
>>
>> Wouldn't the real hardware lower irq on the hardware reset?
>
> Yes, but since qemu_irqs have no state, and on a cold start or system
> reset all other devices are guaranteed to be reset, the callback would
> be useless.
>
>> And if it would not, would it still clear the corresponding bit in
>> the ESP_RSTAT register?
>
> All registers are set to zero in the lines below.
>
>>
>>> See
>>> 0d0a7e69e853639b123798877e019c3c7ee6634a,
>>> bc26e55a6615dc594be425d293db40d5cdcdb84b and
>>> 42f1ced228c9b616cfa2b69846025271618e4ef5.
>>>
>>> For ESP there are two other sources of reset: signal from DMA and chip
>>> reset command. On those cases, lowering IRQ makes sense.
>>>
>>> So the correct fix is to refactor the reset handling a bit. Does this
>>> patch also fix your test case?
>>
>> It does, but
>>
>> +static void esp_soft_reset(DeviceState *d)
>> +{
>> +    ESPState *s = container_of(d, ESPState, busdev.qdev);
>> +
>> +    qemu_irq_lower(s->irq);
>>
>> Shouldn't it be esp_lower_irq(s)? What's going to happen to the
>> DMA_INTR bit if dma was the source of the irq?
>
> Again, the registers are zeroed in esp_hard_reset().

How does it zero the _DMA_ registers? And sparc32_dma does share the
IRQ line with ESP, doesn't it?

>
> Thanks for testing.
>
Blue Swirl - June 4, 2010, 7:13 p.m.
On Tue, Jun 1, 2010 at 8:16 PM, Artyom Tarasenko
<atar4qemu@googlemail.com> wrote:
> 2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
>> On Tue, Jun 1, 2010 at 7:56 PM, Artyom Tarasenko
>> <atar4qemu@googlemail.com> wrote:
>>> 2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
>>>> On Sun, May 30, 2010 at 10:35 PM, Artyom Tarasenko
>>>> <atar4qemu@googlemail.com> wrote:
>>>>> lower interrupt during chip reset. Otherwise the ESP_RSTAT register
>>>>> may get out of sync with the IRQ line status. This effect became
>>>>> visible after commit 65899fe3
>>>>
>>>> Hard reset handlers should not touch qemu_irqs, because on cold start,
>>>> the receiving end may be unprepared to handle the signal.
>>>
>>> Wouldn't the real hardware lower irq on the hardware reset?
>>
>> Yes, but since qemu_irqs have no state, and on a cold start or system
>> reset all other devices are guaranteed to be reset, the callback would
>> be useless.
>>
>>> And if it would not, would it still clear the corresponding bit in
>>> the ESP_RSTAT register?
>>
>> All registers are set to zero in the lines below.
>>
>>>
>>>> See
>>>> 0d0a7e69e853639b123798877e019c3c7ee6634a,
>>>> bc26e55a6615dc594be425d293db40d5cdcdb84b and
>>>> 42f1ced228c9b616cfa2b69846025271618e4ef5.
>>>>
>>>> For ESP there are two other sources of reset: signal from DMA and chip
>>>> reset command. On those cases, lowering IRQ makes sense.
>>>>
>>>> So the correct fix is to refactor the reset handling a bit. Does this
>>>> patch also fix your test case?
>>>
>>> It does, but
>>>
>>> +static void esp_soft_reset(DeviceState *d)
>>> +{
>>> +    ESPState *s = container_of(d, ESPState, busdev.qdev);
>>> +
>>> +    qemu_irq_lower(s->irq);
>>>
>>> Shouldn't it be esp_lower_irq(s)? What's going to happen to the
>>> DMA_INTR bit if dma was the source of the irq?
>>
>> Again, the registers are zeroed in esp_hard_reset().
>
> How does it zero the _DMA_ registers? And sparc32_dma does share the
> IRQ line with ESP, doesn't it?

I'd suppose DMA registers are separate and they would not be cleared
by for example ESP chip reset command. The IRQ goes from ESP to DMA,
DMA has another line going to interrupt controller.
Artyom Tarasenko - June 4, 2010, 8:30 p.m.
2010/6/4 Blue Swirl <blauwirbel@gmail.com>:
> On Tue, Jun 1, 2010 at 8:16 PM, Artyom Tarasenko
> <atar4qemu@googlemail.com> wrote:
>> 2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
>>> On Tue, Jun 1, 2010 at 7:56 PM, Artyom Tarasenko
>>> <atar4qemu@googlemail.com> wrote:
>>>> 2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
>>>>> On Sun, May 30, 2010 at 10:35 PM, Artyom Tarasenko
>>>>> <atar4qemu@googlemail.com> wrote:
>>>>>> lower interrupt during chip reset. Otherwise the ESP_RSTAT register
>>>>>> may get out of sync with the IRQ line status. This effect became
>>>>>> visible after commit 65899fe3
>>>>>
>>>>> Hard reset handlers should not touch qemu_irqs, because on cold start,
>>>>> the receiving end may be unprepared to handle the signal.
>>>>
>>>> Wouldn't the real hardware lower irq on the hardware reset?
>>>
>>> Yes, but since qemu_irqs have no state, and on a cold start or system
>>> reset all other devices are guaranteed to be reset, the callback would
>>> be useless.
>>>
>>>> And if it would not, would it still clear the corresponding bit in
>>>> the ESP_RSTAT register?
>>>
>>> All registers are set to zero in the lines below.
>>>
>>>>
>>>>> See
>>>>> 0d0a7e69e853639b123798877e019c3c7ee6634a,
>>>>> bc26e55a6615dc594be425d293db40d5cdcdb84b and
>>>>> 42f1ced228c9b616cfa2b69846025271618e4ef5.
>>>>>
>>>>> For ESP there are two other sources of reset: signal from DMA and chip
>>>>> reset command. On those cases, lowering IRQ makes sense.
>>>>>
>>>>> So the correct fix is to refactor the reset handling a bit. Does this
>>>>> patch also fix your test case?
>>>>
>>>> It does, but
>>>>
>>>> +static void esp_soft_reset(DeviceState *d)
>>>> +{
>>>> +    ESPState *s = container_of(d, ESPState, busdev.qdev);
>>>> +
>>>> +    qemu_irq_lower(s->irq);
>>>>
>>>> Shouldn't it be esp_lower_irq(s)? What's going to happen to the
>>>> DMA_INTR bit if dma was the source of the irq?
>>>
>>> Again, the registers are zeroed in esp_hard_reset().
>>
>> How does it zero the _DMA_ registers? And sparc32_dma does share the
>> IRQ line with ESP, doesn't it?
>
> I'd suppose DMA registers are separate and they would not be cleared
> by for example ESP chip reset command. The IRQ goes from ESP to DMA,
> DMA has another line going to interrupt controller.

But do we have separate DMA lines in qemu? If we do, I'm absolutely fine with
qemu_irq_lower(s->irq) . If we don't, imagine the following scenario: DMA
rises an IRQ, then esp chip reset happens, and then... DMA can't rise
the IRQ anymore.
Blue Swirl - June 9, 2010, 8:35 p.m.
On Fri, Jun 4, 2010 at 8:30 PM, Artyom Tarasenko
<atar4qemu@googlemail.com> wrote:
> 2010/6/4 Blue Swirl <blauwirbel@gmail.com>:
>> On Tue, Jun 1, 2010 at 8:16 PM, Artyom Tarasenko
>> <atar4qemu@googlemail.com> wrote:
>>> 2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
>>>> On Tue, Jun 1, 2010 at 7:56 PM, Artyom Tarasenko
>>>> <atar4qemu@googlemail.com> wrote:
>>>>> 2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
>>>>>> On Sun, May 30, 2010 at 10:35 PM, Artyom Tarasenko
>>>>>> <atar4qemu@googlemail.com> wrote:
>>>>>>> lower interrupt during chip reset. Otherwise the ESP_RSTAT register
>>>>>>> may get out of sync with the IRQ line status. This effect became
>>>>>>> visible after commit 65899fe3
>>>>>>
>>>>>> Hard reset handlers should not touch qemu_irqs, because on cold start,
>>>>>> the receiving end may be unprepared to handle the signal.
>>>>>
>>>>> Wouldn't the real hardware lower irq on the hardware reset?
>>>>
>>>> Yes, but since qemu_irqs have no state, and on a cold start or system
>>>> reset all other devices are guaranteed to be reset, the callback would
>>>> be useless.
>>>>
>>>>> And if it would not, would it still clear the corresponding bit in
>>>>> the ESP_RSTAT register?
>>>>
>>>> All registers are set to zero in the lines below.
>>>>
>>>>>
>>>>>> See
>>>>>> 0d0a7e69e853639b123798877e019c3c7ee6634a,
>>>>>> bc26e55a6615dc594be425d293db40d5cdcdb84b and
>>>>>> 42f1ced228c9b616cfa2b69846025271618e4ef5.
>>>>>>
>>>>>> For ESP there are two other sources of reset: signal from DMA and chip
>>>>>> reset command. On those cases, lowering IRQ makes sense.
>>>>>>
>>>>>> So the correct fix is to refactor the reset handling a bit. Does this
>>>>>> patch also fix your test case?
>>>>>
>>>>> It does, but
>>>>>
>>>>> +static void esp_soft_reset(DeviceState *d)
>>>>> +{
>>>>> +    ESPState *s = container_of(d, ESPState, busdev.qdev);
>>>>> +
>>>>> +    qemu_irq_lower(s->irq);
>>>>>
>>>>> Shouldn't it be esp_lower_irq(s)? What's going to happen to the
>>>>> DMA_INTR bit if dma was the source of the irq?
>>>>
>>>> Again, the registers are zeroed in esp_hard_reset().
>>>
>>> How does it zero the _DMA_ registers? And sparc32_dma does share the
>>> IRQ line with ESP, doesn't it?
>>
>> I'd suppose DMA registers are separate and they would not be cleared
>> by for example ESP chip reset command. The IRQ goes from ESP to DMA,
>> DMA has another line going to interrupt controller.
>
> But do we have separate DMA lines in qemu? If we do, I'm absolutely fine with
> qemu_irq_lower(s->irq) . If we don't, imagine the following scenario: DMA
> rises an IRQ, then esp chip reset happens, and then... DMA can't rise
> the IRQ anymore.

What ESP does with its IRQ line does not stop DMA from using its line.
Artyom Tarasenko - June 10, 2010, 8:15 a.m.
2010/6/9 Blue Swirl <blauwirbel@gmail.com>:
> On Fri, Jun 4, 2010 at 8:30 PM, Artyom Tarasenko
> <atar4qemu@googlemail.com> wrote:
>> 2010/6/4 Blue Swirl <blauwirbel@gmail.com>:
>>> On Tue, Jun 1, 2010 at 8:16 PM, Artyom Tarasenko
>>> <atar4qemu@googlemail.com> wrote:
>>>> 2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
>>>>> On Tue, Jun 1, 2010 at 7:56 PM, Artyom Tarasenko
>>>>> <atar4qemu@googlemail.com> wrote:
>>>>>> 2010/6/1 Blue Swirl <blauwirbel@gmail.com>:
>>>>>>> On Sun, May 30, 2010 at 10:35 PM, Artyom Tarasenko
>>>>>>> <atar4qemu@googlemail.com> wrote:
>>>>>>>> lower interrupt during chip reset. Otherwise the ESP_RSTAT register
>>>>>>>> may get out of sync with the IRQ line status. This effect became
>>>>>>>> visible after commit 65899fe3
>>>>>>>
>>>>>>> Hard reset handlers should not touch qemu_irqs, because on cold start,
>>>>>>> the receiving end may be unprepared to handle the signal.
>>>>>>
>>>>>> Wouldn't the real hardware lower irq on the hardware reset?
>>>>>
>>>>> Yes, but since qemu_irqs have no state, and on a cold start or system
>>>>> reset all other devices are guaranteed to be reset, the callback would
>>>>> be useless.
>>>>>
>>>>>> And if it would not, would it still clear the corresponding bit in
>>>>>> the ESP_RSTAT register?
>>>>>
>>>>> All registers are set to zero in the lines below.
>>>>>
>>>>>>
>>>>>>> See
>>>>>>> 0d0a7e69e853639b123798877e019c3c7ee6634a,
>>>>>>> bc26e55a6615dc594be425d293db40d5cdcdb84b and
>>>>>>> 42f1ced228c9b616cfa2b69846025271618e4ef5.
>>>>>>>
>>>>>>> For ESP there are two other sources of reset: signal from DMA and chip
>>>>>>> reset command. On those cases, lowering IRQ makes sense.
>>>>>>>
>>>>>>> So the correct fix is to refactor the reset handling a bit. Does this
>>>>>>> patch also fix your test case?
>>>>>>
>>>>>> It does, but
>>>>>>
>>>>>> +static void esp_soft_reset(DeviceState *d)
>>>>>> +{
>>>>>> +    ESPState *s = container_of(d, ESPState, busdev.qdev);
>>>>>> +
>>>>>> +    qemu_irq_lower(s->irq);
>>>>>>
>>>>>> Shouldn't it be esp_lower_irq(s)? What's going to happen to the
>>>>>> DMA_INTR bit if dma was the source of the irq?
>>>>>
>>>>> Again, the registers are zeroed in esp_hard_reset().
>>>>
>>>> How does it zero the _DMA_ registers? And sparc32_dma does share the
>>>> IRQ line with ESP, doesn't it?
>>>
>>> I'd suppose DMA registers are separate and they would not be cleared
>>> by for example ESP chip reset command. The IRQ goes from ESP to DMA,
>>> DMA has another line going to interrupt controller.
>>
>> But do we have separate DMA lines in qemu? If we do, I'm absolutely fine with
>> qemu_irq_lower(s->irq) . If we don't, imagine the following scenario: DMA
>> rises an IRQ, then esp chip reset happens, and then... DMA can't rise
>> the IRQ anymore.
>
> What ESP does with its IRQ line does not stop DMA from using its line.

Then I'm fine with your patch.

Patch

diff --git a/hw/esp.c b/hw/esp.c
index 0a8cf6e..0532c67 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -423,6 +423,7 @@  static void esp_reset(DeviceState *d)
 {
     ESPState *s = container_of(d, ESPState, busdev.qdev);
 
+    esp_lower_irq(s);
     memset(s->rregs, 0, ESP_REGS);
     memset(s->wregs, 0, ESP_REGS);
     s->rregs[ESP_TCHI] = TCHI_FAS100A; // Indicate fas100a