Re: [PATCH 04/26] pci: add accessor function to get irq levels

On Thu, Mar 17, 2011 at 07:29:09AM +0200, Michael S. Tsirkin wrote:
> On Wed, Mar 16, 2011 at 06:29:15PM +0900, Isaku Yamahata wrote:
> > Introduce accessor function to know INTx levels.
> > It will be used later by q35.
> > Although piix_pci tracks the intx line levels, it can be eliminated
> > by this helper function.
> 
> At least for piix, the right thing to IMO is to have bit per
> IRQ, then the for loop can be replaced with a single !!.  There's a TODO
> there which this will fix.  I think we can reuse pci device irq_state
> for this: need to check. Haven't looked at q35 yet - applies there as
> well?

Yes, such bitmap optimization is possible.
But this accessor function is still necessary,
please see the following. (I didn't do any test yet. Just to show the idea)
If you like it, I'll post it as separate patch.

On Thu, Mar 17, 2011 at 03:05:00PM +0900, Isaku Yamahata wrote:
> On Thu, Mar 17, 2011 at 07:29:09AM +0200, Michael S. Tsirkin wrote:
> > On Wed, Mar 16, 2011 at 06:29:15PM +0900, Isaku Yamahata wrote:
> > > Introduce accessor function to know INTx levels.
> > > It will be used later by q35.
> > > Although piix_pci tracks the intx line levels, it can be eliminated
> > > by this helper function.
> > 
> > At least for piix, the right thing to IMO is to have bit per
> > IRQ, then the for loop can be replaced with a single !!.  There's a TODO
> > there which this will fix.  I think we can reuse pci device irq_state
> > for this: need to check. Haven't looked at q35 yet - applies there as
> > well?
> 
> Yes, such bitmap optimization is possible.
> But this accessor function is still necessary,

OK, I'm convinced. It makes sense off data path,
much easier than try to unswizzle and swizzle back
to the new values.

> please see the following. (I didn't do any test yet. Just to show the idea)
> If you like it, I'll post it as separate patch.

Yes. BTW as long as we touch it, we might want some symbolic
name for constants 0x60, 16, and use PCI_NUM_PINS instead of 4.
Some more suggestions below.

Also, save/restore needs to be updated.

> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 151353c..82b7daf 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -40,6 +40,7 @@ typedef PCIHostState I440FXState;
>  
>  typedef struct PIIX3State {
>      PCIDevice dev;
> +    unsigned long irq_level[16];

That's 1024 bits. We really only need 4*16 = 64 bits.
Also pic_levels might be a better name.
So just
   uint64_t pic_levels;

Maybe stick a check there:
#if PCI_NUM_PINS * PIIX_NUM_PIC_IRQS > 64
#error unable to encode pic state in 64 bit in pic_levels
#endif
Also, need to clear on init?

>      int32_t dummy_for_save_load_compat[4];
>      qemu_irq *pic;
>  } PIIX3State;
> @@ -200,25 +201,51 @@ PCIBus *i440fx_init(int *piix3_devfn, qemu_irq *pic, ram_addr_t ram_size)
>  }
>  
>  /* PIIX3 PCI to ISA bridge */
> -
>  static void piix3_set_irq(void *opaque, int irq_num, int level)
>  {
>      int i, pic_irq, pic_level;
>      PIIX3State *piix3 = opaque;
>  
> -    /* now we change the pic irq level according to the piix irq mappings */
> -    /* XXX: optimize */
>      pic_irq = piix3->dev.config[0x60 + irq_num];
> -    if (pic_irq < 16) {
> -        /* The pic level is the logical OR of all the PCI irqs mapped
> -           to it */
> -        pic_level = 0;
> -        for (i = 0; i < 4; i++) {
> -            if (pic_irq == piix3->dev.config[0x60 + i]) {
> -                pic_level |= pci_bus_get_irq_level(piix3->dev.bus, i);
> -            }
> +    if (pic_irq >= 16) {
> +        return;
> +    }
> +
> +    /* The pic level is the logical OR of all the PCI irqs mapped to it */
> +    if (level) {
> +        set_bit(&piix3->irq_level[pic_irq], irq_num);
> +    } else {
> +        clear_bit(&piix3->irq_level[pic_irq], irq_num);
> +    }

We can do this without a branch too I think (assuming uint64_t suggested
above):
	mask = 0x1ull << (pic_irq * 16 + irq_num);
	piix3->pic_levels &= ~mask;
	piix3->pic_levels |= mask;

> +    qemu_set_irq(piix3->pic[pic_irq], !!piix3->irq_level[pic_irq]);
> +}
> +
> +/* irq routing is changed. so rebuild bitmap */
> +static void piix3_update_irq_levels(PIIX3State *piix3)
> +{
> +    int i;
> +    for (i = 0; i < 16; i++) {
> +        piix3->irq_level[i] = 0;
> +    }

memset(piix3->irq_level, 0, sizeof piix3->irq_level);

> +    for (i = 0; i < 4; i++) {
> +        int pic_irq = piix3->dev.config[0x60 + irq_num];
> +        if (pic_irq >= 16) {
> +            continue;
> +        }
> +        if (pci_bus_get_irq_level(piix3->dev.bus, i)) {
> +            set_bit(&piix3->irq_level[pic_irq], i);
>          }
> -        qemu_set_irq(piix3->pic[pic_irq], pic_level);

Hmm, don't we need to set the levels in guest appropriately?

There's also some duplication here.
Can't we just do
	for (i = 0; i < 4; i++) {
		piix3_set_irq(piix3, i, pci_bus_get_irq_level(piix3->dev.bus, i));
	}
?

> +    }
> +}
> +
> +static void piix3_write_config(PCIDevice *dev,
> +                               uint32_t address, uint32_t val, int len)
> +{
> +    PIIX3State *piix3 = DO_UPCAST(PIIX3State, dev, dev);
> +
> +    pci_default_write_config(dev, address, val, len);
> +    if (ranges_overlap(address, len, 0x60, 4)) {
> +        piix3_update_irq_levels(piix3);
>      }
>  }
>  
> @@ -318,6 +345,7 @@ static PCIDeviceInfo i440fx_info[] = {
>          .qdev.no_user = 1,
>          .no_hotplug   = 1,
>          .init         = piix3_initfn,
> +        .config_write = piix3_write_config,
>      },{
>          /* end of list */
>      }
> 
> -- 
> yamahata