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

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

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