[Qemu-devel] Re: [PATCH 0/4] pci: interrupt status/interrupt disable support

[Michael S. Tsirkin]

On Thu, Nov 26, 2009 at 12:38:20PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 26, 2009 at 12:21:46PM +0900, Isaku Yamahata wrote:
> > At least I think irq_disable can be removed
> 
> The following patch on top of mine removes irq_disabled field in
> PCIDevice.  I am of two minds whether this makes the code better.
> What is your opinion?

Thinking about this some more, I really like the original
approach better, mostly because it helps keep the fastpath
streamlined.

On thing to note is that irq_disabled field was in the same or
adjacent cache line with irq_state, and it is accessed on fast path.
Reading config space instead will access an extra cache line which would
be cold otherwise (config space is normally not accessed on fast path),
and also needs an extra memory access (it is allocated on heap
separately).

As I said separately, the whole of interrupt delivery
is far from optimal, but we have to start somewhere.

> diff --git a/hw/pci.c b/hw/pci.c
> index 3daae46..75f97df 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -128,15 +128,18 @@ static void pci_change_irq_level(PCIDevice *pci_dev, 
> int irq_num, int change)
>      bus->set_irq(bus->irq_opaque, irq_num, bus->irq_count[irq_num] != 0);
>  }
>  
> -/* Update irq disabled field after config space change,
> - * assert/deassert interrupts if necessary. */
> -static void pci_update_irq_disabled(PCIDevice *d)
> +static inline int pci_irq_disabled(PCIDevice *d)
>  {
> -    int i;
> -    int disabled = !!(pci_get_word(d->config + PCI_COMMAND) &
> -                      PCI_COMMAND_INTX_DISABLE);
> -    if (disabled == d->irq_disabled)
> +    return !!(pci_get_word(d->config + PCI_COMMAND) & 
> PCI_COMMAND_INTX_DISABLE);
> +}
> +
> +/* Called after interrupt disabled field update in config space,
> + * assert/deassert interrupts if necessary.
> + * Gets original interrupt disable bit value (before update). */
> +static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled)
> +{
> +    int i, disabled = pci_irq_disabled(d);
> +    if (disabled == was_irq_disabled)
>          return;
> -    d->irq_disabled = disabled;
>      for (i = 0; i < ARRAY_SIZE(d->irq_state); ++i) {
>          int state = d->irq_state[i];
> @@ -150,7 +153,6 @@ static void pci_device_reset(PCIDevice *dev)
>  
>      memset(dev->irq_state, 0, sizeof dev->irq_state);
>      dev->irq_status = 0;
> -    dev->irq_disabled = 0;
>      pci_update_irq_status(dev);
>      dev->config[PCI_COMMAND] &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
>                                    PCI_COMMAND_MASTER);
> @@ -369,14 +371,14 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
>  
>  int pci_device_load(PCIDevice *s, QEMUFile *f)
>  {
> -    int ret, i;
> +    int ret, i, was_irq_disabled = pci_irq_disabled(d);
>      ret = vmstate_load_state(f, pci_get_vmstate(s), s, s->version_id);
>      for (i = 0; i < ARRAY_SIZE(s->irq_state); ++i) {
>          s->irq_status += s->irq_state[i];
>      }
>      /* Restore the interrupt status bit. */
>      pci_update_irq_status(s);
> -    pci_update_irq_disabled(s);
> +    pci_update_irq_disabled(s, was_irq_disabled);
>      return ret;
>  }
>  
> @@ -924,7 +926,7 @@ uint32_t pci_default_read_config(PCIDevice *d,
>  
>  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int 
> l)
>  {
> -    int i;
> +    int i, was_irq_disabled = pci_irq_disabled(d);
>      uint32_t config_size = pci_config_size(d);
>  
>      for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
> @@ -938,7 +940,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t 
> addr, uint32_t val, int l)
>          pci_update_mappings(d);
>  
>      if (range_covers_byte(addr, l, PCI_COMMAND))
> -        pci_update_irq_disabled(d);
> +        pci_update_irq_disabled(d, was_irq_disabled);
>  }
>  
>  /***********************************************************/
> @@ -957,7 +959,7 @@ static void pci_set_irq(void *opaque, int irq_num, int 
> level)
>      pci_dev->irq_state[irq_num] = level;
>      pci_dev->irq_status += change;
>      pci_update_irq_status(pci_dev);
> -    if (pci_dev->irq_disabled)
> +    if (pci_irq_disabled(pci_dev))
>          return;
>      pci_change_irq_level(pci_dev, irq_num, change);
>  }
> diff --git a/hw/pci.h b/hw/pci.h
> index 994b8bc..25ad114 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -227,9 +227,6 @@ struct PCIDevice {
>      /* Sum of all irq levels. Used to implement irq status register. */
>      int irq_status;
>  
> -    /* Whether interrupts are disabled by command bit. */
> -    int irq_disabled;
> -
>      /* Capability bits */
>      uint32_t cap_present;
>