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

[Artyom Tarasenko]

2010/6/9 Blue Swirl <address@hidden>:
> 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.

Then I'm fine with your patch.


-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/