Re: [Qemu-devel] [PATCH 09/10] piix: do not raise irq while loading vmstate

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Sep 10, 2014 at 10:38:46AM +0200, Paolo Bonzini wrote:
> Il 09/09/2014 22:51, Michael S. Tsirkin ha scritto:
> > > i440FX/PIIX3 state is loaded before i8259, so the interrupt will never
> > > be in the i8259 ISR.  I am not sure why it is a problem for
> > > record/replay, but I think it's plausible to consider this a bug.  i8259
> > > state should not be affected by the load of PIIX3 state, since i8259 is
> > > migrated separately.
> > 
> > Sorry I still don't understand. Why do stuff from vmstate callback then?
> > How is it different?
> 
> Reconstructing internal state from post_load is okay.
> 
> What is not okay (and I think it should be a rule) is to touch other
> devices from post_load, unless you know that they are deserialized
> first.  For example it's okay for a PCI device to talk to the parent
> bridge in its post_load function.
> 
> In the case of PIIX3 vs. i8259, however, you know that i8259 is
> deserialized _last_ because i8259 is an ISA device and PIIX3 provides
> the ISA bus.  So it's incorrect, even though it's currently harmless, to
> touch the i8259 before it's deserialized.

OK, got this, thanks for the explanation!
So the reason i8259 might be out of sync is
because it's not yet deserialized.

I think it's a good idea to put (at least the
last part) in the commit log.
Also it's updating irq state, not just raising irq,
that might be problematic, right?

So also, something like this for the comment:
+    /* We update irq levels in PIIX3 but don't set IRQ, since
+     *	IRQ state is serialized separately through the i8259,
+     * which is not deserialized yet, at this point.
+     */





> > I'd like to see a description of a scenario where this patch makes
> > a difference.
> 
> Of course it would be nice to have testcases for this, but I guess one
> case could be:
> 
> - LAPIC configured in ExtINT mode
> 
> - interrupts are masked in the i8259, but the i8259 doesn't know that
> yet because it's not been loaded yet
> 
> - the PIIX3 loads the state and the interrupt is set.  pic_set_irq is
> called, calls pic_update_irq
> 
> - pic_update_irq calls pic_get_irq, which uses IMR=0 and thus raises LINT0
> 
> - the APIC has been loaded already, so LINT0 is injected incorrectly
> 
> 
> Another case could be:
> 
> - i8259 is processing IRQ0.  The lower-priority interrupt from PIIX3 is
> in IRR.  Machine is migrated.
> 
> - the PIIX3 loads the state and sets the interrupt in the i8259.
> pic_set_irq is called, calls pic_update_irq, calls pic_get_irq
> 
> - because i8259 has not been loaded yet, pic_get_irq sees ISR=0 and the
> interrupt is injected even though IRQ0 (higher priority) is being serviced.
> 
> 
> In both cases, the saved i8259 state will have the PIIX3 interrupt in
> IRR, so the interrupt is not lost, just held (as it would have been on
> the source machine).
> 
> Paolo