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

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

[Michael S. Tsirkin]

This patchset adds support for mandatory interupt
status and interrupt disable bits to all
PCI devices.  This is required for PCI compliancy.

These patches are on top of my pci tree,
including Isaku Yamahata's fixes.
If this is a problem, let me know and
I will rebase.

This works fine for me, but since this touches
all PCI devices, please review carefully.

Michael S. Tsirkin (4):
  pci: rearrange code for interrupts
  pci: track IRQ status
  pci: interrupt status bit implementation
  pci: interrupt disable bit support

 hw/pci.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 hw/pci.h |    8 ++++++
 2 files changed, 79 insertions(+), 12 deletions(-)

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

[Isaku Yamahata]

On Wed, Nov 25, 2009 at 06:58:34PM +0200, Michael S. Tsirkin wrote:
> This patchset adds support for mandatory interupt
> status and interrupt disable bits to all
> PCI devices.  This is required for PCI compliancy.
> 
> These patches are on top of my pci tree,
> including Isaku Yamahata's fixes.
> If this is a problem, let me know and
> I will rebase.
> 
> This works fine for me, but since this touches
> all PCI devices, please review carefully.

Just a curiosity, what OS do you have in your mind?

You introduced new members, irq_status and irq_disabled
and maintain them according configuration space write.
Another approach is to use irq_state[PCI_NUM_PINS] and interrupt disabled
bit in command register.
At least I think irq_disable can be removed by moving !change check
from pci_set_irq() into pci_change_irq_level().

As for irq_status, only user of irq_status is pci_update_irq_status()
so if (irq_statue) can be open coded. On the other hand,
PCIBus already has irq_count member for same purpose.
So probably open coding or introducing irq_count instead of irq_status
would be reasonable.


> 
> Michael S. Tsirkin (4):
>   pci: rearrange code for interrupts
>   pci: track IRQ status
>   pci: interrupt status bit implementation
>   pci: interrupt disable bit support
> 
>  hw/pci.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  hw/pci.h |    8 ++++++
>  2 files changed, 79 insertions(+), 12 deletions(-)
> 

-- 
yamahata

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

[Michael S. Tsirkin]

On Thu, Nov 26, 2009 at 12:21:46PM +0900, Isaku Yamahata wrote:
> On Wed, Nov 25, 2009 at 06:58:34PM +0200, Michael S. Tsirkin wrote:
> > This patchset adds support for mandatory interupt
> > status and interrupt disable bits to all
> > PCI devices.  This is required for PCI compliancy.
> > 
> > These patches are on top of my pci tree,
> > including Isaku Yamahata's fixes.
> > If this is a problem, let me know and
> > I will rebase.
> > 
> > This works fine for me, but since this touches
> > all PCI devices, please review carefully.
> 
> Just a curiosity, what OS do you have in your mind?

windows and linux :)

> You introduced new members, irq_status and irq_disabled
> and maintain them according configuration space write.
> Another approach is to use irq_state[PCI_NUM_PINS] and interrupt disabled
> bit in command register.

I will explain. irq_status is an optimization: it is a sum of
all irq_state values. Since interrupts is a fast-path operation,
we do not want to add another loop there.

> At least I think irq_disable can be removed by moving !change check
> from pci_set_irq() into pci_change_irq_level().

I don't see how is pci_set_irq relevant here: the reason I added
irq_disabled is because we need to re-trigger interrupts when interrupts
are enabled, either by load or config cycle. So it is there simply to
detect change.

Another approach would be to make irq_disabled a local variable in
pci_default_write_config and in pci_device_load, and pass old value to
pci_update_irq_disabled.  I tried and it seems more code, and routines
become less self-contained.  As it is, I think it's cleaner because we
have an idem-potent routine that is always safe it call, just like
pci_update_mappings.


> As for irq_status, only user of irq_status is pci_update_irq_status()
> so if (irq_statue) can be open coded. On the other hand,
> PCIBus already has irq_count member for same purpose.
> So probably open coding or introducing irq_count instead of irq_status
> would be reasonable.

No, this would slow us down because these are per-pin.
We need a sum of interrupts so that config space
can be updated by a single command.
Interrupts are a fastpath, extra loops there should be avoided.

> 
> > 
> > Michael S. Tsirkin (4):
> >   pci: rearrange code for interrupts
> >   pci: track IRQ status
> >   pci: interrupt status bit implementation
> >   pci: interrupt disable bit support
> > 
> >  hw/pci.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++---------
> >  hw/pci.h |    8 ++++++
> >  2 files changed, 79 insertions(+), 12 deletions(-)
> > 
> 
> -- 
> yamahata

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

[Paul Brook]

> No, this would slow us down because these are per-pin.
> We need a sum of interrupts so that config space
> can be updated by a single command.
> Interrupts are a fastpath, extra loops there should be avoided.

It's really not that much of a fast path. Unless you're doing something 
particularly obscure then even under heavy load you're unlikely to exceed a 
few kHz.  Compared to the average PIC implementation, and the overhead of the 
actual CPU interrupt, I find it hard to believe that looping over precisely 4 
entries has any real performance hit.

Paul

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

[Michael S. Tsirkin]

On Thu, Nov 26, 2009 at 12:41:03PM +0000, Paul Brook wrote:
> > No, this would slow us down because these are per-pin.
> > We need a sum of interrupts so that config space
> > can be updated by a single command.
> > Interrupts are a fastpath, extra loops there should be avoided.
> 
> It's really not that much of a fast path. Unless you're doing something 
> particularly obscure then even under heavy load you're unlikely to exceed a 
> few kHz.

I think with kvm, heavy disk stressing benchmark can get higher.

> Compared to the average PIC implementation, and the overhead of the 
> actual CPU interrupt, I find it hard to believe that looping over precisely 4 
> entries has any real performance hit.
> 
> Paul

I don't think it is major, but I definitely have seen, in the past,
that extra branches and memory accesses have small but measureable effect
when taken in interrupt handler routines in drivers, and same should
apply here.

OTOH keeping the sum around is trivial.

It might not be easily measureable now, but IMO that's just because the
whole interrupt delivery is so complex. E.g.  we currently have there a
loop re-computing interrupt routing on each access, this is just silly
and I intend to fix it.  I would rather not introduce more code that
will have to be cleaned up later.

-- 
MST

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

[Paul Brook]

>> It's really not that much of a fast path. Unless you're doing something
>> particularly obscure then even under heavy load you're unlikely to exceed
>> a few kHz.
>
>I think with kvm, heavy disk stressing benchmark can get higher.

I'd still expect this to be the least of your problems. 

If nothing else you've at least one host signal delivery and/or thread context 
switch in there.  Not to mention the overhead to forwarding the interrupt to 
the guest CPU.

> > Compared to the average PIC implementation, and the overhead of the
> > actual CPU interrupt, I find it hard to believe that looping over
> > precisely 4 entries has any real performance hit.
> 
> I don't think it is major, but I definitely have seen, in the past,
> that extra branches and memory accesses have small but measureable effect
> when taken in interrupt handler routines in drivers, and same should
> apply here.
> 
> OTOH keeping the sum around is trivial.

Not entirely. You now have two different bits of information that you have to 
keep consistent.

Unless you can show that this is performance critical code I strongly 
recommend keeping it as simple as possible.

Paul

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

[Michael S. Tsirkin]

On Thu, Nov 26, 2009 at 01:21:39PM +0000, Paul Brook wrote:
> >> It's really not that much of a fast path. Unless you're doing something
> >> particularly obscure then even under heavy load you're unlikely to exceed
> >> a few kHz.
> >
> >I think with kvm, heavy disk stressing benchmark can get higher.
> 
> I'd still expect this to be the least of your problems. 
> 
> If nothing else you've at least one host signal delivery and/or thread context 
> switch in there.

iotread which does the signalling might be running in parallel
with the guest CPU.

> Not to mention the overhead to forwarding the interrupt to 
> the guest CPU.

This is often mitigated as KVM knows to inject the interrupt
on the next vmexit.

> > > Compared to the average PIC implementation, and the overhead of the
> > > actual CPU interrupt, I find it hard to believe that looping over
> > > precisely 4 entries has any real performance hit.
> > 
> > I don't think it is major, but I definitely have seen, in the past,
> > that extra branches and memory accesses have small but measureable effect
> > when taken in interrupt handler routines in drivers, and same should
> > apply here.
> > 
> > OTOH keeping the sum around is trivial.
> 
> Not entirely. You now have two different bits of information that you have to 
> keep consistent.

This is inherent in pci spec definition: interrupt status
bit in config space duplicates interrupt state.

> Unless you can show that this is performance critical code I strongly 
> recommend keeping it as simple as possible.
> 
> Paul

I don't see there is anything left show: interrupt delivery is *obviously*
performance critical: people are running *latency benchmarks* measuring
how fast a packet can get from an external interface into guest,
in microseconds. We definitely want to remove obvious waste there.


-- 
MST

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

[Michael S. Tsirkin]

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?

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;
 

[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;
>