Re: [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it

[Marcel Apfelbaum]

On 05/17/2016 01:08 PM, Cao jin wrote:

On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote:

diff --git a/hw/pci-bridge/pci_bridge_dev.c
b/hw/pci-bridge/pci_bridge_dev.c
index 9e31f0e..af71c98 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)
      PCIBridge *br = PCI_BRIDGE(dev);
      PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
      int err;
+    Error *local_err = NULL;

      pci_bridge_initfn(dev, TYPE_PCI_BUS);

@@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
          goto slotid_error;
      }

-    if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
-                bridge_dev->msi == ON_OFF_AUTO_ON) &&
+    if (bridge_dev->msi != ON_OFF_AUTO_OFF &&

So we made the msi property OnOffAuto, but we don't make a difference
between ON and Auto?

That is why I am not quite sure about this device, msi has a relation with shpc.

 From its previous behaviour, it can be seen that it don`t treat 'on' as 
'auto'.(I am not sure why it is different with others)

Its previous behaviour treat msi_init`s failure as fatal, and lead to device 
creation fail. According to Markus`s comments(if I understand him right), this 
device has no non-msi variants.

Actually it has. The bridge needs msi for the SHPC controller, as you said.
If you look into shpc_interrupt_update function (hw/pci/shpc.c) you can see it 
can fall
back to legacy interrupts.

The only complication here is that msi is needed only if shpc is present.
So maybe having msi=on/off/auto is OK.

msi=auto => if shpc not present or msi broken results in msi = off, else msi = 
on
msi=on => fails if shpc present and msi broken
msi=off => use legacy interrupts if shpc is present


Basically the msi flag has no meaning if shpc not present.

Thanks,
Marcel

 I think my

patch follows the previous behaviour.

And also according to function comments:
/* MSI is not applicable without SHPC */
which means device either has both, or neither(if I understand it right), so 
that is why I don`t make a difference

          msi_nonbroken) {
-        err = msi_init(dev, 0, 1, true, true);
+        err = msi_init(dev, 0, 1, true, true, &local_err);
          if (err < 0) {
+            error_report_err(local_err);
              goto msi_error;
          }
      }