Re: [Qemu-devel] [PATCH 11/22] i8259: Update IRQ state after reset

[Blue Swirl]

On Fri, Sep 30, 2011 at 6:47 AM, Jan Kiszka <address@hidden> wrote:
> On 2011-09-29 21:45, Blue Swirl wrote:
>> On Wed, Sep 28, 2011 at 9:18 PM, Jan Kiszka <address@hidden> wrote:
>>> On 2011-09-28 20:01, Blue Swirl wrote:
>>>>
>>>> On Wed, Sep 28, 2011 at 11:00 AM, Jan Kiszka<address@hidden>
>>>>  wrote:
>>>>>
>>>>> As we clearly modify the PIC state on pic_reset, we also have to update
>>>>> the IRQ output. This only happened on init so far. Apply this
>>>>> consistently.
>>>>
>>>> Nack, IRQ lines shouldn't be touched on reset. The other side may not
>>>> be ready for receiving the interrupt change and qemu_irqs are
>>>> stateless anyway.
>>>
>>> Sorry, but failing to clear the line (this is what pic_update_irq will
>>> effectively do) is a clear bug in the current code. This patch is 100%
>>> analogue to what, e.g. the PCI layer does on reset. Please re-read.
>>
>> Reset will happen also when the devices are created. At that time,
>> qemu_irq callback triggered by changing of the state may produce
>> undesired effects on the other side.
>
> All those potential effects will be cleared again when the receiver is
> reset as well. If not, that would be a bug which requires fixing.

No, the order of resetting devices is not known so the IRQ may or may
not have the effect. If the other device still has pre-reset
configuration, it may spread incorrect effects to devices which
already were reset.

If reliable post reset effects are needed, reset should be divided
into two separate phases. So far no interesting cases have been
demonstrated.

>> There have been bugs earlier, see
>> bc26e55a6615dc594be425d293db40d5cdcdb84b and
>> 42f1ced228c9b616cfa2b69846025271618e4ef5 and discussion in
>> http://lists.nongnu.org/archive/html/qemu-devel/2009-06/msg01024.html.
>
> Deasserting IRQs at PCI device level is indeed useless as we already do
> this in the PCI core.
>
> There is no difference between system reset after power-up or later on.
> And there should be no difference between per device, per group of
> devices or system-wide reset. A device model cannot tell these apart.
> That the devices reset handler is only called on system reset is an odd
> and fragile assumption - not that fragile for platform devices like the
> i8259, but definitely bogus for pluggable ones.

That part of the discussion is obsolete (or at least uninteresting
here). For example this message has a relevant example:
http://lists.nongnu.org/archive/html/qemu-devel/2009-06/msg01081.html

It's about VM restore, but the situation is similar during reset.

> My series does not depend on this cleanup/fix, but the reason not to
> apply remains wrong IMHO.

Sorry, but if a patch is incorrect, it is not a cleanup nor a fix.