[Qemu-devel] [PATCH 0/6] pcie aer fixes

[Qemu-devel] [PATCH 0/6] pcie aer fixes

[Michael S. Tsirkin]

Here are a bunch of fixes and cleanups to aer interrupt injection.
Compile tested only, issues were found by reading the
code and spec.

I put these on my pci branch for ease of testing, but can
back them out if needed.

Pls review and test.

Thanks!

Michael S. Tsirkin (6):
  pci: untangle pci/msi dependency
  Makefile: make msix/msi depend on CONFIG_PCI
  pci/aer: remove dead code
  pci/aer: fix error injection
  pci/aer: fix interrupt on config write
  pci/aer: factor out common code

 Makefile.objs |    3 +-
 hw/pci.c      |   19 ----------
 hw/pci.h      |    3 --
 hw/pcie.c     |    8 +++--
 hw/pcie_aer.c |  103 +++++++++++++++++++++++++++++----------------------------
 5 files changed, 59 insertions(+), 77 deletions(-)

-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH 1/6] pci: untangle pci/msi dependency

[Michael S. Tsirkin]

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(-)

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);
         }
     }
 }
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH 3/6] pci/aer: remove dead code

[Michael S. Tsirkin]

Remove some unused variables and return values.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pcie_aer.c |   15 +--------------
 1 files changed, 1 insertions(+), 14 deletions(-)

diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
index 18bbd5a..204155b 100644
--- a/hw/pcie_aer.c
+++ b/hw/pcie_aer.c
@@ -258,29 +258,21 @@ static unsigned int pcie_aer_root_get_vector(PCIDevice *dev)
 }
 
 /*
- * return value:
- * true: error message is sent up
- * false: error message is masked
- *
  * 6.2.6 Error Message Control
  * Figure 6-3
  * root port part
  */
-static bool pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
+static void pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
 {
-    bool msg_sent;
     uint16_t cmd;
     uint8_t *aer_cap;
     uint32_t root_cmd;
     uint32_t root_status;
-    bool msi_trigger;
 
-    msg_sent = false;
     cmd = pci_get_word(dev->config + PCI_COMMAND);
     aer_cap = dev->config + dev->exp.aer_cap;
     root_cmd = pci_get_long(aer_cap + PCI_ERR_ROOT_COMMAND);
     root_status = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
-    msi_trigger = false;
 
     if (cmd & PCI_COMMAND_SERR) {
         /* System Error.
@@ -300,7 +292,6 @@ static bool pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
             root_status |= PCI_ERR_ROOT_MULTI_COR_RCV;
         } else {
             if (root_cmd & PCI_ERR_ROOT_CMD_COR_EN) {
-                msi_trigger = true;
             }
             pci_set_word(aer_cap + PCI_ERR_ROOT_COR_SRC, msg->source_id);
         }
@@ -309,14 +300,12 @@ static bool pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
     case PCI_ERR_ROOT_CMD_NONFATAL_EN:
         if (!(root_status & PCI_ERR_ROOT_NONFATAL_RCV) &&
             root_cmd & PCI_ERR_ROOT_CMD_NONFATAL_EN) {
-            msi_trigger = true;
         }
         root_status |= PCI_ERR_ROOT_NONFATAL_RCV;
         break;
     case PCI_ERR_ROOT_CMD_FATAL_EN:
         if (!(root_status & PCI_ERR_ROOT_FATAL_RCV) &&
             root_cmd & PCI_ERR_ROOT_CMD_FATAL_EN) {
-            msi_trigger = true;
         }
         if (!(root_status & PCI_ERR_ROOT_UNCOR_RCV)) {
             root_status |= PCI_ERR_ROOT_FIRST_FATAL;
@@ -346,9 +335,7 @@ static bool pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
         } else {
             qemu_set_irq(dev->irq[dev->exp.aer_intx], 1);
         }
-        msg_sent = true;
     }
-    return msg_sent;
 }
 
 /*
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH 4/6] pci/aer: fix error injection

[Michael S. Tsirkin]

Fix the injection logic upon aer message to follow 6.2.4.1.2 more
closely: specifically only send an msi interrupt when the logical or of
the enabled bits changed, not when a bit which was previously clear
becomes set.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pcie_aer.c |   45 ++++++++++++++++++++++++++++++++++-----------
 1 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
index 204155b..0fc191f 100644
--- a/hw/pcie_aer.c
+++ b/hw/pcie_aer.c
@@ -257,6 +257,22 @@ static unsigned int pcie_aer_root_get_vector(PCIDevice *dev)
     return (root_status & PCI_ERR_ROOT_IRQ) >> PCI_ERR_ROOT_IRQ_SHIFT;
 }
 
+/* Given a status register, get corresponding bits in the command register */
+static uint32_t pcie_aer_status_to_cmd(uint32_t status)
+{
+    uint32_t cmd = 0;
+    if (status & PCI_ERR_ROOT_COR_RCV) {
+        cmd |= PCI_ERR_ROOT_CMD_COR_EN;
+    }
+    if (status & PCI_ERR_ROOT_NONFATAL_RCV) {
+        cmd |= PCI_ERR_ROOT_CMD_NONFATAL_EN;
+    }
+    if (status & PCI_ERR_ROOT_FATAL_RCV) {
+        cmd |= PCI_ERR_ROOT_CMD_FATAL_EN;
+    }
+    return cmd;
+}
+
 /*
  * 6.2.6 Error Message Control
  * Figure 6-3
@@ -267,12 +283,12 @@ static void pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
     uint16_t cmd;
     uint8_t *aer_cap;
     uint32_t root_cmd;
-    uint32_t root_status;
+    uint32_t root_status, prev_status;
 
     cmd = pci_get_word(dev->config + PCI_COMMAND);
     aer_cap = dev->config + dev->exp.aer_cap;
     root_cmd = pci_get_long(aer_cap + PCI_ERR_ROOT_COMMAND);
-    root_status = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
+    prev_status = root_status = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
 
     if (cmd & PCI_COMMAND_SERR) {
         /* System Error.
@@ -326,15 +342,22 @@ static void pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
     }
     pci_set_long(aer_cap + PCI_ERR_ROOT_STATUS, root_status);
 
-    if (root_cmd & msg->severity) {
-        /* 6.2.4.1.2 Interrupt Generation */
-        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);
-        }
+    /* 6.2.4.1.2 Interrupt Generation */
+    /* All the above did was set some bits in the status register.
+     * Specifically these that match message severity.
+     * The below code relies on this fact. */
+    if (!(root_cmd & msg->severity) ||
+        (pcie_aer_status_to_cmd(prev_status) & root_cmd)) {
+        /* Condition is not being set or was already true so nothing to do. */
+        return;
+    }
+
+    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);
     }
 }
 
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH 5/6] pci/aer: fix interrupt on config write

[Michael S. Tsirkin]

config write handling for aer seems broken:
For example, it won't clear a level interrupt
when command register is set to 0.

Make it match the spec: level should equal
the logical or of enabled bits, msi only
be sent when the logical or changes.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pcie_aer.c |   46 +++++++++++++++++-----------------------------
 1 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
index 0fc191f..1c513a7 100644
--- a/hw/pcie_aer.c
+++ b/hw/pcie_aer.c
@@ -749,43 +749,31 @@ void pcie_aer_root_reset(PCIDevice *dev)
      */
 }
 
-static bool pcie_aer_root_does_trigger(uint32_t cmd, uint32_t status)
-{
-    return
-        ((cmd & PCI_ERR_ROOT_CMD_COR_EN) && (status & PCI_ERR_ROOT_COR_RCV)) ||
-        ((cmd & PCI_ERR_ROOT_CMD_NONFATAL_EN) &&
-         (status & PCI_ERR_ROOT_NONFATAL_RCV)) ||
-        ((cmd & PCI_ERR_ROOT_CMD_FATAL_EN) &&
-         (status & PCI_ERR_ROOT_FATAL_RCV));
-}
-
 void pcie_aer_root_write_config(PCIDevice *dev,
                                 uint32_t addr, uint32_t val, int len,
                                 uint32_t root_cmd_prev)
 {
     uint8_t *aer_cap = dev->config + dev->exp.aer_cap;
-
-    /* root command register */
+    uint32_t root_status = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
+    uint32_t enabled_cmd = pcie_aer_status_to_cmd(root_status);
     uint32_t root_cmd = pci_get_long(aer_cap + PCI_ERR_ROOT_COMMAND);
-    if (root_cmd & PCI_ERR_ROOT_CMD_EN_MASK) {
-        /* 6.2.4.1.2 Interrupt Generation */
+    /* 6.2.4.1.2 Interrupt Generation */
+    if (!msix_enabled(dev) && !msi_enabled(dev)) {
+        qemu_set_irq(dev->irq[dev->exp.aer_intx], !!(root_cmd & enabled_cmd));
+        return;
+    }
 
-        /* 0 -> 1 */
-        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 ((root_cmd_prev & enabled_cmd) || !(root_cmd & enabled_cmd)) {
+        /* Send MSI on transition from false to true. */
+        return;
+    }
 
-        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 {
-            qemu_set_irq(dev->irq[dev->exp.aer_intx], assert);
-        }
+    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 {
+        assert(0);
     }
 }
 
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH 6/6] pci/aer: factor out common code

[Michael S. Tsirkin]

Same logic is used to assert interrupts
and send msix messages, so add a static functin for this.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pcie_aer.c |   27 +++++++++++++--------------
 1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
index 1c513a7..ca4f517 100644
--- a/hw/pcie_aer.c
+++ b/hw/pcie_aer.c
@@ -273,6 +273,17 @@ static uint32_t pcie_aer_status_to_cmd(uint32_t status)
     return cmd;
 }
 
+static void pcie_aer_root_notify(PCIDevice *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);
+    }
+}
+
 /*
  * 6.2.6 Error Message Control
  * Figure 6-3
@@ -352,13 +363,7 @@ static void pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
         return;
     }
 
-    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);
-    }
+    pcie_aer_root_notify(dev);
 }
 
 /*
@@ -768,13 +773,7 @@ void pcie_aer_root_write_config(PCIDevice *dev,
         return;
     }
 
-    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 {
-        assert(0);
-    }
+    pcie_aer_root_notify(dev);
 }
 
 static const VMStateDescription vmstate_pcie_aer_err = {
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH 2/6] Makefile: make msix/msi depend on CONFIG_PCI

[Michael S. Tsirkin]

Possible now that pci is not depending on these.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 Makefile.objs |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index 04625eb..d1e63ce 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -168,7 +168,8 @@ hw-obj-$(CONFIG_VIRTIO) += virtio.o virtio-console.o
 hw-obj-y += fw_cfg.o
 # FIXME: Core PCI code and its direct dependencies are required by the
 # QMP query-pci command.
-hw-obj-y += pci.o pci_bridge.o msix.o msi.o
+hw-obj-y += pci.o pci_bridge.o
+hw-obj-$(CONFIG_PCI) += msix.o msi.o
 hw-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o
 hw-obj-$(CONFIG_PCI) += ioh3420.o xio3130_upstream.o xio3130_downstream.o
 hw-obj-y += watchdog.o
-- 
1.7.3.2.91.g446ac

[Qemu-devel] Re: [PATCH 1/6] pci: untangle pci/msi dependency

[Paolo Bonzini]

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

[Qemu-devel] Re: [PATCH 1/6] pci: untangle pci/msi dependency

[Isaku Yamahata]

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.
-- 
yamahata

[Qemu-devel] Re: [PATCH 5/6] pci/aer: fix interrupt on config write

[Isaku Yamahata]

Looks good.

On Fri, Dec 03, 2010 at 12:54:43AM +0200, Michael S. Tsirkin wrote:
> config write handling for aer seems broken:
> For example, it won't clear a level interrupt
> when command register is set to 0.
> 
> Make it match the spec: level should equal
> the logical or of enabled bits, msi only
> be sent when the logical or changes.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/pcie_aer.c |   46 +++++++++++++++++-----------------------------
>  1 files changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
> index 0fc191f..1c513a7 100644
> --- a/hw/pcie_aer.c
> +++ b/hw/pcie_aer.c
> @@ -749,43 +749,31 @@ void pcie_aer_root_reset(PCIDevice *dev)
>       */
>  }
>  
> -static bool pcie_aer_root_does_trigger(uint32_t cmd, uint32_t status)
> -{
> -    return
> -        ((cmd & PCI_ERR_ROOT_CMD_COR_EN) && (status & PCI_ERR_ROOT_COR_RCV)) ||
> -        ((cmd & PCI_ERR_ROOT_CMD_NONFATAL_EN) &&
> -         (status & PCI_ERR_ROOT_NONFATAL_RCV)) ||
> -        ((cmd & PCI_ERR_ROOT_CMD_FATAL_EN) &&
> -         (status & PCI_ERR_ROOT_FATAL_RCV));
> -}
> -
>  void pcie_aer_root_write_config(PCIDevice *dev,
>                                  uint32_t addr, uint32_t val, int len,
>                                  uint32_t root_cmd_prev)
>  {
>      uint8_t *aer_cap = dev->config + dev->exp.aer_cap;
> -
> -    /* root command register */
> +    uint32_t root_status = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
> +    uint32_t enabled_cmd = pcie_aer_status_to_cmd(root_status);
>      uint32_t root_cmd = pci_get_long(aer_cap + PCI_ERR_ROOT_COMMAND);
> -    if (root_cmd & PCI_ERR_ROOT_CMD_EN_MASK) {
> -        /* 6.2.4.1.2 Interrupt Generation */
> +    /* 6.2.4.1.2 Interrupt Generation */
> +    if (!msix_enabled(dev) && !msi_enabled(dev)) {
> +        qemu_set_irq(dev->irq[dev->exp.aer_intx], !!(root_cmd & enabled_cmd));
> +        return;
> +    }
>  
> -        /* 0 -> 1 */
> -        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 ((root_cmd_prev & enabled_cmd) || !(root_cmd & enabled_cmd)) {
> +        /* Send MSI on transition from false to true. */
> +        return;
> +    }
>  
> -        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 {
> -            qemu_set_irq(dev->irq[dev->exp.aer_intx], assert);
> -        }
> +    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 {
> +        assert(0);
>      }
>  }
>  
> -- 
> 1.7.3.2.91.g446ac
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH 6/6] pci/aer: factor out common code

[Isaku Yamahata]

Looks good.
Maybe assert(0) should be replaced by abort().
Anyway this patch removes it, though.

On Fri, Dec 03, 2010 at 12:54:47AM +0200, Michael S. Tsirkin wrote:
> Same logic is used to assert interrupts
> and send msix messages, so add a static functin for this.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/pcie_aer.c |   27 +++++++++++++--------------
>  1 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
> index 1c513a7..ca4f517 100644
> --- a/hw/pcie_aer.c
> +++ b/hw/pcie_aer.c
> @@ -273,6 +273,17 @@ static uint32_t pcie_aer_status_to_cmd(uint32_t status)
>      return cmd;
>  }
>  
> +static void pcie_aer_root_notify(PCIDevice *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);
> +    }
> +}
> +
>  /*
>   * 6.2.6 Error Message Control
>   * Figure 6-3
> @@ -352,13 +363,7 @@ static void pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
>          return;
>      }
>  
> -    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);
> -    }
> +    pcie_aer_root_notify(dev);
>  }
>  
>  /*
> @@ -768,13 +773,7 @@ void pcie_aer_root_write_config(PCIDevice *dev,
>          return;
>      }
>  
> -    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 {
> -        assert(0);
> -    }
> +    pcie_aer_root_notify(dev);
>  }
>  
>  static const VMStateDescription vmstate_pcie_aer_err = {
> -- 
> 1.7.3.2.91.g446ac
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH 3/6] pci/aer: remove dead code

[Isaku Yamahata]

The eventual result looks okay.
But if-clauses that surrounds msi_trigger = true should be eliminated
at the same time.
1/6, 3/6 and, 4/6 could be reordered for bisectability.

thanks,

On Fri, Dec 03, 2010 at 12:54:37AM +0200, Michael S. Tsirkin wrote:
> Remove some unused variables and return values.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/pcie_aer.c |   15 +--------------
>  1 files changed, 1 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/pcie_aer.c b/hw/pcie_aer.c
> index 18bbd5a..204155b 100644
> --- a/hw/pcie_aer.c
> +++ b/hw/pcie_aer.c
> @@ -258,29 +258,21 @@ static unsigned int pcie_aer_root_get_vector(PCIDevice *dev)
>  }
>  
>  /*
> - * return value:
> - * true: error message is sent up
> - * false: error message is masked
> - *
>   * 6.2.6 Error Message Control
>   * Figure 6-3
>   * root port part
>   */
> -static bool pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
> +static void pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
>  {
> -    bool msg_sent;
>      uint16_t cmd;
>      uint8_t *aer_cap;
>      uint32_t root_cmd;
>      uint32_t root_status;
> -    bool msi_trigger;
>  
> -    msg_sent = false;
>      cmd = pci_get_word(dev->config + PCI_COMMAND);
>      aer_cap = dev->config + dev->exp.aer_cap;
>      root_cmd = pci_get_long(aer_cap + PCI_ERR_ROOT_COMMAND);
>      root_status = pci_get_long(aer_cap + PCI_ERR_ROOT_STATUS);
> -    msi_trigger = false;
>  
>      if (cmd & PCI_COMMAND_SERR) {
>          /* System Error.
> @@ -300,7 +292,6 @@ static bool pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
>              root_status |= PCI_ERR_ROOT_MULTI_COR_RCV;
>          } else {
>              if (root_cmd & PCI_ERR_ROOT_CMD_COR_EN) {
> -                msi_trigger = true;
>              }
>              pci_set_word(aer_cap + PCI_ERR_ROOT_COR_SRC, msg->source_id);
>          }
> @@ -309,14 +300,12 @@ static bool pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
>      case PCI_ERR_ROOT_CMD_NONFATAL_EN:
>          if (!(root_status & PCI_ERR_ROOT_NONFATAL_RCV) &&
>              root_cmd & PCI_ERR_ROOT_CMD_NONFATAL_EN) {
> -            msi_trigger = true;
>          }
>          root_status |= PCI_ERR_ROOT_NONFATAL_RCV;
>          break;
>      case PCI_ERR_ROOT_CMD_FATAL_EN:
>          if (!(root_status & PCI_ERR_ROOT_FATAL_RCV) &&
>              root_cmd & PCI_ERR_ROOT_CMD_FATAL_EN) {
> -            msi_trigger = true;
>          }
>          if (!(root_status & PCI_ERR_ROOT_UNCOR_RCV)) {
>              root_status |= PCI_ERR_ROOT_FIRST_FATAL;
> @@ -346,9 +335,7 @@ static bool pcie_aer_msg_root_port(PCIDevice *dev, const PCIEAERMsg *msg)
>          } else {
>              qemu_set_irq(dev->irq[dev->exp.aer_intx], 1);
>          }
> -        msg_sent = true;
>      }
> -    return msg_sent;
>  }
>  
>  /*
> -- 
> 1.7.3.2.91.g446ac
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH 1/6] pci: untangle pci/msi dependency

[Michael S. Tsirkin]

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.