diff mbox

[1/6] pci: untangle pci/msi dependency

Message ID 578a2c45b25f154965c6da15a703646b2d22f57b.1291330353.git.mst@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Dec. 2, 2010, 10:54 p.m. UTC 
msi depends on pci but pci should not depend on msi.
The only dependency we have is a recent addition
of pci_msi_ functions, IMO they add little enough to
open-code in the small number of users.

Follow-up patches add more cleanups.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.c      |   19 -------------------
 hw/pci.h      |    3 ---
 hw/pcie.c     |    8 +++++---
 hw/pcie_aer.c |   24 ++++++++++++++----------
 4 files changed, 19 insertions(+), 35 deletions(-)

Comments

Paolo Bonzini Dec. 4, 2010, 1:35 p.m. UTC | #1 
On 12/02/2010 11:54 PM, Michael S. Tsirkin wrote:
> +        bool assert = pcie_aer_root_does_trigger(root_cmd_set, root_status);

Risky variable name, I think it would fail if someone includes assert.h.

Paolo
Isaku Yamahata Dec. 7, 2010, 7:21 a.m. UTC | #2 
On Fri, Dec 03, 2010 at 12:54:33AM +0200, Michael S. Tsirkin wrote:
> diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
> index 235ac53..18bbd5a 100644
> --- a/hw/pcie_aer.c
> +++ b/hw/pcie_aer.c
> @@ -339,10 +339,10 @@ static bool pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
>  
>      if (root_cmd & msg->severity) {
>          /* 6.2.4.1.2 Interrupt Generation */
> -        if (pci_msi_enabled(dev)) {
> -            if (msi_trigger) {
> -                pci_msi_notify(dev, pcie_aer_root_get_vector(dev));
> -            }
> +        if (msix_enabled(dev)) {
> +            msix_notify(dev, pcie_aer_root_get_vector(dev));
> +        } else if (msi_enabled(dev)) {
> +            msi_notify(dev, pcie_aer_root_get_vector(dev));
>          } else {
>              qemu_set_irq(dev->irq[dev->exp.aer_intx], 1);
>          }

This changes the logic which is fixed by 3/6. It breaks bisecability.
Except this, the eventual result seems correct.
Michael S. Tsirkin Dec. 9, 2010, 9:53 a.m. UTC | #3 
On Sat, Dec 04, 2010 at 02:35:53PM +0100, Paolo Bonzini wrote:
> On 12/02/2010 11:54 PM, Michael S. Tsirkin wrote:
> >+        bool assert = pcie_aer_root_does_trigger(root_cmd_set, root_status);
> 
> Risky variable name, I think it would fail if someone includes assert.h.
> 
> Paolo

We already do, it does not seem to fail.
diff mbox

Patch

diff --git a/hw/pci.c b/hw/pci.c
index ca878e8..254647b 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -25,8 +25,6 @@ 
 #include "pci.h"
 #include "pci_bridge.h"
 #include "pci_internals.h"
-#include "msix.h"
-#include "msi.h"
 #include "monitor.h"
 #include "net.h"
 #include "sysemu.h"
@@ -1099,23 +1097,6 @@  static void pci_set_irq(void *opaque, int irq_num, int level)
     pci_change_irq_level(pci_dev, irq_num, change);
 }
 
-bool pci_msi_enabled(PCIDevice *dev)
-{
-    return msix_enabled(dev) || msi_enabled(dev);
-}
-
-void pci_msi_notify(PCIDevice *dev, unsigned int vector)
-{
-    if (msix_enabled(dev)) {
-        msix_notify(dev, vector);
-    } else if (msi_enabled(dev)) {
-        msi_notify(dev, vector);
-    } else {
-        /* MSI/MSI-X must be enabled */
-        abort();
-    }
-}
-
 /***********************************************************/
 /* monitor info on PCI */
 
diff --git a/hw/pci.h b/hw/pci.h
index 099c251..aa3afe9 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -261,9 +261,6 @@  void do_pci_info_print(Monitor *mon, const QObject *data);
 void do_pci_info(Monitor *mon, QObject **ret_data);
 void pci_bridge_update_mappings(PCIBus *b);
 
-bool pci_msi_enabled(PCIDevice *dev);
-void pci_msi_notify(PCIDevice *dev, unsigned int vector);
-
 static inline void
 pci_set_byte(uint8_t *config, uint8_t val)
 {
diff --git a/hw/pcie.c b/hw/pcie.c
index f461c1c..d1f0086 100644
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -167,10 +167,12 @@  static void hotplug_event_notify(PCIDevice *dev)
      * The Port may optionally send an MSI when there are hot-plug events that
      * occur while interrupt generation is disabled, and interrupt generation is
      * subsequently enabled. */
-    if (!pci_msi_enabled(dev)) {
+    if (msix_enabled(dev)) {
+        msix_notify(dev, pcie_cap_flags_get_vector(dev));
+    } else if (msi_enabled(dev)) {
+        msi_notify(dev, pcie_cap_flags_get_vector(dev));
+    } else {
         qemu_set_irq(dev->irq[dev->exp.hpev_intx], dev->exp.hpev_notified);
-    } else if (dev->exp.hpev_notified) {
-        pci_msi_notify(dev, pcie_cap_flags_get_vector(dev));
     }
 }
 
diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
index 235ac53..18bbd5a 100644
--- a/hw/pcie_aer.c
+++ b/hw/pcie_aer.c
@@ -339,10 +339,10 @@  static bool pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
 
     if (root_cmd & msg->severity) {
         /* 6.2.4.1.2 Interrupt Generation */
-        if (pci_msi_enabled(dev)) {
-            if (msi_trigger) {
-                pci_msi_notify(dev, pcie_aer_root_get_vector(dev));
-            }
+        if (msix_enabled(dev)) {
+            msix_notify(dev, pcie_aer_root_get_vector(dev));
+        } else if (msi_enabled(dev)) {
+            msi_notify(dev, pcie_aer_root_get_vector(dev));
         } else {
             qemu_set_irq(dev->irq[dev->exp.aer_intx], 1);
         }
@@ -761,16 +761,20 @@  void pcie_aer_root_write_config(PCIDevice *dev,
         /* 6.2.4.1.2 Interrupt Generation */
 
         /* 0 -> 1 */
-        uint32_t root_cmd_set = (root_cmd_prev ^ root_cmd) & root_cmd;
+        uint32_t root_cmd_set = ~root_cmd_prev & root_cmd;
         uint32_t root_status = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
+        bool assert = pcie_aer_root_does_trigger(root_cmd_set, root_status);
 
-        if (pci_msi_enabled(dev)) {
-            if (pcie_aer_root_does_trigger(root_cmd_set, root_status)) {
-                pci_msi_notify(dev, pcie_aer_root_get_vector(dev));
+        if (msix_enabled(dev)) {
+            if (assert) {
+                msix_notify(dev, pcie_aer_root_get_vector(dev));
+            }
+        } else if (msi_enabled(dev)) {
+            if (assert) {
+                msi_notify(dev, pcie_aer_root_get_vector(dev));
             }
         } else {
-            int int_level = pcie_aer_root_does_trigger(root_cmd, root_status);
-            qemu_set_irq(dev->irq[dev->exp.aer_intx], int_level);
+            qemu_set_irq(dev->irq[dev->exp.aer_intx], assert);
         }
     }
 }