[Qemu-devel] Re: [PATCH] sparc32 esp fix spurious interrupts in chip reset

[Blue Swirl]

On Fri, Jun 4, 2010 at 8:30 PM, Artyom Tarasenko
<address@hidden> wrote:
> 2010/6/4 Blue Swirl <address@hidden>:
>> On Tue, Jun 1, 2010 at 8:16 PM, Artyom Tarasenko
>> <address@hidden> wrote:
>>> 2010/6/1 Blue Swirl <address@hidden>:
>>>> On Tue, Jun 1, 2010 at 7:56 PM, Artyom Tarasenko
>>>> <address@hidden> wrote:
>>>>> 2010/6/1 Blue Swirl <address@hidden>:
>>>>>> On Sun, May 30, 2010 at 10:35 PM, Artyom Tarasenko
>>>>>> <address@hidden> 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.