diff mbox

[1/2] pci: Add pci_device_get_host_irq

Message ID 4FBA3F8B.9060103@siemens.com
State New
Headers show

Commit Message

Jan Kiszka May 21, 2012, 1:13 p.m. UTC
Add a PCI IRQ path discovery function that walks from a given device to
the host bridge, returning the IRQ number that is reported to the
attached interrupt controller. For this purpose, another PCI bridge
callback function is introduced: map_host_irq. It is so far only
implemented by the PIIX3, other host bridges can be added later on as
required.

Will be used for KVM PCI device assignment.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/alpha_typhoon.c |    2 +-
 hw/apb_pci.c       |    2 +-
 hw/bonito.c        |    2 +-
 hw/grackle_pci.c   |    1 +
 hw/gt64xxx.c       |    2 +-
 hw/pci.c           |   23 ++++++++++++++++++++---
 hw/pci.h           |    7 +++++--
 hw/pci_internals.h |    1 +
 hw/piix_pci.c      |   15 ++++++++++++---
 hw/ppc4xx_pci.c    |    2 +-
 hw/ppce500_pci.c   |    2 +-
 hw/prep_pci.c      |    2 +-
 hw/sh_pci.c        |    2 +-
 hw/spapr_pci.c     |    2 +-
 hw/unin_pci.c      |    4 ++--
 hw/versatile_pci.c |    2 +-
 16 files changed, 51 insertions(+), 20 deletions(-)

Comments

Alex Williamson May 21, 2012, 2:10 p.m. UTC | #1
On Mon, 2012-05-21 at 10:13 -0300, Jan Kiszka wrote:
> Add a PCI IRQ path discovery function that walks from a given device to
> the host bridge, returning the IRQ number that is reported to the
> attached interrupt controller. For this purpose, another PCI bridge
> callback function is introduced: map_host_irq. It is so far only
> implemented by the PIIX3, other host bridges can be added later on as
> required.
> 
> Will be used for KVM PCI device assignment.
And VFIO.  Not so different from the get_irq function I added in the
VFIO tree.

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/alpha_typhoon.c |    2 +-
>  hw/apb_pci.c       |    2 +-
>  hw/bonito.c        |    2 +-
>  hw/grackle_pci.c   |    1 +
>  hw/gt64xxx.c       |    2 +-
>  hw/pci.c           |   23 ++++++++++++++++++++---
>  hw/pci.h           |    7 +++++--
>  hw/pci_internals.h |    1 +
>  hw/piix_pci.c      |   15 ++++++++++++---
>  hw/ppc4xx_pci.c    |    2 +-
>  hw/ppce500_pci.c   |    2 +-
>  hw/prep_pci.c      |    2 +-
>  hw/sh_pci.c        |    2 +-
>  hw/spapr_pci.c     |    2 +-
>  hw/unin_pci.c      |    4 ++--
>  hw/versatile_pci.c |    2 +-
>  16 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
> index 872e112..fc2e4b3 100644
> --- a/hw/alpha_typhoon.c
> +++ b/hw/alpha_typhoon.c
> @@ -764,7 +764,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
>                                  &s->pchip.reg_io);
>  
>      b = pci_register_bus(&s->host.busdev.qdev, "pci",
> -                         typhoon_set_irq, sys_map_irq, s,
> +                         typhoon_set_irq, sys_map_irq, NULL, s,
>                           &s->pchip.reg_mem, addr_space_io, 0, 64);
>      s->host.bus = b;
>  
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 7e28808..819bf1d 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -366,7 +366,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
>      memory_region_add_subregion(get_system_memory(), mem_base, &d->pci_mmio);
>  
>      d->bus = pci_register_bus(&d->busdev.qdev, "pci",
> -                              pci_apb_set_irq, pci_pbm_map_irq, d,
> +                              pci_apb_set_irq, pci_pbm_map_irq, NULL, d,
>                                &d->pci_mmio,
>                                get_system_io(),
>                                0, 32);
> diff --git a/hw/bonito.c b/hw/bonito.c
> index 77786f8..7ce5993 100644
> --- a/hw/bonito.c
> +++ b/hw/bonito.c
> @@ -750,7 +750,7 @@ PCIBus *bonito_init(qemu_irq *pic)
>      dev = qdev_create(NULL, "Bonito-pcihost");
>      pcihost = FROM_SYSBUS(BonitoState, sysbus_from_qdev(dev));
>      b = pci_register_bus(&pcihost->busdev.qdev, "pci", pci_bonito_set_irq,
> -                         pci_bonito_map_irq, pic, get_system_memory(),
> +                         pci_bonito_map_irq, NULL, pic, get_system_memory(),
>                           get_system_io(),
>                           0x28, 32);
>      pcihost->bus = b;
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index 81ff3a3..f47d9fe 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -85,6 +85,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
>      d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
>                                           pci_grackle_set_irq,
>                                           pci_grackle_map_irq,
> +                                         NULL,
>                                           pic,
>                                           &d->pci_mmio,
>                                           address_space_io,
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index a2d0e5a..a97bbf0 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -1093,7 +1093,7 @@ PCIBus *gt64120_register(qemu_irq *pic)
>      d = FROM_SYSBUS(GT64120State, s);
>      d->pci.bus = pci_register_bus(&d->busdev.qdev, "pci",
>                                    gt64120_pci_set_irq, gt64120_pci_map_irq,
> -                                  pic,
> +                                  NULL, pic,
>                                    get_system_memory(),
>                                    get_system_io(),
>                                    PCI_DEVFN(18, 0), 4);
> diff --git a/hw/pci.c b/hw/pci.c
> index 439f3ce..df4d93e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -298,10 +298,11 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
>  }
>  
>  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> -                  void *irq_opaque, int nirq)
> +                  pci_map_host_irq_fn map_host_irq, void *irq_opaque, int nirq)
>  {
>      bus->set_irq = set_irq;
>      bus->map_irq = map_irq;
> +    bus->map_host_irq = map_host_irq;
>      bus->irq_opaque = irq_opaque;
>      bus->nirq = nirq;
>      bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
> @@ -316,7 +317,7 @@ void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
>  
>  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> -                         void *irq_opaque,
> +                         pci_map_host_irq_fn map_host_irq, void *irq_opaque,
>                           MemoryRegion *address_space_mem,
>                           MemoryRegion *address_space_io,
>                           uint8_t devfn_min, int nirq)
> @@ -325,7 +326,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>  
>      bus = pci_bus_new(parent, name, address_space_mem,
>                        address_space_io, devfn_min);
> -    pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
> +    pci_bus_irqs(bus, set_irq, map_irq, map_host_irq, irq_opaque, nirq);
>      return bus;
>  }
>  
> @@ -1067,6 +1068,22 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
>      pci_change_irq_level(pci_dev, irq_num, change);
>  }
>  
> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
> +{
> +    PCIBus *bus;
> +
> +    for (;;) {
> +        bus = pci_dev->bus;
> +        irq_num = bus->map_irq(pci_dev, irq_num);
> +        if (bus->map_host_irq) {
> +            break;
> +        }
> +        pci_dev = bus->parent_dev;
> +        assert(pci_dev);
> +    }
> +    return bus->map_host_irq(bus->irq_opaque, irq_num);
> +}
> +
>  /***********************************************************/
>  /* monitor info on PCI */
>  
> diff --git a/hw/pci.h b/hw/pci.h
> index c3cacce..29bc8bf 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -276,6 +276,7 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev);
>  
>  typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
>  typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> +typedef int (*pci_map_host_irq_fn)(void *opaque, int irq_num);
>  
>  typedef enum {
>      PCI_HOTPLUG_DISABLED,
> @@ -295,15 +296,17 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
>                      MemoryRegion *address_space_io,
>                      uint8_t devfn_min);
>  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> -                  void *irq_opaque, int nirq);
> +                  pci_map_host_irq_fn map_host_irq, void *irq_opaque,
> +                  int nirq);
>  int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
>  void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
>  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> -                         void *irq_opaque,
> +                         pci_map_host_irq_fn map_host_irq, void *irq_opaque,
>                           MemoryRegion *address_space_mem,
>                           MemoryRegion *address_space_io,
>                           uint8_t devfn_min, int nirq);
> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num);
>  void pci_device_reset(PCIDevice *dev);
>  void pci_bus_reset(PCIBus *bus);
>  
> diff --git a/hw/pci_internals.h b/hw/pci_internals.h
> index 96690b7..a92353e 100644
> --- a/hw/pci_internals.h
> +++ b/hw/pci_internals.h
> @@ -19,6 +19,7 @@ struct PCIBus {
>      uint8_t devfn_min;
>      pci_set_irq_fn set_irq;
>      pci_map_irq_fn map_irq;
> +    pci_map_host_irq_fn map_host_irq;
>      pci_hotplug_fn hotplug;
>      DeviceState *hotplug_qdev;
>      void *irq_opaque;
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 09e84f5..cfea97c 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -89,6 +89,7 @@ struct PCII440FXState {
>  #define I440FX_SMRAM    0x72
>  
>  static void piix3_set_irq(void *opaque, int pirq, int level);
> +static int piix3_map_host_irq(void *opaque, int pci_intx);
>  static void piix3_write_config_xen(PCIDevice *dev,
>                                 uint32_t address, uint32_t val, int len);
>  
> @@ -308,13 +309,13 @@ static PCIBus *i440fx_common_init(const char *device_name,
>      if (xen_enabled()) {
>          piix3 = DO_UPCAST(PIIX3State, dev,
>                  pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
> -        pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
> +        pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq, NULL,
>                  piix3, XEN_PIIX_NUM_PIRQS);
>      } else {
>          piix3 = DO_UPCAST(PIIX3State, dev,
>                  pci_create_simple_multifunction(b, -1, true, "PIIX3"));
> -        pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
> -                PIIX_NUM_PIRQS);
> +        pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3_map_host_irq,
> +                     piix3, PIIX_NUM_PIRQS);
>      }
>      piix3->pic = pic;
>      *isa_bus = DO_UPCAST(ISABus, qbus,
> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
>      piix3_set_irq_level(piix3, pirq, level);
>  }
>  
> +static int piix3_map_host_irq(void *opaque, int pci_intx)
> +{
> +    PIIX3State *piix3 = opaque;
> +    int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
> +
> +    return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
> +}
> +
>  /* irq routing is changed. so rebuild bitmap */
>  static void piix3_update_irq_levels(PIIX3State *piix3)
>  {
> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> index 203c3cd..224c4a0 100644
> --- a/hw/ppc4xx_pci.c
> +++ b/hw/ppc4xx_pci.c
> @@ -343,7 +343,7 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
>      }
>  
>      b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, ppc4xx_pci_set_irq,
> -                         ppc4xx_pci_map_irq, s->irq, get_system_memory(),
> +                         ppc4xx_pci_map_irq, NULL, s->irq, get_system_memory(),
>                           get_system_io(), 0, 4);
>      s->pci_state.bus = b;
>  
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 0f60b24..dd924ae 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -318,7 +318,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>      }
>  
>      b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
> -                         mpc85xx_pci_map_irq, s->irq, address_space_mem,
> +                         mpc85xx_pci_map_irq, NULL, s->irq, address_space_mem,
>                           address_space_io, PCI_DEVFN(0x11, 0), 4);
>      s->pci_state.bus = b;
>  
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 38dbff4..9d7bec7 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -108,7 +108,7 @@ static int raven_pcihost_init(SysBusDevice *dev)
>      }
>  
>      bus = pci_register_bus(&h->busdev.qdev, NULL,
> -                           prep_set_irq, prep_map_irq, s->irq,
> +                           prep_set_irq, prep_map_irq, NULL, s->irq,
>                             address_space_mem, address_space_io, 0, 4);
>      h->bus = bus;
>  
> diff --git a/hw/sh_pci.c b/hw/sh_pci.c
> index 0cfac46..1cea12b 100644
> --- a/hw/sh_pci.c
> +++ b/hw/sh_pci.c
> @@ -120,7 +120,7 @@ static int sh_pci_device_init(SysBusDevice *dev)
>          sysbus_init_irq(dev, &s->irq[i]);
>      }
>      s->bus = pci_register_bus(&s->busdev.qdev, "pci",
> -                              sh_pci_set_irq, sh_pci_map_irq,
> +                              sh_pci_set_irq, sh_pci_map_irq, NULL,
>                                s->irq,
>                                get_system_memory(),
>                                get_system_io(),
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 25b400a..4a43062 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -306,7 +306,7 @@ static int spapr_phb_init(SysBusDevice *s)
>  
>      bus = pci_register_bus(&phb->busdev.qdev,
>                             phb->busname ? phb->busname : phb->dtbusname,
> -                           pci_spapr_set_irq, pci_spapr_map_irq, phb,
> +                           pci_spapr_set_irq, pci_spapr_map_irq, NULL, phb,
>                             &phb->memspace, &phb->iospace,
>                             PCI_DEVFN(0, 0), PCI_NUM_PINS);
>      phb->host_state.bus = bus;
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 409bcd4..056e3bc 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -227,7 +227,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
>  
>      d->host_state.bus = pci_register_bus(dev, "pci",
>                                           pci_unin_set_irq, pci_unin_map_irq,
> -                                         pic,
> +                                         NULL, pic,
>                                           &d->pci_mmio,
>                                           address_space_io,
>                                           PCI_DEVFN(11, 0), 4);
> @@ -293,7 +293,7 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
>  
>      d->host_state.bus = pci_register_bus(dev, "pci",
>                                           pci_unin_set_irq, pci_unin_map_irq,
> -                                         pic,
> +                                         NULL, pic,
>                                           &d->pci_mmio,
>                                           address_space_io,
>                                           PCI_DEVFN(11, 0), 4);
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index ae53a8b..90c400e 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -68,7 +68,7 @@ static int pci_vpb_init(SysBusDevice *dev)
>          sysbus_init_irq(dev, &s->irq[i]);
>      }
>      bus = pci_register_bus(&dev->qdev, "pci",
> -                           pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
> +                           pci_vpb_set_irq, pci_vpb_map_irq, NULL, s->irq,
>                             get_system_memory(), get_system_io(),
>                             PCI_DEVFN(11, 0), 4);
>
Avi Kivity May 21, 2012, 2:36 p.m. UTC | #2
On 05/21/2012 04:13 PM, Jan Kiszka wrote:
> Add a PCI IRQ path discovery function that walks from a given device to
> the host bridge, returning the IRQ number that is reported to the
> attached interrupt controller. For this purpose, another PCI bridge
> callback function is introduced: map_host_irq. It is so far only
> implemented by the PIIX3, other host bridges can be added later on as
> required.
>
> Will be used for KVM PCI device assignment.

This is similar to the memory API, which converts a memory region
hierarchy to a flat list and fires notifiers whenever it changes.

> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
> +{
> +    PCIBus *bus;
> +
> +    for (;;) {
> +        bus = pci_dev->bus;
> +        irq_num = bus->map_irq(pci_dev, irq_num);
> +        if (bus->map_host_irq) {
> +            break;
> +        }
> +        pci_dev = bus->parent_dev;
> +        assert(pci_dev);
> +    }
> +    return bus->map_host_irq(bus->irq_opaque, irq_num);
> +}
> +

My personal preference is to avoid infinite loops with breaks, I'd write
this as a do/while (without the assert).  Or maybe supply all buses with
a default map_host_irq that recurses back into
pci_device_get_host_irq().  But this is not an objection to the patch.
Jan Kiszka May 21, 2012, 2:47 p.m. UTC | #3
On 2012-05-21 11:36, Avi Kivity wrote:
> On 05/21/2012 04:13 PM, Jan Kiszka wrote:
>> Add a PCI IRQ path discovery function that walks from a given device to
>> the host bridge, returning the IRQ number that is reported to the
>> attached interrupt controller. For this purpose, another PCI bridge
>> callback function is introduced: map_host_irq. It is so far only
>> implemented by the PIIX3, other host bridges can be added later on as
>> required.
>>
>> Will be used for KVM PCI device assignment.
> 
> This is similar to the memory API, which converts a memory region
> hierarchy to a flat list and fires notifiers whenever it changes.

In fact, this should become a generic thing one day, independent of PCI.
But that's an exercise to be done while reworking the IRQ layer of QEMU
(e.g. to support delivery feedback...).

> 
>> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
>> +{
>> +    PCIBus *bus;
>> +
>> +    for (;;) {
>> +        bus = pci_dev->bus;
>> +        irq_num = bus->map_irq(pci_dev, irq_num);
>> +        if (bus->map_host_irq) {
>> +            break;
>> +        }
>> +        pci_dev = bus->parent_dev;
>> +        assert(pci_dev);
>> +    }
>> +    return bus->map_host_irq(bus->irq_opaque, irq_num);
>> +}
>> +
> 
> My personal preference is to avoid infinite loops with breaks, I'd write
> this as a do/while (without the assert).  Or maybe supply all buses with
> a default map_host_irq that recurses back into
> pci_device_get_host_irq().  But this is not an objection to the patch.

I've modeled it after pci_change_irq_level. I would suggest to change
both later on.

BTW, the assert catches host bridges that do not yet implement the
required mapping service.

Jan
Michael S. Tsirkin May 21, 2012, 5:34 p.m. UTC | #4
On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
> Add a PCI IRQ path discovery function that walks from a given device to
> the host bridge, returning the IRQ number that is reported to the
> attached interrupt controller. For this purpose, another PCI bridge
> callback function is introduced: map_host_irq. It is so far only
> implemented by the PIIX3, other host bridges can be added later on as
> required.
> 
> Will be used for KVM PCI device assignment.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

interrupt injection is data path even for emulated devices.
So instead of special casing device assignment I would like to see all
devices converted to an API that caches irqs.

This will likely mean that we can maintain the final
irq as part of the pci device structure, and
this api will simply return it.

> ---
>  hw/alpha_typhoon.c |    2 +-
>  hw/apb_pci.c       |    2 +-
>  hw/bonito.c        |    2 +-
>  hw/grackle_pci.c   |    1 +
>  hw/gt64xxx.c       |    2 +-
>  hw/pci.c           |   23 ++++++++++++++++++++---
>  hw/pci.h           |    7 +++++--
>  hw/pci_internals.h |    1 +
>  hw/piix_pci.c      |   15 ++++++++++++---
>  hw/ppc4xx_pci.c    |    2 +-
>  hw/ppce500_pci.c   |    2 +-
>  hw/prep_pci.c      |    2 +-
>  hw/sh_pci.c        |    2 +-
>  hw/spapr_pci.c     |    2 +-
>  hw/unin_pci.c      |    4 ++--
>  hw/versatile_pci.c |    2 +-
>  16 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
> index 872e112..fc2e4b3 100644
> --- a/hw/alpha_typhoon.c
> +++ b/hw/alpha_typhoon.c
> @@ -764,7 +764,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
>                                  &s->pchip.reg_io);
>  
>      b = pci_register_bus(&s->host.busdev.qdev, "pci",
> -                         typhoon_set_irq, sys_map_irq, s,
> +                         typhoon_set_irq, sys_map_irq, NULL, s,
>                           &s->pchip.reg_mem, addr_space_io, 0, 64);
>      s->host.bus = b;
>  
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 7e28808..819bf1d 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -366,7 +366,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
>      memory_region_add_subregion(get_system_memory(), mem_base, &d->pci_mmio);
>  
>      d->bus = pci_register_bus(&d->busdev.qdev, "pci",
> -                              pci_apb_set_irq, pci_pbm_map_irq, d,
> +                              pci_apb_set_irq, pci_pbm_map_irq, NULL, d,
>                                &d->pci_mmio,
>                                get_system_io(),
>                                0, 32);
> diff --git a/hw/bonito.c b/hw/bonito.c
> index 77786f8..7ce5993 100644
> --- a/hw/bonito.c
> +++ b/hw/bonito.c
> @@ -750,7 +750,7 @@ PCIBus *bonito_init(qemu_irq *pic)
>      dev = qdev_create(NULL, "Bonito-pcihost");
>      pcihost = FROM_SYSBUS(BonitoState, sysbus_from_qdev(dev));
>      b = pci_register_bus(&pcihost->busdev.qdev, "pci", pci_bonito_set_irq,
> -                         pci_bonito_map_irq, pic, get_system_memory(),
> +                         pci_bonito_map_irq, NULL, pic, get_system_memory(),
>                           get_system_io(),
>                           0x28, 32);
>      pcihost->bus = b;
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index 81ff3a3..f47d9fe 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -85,6 +85,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
>      d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
>                                           pci_grackle_set_irq,
>                                           pci_grackle_map_irq,
> +                                         NULL,
>                                           pic,
>                                           &d->pci_mmio,
>                                           address_space_io,
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index a2d0e5a..a97bbf0 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -1093,7 +1093,7 @@ PCIBus *gt64120_register(qemu_irq *pic)
>      d = FROM_SYSBUS(GT64120State, s);
>      d->pci.bus = pci_register_bus(&d->busdev.qdev, "pci",
>                                    gt64120_pci_set_irq, gt64120_pci_map_irq,
> -                                  pic,
> +                                  NULL, pic,
>                                    get_system_memory(),
>                                    get_system_io(),
>                                    PCI_DEVFN(18, 0), 4);
> diff --git a/hw/pci.c b/hw/pci.c
> index 439f3ce..df4d93e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -298,10 +298,11 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
>  }
>  
>  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> -                  void *irq_opaque, int nirq)
> +                  pci_map_host_irq_fn map_host_irq, void *irq_opaque, int nirq)
>  {
>      bus->set_irq = set_irq;
>      bus->map_irq = map_irq;
> +    bus->map_host_irq = map_host_irq;
>      bus->irq_opaque = irq_opaque;
>      bus->nirq = nirq;
>      bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
> @@ -316,7 +317,7 @@ void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
>  
>  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> -                         void *irq_opaque,
> +                         pci_map_host_irq_fn map_host_irq, void *irq_opaque,
>                           MemoryRegion *address_space_mem,
>                           MemoryRegion *address_space_io,
>                           uint8_t devfn_min, int nirq)
> @@ -325,7 +326,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>  
>      bus = pci_bus_new(parent, name, address_space_mem,
>                        address_space_io, devfn_min);
> -    pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
> +    pci_bus_irqs(bus, set_irq, map_irq, map_host_irq, irq_opaque, nirq);
>      return bus;
>  }
>  
> @@ -1067,6 +1068,22 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
>      pci_change_irq_level(pci_dev, irq_num, change);
>  }
>  
> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
> +{
> +    PCIBus *bus;
> +
> +    for (;;) {
> +        bus = pci_dev->bus;
> +        irq_num = bus->map_irq(pci_dev, irq_num);
> +        if (bus->map_host_irq) {
> +            break;
> +        }
> +        pci_dev = bus->parent_dev;
> +        assert(pci_dev);
> +    }
> +    return bus->map_host_irq(bus->irq_opaque, irq_num);
> +}
> +
>  /***********************************************************/
>  /* monitor info on PCI */
>  
> diff --git a/hw/pci.h b/hw/pci.h
> index c3cacce..29bc8bf 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -276,6 +276,7 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev);
>  
>  typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
>  typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> +typedef int (*pci_map_host_irq_fn)(void *opaque, int irq_num);
>  
>  typedef enum {
>      PCI_HOTPLUG_DISABLED,
> @@ -295,15 +296,17 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
>                      MemoryRegion *address_space_io,
>                      uint8_t devfn_min);
>  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> -                  void *irq_opaque, int nirq);
> +                  pci_map_host_irq_fn map_host_irq, void *irq_opaque,
> +                  int nirq);
>  int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
>  void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
>  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
>                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> -                         void *irq_opaque,
> +                         pci_map_host_irq_fn map_host_irq, void *irq_opaque,
>                           MemoryRegion *address_space_mem,
>                           MemoryRegion *address_space_io,
>                           uint8_t devfn_min, int nirq);
> +int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num);
>  void pci_device_reset(PCIDevice *dev);
>  void pci_bus_reset(PCIBus *bus);
>  
> diff --git a/hw/pci_internals.h b/hw/pci_internals.h
> index 96690b7..a92353e 100644
> --- a/hw/pci_internals.h
> +++ b/hw/pci_internals.h
> @@ -19,6 +19,7 @@ struct PCIBus {
>      uint8_t devfn_min;
>      pci_set_irq_fn set_irq;
>      pci_map_irq_fn map_irq;
> +    pci_map_host_irq_fn map_host_irq;
>      pci_hotplug_fn hotplug;
>      DeviceState *hotplug_qdev;
>      void *irq_opaque;
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 09e84f5..cfea97c 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -89,6 +89,7 @@ struct PCII440FXState {
>  #define I440FX_SMRAM    0x72
>  
>  static void piix3_set_irq(void *opaque, int pirq, int level);
> +static int piix3_map_host_irq(void *opaque, int pci_intx);
>  static void piix3_write_config_xen(PCIDevice *dev,
>                                 uint32_t address, uint32_t val, int len);
>  
> @@ -308,13 +309,13 @@ static PCIBus *i440fx_common_init(const char *device_name,
>      if (xen_enabled()) {
>          piix3 = DO_UPCAST(PIIX3State, dev,
>                  pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
> -        pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
> +        pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq, NULL,
>                  piix3, XEN_PIIX_NUM_PIRQS);
>      } else {
>          piix3 = DO_UPCAST(PIIX3State, dev,
>                  pci_create_simple_multifunction(b, -1, true, "PIIX3"));
> -        pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
> -                PIIX_NUM_PIRQS);
> +        pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3_map_host_irq,
> +                     piix3, PIIX_NUM_PIRQS);
>      }
>      piix3->pic = pic;
>      *isa_bus = DO_UPCAST(ISABus, qbus,
> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
>      piix3_set_irq_level(piix3, pirq, level);
>  }
>  
> +static int piix3_map_host_irq(void *opaque, int pci_intx)
> +{
> +    PIIX3State *piix3 = opaque;
> +    int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
> +
> +    return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
> +}
> +
>  /* irq routing is changed. so rebuild bitmap */
>  static void piix3_update_irq_levels(PIIX3State *piix3)
>  {
> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> index 203c3cd..224c4a0 100644
> --- a/hw/ppc4xx_pci.c
> +++ b/hw/ppc4xx_pci.c
> @@ -343,7 +343,7 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
>      }
>  
>      b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, ppc4xx_pci_set_irq,
> -                         ppc4xx_pci_map_irq, s->irq, get_system_memory(),
> +                         ppc4xx_pci_map_irq, NULL, s->irq, get_system_memory(),
>                           get_system_io(), 0, 4);
>      s->pci_state.bus = b;
>  
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 0f60b24..dd924ae 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -318,7 +318,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
>      }
>  
>      b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
> -                         mpc85xx_pci_map_irq, s->irq, address_space_mem,
> +                         mpc85xx_pci_map_irq, NULL, s->irq, address_space_mem,
>                           address_space_io, PCI_DEVFN(0x11, 0), 4);
>      s->pci_state.bus = b;
>  
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 38dbff4..9d7bec7 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -108,7 +108,7 @@ static int raven_pcihost_init(SysBusDevice *dev)
>      }
>  
>      bus = pci_register_bus(&h->busdev.qdev, NULL,
> -                           prep_set_irq, prep_map_irq, s->irq,
> +                           prep_set_irq, prep_map_irq, NULL, s->irq,
>                             address_space_mem, address_space_io, 0, 4);
>      h->bus = bus;
>  
> diff --git a/hw/sh_pci.c b/hw/sh_pci.c
> index 0cfac46..1cea12b 100644
> --- a/hw/sh_pci.c
> +++ b/hw/sh_pci.c
> @@ -120,7 +120,7 @@ static int sh_pci_device_init(SysBusDevice *dev)
>          sysbus_init_irq(dev, &s->irq[i]);
>      }
>      s->bus = pci_register_bus(&s->busdev.qdev, "pci",
> -                              sh_pci_set_irq, sh_pci_map_irq,
> +                              sh_pci_set_irq, sh_pci_map_irq, NULL,
>                                s->irq,
>                                get_system_memory(),
>                                get_system_io(),
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 25b400a..4a43062 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -306,7 +306,7 @@ static int spapr_phb_init(SysBusDevice *s)
>  
>      bus = pci_register_bus(&phb->busdev.qdev,
>                             phb->busname ? phb->busname : phb->dtbusname,
> -                           pci_spapr_set_irq, pci_spapr_map_irq, phb,
> +                           pci_spapr_set_irq, pci_spapr_map_irq, NULL, phb,
>                             &phb->memspace, &phb->iospace,
>                             PCI_DEVFN(0, 0), PCI_NUM_PINS);
>      phb->host_state.bus = bus;
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 409bcd4..056e3bc 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -227,7 +227,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
>  
>      d->host_state.bus = pci_register_bus(dev, "pci",
>                                           pci_unin_set_irq, pci_unin_map_irq,
> -                                         pic,
> +                                         NULL, pic,
>                                           &d->pci_mmio,
>                                           address_space_io,
>                                           PCI_DEVFN(11, 0), 4);
> @@ -293,7 +293,7 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
>  
>      d->host_state.bus = pci_register_bus(dev, "pci",
>                                           pci_unin_set_irq, pci_unin_map_irq,
> -                                         pic,
> +                                         NULL, pic,
>                                           &d->pci_mmio,
>                                           address_space_io,
>                                           PCI_DEVFN(11, 0), 4);
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index ae53a8b..90c400e 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -68,7 +68,7 @@ static int pci_vpb_init(SysBusDevice *dev)
>          sysbus_init_irq(dev, &s->irq[i]);
>      }
>      bus = pci_register_bus(&dev->qdev, "pci",
> -                           pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
> +                           pci_vpb_set_irq, pci_vpb_map_irq, NULL, s->irq,
>                             get_system_memory(), get_system_io(),
>                             PCI_DEVFN(11, 0), 4);
>  
> -- 
> 1.7.3.4
Jan Kiszka May 21, 2012, 6:58 p.m. UTC | #5
On 2012-05-21 14:34, Michael S. Tsirkin wrote:
> On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
>> Add a PCI IRQ path discovery function that walks from a given device to
>> the host bridge, returning the IRQ number that is reported to the
>> attached interrupt controller. For this purpose, another PCI bridge
>> callback function is introduced: map_host_irq. It is so far only
>> implemented by the PIIX3, other host bridges can be added later on as
>> required.
>>
>> Will be used for KVM PCI device assignment.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> interrupt injection is data path even for emulated devices.
> So instead of special casing device assignment I would like to see all
> devices converted to an API that caches irqs.
> 
> This will likely mean that we can maintain the final
> irq as part of the pci device structure, and
> this api will simply return it.

Yep, I definitely agree. It's just that such a design has to please even
more users than PCI devices, thus will likely take longer to settle than
the device assignment effort. Therefore I decided to rush forward with
an intermediate approach first.

Jan
Michael S. Tsirkin May 21, 2012, 7:05 p.m. UTC | #6
On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
>      piix3_set_irq_level(piix3, pirq, level);
>  }
>  
> +static int piix3_map_host_irq(void *opaque, int pci_intx)
> +{
> +    PIIX3State *piix3 = opaque;
> +    int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
> +
> +    return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
> +}
> +
>  /* irq routing is changed. so rebuild bitmap */
>  static void piix3_update_irq_levels(PIIX3State *piix3)
>  {


So, instead of special API just for assignment,
I would like to see map_irq in piix being reworked
to take dev config into account.
I think piix is almost unique in this but need to check,
if not fix other host buses that are programmable.
PCI bridges are all fixed routing.

Then we can drop set_irq callback.

Finally all devices can cache the irq#,
and piix would scan devices behind it
and update the irq.

Assignment then just needs a notifier, since
it owns the device just a pointer in device is
enough.

Could you look at doing this please?
If no I can give it a stub.
Jan Kiszka May 21, 2012, 8:35 p.m. UTC | #7
On 2012-05-21 16:05, Michael S. Tsirkin wrote:
> On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
>> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
>>      piix3_set_irq_level(piix3, pirq, level);
>>  }
>>  
>> +static int piix3_map_host_irq(void *opaque, int pci_intx)
>> +{
>> +    PIIX3State *piix3 = opaque;
>> +    int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
>> +
>> +    return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
>> +}
>> +
>>  /* irq routing is changed. so rebuild bitmap */
>>  static void piix3_update_irq_levels(PIIX3State *piix3)
>>  {
> 
> 
> So, instead of special API just for assignment,
> I would like to see map_irq in piix being reworked
> to take dev config into account.
> I think piix is almost unique in this but need to check,

Maybe it is the only host bridge with routing that we have in QEMU right
now, but it is surely not unique (e.g. all later Intel chipsets support
this).

> if not fix other host buses that are programmable.
> PCI bridges are all fixed routing.
> 
> Then we can drop set_irq callback.

set_irq may do more than IRQ routing. It may also flip polarity (see
bonito.c). I guess we need to analyze the requirements of all in-tree
host bridges first.

> 
> Finally all devices can cache the irq#,
> and piix would scan devices behind it
> and update the irq.
> 
> Assignment then just needs a notifier, since
> it owns the device just a pointer in device is
> enough.

I cannot completely follow your ideas here yet. Do you want to pass the
mapping information along the notifier, or why "just ... a notifier"?

> 
> Could you look at doing this please?
> If no I can give it a stub.
> 

Unless we can converge over a final API quickly, I would suggest to base
these refactorings on top of what I propose here. We will have to touch
all host bridges more invasively for this anyway. If you can explain to
me how simple the refactoring can be (but I'm a bit skeptical ;) ), I
could do this, otherwise I would prefer to focus on the device
assignment topic which provide some more nuts.

Jan
Michael S. Tsirkin May 21, 2012, 9:03 p.m. UTC | #8
On Mon, May 21, 2012 at 05:35:34PM -0300, Jan Kiszka wrote:
> On 2012-05-21 16:05, Michael S. Tsirkin wrote:
> > On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
> >> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
> >>      piix3_set_irq_level(piix3, pirq, level);
> >>  }
> >>  
> >> +static int piix3_map_host_irq(void *opaque, int pci_intx)
> >> +{
> >> +    PIIX3State *piix3 = opaque;
> >> +    int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
> >> +
> >> +    return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
> >> +}
> >> +
> >>  /* irq routing is changed. so rebuild bitmap */
> >>  static void piix3_update_irq_levels(PIIX3State *piix3)
> >>  {
> > 
> > 
> > So, instead of special API just for assignment,
> > I would like to see map_irq in piix being reworked
> > to take dev config into account.
> > I think piix is almost unique in this but need to check,
> 
> Maybe it is the only host bridge with routing that we have in QEMU right
> now, but it is surely not unique (e.g. all later Intel chipsets support
> this).

Yes. APIs for this should be in place. Just saying
instead of failing we can easily just make it work
if there are no remappings.

> > if not fix other host buses that are programmable.
> > PCI bridges are all fixed routing.
> > 
> > Then we can drop set_irq callback.
> 
> set_irq may do more than IRQ routing. It may also flip polarity (see
> bonito.c).

In that case host_map_irq is not a good API?
Need to investigate.

> I guess we need to analyze the requirements of all in-tree
> host bridges first.

Yes.

> > 
> > Finally all devices can cache the irq#,
> > and piix would scan devices behind it
> > and update the irq.
> > 
> > Assignment then just needs a notifier, since
> > it owns the device just a pointer in device is
> > enough.
> 
> I cannot completely follow your ideas here yet. Do you want to pass the
> mapping information along the notifier, or why "just ... a notifier"?


Each device would resolve IRQs that it uses.


> > 
> > Could you look at doing this please?
> > If no I can give it a stub.
> > 
> 
> Unless we can converge over a final API quickly, I would suggest to base
> these refactorings on top of what I propose here. We will have to touch
> all host bridges more invasively for this anyway. If you can explain to
> me how simple the refactoring can be (but I'm a bit skeptical ;) ), I
> could do this, otherwise I would prefer to focus on the device
> assignment topic which provide some more nuts.
> 
> Jan

Yes it's simple. I'll post in a couple of days (lots of
meetings tomorrow). I think you'll
see it's easier and cleaner.

> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
Michael S. Tsirkin May 21, 2012, 9:04 p.m. UTC | #9
On Mon, May 21, 2012 at 03:58:57PM -0300, Jan Kiszka wrote:
> On 2012-05-21 14:34, Michael S. Tsirkin wrote:
> > On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
> >> Add a PCI IRQ path discovery function that walks from a given device to
> >> the host bridge, returning the IRQ number that is reported to the
> >> attached interrupt controller. For this purpose, another PCI bridge
> >> callback function is introduced: map_host_irq. It is so far only
> >> implemented by the PIIX3, other host bridges can be added later on as
> >> required.
> >>
> >> Will be used for KVM PCI device assignment.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > interrupt injection is data path even for emulated devices.
> > So instead of special casing device assignment I would like to see all
> > devices converted to an API that caches irqs.
> > 
> > This will likely mean that we can maintain the final
> > irq as part of the pci device structure, and
> > this api will simply return it.
> 
> Yep, I definitely agree. It's just that such a design has to please even
> more users than PCI devices, thus will likely take longer to settle than
> the device assignment effort. Therefore I decided to rush forward with
> an intermediate approach first.
> 
> Jan

I think it's easy, will try to do it soon.

> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
Jan Kiszka May 30, 2012, 12:11 p.m. UTC | #10
On 2012-05-21 23:03, Michael S. Tsirkin wrote:
> On Mon, May 21, 2012 at 05:35:34PM -0300, Jan Kiszka wrote:
>> On 2012-05-21 16:05, Michael S. Tsirkin wrote:
>>> On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
>>>> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int level)
>>>>      piix3_set_irq_level(piix3, pirq, level);
>>>>  }
>>>>  
>>>> +static int piix3_map_host_irq(void *opaque, int pci_intx)
>>>> +{
>>>> +    PIIX3State *piix3 = opaque;
>>>> +    int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
>>>> +
>>>> +    return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
>>>> +}
>>>> +
>>>>  /* irq routing is changed. so rebuild bitmap */
>>>>  static void piix3_update_irq_levels(PIIX3State *piix3)
>>>>  {
>>>
>>>
>>> So, instead of special API just for assignment,
>>> I would like to see map_irq in piix being reworked
>>> to take dev config into account.
>>> I think piix is almost unique in this but need to check,
>>
>> Maybe it is the only host bridge with routing that we have in QEMU right
>> now, but it is surely not unique (e.g. all later Intel chipsets support
>> this).
> 
> Yes. APIs for this should be in place. Just saying
> instead of failing we can easily just make it work
> if there are no remappings.
> 
>>> if not fix other host buses that are programmable.
>>> PCI bridges are all fixed routing.
>>>
>>> Then we can drop set_irq callback.
>>
>> set_irq may do more than IRQ routing. It may also flip polarity (see
>> bonito.c).
> 
> In that case host_map_irq is not a good API?
> Need to investigate.
> 
>> I guess we need to analyze the requirements of all in-tree
>> host bridges first.
> 
> Yes.
> 
>>>
>>> Finally all devices can cache the irq#,
>>> and piix would scan devices behind it
>>> and update the irq.
>>>
>>> Assignment then just needs a notifier, since
>>> it owns the device just a pointer in device is
>>> enough.
>>
>> I cannot completely follow your ideas here yet. Do you want to pass the
>> mapping information along the notifier, or why "just ... a notifier"?
> 
> 
> Each device would resolve IRQs that it uses.
> 
> 
>>>
>>> Could you look at doing this please?
>>> If no I can give it a stub.
>>>
>>
>> Unless we can converge over a final API quickly, I would suggest to base
>> these refactorings on top of what I propose here. We will have to touch
>> all host bridges more invasively for this anyway. If you can explain to
>> me how simple the refactoring can be (but I'm a bit skeptical ;) ), I
>> could do this, otherwise I would prefer to focus on the device
>> assignment topic which provide some more nuts.
>>
>> Jan
> 
> Yes it's simple. I'll post in a couple of days (lots of
> meetings tomorrow). I think you'll
> see it's easier and cleaner.

I looked into this again and it appears to me that, in fact, more work
is required to bypass the current interrupt routing on delivery:

The PIIX2 tracks the interrupt level of each device on its bus with the
help of PCIBus::irq_count. On routing changes via its config space,
those levels are currently used to generate the new host IRQ states.
But, once we bypass that level state tracking, we need to do something
else, and that better for _all_ devices: clear all outputs and let the
devices issue an update. The assigned device could provide this based on
the INTx status bit, for all others we need to track the level generically.

Did you start working on this topic already?

Jan
diff mbox

Patch

diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
index 872e112..fc2e4b3 100644
--- a/hw/alpha_typhoon.c
+++ b/hw/alpha_typhoon.c
@@ -764,7 +764,7 @@  PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
                                 &s->pchip.reg_io);
 
     b = pci_register_bus(&s->host.busdev.qdev, "pci",
-                         typhoon_set_irq, sys_map_irq, s,
+                         typhoon_set_irq, sys_map_irq, NULL, s,
                          &s->pchip.reg_mem, addr_space_io, 0, 64);
     s->host.bus = b;
 
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 7e28808..819bf1d 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -366,7 +366,7 @@  PCIBus *pci_apb_init(target_phys_addr_t special_base,
     memory_region_add_subregion(get_system_memory(), mem_base, &d->pci_mmio);
 
     d->bus = pci_register_bus(&d->busdev.qdev, "pci",
-                              pci_apb_set_irq, pci_pbm_map_irq, d,
+                              pci_apb_set_irq, pci_pbm_map_irq, NULL, d,
                               &d->pci_mmio,
                               get_system_io(),
                               0, 32);
diff --git a/hw/bonito.c b/hw/bonito.c
index 77786f8..7ce5993 100644
--- a/hw/bonito.c
+++ b/hw/bonito.c
@@ -750,7 +750,7 @@  PCIBus *bonito_init(qemu_irq *pic)
     dev = qdev_create(NULL, "Bonito-pcihost");
     pcihost = FROM_SYSBUS(BonitoState, sysbus_from_qdev(dev));
     b = pci_register_bus(&pcihost->busdev.qdev, "pci", pci_bonito_set_irq,
-                         pci_bonito_map_irq, pic, get_system_memory(),
+                         pci_bonito_map_irq, NULL, pic, get_system_memory(),
                          get_system_io(),
                          0x28, 32);
     pcihost->bus = b;
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index 81ff3a3..f47d9fe 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -85,6 +85,7 @@  PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
     d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
                                          pci_grackle_set_irq,
                                          pci_grackle_map_irq,
+                                         NULL,
                                          pic,
                                          &d->pci_mmio,
                                          address_space_io,
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index a2d0e5a..a97bbf0 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -1093,7 +1093,7 @@  PCIBus *gt64120_register(qemu_irq *pic)
     d = FROM_SYSBUS(GT64120State, s);
     d->pci.bus = pci_register_bus(&d->busdev.qdev, "pci",
                                   gt64120_pci_set_irq, gt64120_pci_map_irq,
-                                  pic,
+                                  NULL, pic,
                                   get_system_memory(),
                                   get_system_io(),
                                   PCI_DEVFN(18, 0), 4);
diff --git a/hw/pci.c b/hw/pci.c
index 439f3ce..df4d93e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -298,10 +298,11 @@  PCIBus *pci_bus_new(DeviceState *parent, const char *name,
 }
 
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
-                  void *irq_opaque, int nirq)
+                  pci_map_host_irq_fn map_host_irq, void *irq_opaque, int nirq)
 {
     bus->set_irq = set_irq;
     bus->map_irq = map_irq;
+    bus->map_host_irq = map_host_irq;
     bus->irq_opaque = irq_opaque;
     bus->nirq = nirq;
     bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
@@ -316,7 +317,7 @@  void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
 
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
-                         void *irq_opaque,
+                         pci_map_host_irq_fn map_host_irq, void *irq_opaque,
                          MemoryRegion *address_space_mem,
                          MemoryRegion *address_space_io,
                          uint8_t devfn_min, int nirq)
@@ -325,7 +326,7 @@  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
 
     bus = pci_bus_new(parent, name, address_space_mem,
                       address_space_io, devfn_min);
-    pci_bus_irqs(bus, set_irq, map_irq, irq_opaque, nirq);
+    pci_bus_irqs(bus, set_irq, map_irq, map_host_irq, irq_opaque, nirq);
     return bus;
 }
 
@@ -1067,6 +1068,22 @@  static void pci_set_irq(void *opaque, int irq_num, int level)
     pci_change_irq_level(pci_dev, irq_num, change);
 }
 
+int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num)
+{
+    PCIBus *bus;
+
+    for (;;) {
+        bus = pci_dev->bus;
+        irq_num = bus->map_irq(pci_dev, irq_num);
+        if (bus->map_host_irq) {
+            break;
+        }
+        pci_dev = bus->parent_dev;
+        assert(pci_dev);
+    }
+    return bus->map_host_irq(bus->irq_opaque, irq_num);
+}
+
 /***********************************************************/
 /* monitor info on PCI */
 
diff --git a/hw/pci.h b/hw/pci.h
index c3cacce..29bc8bf 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -276,6 +276,7 @@  MemoryRegion *pci_address_space_io(PCIDevice *dev);
 
 typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
 typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
+typedef int (*pci_map_host_irq_fn)(void *opaque, int irq_num);
 
 typedef enum {
     PCI_HOTPLUG_DISABLED,
@@ -295,15 +296,17 @@  PCIBus *pci_bus_new(DeviceState *parent, const char *name,
                     MemoryRegion *address_space_io,
                     uint8_t devfn_min);
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
-                  void *irq_opaque, int nirq);
+                  pci_map_host_irq_fn map_host_irq, void *irq_opaque,
+                  int nirq);
 int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
 void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
-                         void *irq_opaque,
+                         pci_map_host_irq_fn map_host_irq, void *irq_opaque,
                          MemoryRegion *address_space_mem,
                          MemoryRegion *address_space_io,
                          uint8_t devfn_min, int nirq);
+int pci_device_get_host_irq(PCIDevice *pci_dev, int irq_num);
 void pci_device_reset(PCIDevice *dev);
 void pci_bus_reset(PCIBus *bus);
 
diff --git a/hw/pci_internals.h b/hw/pci_internals.h
index 96690b7..a92353e 100644
--- a/hw/pci_internals.h
+++ b/hw/pci_internals.h
@@ -19,6 +19,7 @@  struct PCIBus {
     uint8_t devfn_min;
     pci_set_irq_fn set_irq;
     pci_map_irq_fn map_irq;
+    pci_map_host_irq_fn map_host_irq;
     pci_hotplug_fn hotplug;
     DeviceState *hotplug_qdev;
     void *irq_opaque;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 09e84f5..cfea97c 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -89,6 +89,7 @@  struct PCII440FXState {
 #define I440FX_SMRAM    0x72
 
 static void piix3_set_irq(void *opaque, int pirq, int level);
+static int piix3_map_host_irq(void *opaque, int pci_intx);
 static void piix3_write_config_xen(PCIDevice *dev,
                                uint32_t address, uint32_t val, int len);
 
@@ -308,13 +309,13 @@  static PCIBus *i440fx_common_init(const char *device_name,
     if (xen_enabled()) {
         piix3 = DO_UPCAST(PIIX3State, dev,
                 pci_create_simple_multifunction(b, -1, true, "PIIX3-xen"));
-        pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+        pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq, NULL,
                 piix3, XEN_PIIX_NUM_PIRQS);
     } else {
         piix3 = DO_UPCAST(PIIX3State, dev,
                 pci_create_simple_multifunction(b, -1, true, "PIIX3"));
-        pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
-                PIIX_NUM_PIRQS);
+        pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3_map_host_irq,
+                     piix3, PIIX_NUM_PIRQS);
     }
     piix3->pic = pic;
     *isa_bus = DO_UPCAST(ISABus, qbus,
@@ -386,6 +387,14 @@  static void piix3_set_irq(void *opaque, int pirq, int level)
     piix3_set_irq_level(piix3, pirq, level);
 }
 
+static int piix3_map_host_irq(void *opaque, int pci_intx)
+{
+    PIIX3State *piix3 = opaque;
+    int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
+
+    return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
+}
+
 /* irq routing is changed. so rebuild bitmap */
 static void piix3_update_irq_levels(PIIX3State *piix3)
 {
diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
index 203c3cd..224c4a0 100644
--- a/hw/ppc4xx_pci.c
+++ b/hw/ppc4xx_pci.c
@@ -343,7 +343,7 @@  static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
     }
 
     b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, ppc4xx_pci_set_irq,
-                         ppc4xx_pci_map_irq, s->irq, get_system_memory(),
+                         ppc4xx_pci_map_irq, NULL, s->irq, get_system_memory(),
                          get_system_io(), 0, 4);
     s->pci_state.bus = b;
 
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 0f60b24..dd924ae 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -318,7 +318,7 @@  static int e500_pcihost_initfn(SysBusDevice *dev)
     }
 
     b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
-                         mpc85xx_pci_map_irq, s->irq, address_space_mem,
+                         mpc85xx_pci_map_irq, NULL, s->irq, address_space_mem,
                          address_space_io, PCI_DEVFN(0x11, 0), 4);
     s->pci_state.bus = b;
 
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 38dbff4..9d7bec7 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -108,7 +108,7 @@  static int raven_pcihost_init(SysBusDevice *dev)
     }
 
     bus = pci_register_bus(&h->busdev.qdev, NULL,
-                           prep_set_irq, prep_map_irq, s->irq,
+                           prep_set_irq, prep_map_irq, NULL, s->irq,
                            address_space_mem, address_space_io, 0, 4);
     h->bus = bus;
 
diff --git a/hw/sh_pci.c b/hw/sh_pci.c
index 0cfac46..1cea12b 100644
--- a/hw/sh_pci.c
+++ b/hw/sh_pci.c
@@ -120,7 +120,7 @@  static int sh_pci_device_init(SysBusDevice *dev)
         sysbus_init_irq(dev, &s->irq[i]);
     }
     s->bus = pci_register_bus(&s->busdev.qdev, "pci",
-                              sh_pci_set_irq, sh_pci_map_irq,
+                              sh_pci_set_irq, sh_pci_map_irq, NULL,
                               s->irq,
                               get_system_memory(),
                               get_system_io(),
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index 25b400a..4a43062 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -306,7 +306,7 @@  static int spapr_phb_init(SysBusDevice *s)
 
     bus = pci_register_bus(&phb->busdev.qdev,
                            phb->busname ? phb->busname : phb->dtbusname,
-                           pci_spapr_set_irq, pci_spapr_map_irq, phb,
+                           pci_spapr_set_irq, pci_spapr_map_irq, NULL, phb,
                            &phb->memspace, &phb->iospace,
                            PCI_DEVFN(0, 0), PCI_NUM_PINS);
     phb->host_state.bus = bus;
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 409bcd4..056e3bc 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -227,7 +227,7 @@  PCIBus *pci_pmac_init(qemu_irq *pic,
 
     d->host_state.bus = pci_register_bus(dev, "pci",
                                          pci_unin_set_irq, pci_unin_map_irq,
-                                         pic,
+                                         NULL, pic,
                                          &d->pci_mmio,
                                          address_space_io,
                                          PCI_DEVFN(11, 0), 4);
@@ -293,7 +293,7 @@  PCIBus *pci_pmac_u3_init(qemu_irq *pic,
 
     d->host_state.bus = pci_register_bus(dev, "pci",
                                          pci_unin_set_irq, pci_unin_map_irq,
-                                         pic,
+                                         NULL, pic,
                                          &d->pci_mmio,
                                          address_space_io,
                                          PCI_DEVFN(11, 0), 4);
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index ae53a8b..90c400e 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -68,7 +68,7 @@  static int pci_vpb_init(SysBusDevice *dev)
         sysbus_init_irq(dev, &s->irq[i]);
     }
     bus = pci_register_bus(&dev->qdev, "pci",
-                           pci_vpb_set_irq, pci_vpb_map_irq, s->irq,
+                           pci_vpb_set_irq, pci_vpb_map_irq, NULL, s->irq,
                            get_system_memory(), get_system_io(),
                            PCI_DEVFN(11, 0), 4);