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

[Michael S. Tsirkin]

On Tue, Sep 09, 2014 at 02:30:08PM +0200, Paolo Bonzini wrote:
> From: Pavel Dovgalyuk <address@hidden>
> 
> This patch disables raising an irq while loading the state of PCI bridge.
> The aim of this patch is preserving the same behavior while saving and
> restoring the VM state. IRQ is not raised while saving the state of the 
> bridge.
> That's why the behavior of the restored system will differ from
> the original one.


Hmm I don't understand.
You are removing call to piix3_update_irq_levels
this call is supposed to just sync up bus to
irq level.

How can this change system state? Saved state
is supposed to already be in sync.


> This patch eliminates raising an irq and just restores
> the calculated state fields in post_load function.
> 
> Signed-off-by: Pavel Dovgalyuk <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/pci-host/piix.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index e0e0946..cd50435 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -409,7 +409,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int 
> pic_irq)
>                       (pic_irq * PIIX_NUM_PIRQS))));
>  }
>  
> -static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
> +static void piix3_set_irq_level_internal(PIIX3State *piix3, int pirq, int 
> level)
>  {
>      int pic_irq;
>      uint64_t mask;
> @@ -422,6 +422,18 @@ static void piix3_set_irq_level(PIIX3State *piix3, int 
> pirq, int level)
>      mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + pirq);
>      piix3->pic_levels &= ~mask;
>      piix3->pic_levels |= mask * !!level;
> +}
> +
> +static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
> +{
> +    int pic_irq;
> +
> +    pic_irq = piix3->dev.config[PIIX_PIRQC + pirq];
> +    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
> +        return;
> +    }
> +
> +    piix3_set_irq_level_internal(piix3, pirq, level);
>  
>      piix3_set_irq_pic(piix3, pic_irq);
>  }
> @@ -527,7 +539,18 @@ static void piix3_reset(void *opaque)
>  static int piix3_post_load(void *opaque, int version_id)
>  {
>      PIIX3State *piix3 = opaque;
> -    piix3_update_irq_levels(piix3);
> +    int pirq;
> +
> +    /* Update irq levels without raising an interrupt which could
> +     * happen in piix3_update_irq_levels.  Raising an IRQ will
> +     * bring the system to a different state than the saved one.

I don't like comments like this: don't discuss what could
have been if code was different. Explain why code
is like it is.

> +     * Interrupt state is serialized separately through the i8259.
> +     */
> +    piix3->pic_levels = 0;
> +    for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
> +        piix3_set_irq_level_internal(piix3, pirq,
> +                            pci_bus_get_irq_level(piix3->dev.bus, pirq));
> +    }
>      return 0;
>  }
>  
> -- 
> 2.1.0
> 
>