Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Liu, Jing2]

Hi Laszlo,
Sorry for late reply. I missed these mails because of wrong filter.
And thanks very much for comments. My reply as belows.
On 8/7/2018 8:19 PM, Laszlo Ersek wrote:
On 08/07/18 09:04, Jing Liu wrote:
[...]
@@ -46,6 +46,12 @@ struct PCIBridgeDev {
      uint32_t flags;
  
      OnOffAuto msi;
+
+    /* additional resources to reserve on firmware init */
+    uint64_t io_reserve;
+    uint64_t mem_reserve;
+    uint64_t pref32_reserve;
+    uint64_t pref64_reserve;
  };
  typedef struct PCIBridgeDev PCIBridgeDev;

(1) Please evaluate the following idea:

This is really good and I'm only not sure if DEFINE_PROP_ like,
DEFINE_PROP_UINT32("bus-reserve", GenPCIERootPort,
res_reserve.bus_reserve, -1), is a right/good way?
I mean the third parameter "_f".

[...]

- gen_rp_props should be updated accordingly. The property names should
stay the same, but they should reference fields of the "res_reserve" field.

(2) Then, in a separate patch, you can add another "res_reserve" field
to "PCIBridgeDev". (As a consequence, "bus_reserve" will also be added,
which I think is the right thing to do.)

I consider whether we need limit the bus_reserve of pci-pci bridge. For 
old codes, we could add "unlimited" numbers of devices (but less than 
than max) onto pci-pci bridge.

(3) There is a class that is derived from TYPE_PCI_BRIDGE_DEV:
TYPE_PCI_BRIDGE_SEAT_DEV. Is it correct to add the new properties /
fields to the latter as well?

Umm, I looked into the codes that doesn't have hotplug property?
So do we need add resource reserve for it?

  
@@ -95,6 +101,13 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
          error_free(local_err);
      }
  
+    err = pci_bridge_qemu_reserve_cap_init(dev, 0, 0,
+          bridge_dev->io_reserve, bridge_dev->mem_reserve,
+          bridge_dev->pref32_reserve, bridge_dev->pref64_reserve, errp);
+    if (err) {
+        goto cap_error;
+    }
+
      if (shpc_present(dev)) {
          /* TODO: spec recommends using 64 bit prefetcheable BAR.
           * Check whether that works well. */
@@ -103,6 +116,8 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error 
**errp)
      }
      return;
  
+cap_error:
+    msi_uninit(dev);

(4) This error handler doesn't look entirely correct; we can reach it
without having initialized MSI. (MSI setup is conditional; and even if
we attempt it, it is permitted to fail with "msi=auto".) Therefore,
msi_uninit() should be wrapped into "if (msi_present())", same as you
see in pci_bridge_dev_exitfn().

sure, can add a pre-check. But I don't understand why we need that,
msi_uninit() will check msi_present()?


  msi_error:
      slotid_cap_cleanup(dev);
  slotid_error:
@@ -162,6 +177,11 @@ static Property pci_bridge_dev_properties[] = {
                              ON_OFF_AUTO_AUTO),
      DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
                      PCI_BRIDGE_DEV_F_SHPC_REQ, true),
+    DEFINE_PROP_SIZE("io-reserve", PCIBridgeDev, io_reserve, -1),
+    DEFINE_PROP_SIZE("mem-reserve", PCIBridgeDev, mem_reserve, -1),
+    DEFINE_PROP_SIZE("pref32-reserve", PCIBridgeDev, pref32_reserve, -1),
+    DEFINE_PROP_SIZE("pref64-reserve", PCIBridgeDev, pref64_reserve, -1),
+
      DEFINE_PROP_END_OF_LIST(),
  };
  


(5) Similarly to (2), this property list should cover "bus-reserve" too.
If we are exposing the same capability in PCI config space, then I think
we should let the user control all fields of it as well.

(6) Please clarify the capability ID in the subject line of the patch.
OK, will add the details.


Thanks very much.
Jing

For example:

   hw/pci: support resource reservation capability on "pci-bridge"

Thanks
Laszlo