Patchwork [RFC,v2,2/9] hw/pci: add pci wrappers for allocating and asserting irqs

login
register
mail settings
Submitter Marcel Apfelbaum
Date Oct. 2, 2013, 12:41 p.m.
Message ID <1380717694-13091-3-git-send-email-marcel.a@redhat.com>
Download mbox | patch
Permalink /patch/279729/
State New
Headers show

Comments

Marcel Apfelbaum - Oct. 2, 2013, 12:41 p.m.
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(-)
Michael S. Tsirkin - Oct. 2, 2013, 12:54 p.m.
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
Marcel Apfelbaum - Oct. 2, 2013, 12:56 p.m.
On Wed, 2013-10-02 at 15:54 +0300, Michael S. Tsirkin wrote:
> 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.
Sure, thanks!
Marcel

> 
> > +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
Paolo Bonzini - Oct. 2, 2013, 3:21 p.m.
Il 02/10/2013 14:41, Marcel Apfelbaum ha scritto:
> +static inline void pci_irq_pulse(PCIDevice *pci_dev)
> +{
> +    pci_irq_lower(pci_dev);
> +    pci_irq_raise(pci_dev);
> +}
> +

Why is this in the opposite order, compared to qemu_irq_pulse?

Paolo
Marcel Apfelbaum - Oct. 2, 2013, 10:03 p.m.
On Wed, 2013-10-02 at 17:21 +0200, Paolo Bonzini wrote:
> Il 02/10/2013 14:41, Marcel Apfelbaum ha scritto:
> > +static inline void pci_irq_pulse(PCIDevice *pci_dev)
> > +{
> > +    pci_irq_lower(pci_dev);
> > +    pci_irq_raise(pci_dev);
> > +}
> > +
> 
> Why is this in the opposite order, compared to qemu_irq_pulse?
It is a bug, thank you for catching it!
Marcel

> 
> Paolo

Patch

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);
+}
+
+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;