[Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Marcel Apfelbaum]

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.

Replaced the call to qemu_set_irq with a new wrapper
pci_set_irq which triggers the irq based on PCI_INTERRUPT_PIN.

Marcel Apfelbaum (3):
  hw/pci: set irq without selecting INTx pin
  hw/pci-bridge: set PCI_INTERRUPT_PIN register before shpc init
  hw: assert/deassert interrupts using pci_set_irq wrapper

 hw/audio/ac97.c                | 4 ++--
 hw/audio/es1370.c              | 2 +-
 hw/audio/intel-hda.c           | 2 +-
 hw/char/serial-pci.c           | 2 +-
 hw/char/tpci200.c              | 4 ++--
 hw/display/qxl.c               | 2 +-
 hw/ide/cmd646.c                | 2 +-
 hw/isa/vt82c686.c              | 2 +-
 hw/misc/ivshmem.c              | 2 +-
 hw/net/e1000.c                 | 2 +-
 hw/net/rtl8139.c               | 2 +-
 hw/pci-bridge/pci_bridge_dev.c | 2 +-
 hw/pci/pci.c                   | 6 +++---
 hw/pci/shpc.c                  | 2 +-
 hw/scsi/lsi53c895a.c           | 2 +-
 hw/scsi/vmw_pvscsi.c           | 2 +-
 hw/virtio/virtio-pci.c         | 4 ++--
 include/hw/pci/pci.h           | 7 +++++++
 18 files changed, 29 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] hw/pci: set irq without selecting INTx pin

[Marcel Apfelbaum]

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.

Replaced the call to qemu_set_irq with a new wrapper
pci_set_irq which triggers the irq based on PCI_INTERRUPT_PIN.

Renamed a static method which was named already pci_set_irq.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci/pci.c         | 6 +++---
 include/hw/pci/pci.h | 7 +++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 1f4e707..f16f4de 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);
 
@@ -975,7 +975,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;
 }
@@ -1292,7 +1292,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;
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d69e06d..0d4521c 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -634,6 +634,13 @@ 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);
 
+static inline void pci_set_irq(PCIDevice *pci_dev, int level)
+{
+    uint8_t intx = pci_get_byte(pci_dev->config + PCI_INTERRUPT_PIN) - 1;
+
+    qemu_set_irq(pci_dev->irq[intx], level);
+}
+
 static inline int pci_is_express(const PCIDevice *d)
 {
     return d->cap_present & QEMU_PCI_CAP_EXPRESS;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] hw/pci-bridge: set PCI_INTERRUPT_PIN register before shpc init

[Marcel Apfelbaum]

The PCI_INTERRUPT_PIN will be used by shpc init, so
was moved before the call to shpc_init.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/pci-bridge/pci_bridge_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index a9392c7..440e187 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
     if (err) {
         goto bridge_error;
     }
+    dev->config[PCI_INTERRUPT_PIN] = 0x1;
     memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", shpc_bar_size(dev));
     err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
     if (err) {
@@ -73,7 +74,6 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
      * Check whether that works well. */
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
 		     PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
-    dev->config[PCI_INTERRUPT_PIN] = 0x1;
     return 0;
 msi_error:
     slotid_cap_cleanup(dev);
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] hw: assert/deassert interrupts using pci_set_irq wrapper

[Marcel Apfelbaum]

pci_set_irq uses PCI_INTERRUPT_PIN config register
to compute device INTx pin to assert/deassert.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/audio/ac97.c        | 4 ++--
 hw/audio/es1370.c      | 2 +-
 hw/audio/intel-hda.c   | 2 +-
 hw/char/serial-pci.c   | 2 +-
 hw/char/tpci200.c      | 4 ++--
 hw/display/qxl.c       | 2 +-
 hw/ide/cmd646.c        | 2 +-
 hw/isa/vt82c686.c      | 2 +-
 hw/misc/ivshmem.c      | 2 +-
 hw/net/e1000.c         | 2 +-
 hw/net/rtl8139.c       | 2 +-
 hw/pci/shpc.c          | 2 +-
 hw/scsi/lsi53c895a.c   | 2 +-
 hw/scsi/vmw_pvscsi.c   | 2 +-
 hw/virtio/virtio-pci.c | 4 ++--
 15 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/hw/audio/ac97.c b/hw/audio/ac97.c
index 01b4dfb..1f94b19 100644
--- a/hw/audio/ac97.c
+++ b/hw/audio/ac97.c
@@ -280,12 +280,12 @@ static void update_sr (AC97LinkState *s, AC97BusMasterRegs *r, uint32_t new_sr)
     if (level) {
         s->glob_sta |= masks[r - s->bm_regs];
         dolog ("set irq level=1\n");
-        qemu_set_irq (s->dev.irq[0], 1);
+        pci_set_irq(&s->dev, 1);
     }
     else {
         s->glob_sta &= ~masks[r - s->bm_regs];
         dolog ("set irq level=0\n");
-        qemu_set_irq (s->dev.irq[0], 0);
+        pci_set_irq(&s->dev, 0);
     }
 }
 
diff --git a/hw/audio/es1370.c b/hw/audio/es1370.c
index adb66ce..a6fbe9b 100644
--- a/hw/audio/es1370.c
+++ b/hw/audio/es1370.c
@@ -323,7 +323,7 @@ static void es1370_update_status (ES1370State *s, uint32_t new_status)
     else {
         s->status = new_status & ~STAT_INTR;
     }
-    qemu_set_irq (s->dev.irq[0], !!level);
+    pci_set_irq(&s->dev, !!level);
 }
 
 static void es1370_reset (ES1370State *s)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index a6666c6..4327264 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -269,7 +269,7 @@ static void intel_hda_update_irq(IntelHDAState *d)
             msi_notify(&d->pci, 0);
         }
     } else {
-        qemu_set_irq(d->pci.irq[0], level);
+        pci_set_irq(&d->pci, level);
     }
 }
 
diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index aec6705..32987bc 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -79,7 +79,7 @@ static void multi_serial_irq_mux(void *opaque, int n, int level)
             pending = 1;
         }
     }
-    qemu_set_irq(pci->dev.irq[0], pending);
+    pci_set_irq(&pci->dev, pending);
 }
 
 static int multi_serial_pci_init(PCIDevice *dev)
diff --git a/hw/char/tpci200.c b/hw/char/tpci200.c
index e04ff26..5d0ced3 100644
--- a/hw/char/tpci200.c
+++ b/hw/char/tpci200.c
@@ -134,8 +134,8 @@ static void tpci200_set_irq(void *opaque, int intno, int level)
     /* Check if the interrupt is edge sensitive */
     if (dev->ctrl[ip_n] & CTRL_INT_EDGE(intno)) {
         if (level) {
-            qemu_set_irq(dev->dev.irq[0], !dev->int_set);
-            qemu_set_irq(dev->dev.irq[0],  dev->int_set);
+            pci_set_irq(&dev->dev, !dev->int_set);
+            pci_set_irq(&dev->dev,  dev->int_set);
         }
     } else {
         unsigned i, j;
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index ee2db0d..678ad38 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1101,7 +1101,7 @@ static void qxl_update_irq(PCIQXLDevice *d)
     uint32_t pending = le32_to_cpu(d->ram->int_pending);
     uint32_t mask    = le32_to_cpu(d->ram->int_mask);
     int level = !!(pending & mask);
-    qemu_set_irq(d->pci.irq[0], level);
+    pci_set_irq(&d->pci, level);
     qxl_ring_set_dirty(d);
 }
 
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 0500a7a..a8e35fe 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -230,7 +230,7 @@ static void cmd646_update_irq(PCIIDEState *d)
                  !(pd->config[MRDMODE] & MRDMODE_BLK_CH0)) ||
         ((pd->config[MRDMODE] & MRDMODE_INTR_CH1) &&
          !(pd->config[MRDMODE] & MRDMODE_BLK_CH1));
-    qemu_set_irq(pd->irq[0], pci_level);
+    pci_set_irq(pd, pci_level);
 }
 
 /* the PCI irq level is the logical OR of the two channels */
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8fe4fcb..5fb8086 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -185,7 +185,7 @@ static void pm_update_sci(VT686PMState *s)
                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
                    ACPI_BITMASK_TIMER_ENABLE)) != 0);
-    qemu_set_irq(s->dev.irq[0], sci_level);
+    pci_set_irq(&s->dev, sci_level);
     /* schedule a timer interruption if needed */
     acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
                        !(pmsts & ACPI_BITMASK_TIMER_STATUS));
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 2838866..8d144ba 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -133,7 +133,7 @@ static void ivshmem_update_irq(IVShmemState *s, int val)
            isr ? 1 : 0, s->intrstatus, s->intrmask);
     }
 
-    qemu_set_irq(d->irq[0], (isr != 0));
+    pci_set_irq(d, (isr != 0));
 }
 
 static void ivshmem_IntrMask_write(IVShmemState *s, uint32_t val)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 151d25e..c497bad 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -328,7 +328,7 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val)
     }
 
     s->mit_irq_level = (pending_ints != 0);
-    qemu_set_irq(d->irq[0], s->mit_irq_level);
+    pci_set_irq(d, s->mit_irq_level);
 }
 
 static void
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index c31199f..7d72b21 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -716,7 +716,7 @@ static void rtl8139_update_irq(RTL8139State *s)
     DPRINTF("Set IRQ to %d (%04x %04x)\n", isr ? 1 : 0, s->IntrStatus,
         s->IntrMask);
 
-    qemu_set_irq(d->irq[0], (isr != 0));
+    pci_set_irq(d, (isr != 0));
 }
 
 static int rtl8139_RxWrap(RTL8139State *s)
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index eb092fd..0bbd36e 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -172,7 +172,7 @@ static void shpc_interrupt_update(PCIDevice *d)
     if (msi_enabled(d) && shpc->msi_requested != level)
         msi_notify(d, 0);
     else
-        qemu_set_irq(d->irq[0], level);
+        pci_set_irq(d, level);
     shpc->msi_requested = level;
 }
 
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 36e5f50..cb30414 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -437,7 +437,7 @@ static void lsi_update_irq(LSIState *s)
                 level, s->dstat, s->sist1, s->sist0);
         last_level = level;
     }
-    qemu_set_irq(d->irq[0], level);
+    pci_set_irq(d, level);
 
     if (!level && lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON)) {
         DPRINTF("Handled IRQs & disconnected, looking for pending "
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 819d671..94b328f 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -330,7 +330,7 @@ pvscsi_update_irq_status(PVSCSIState *s)
         return;
     }
 
-    qemu_set_irq(d->irq[0], !!should_raise);
+    pci_set_irq(d, !!should_raise);
 }
 
 static void
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 4825802..6b84c4a 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -116,7 +116,7 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector)
     if (msix_enabled(&proxy->pci_dev))
         msix_notify(&proxy->pci_dev, vector);
     else
-        qemu_set_irq(proxy->pci_dev.irq[0], proxy->vdev->isr & 1);
+        pci_set_irq(&proxy->pci_dev, proxy->vdev->isr & 1);
 }
 
 static void virtio_pci_save_config(DeviceState *d, QEMUFile *f)
@@ -362,7 +362,7 @@ static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr)
         /* reading from the ISR also clears it. */
         ret = vdev->isr;
         vdev->isr = 0;
-        qemu_set_irq(proxy->pci_dev.irq[0], 0);
+        pci_set_irq(&proxy->pci_dev, 0);
         break;
     case VIRTIO_MSI_CONFIG_VECTOR:
         ret = vdev->config_vector;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Michael S. Tsirkin]

On Sun, Sep 29, 2013 at 05:40:54PM +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.
> 
> Replaced the call to qemu_set_irq with a new wrapper
> pci_set_irq which triggers the irq based on PCI_INTERRUPT_PIN.

Looks good overall.
As a next step, can we make pci_set_irq non-inline and make
it call pci_irq_handler directly, and get rid of the irq field?

This way we know no one is using it directly ...

> Marcel Apfelbaum (3):
>   hw/pci: set irq without selecting INTx pin
>   hw/pci-bridge: set PCI_INTERRUPT_PIN register before shpc init
>   hw: assert/deassert interrupts using pci_set_irq wrapper
> 
>  hw/audio/ac97.c                | 4 ++--
>  hw/audio/es1370.c              | 2 +-
>  hw/audio/intel-hda.c           | 2 +-
>  hw/char/serial-pci.c           | 2 +-
>  hw/char/tpci200.c              | 4 ++--
>  hw/display/qxl.c               | 2 +-
>  hw/ide/cmd646.c                | 2 +-
>  hw/isa/vt82c686.c              | 2 +-
>  hw/misc/ivshmem.c              | 2 +-
>  hw/net/e1000.c                 | 2 +-
>  hw/net/rtl8139.c               | 2 +-
>  hw/pci-bridge/pci_bridge_dev.c | 2 +-
>  hw/pci/pci.c                   | 6 +++---
>  hw/pci/shpc.c                  | 2 +-
>  hw/scsi/lsi53c895a.c           | 2 +-
>  hw/scsi/vmw_pvscsi.c           | 2 +-
>  hw/virtio/virtio-pci.c         | 4 ++--
>  include/hw/pci/pci.h           | 7 +++++++
>  18 files changed, 29 insertions(+), 22 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Marcel Apfelbaum]

removed addresses:
av1474@comtv.ru
paul@codesourcery.com


On Sun, 2013-09-29 at 18:06 +0300, Michael S. Tsirkin wrote:
> On Sun, Sep 29, 2013 at 05:40:54PM +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.
> > 
> > Replaced the call to qemu_set_irq with a new wrapper
> > pci_set_irq which triggers the irq based on PCI_INTERRUPT_PIN.
> 
> Looks good overall.
> As a next step, can we make pci_set_irq non-inline and make
> it call pci_irq_handler directly, and get rid of the irq field?
OK, I hope it will not affect performance.

Thanks,
Marcel

> 
> This way we know no one is using it directly ...
> 
> > Marcel Apfelbaum (3):
> >   hw/pci: set irq without selecting INTx pin
> >   hw/pci-bridge: set PCI_INTERRUPT_PIN register before shpc init
> >   hw: assert/deassert interrupts using pci_set_irq wrapper
> > 
> >  hw/audio/ac97.c                | 4 ++--
> >  hw/audio/es1370.c              | 2 +-
> >  hw/audio/intel-hda.c           | 2 +-
> >  hw/char/serial-pci.c           | 2 +-
> >  hw/char/tpci200.c              | 4 ++--
> >  hw/display/qxl.c               | 2 +-
> >  hw/ide/cmd646.c                | 2 +-
> >  hw/isa/vt82c686.c              | 2 +-
> >  hw/misc/ivshmem.c              | 2 +-
> >  hw/net/e1000.c                 | 2 +-
> >  hw/net/rtl8139.c               | 2 +-
> >  hw/pci-bridge/pci_bridge_dev.c | 2 +-
> >  hw/pci/pci.c                   | 6 +++---
> >  hw/pci/shpc.c                  | 2 +-
> >  hw/scsi/lsi53c895a.c           | 2 +-
> >  hw/scsi/vmw_pvscsi.c           | 2 +-
> >  hw/virtio/virtio-pci.c         | 4 ++--
> >  include/hw/pci/pci.h           | 7 +++++++
> >  18 files changed, 29 insertions(+), 22 deletions(-)
> > 
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Michael S. Tsirkin]

On Sun, Sep 29, 2013 at 06:24:48PM +0300, Marcel Apfelbaum wrote:
> removed addresses:
> av1474@comtv.ru
> paul@codesourcery.com
> 
> 
> On Sun, 2013-09-29 at 18:06 +0300, Michael S. Tsirkin wrote:
> > On Sun, Sep 29, 2013 at 05:40:54PM +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.
> > > 
> > > Replaced the call to qemu_set_irq with a new wrapper
> > > pci_set_irq which triggers the irq based on PCI_INTERRUPT_PIN.
> > 
> > Looks good overall.
> > As a next step, can we make pci_set_irq non-inline and make
> > it call pci_irq_handler directly, and get rid of the irq field?
> OK, I hope it will not affect performance.
> 
> Thanks,
> Marcel

Avoiding call through a pointer is likely to help performance.

> > 
> > This way we know no one is using it directly ...
> > 
> > > Marcel Apfelbaum (3):
> > >   hw/pci: set irq without selecting INTx pin
> > >   hw/pci-bridge: set PCI_INTERRUPT_PIN register before shpc init
> > >   hw: assert/deassert interrupts using pci_set_irq wrapper
> > > 
> > >  hw/audio/ac97.c                | 4 ++--
> > >  hw/audio/es1370.c              | 2 +-
> > >  hw/audio/intel-hda.c           | 2 +-
> > >  hw/char/serial-pci.c           | 2 +-
> > >  hw/char/tpci200.c              | 4 ++--
> > >  hw/display/qxl.c               | 2 +-
> > >  hw/ide/cmd646.c                | 2 +-
> > >  hw/isa/vt82c686.c              | 2 +-
> > >  hw/misc/ivshmem.c              | 2 +-
> > >  hw/net/e1000.c                 | 2 +-
> > >  hw/net/rtl8139.c               | 2 +-
> > >  hw/pci-bridge/pci_bridge_dev.c | 2 +-
> > >  hw/pci/pci.c                   | 6 +++---
> > >  hw/pci/shpc.c                  | 2 +-
> > >  hw/scsi/lsi53c895a.c           | 2 +-
> > >  hw/scsi/vmw_pvscsi.c           | 2 +-
> > >  hw/virtio/virtio-pci.c         | 4 ++--
> > >  include/hw/pci/pci.h           | 7 +++++++
> > >  18 files changed, 29 insertions(+), 22 deletions(-)
> > > 
> > > -- 
> > > 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Marcel Apfelbaum]

On Sun, 2013-09-29 at 18:06 +0300, Michael S. Tsirkin wrote:
> On Sun, Sep 29, 2013 at 05:40:54PM +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.
> > 
> > Replaced the call to qemu_set_irq with a new wrapper
> > pci_set_irq which triggers the irq based on PCI_INTERRUPT_PIN.
> 
> Looks good overall.
> As a next step, can we make pci_set_irq non-inline and make
> it call pci_irq_handler directly, and get rid of the irq field?
What irq field? 

Thanks,
Marcel

> 
> This way we know no one is using it directly ...
> 
> > Marcel Apfelbaum (3):
> >   hw/pci: set irq without selecting INTx pin
> >   hw/pci-bridge: set PCI_INTERRUPT_PIN register before shpc init
> >   hw: assert/deassert interrupts using pci_set_irq wrapper
> > 
> >  hw/audio/ac97.c                | 4 ++--
> >  hw/audio/es1370.c              | 2 +-
> >  hw/audio/intel-hda.c           | 2 +-
> >  hw/char/serial-pci.c           | 2 +-
> >  hw/char/tpci200.c              | 4 ++--
> >  hw/display/qxl.c               | 2 +-
> >  hw/ide/cmd646.c                | 2 +-
> >  hw/isa/vt82c686.c              | 2 +-
> >  hw/misc/ivshmem.c              | 2 +-
> >  hw/net/e1000.c                 | 2 +-
> >  hw/net/rtl8139.c               | 2 +-
> >  hw/pci-bridge/pci_bridge_dev.c | 2 +-
> >  hw/pci/pci.c                   | 6 +++---
> >  hw/pci/shpc.c                  | 2 +-
> >  hw/scsi/lsi53c895a.c           | 2 +-
> >  hw/scsi/vmw_pvscsi.c           | 2 +-
> >  hw/virtio/virtio-pci.c         | 4 ++--
> >  include/hw/pci/pci.h           | 7 +++++++
> >  18 files changed, 29 insertions(+), 22 deletions(-)
> > 
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Michael S. Tsirkin]

On Mon, Sep 30, 2013 at 11:14:56AM +0300, Marcel Apfelbaum wrote:
> On Sun, 2013-09-29 at 18:06 +0300, Michael S. Tsirkin wrote:
> > On Sun, Sep 29, 2013 at 05:40:54PM +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.
> > > 
> > > Replaced the call to qemu_set_irq with a new wrapper
> > > pci_set_irq which triggers the irq based on PCI_INTERRUPT_PIN.
> > 
> > Looks good overall.
> > As a next step, can we make pci_set_irq non-inline and make
> > it call pci_irq_handler directly, and get rid of the irq field?
> What irq field? 

    /* IRQ objects for the INTA-INTD pins.  */
    qemu_irq *irq;

> Thanks,
> Marcel
> 
> > 
> > This way we know no one is using it directly ...
> > 
> > > Marcel Apfelbaum (3):
> > >   hw/pci: set irq without selecting INTx pin
> > >   hw/pci-bridge: set PCI_INTERRUPT_PIN register before shpc init
> > >   hw: assert/deassert interrupts using pci_set_irq wrapper
> > > 
> > >  hw/audio/ac97.c                | 4 ++--
> > >  hw/audio/es1370.c              | 2 +-
> > >  hw/audio/intel-hda.c           | 2 +-
> > >  hw/char/serial-pci.c           | 2 +-
> > >  hw/char/tpci200.c              | 4 ++--
> > >  hw/display/qxl.c               | 2 +-
> > >  hw/ide/cmd646.c                | 2 +-
> > >  hw/isa/vt82c686.c              | 2 +-
> > >  hw/misc/ivshmem.c              | 2 +-
> > >  hw/net/e1000.c                 | 2 +-
> > >  hw/net/rtl8139.c               | 2 +-
> > >  hw/pci-bridge/pci_bridge_dev.c | 2 +-
> > >  hw/pci/pci.c                   | 6 +++---
> > >  hw/pci/shpc.c                  | 2 +-
> > >  hw/scsi/lsi53c895a.c           | 2 +-
> > >  hw/scsi/vmw_pvscsi.c           | 2 +-
> > >  hw/virtio/virtio-pci.c         | 4 ++--
> > >  include/hw/pci/pci.h           | 7 +++++++
> > >  18 files changed, 29 insertions(+), 22 deletions(-)
> > > 
> > > -- 
> > > 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Paolo Bonzini]

Il 30/09/2013 10:58, Michael S. Tsirkin ha scritto:
>>> > > As a next step, can we make pci_set_irq non-inline and make
>>> > > it call pci_irq_handler directly, and get rid of the irq field?
>> > What irq field? 
>     /* IRQ objects for the INTA-INTD pins.  */
>     qemu_irq *irq;
> 

That's still used by devices that use common code for PCI and sysbus
versions (e.g. USB OHCI and EHCI).

Paolo

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Michael S. Tsirkin]

On Mon, Sep 30, 2013 at 11:02:06AM +0200, Paolo Bonzini wrote:
> Il 30/09/2013 10:58, Michael S. Tsirkin ha scritto:
> >>> > > As a next step, can we make pci_set_irq non-inline and make
> >>> > > it call pci_irq_handler directly, and get rid of the irq field?
> >> > What irq field? 
> >     /* IRQ objects for the INTA-INTD pins.  */
> >     qemu_irq *irq;
> > 
> 
> That's still used by devices that use common code for PCI and sysbus
> versions (e.g. USB OHCI and EHCI).
> 
> Paolo

Well this work wouldn't be complete without
addressing them anyway.

These devices would have to create their own
irq in pci-specific code, along the lines of:

- s->irq = dev->irq[3];
+ s->irq = qemu_allocate_irqs(pci_set_irq, dev, 1);


If there's more than one device like this, we should add

/* Return an irq that calls pci_set_irq internally */
qemu_irq *pci_allocate_irq(PCIDevice *);

-- 
MST

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Marcel Apfelbaum]

On Mon, 2013-09-30 at 12:14 +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 30, 2013 at 11:02:06AM +0200, Paolo Bonzini wrote:
> > Il 30/09/2013 10:58, Michael S. Tsirkin ha scritto:
> > >>> > > As a next step, can we make pci_set_irq non-inline and make
> > >>> > > it call pci_irq_handler directly, and get rid of the irq field?
> > >> > What irq field? 
> > >     /* IRQ objects for the INTA-INTD pins.  */
> > >     qemu_irq *irq;
> > > 
> > 
> > That's still used by devices that use common code for PCI and sysbus
> > versions (e.g. USB OHCI and EHCI).
> > 
> > Paolo
> 
> Well this work wouldn't be complete without
> addressing them anyway.
> 
> These devices would have to create their own
> irq in pci-specific code, along the lines of:

This irq field is used also in places where pci_set_irq(PCIDevice dev, level)
can't infer the INTx:
- PCIExpress: qemu_set_irq(dev->irq[dev->exp.hpev_intx],dev->exp.hpev_notified);
- vmxnet3 device: qemu_set_irq(d->irq[int_idx], 1);

What approach should be used here?

Thanks,
Marcel

> - s->irq = dev->irq[3];
> + s->irq = qemu_allocate_irqs(pci_set_irq, dev, 1);
> 
> 
> If there's more than one device like this, we should add
> 
> /* Return an irq that calls pci_set_irq internally */
> qemu_irq *pci_allocate_irq(PCIDevice *);
> 

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Michael S. Tsirkin]

On Mon, Sep 30, 2013 at 12:43:20PM +0300, Marcel Apfelbaum wrote:
> On Mon, 2013-09-30 at 12:14 +0300, Michael S. Tsirkin wrote:
> > On Mon, Sep 30, 2013 at 11:02:06AM +0200, Paolo Bonzini wrote:
> > > Il 30/09/2013 10:58, Michael S. Tsirkin ha scritto:
> > > >>> > > As a next step, can we make pci_set_irq non-inline and make
> > > >>> > > it call pci_irq_handler directly, and get rid of the irq field?
> > > >> > What irq field? 
> > > >     /* IRQ objects for the INTA-INTD pins.  */
> > > >     qemu_irq *irq;
> > > > 
> > > 
> > > That's still used by devices that use common code for PCI and sysbus
> > > versions (e.g. USB OHCI and EHCI).
> > > 
> > > Paolo
> > 
> > Well this work wouldn't be complete without
> > addressing them anyway.
> > 
> > These devices would have to create their own
> > irq in pci-specific code, along the lines of:
> 
> This irq field is used also in places where pci_set_irq(PCIDevice dev, level)
> can't infer the INTx:
> - PCIExpress: qemu_set_irq(dev->irq[dev->exp.hpev_intx],dev->exp.hpev_notified);

Well the spec says, explicitly:
6.7.3.4.
 Software Notification of Hot-Plug Events
...
Note that all other interrupt sources within the same Function will
assert the same virtual INTx wire
when requesting service.

I read this to mean that this is a bug,
and it should simply use pci_set_irq like all other
devices.

> - vmxnet3 device: qemu_set_irq(d->irq[int_idx], 1);
> 
> What approach should be used here?
> 
> Thanks,
> Marcel
> > - s->irq = dev->irq[3];
> > + s->irq = qemu_allocate_irqs(pci_set_irq, dev, 1);
> > 
> > 
> > If there's more than one device like this, we should add
> > 
> > /* Return an irq that calls pci_set_irq internally */
> > qemu_irq *pci_allocate_irq(PCIDevice *);
> > 
> 
> 

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Marcel Apfelbaum]

On Mon, 2013-09-30 at 13:10 +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 30, 2013 at 12:43:20PM +0300, Marcel Apfelbaum wrote:
> > On Mon, 2013-09-30 at 12:14 +0300, Michael S. Tsirkin wrote:
> > > On Mon, Sep 30, 2013 at 11:02:06AM +0200, Paolo Bonzini wrote:
> > > > Il 30/09/2013 10:58, Michael S. Tsirkin ha scritto:
> > > > >>> > > As a next step, can we make pci_set_irq non-inline and make
> > > > >>> > > it call pci_irq_handler directly, and get rid of the irq field?
> > > > >> > What irq field? 
> > > > >     /* IRQ objects for the INTA-INTD pins.  */
> > > > >     qemu_irq *irq;
> > > > > 
> > > > 
> > > > That's still used by devices that use common code for PCI and sysbus
> > > > versions (e.g. USB OHCI and EHCI).
> > > > 
> > > > Paolo
> > > 
> > > Well this work wouldn't be complete without
> > > addressing them anyway.
> > > 
> > > These devices would have to create their own
> > > irq in pci-specific code, along the lines of:
> > 
> > This irq field is used also in places where pci_set_irq(PCIDevice dev, level)
> > can't infer the INTx:
> > - PCIExpress: qemu_set_irq(dev->irq[dev->exp.hpev_intx],dev->exp.hpev_notified);
> 
> Well the spec says, explicitly:
> 6.7.3.4.
>  Software Notification of Hot-Plug Events
> ...
> Note that all other interrupt sources within the same Function will
> assert the same virtual INTx wire
> when requesting service.
> 
> I read this to mean that this is a bug,
> and it should simply use pci_set_irq like all other
> devices.
Thanks!
That means that hpev_intx is not necessary at all.
By the way, aer_intx used by Advanced Error Reporting
is also unnecessary. (6.2.4.1.2 has the same note)

I will remove the above fields from PCIExpressDevice.
Marcel

> > - vmxnet3 device: qemu_set_irq(d->irq[int_idx], 1);
> > 
> > What approach should be used here?
> > 
> > Thanks,
> > Marcel
> > > - s->irq = dev->irq[3];
> > > + s->irq = qemu_allocate_irqs(pci_set_irq, dev, 1);
> > > 
> > > 
> > > If there's more than one device like this, we should add
> > > 
> > > /* Return an irq that calls pci_set_irq internally */
> > > qemu_irq *pci_allocate_irq(PCIDevice *);
> > > 
> > 
> > 

Re: [Qemu-devel] [PATCH 0/3] hw: set irq without selecting INTx pin

[Paolo Bonzini]

Il 30/09/2013 12:39, Marcel Apfelbaum ha scritto:
> Thanks!
> That means that hpev_intx is not necessary at all.
> By the way, aer_intx used by Advanced Error Reporting
> is also unnecessary. (6.2.4.1.2 has the same note)
> 
> I will remove the above fields from PCIExpressDevice.

vmxnet is also buggy and should just use pci_set_irq for INTx.

Paolo