[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 0/4] pci: interrupt status/interrupt disable sup
From: |
Michael S. Tsirkin |
Subject: |
[Qemu-devel] Re: [PATCH 0/4] pci: interrupt status/interrupt disable support |
Date: |
Thu, 26 Nov 2009 11:48:11 +0200 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
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
[Qemu-devel] Re: [PATCH 0/4] pci: interrupt status/interrupt disable support, Michael S. Tsirkin, 2009/11/26