[v2] hw/pci-bridge/pcie_pci_bridge: properly handle MSI unavailability case

Commit Message

Aleksandr Bezzubikov Sept. 24, 2017, 11:21 p.m. UTC

QEMU with the pcie-pci-bridge device crashes if the guest board doesn't support MSI,
e.g. 'qemu-system-ppc64 -M prep -device pcie-pci-bridge'.
This is caused by wrong pcie-pci-bridge instantiation error handling. This patch fixes this issue
by falling back to legacy INTx if MSI is not available.
Also set the bridge's 'msi' property default value to 'auto' in order to trigger errors 
only when user explicitly set msi=on.

v2:
rewrite the commit message

Reported-by: Eduardo Habkost <[e-mail]>
Signed-off-by: Aleksandr Bezzubikov <[e-mail]>
Reviewed-by: Marcel Apfelbaum <[e-mail]>
---
 hw/pci-bridge/pcie_pci_bridge.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

Comments

Thomas Huth Sept. 27, 2017, 4:40 p.m. UTC | #1

On 25.09.2017 01:21, Aleksandr Bezzubikov wrote:
> QEMU with the pcie-pci-bridge device crashes if the guest board doesn't support MSI,
> e.g. 'qemu-system-ppc64 -M prep -device pcie-pci-bridge'.
> This is caused by wrong pcie-pci-bridge instantiation error handling. This patch fixes this issue
> by falling back to legacy INTx if MSI is not available.
> Also set the bridge's 'msi' property default value to 'auto' in order to trigger errors 
> only when user explicitly set msi=on.
> 
> v2:
> rewrite the commit message
> 
> Reported-by: Eduardo Habkost <[e-mail]>
> Signed-off-by: Aleksandr Bezzubikov <[e-mail]>
> Reviewed-by: Marcel Apfelbaum <[e-mail]>
> ---
>  hw/pci-bridge/pcie_pci_bridge.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)

This also fixes the issue that I've seen with qemu-system-mips64el today:

$ mips64el-softmmu/qemu-system-mips64el -M malta -nographic -S -device
pcie-pci-bridge -bios pc-bios/bios.bin
qemu-system-mips64el: memory.c:1699: memory_region_finalize:
 Assertion `!mr->container' failed.
Aborted (core dumped)

So feel free to add:

Tested-by: Thomas Huth <[e-mail]>

Patch

diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
index 9aa5cc3..da562fe 100644
--- a/hw/pci-bridge/pcie_pci_bridge.c
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -65,10 +65,18 @@  static void pcie_pci_bridge_realize(PCIDevice *d, Error **errp)
         goto aer_error;
     }
 
+    Error *local_err = NULL;
     if (pcie_br->msi != ON_OFF_AUTO_OFF) {
-        rc = msi_init(d, 0, 1, true, true, errp);
+        rc = msi_init(d, 0, 1, true, true, &local_err);
         if (rc < 0) {
-            goto msi_error;
+            assert(rc == -ENOTSUP);
+            if (pcie_br->msi != ON_OFF_AUTO_ON) {
+                error_free(local_err);
+            } else {
+                /* failed to satisfy user's explicit request for MSI */
+                error_propagate(errp, local_err);
+                goto msi_error;
+            }
         }
     }
     pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
@@ -81,7 +89,7 @@  aer_error:
 pm_error:
     pcie_cap_exit(d);
 cap_error:
-    shpc_free(d);
+    shpc_cleanup(d, &pcie_br->shpc_bar);
 error:
     pci_bridge_exitfn(d);
 }
@@ -98,7 +106,9 @@  static void pcie_pci_bridge_reset(DeviceState *qdev)
 {
     PCIDevice *d = PCI_DEVICE(qdev);
     pci_bridge_reset(qdev);
-    msi_reset(d);
+    if (msi_present(d)) {
+        msi_reset(d);
+    }
     shpc_reset(d);
 }
 
@@ -106,12 +116,14 @@  static void pcie_pci_bridge_write_config(PCIDevice *d,
         uint32_t address, uint32_t val, int len)
 {
     pci_bridge_write_config(d, address, val, len);
-    msi_write_config(d, address, val, len);
+    if (msi_present(d)) {
+        msi_write_config(d, address, val, len);
+    }
     shpc_cap_write_config(d, address, val, len);
 }
 
 static Property pcie_pci_bridge_dev_properties[] = {
-        DEFINE_PROP_ON_OFF_AUTO("msi", PCIEPCIBridge, msi, ON_OFF_AUTO_ON),
+        DEFINE_PROP_ON_OFF_AUTO("msi", PCIEPCIBridge, msi, ON_OFF_AUTO_AUTO),
         DEFINE_PROP_END_OF_LIST(),
 };