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

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

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