Re: [Qemu-devel] [PATCH RFC v2 2/9] hw/pci: add pci wrappers for allocating and asserting irqs

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

On Wed, Oct 02, 2013 at 03:41:27PM +0300, Marcel Apfelbaum wrote:
> Interrupt pin is selected and saved into PCI_INTERRUPT_PIN
> register during device initialization. Devices should not call
> directly qemu_set_irq and specify the INTx pin on each call.
> 
> Added pci_* wrappers to replace qemu_set_irq, qemu_irq_raise,
> qemu_irq_lower and qemu_irq_pulse, setting the irq
> based on PCI_INTERRUPT_PIN.
> 
> Added pci_allocate_irq wrapper to be used by devices that
> still need PCIDevice infrastructure to assert irqs.
> 
> Renamed a static method which was named already pci_set_irq.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> Changes from v1:
>  - Added pci irq wrappers for all qemu methods
>    setting irq and not only qemu_set_irq 
>  - Added pci wrappers to allocate and pci irq
> 
>  hw/pci/pci.c         | 26 ++++++++++++++++++++++----
>  include/hw/pci/pci.h | 19 +++++++++++++++++++
>  2 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 00554a0..fbfd8f7 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -83,7 +83,7 @@ static const TypeInfo pcie_bus_info = {
>  
>  static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num);
>  static void pci_update_mappings(PCIDevice *d);
> -static void pci_set_irq(void *opaque, int irq_num, int level);
> +static void pci_irq_handler(void *opaque, int irq_num, int level);
>  static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom);
>  static void pci_del_option_rom(PCIDevice *pdev);
>  
> @@ -161,7 +161,7 @@ void pci_device_deassert_intx(PCIDevice *dev)
>  {
>      int i;
>      for (i = 0; i < PCI_NUM_PINS; ++i) {
> -        qemu_set_irq(dev->irq[i], 0);
> +        pci_irq_handler(dev, i, 0);
>      }
>  }
>  
> @@ -863,7 +863,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      pci_dev->config_read = config_read;
>      pci_dev->config_write = config_write;
>      bus->devices[devfn] = pci_dev;
> -    pci_dev->irq = qemu_allocate_irqs(pci_set_irq, pci_dev, PCI_NUM_PINS);
> +    pci_dev->irq = qemu_allocate_irqs(pci_irq_handler, pci_dev, PCI_NUM_PINS);
>      pci_dev->version_id = 2; /* Current pci device vmstate version */
>      return pci_dev;
>  }
> @@ -1175,7 +1175,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>  /* generic PCI irq support */
>  
>  /* 0 <= irq_num <= 3. level must be 0 or 1 */
> -static void pci_set_irq(void *opaque, int irq_num, int level)
> +static void pci_irq_handler(void *opaque, int irq_num, int level)
>  {
>      PCIDevice *pci_dev = opaque;
>      int change;
> @@ -1191,6 +1191,24 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
>      pci_change_irq_level(pci_dev, irq_num, change);
>  }
>  
> +static inline int pci_intx(PCIDevice *pci_dev)
> +{
> +    return pci_get_byte(pci_dev->config + PCI_INTERRUPT_PIN) - 1;
> +}
> +
> +qemu_irq pci_allocate_irq(PCIDevice *pci_dev)
> +{
> +    int intx = pci_intx(pci_dev);
> +
> +    return qemu_allocate_irq(pci_irq_handler, pci_dev, intx);
> +}
> +
> +void pci_set_irq(PCIDevice *pci_dev, int level)
> +{
> +    int intx = pci_intx(pci_dev);
> +    pci_irq_handler(pci_dev, intx, level);
> +}
> +
>  /* Special hooks used by device assignment */
>  void pci_bus_set_route_irq_fn(PCIBus *bus, pci_route_irq_fn route_intx_to_irq)
>  {
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 4b90e5d..df7d316 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -632,6 +632,25 @@ PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
>  PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
>  PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
>  
> +qemu_irq pci_allocate_irq(PCIDevice *pci_dev);
> +void pci_set_irq(PCIDevice *pci_dev, int level);
> +
> +static inline void pci_irq_raise(PCIDevice *pci_dev)
> +{
> +    pci_set_irq(pci_dev, 1);
> +}
> +
> +static inline void pci_irq_lower(PCIDevice *pci_dev)
> +{
> +    pci_set_irq(pci_dev, 0);
> +}
> +

I'd like to add that any users of pci_irq_pulse
are immediately suspect. PCI does not work this way.
If you resend this, maybe add a comment that all users
of this should be fixed.

> +static inline void pci_irq_pulse(PCIDevice *pci_dev)
> +{
> +    pci_irq_lower(pci_dev);
> +    pci_irq_raise(pci_dev);
> +}
> +
>  static inline int pci_is_express(const PCIDevice *d)
>  {
>      return d->cap_present & QEMU_PCI_CAP_EXPRESS;
> -- 
> 1.8.3.1