Re: [PATCH v3 3/3] piix_pci: optimize set irq path

On Mon, Mar 21, 2011 at 02:31:11PM +0200, Michael S. Tsirkin wrote:
> On Mon, Mar 21, 2011 at 09:10:32PM +0900, Isaku Yamahata wrote:
> > On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote:
> > > > +static int piix3_post_load(void *opaque, int version_id)
> > > > +{
> > > > +    PIIX3State *piix3 = opaque;
> > > > +    piix3_update_irq_levels(piix3);
> > > 
> > > Couldn't figure out why would we not want to
> > > propagate the interrupts here.
> > > Could you explain please?
> > > What happens if we do propagate them?
> > > Nothing bad, right?
> > 
> > I wanted to be just conservative.
> > If you are brave enough to change the behavior, I'm fine with propagating
> > interrupts.
> > 
> > If we propagate the interrupts, guest OS may see interrupts
> > unnecessarily/spuriously injected after load.
> > Probably such interrupts doesn't harm OSes, so there is nothing
> > bad in theory as you said.
> > On the other hand, I hesitated to change the existing behavior because
> > it would be very difficult to debug it and to test many OSes.
> 
> I expect it won't change the behaviour because the interrupts
> are level: at the moment e.g. pci devices already reassert
> interrupts on load.
> 
> But I agree it better be a separate patch, and needs a lot of testing.

Like this?

On Mon, Mar 21, 2011 at 09:56:56PM +0900, Isaku Yamahata wrote:
> On Mon, Mar 21, 2011 at 02:31:11PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Mar 21, 2011 at 09:10:32PM +0900, Isaku Yamahata wrote:
> > > On Mon, Mar 21, 2011 at 01:37:07PM +0200, Michael S. Tsirkin wrote:
> > > > > +static int piix3_post_load(void *opaque, int version_id)
> > > > > +{
> > > > > +    PIIX3State *piix3 = opaque;
> > > > > +    piix3_update_irq_levels(piix3);
> > > > 
> > > > Couldn't figure out why would we not want to
> > > > propagate the interrupts here.
> > > > Could you explain please?
> > > > What happens if we do propagate them?
> > > > Nothing bad, right?
> > > 
> > > I wanted to be just conservative.
> > > If you are brave enough to change the behavior, I'm fine with propagating
> > > interrupts.
> > > 
> > > If we propagate the interrupts, guest OS may see interrupts
> > > unnecessarily/spuriously injected after load.
> > > Probably such interrupts doesn't harm OSes, so there is nothing
> > > bad in theory as you said.
> > > On the other hand, I hesitated to change the existing behavior because
> > > it would be very difficult to debug it and to test many OSes.
> > 
> > I expect it won't change the behaviour because the interrupts
> > are level: at the moment e.g. pci devices already reassert
> > interrupts on load.
> > 
> > But I agree it better be a separate patch, and needs a lot of testing.
> 
> Like this?

Yes. But need to stress-test with migration, windows and linux guests
at least.

> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index f07e19d..8052c1e 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -280,8 +280,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
>                      ((PIIX_NUM_PIRQS - 1) << (pic_irq * PIIX_NUM_PIRQS))));
>  }
>  
> -static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
> -                                bool propagate)
> +static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level)
>  {
>      int pic_irq;
>      uint64_t mask;
> @@ -295,15 +294,13 @@ static void piix3_set_irq_level(PIIX3State *piix3, int irq_num, int level,
>      piix3->pic_levels &= ~mask;
>      piix3->pic_levels |= mask * !!level;
>  
> -    if (propagate) {
> -        piix3_set_irq_pic(piix3, pic_irq);
> -    }
> +    piix3_set_irq_pic(piix3, pic_irq);
>  }
>  
>  static void piix3_set_irq(void *opaque, int irq_num, int level)
>  {
>      PIIX3State *piix3 = opaque;
> -    piix3_set_irq_level(piix3, irq_num, level, true);
> +    piix3_set_irq_level(piix3, irq_num, level);
>  }
>  
>  /* irq routing is changed. so rebuild bitmap */
> @@ -314,8 +311,7 @@ static void piix3_update_irq_levels(PIIX3State *piix3)
>      piix3->pic_levels = 0;
>      for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
>          piix3_set_irq_level(piix3, pirq,
> -                            pci_bus_get_irq_level(piix3->dev.bus, pirq),
> -                            false);
> +                            pci_bus_get_irq_level(piix3->dev.bus, pirq));
>      }
>  }
>  
> 
> 
> -- 
> yamahata