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
