| 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
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?
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); +} +
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.
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. >
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.
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.
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.
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
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(-)