[Qemu-devel] [RFC PATCH v2 0/6] Generic PCIE-PCI Bridge

[Qemu-devel] [RFC PATCH v2 0/6] Generic PCIE-PCI Bridge

[Aleksandr Bezzubikov]

This series introduces a new device - generic PCI Express to PCI bridge,
and also makes all necessary changes to enable hotplug of the bridge itself
and any device into the bridge.

Obvious reasons to remain RFC:
1. The new PCI capability: layout, creation interface.
2. Windows SHPC issue - devices hotplugging into the bridge
   doens't work on Windows 7 (unlike Windows 10),
   whereas an old pci-bridge usage is fine.
3. Hot-unplug not completely tested.

Changes v1->v2:
1. Enable SHPC for the bridge.
2. Enable SHPC support for the Q35 machine (ACPI stuff).
3. Introduce PCI capability to help firmware on the system init.
   This allows the bridge to be hotpluggable. Now it's supported 
   only for pcie-root-port. Now it's supposed to used with 
   SeaBIOS only, look at the SeaBIOS corresponding series
   "".

Aleksandr Bezzubikov (6):
  hw/pci: introduce pcie-pci-bridge device
  hw/i386: allow SHPC for Q35 machine
  hw/pci: enable SHPC for PCIE-PCI bridge
  hw/pci: introduce bridge-only vendor-specific capability to provide
    some hints to firmware
  hw/pci: add bus_reserve property to pcie-root-port
  hw/pci: add hint capabilty for additional bus reservation to
    pcie-root-port

 hw/i386/acpi-build.c            |   2 +-
 hw/pci-bridge/Makefile.objs     |   2 +-
 hw/pci-bridge/pcie_pci_bridge.c | 212 ++++++++++++++++++++++++++++++++++++++++
 hw/pci-bridge/pcie_root_port.c  |   6 ++
 hw/pci/pci_bridge.c             |  27 +++++
 include/hw/pci/pci.h            |   1 +
 include/hw/pci/pci_bridge.h     |  18 ++++
 include/hw/pci/pcie_port.h      |   3 +
 8 files changed, 269 insertions(+), 2 deletions(-)
 create mode 100644 hw/pci-bridge/pcie_pci_bridge.c

-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 1/6] hw/pci: introduce pcie-pci-bridge device

[Aleksandr Bezzubikov]

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/pci-bridge/Makefile.objs     |   2 +-
 hw/pci-bridge/pcie_pci_bridge.c | 151 ++++++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h            |   1 +
 3 files changed, 153 insertions(+), 1 deletion(-)
 create mode 100644 hw/pci-bridge/pcie_pci_bridge.c

diff --git a/hw/pci-bridge/Makefile.objs b/hw/pci-bridge/Makefile.objs
index c4683cf..666db37 100644
--- a/hw/pci-bridge/Makefile.objs
+++ b/hw/pci-bridge/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-y += pci_bridge_dev.o
+common-obj-y += pci_bridge_dev.o pcie_pci_bridge.o
 common-obj-$(CONFIG_PCIE_PORT) += pcie_root_port.o gen_pcie_root_port.o
 common-obj-$(CONFIG_PXB) += pci_expander_bridge.o
 common-obj-$(CONFIG_XIO3130) += xio3130_upstream.o xio3130_downstream.o
diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
new file mode 100644
index 0000000..0991a7b
--- /dev/null
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -0,0 +1,151 @@
+/*
+ * QEMU Generic PCIE-PCI Bridge
+ *
+ * Copyright (c) 2017 Aleksandr Bezzubikov
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
+#include "hw/pci/pci_bridge.h"
+#include "hw/pci/msi.h"
+#include "hw/pci/slotid_cap.h"
+
+typedef struct PCIEPCIBridge {
+    /*< private >*/
+    PCIBridge parent_obj;
+    uint32_t flags;
+
+    /*< public >*/
+} PCIEPCIBridge;
+
+#define TYPE_PCIE_PCI_BRIDGE_DEV      "pcie-pci-bridge"
+#define PCIE_PCI_BRIDGE_DEV(obj) \
+        OBJECT_CHECK(PCIEPCIBridge, (obj), TYPE_PCIE_PCI_BRIDGE_DEV)
+
+static void pciepci_bridge_realize(PCIDevice *d, Error **errp)
+{
+    int rc, pos;
+    Error *local_err = NULL;
+
+    pci_bridge_initfn(d, TYPE_PCI_BUS);
+
+    rc = pcie_cap_init(d, 0, PCI_EXP_TYPE_PCI_BRIDGE, 0, &local_err);
+    if (rc < 0) {
+        error_propagate(errp, local_err);
+        goto error;
+    }
+
+    pos = pci_add_capability(d, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF, &local_err);
+    if (pos < 0) {
+        error_propagate(errp, local_err);
+        goto error;
+    }
+    d->exp.pm_cap = pos;
+    pci_set_word(d->config + pos + PCI_PM_PMC, 0x3);
+
+    pcie_cap_arifwd_init(d);
+    pcie_cap_deverr_init(d);
+
+    rc = pcie_aer_init(d, PCI_ERR_VER, 0x100, PCI_ERR_SIZEOF, &local_err);
+    if (rc < 0) {
+        error_propagate(errp, local_err);
+        goto error;
+    }
+
+    rc = msi_init(d, 0, 1, true, true, &local_err);
+    if (rc < 0) {
+        error_propagate(errp, local_err);
+        goto error;
+    }
+
+    return;
+
+    error:
+    pci_bridge_exitfn(d);
+}
+
+static void pciepci_bridge_exit(PCIDevice *d)
+{
+    pcie_cap_exit(d);
+    pci_bridge_exitfn(d);
+}
+
+static void pciepci_bridge_reset(DeviceState *qdev)
+{
+    PCIDevice *d = PCI_DEVICE(qdev);
+    pci_bridge_reset(qdev);
+    msi_reset(d);
+}
+
+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);
+}
+
+
+static Property pcie_pci_bridge_dev_properties[] = {
+        DEFINE_PROP_END_OF_LIST(),
+};
+
+static const VMStateDescription pciepci_bridge_dev_vmstate = {
+        .name = TYPE_PCIE_PCI_BRIDGE_DEV,
+        .fields = (VMStateField[]) {
+            VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
+            VMSTATE_END_OF_LIST()
+        }
+};
+
+static void pciepci_bridge_class_init(ObjectClass *klass, void *data)
+{
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    k->is_express = 1;
+    k->is_bridge = 1;
+    k->vendor_id = PCI_VENDOR_ID_REDHAT;
+    k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE;
+    k->realize = pciepci_bridge_realize;
+    k->exit = pciepci_bridge_exit;
+    k->config_write = pcie_pci_bridge_write_config;
+    dc->vmsd = &pciepci_bridge_dev_vmstate;
+    dc->props = pcie_pci_bridge_dev_properties;
+    dc->vmsd = &pciepci_bridge_dev_vmstate;
+    dc->reset = &pciepci_bridge_reset;
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+}
+
+static const TypeInfo pciepci_bridge_info = {
+        .name = TYPE_PCIE_PCI_BRIDGE_DEV,
+        .parent = TYPE_PCI_BRIDGE,
+        .instance_size = sizeof(PCIEPCIBridge),
+        .class_init = pciepci_bridge_class_init
+};
+
+static void pciepci_register(void)
+{
+    type_register_static(&pciepci_bridge_info);
+}
+
+type_init(pciepci_register);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index e598b09..b33a34f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -98,6 +98,7 @@
 #define PCI_DEVICE_ID_REDHAT_PXB_PCIE    0x000b
 #define PCI_DEVICE_ID_REDHAT_PCIE_RP     0x000c
 #define PCI_DEVICE_ID_REDHAT_XHCI        0x000d
+#define PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE 0x000e
 #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
 
 #define FMT_PCIBUS                      PRIx64
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 2/6] hw/i386: allow SHPC for Q35 machine

[Aleksandr Bezzubikov]

Unmask previously masked SHPC feature in _OSC method.

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/i386/acpi-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6b7bade..0d99585 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1850,7 +1850,7 @@ static Aml *build_q35_osc_method(void)
      * Always allow native PME, AER (no dependencies)
      * Never allow SHPC (no SHPC controller in this system)
      */
-    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1D), a_ctrl));
+    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
 
     if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
     /* Unknown revision */
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 3/6] hw/pci: enable SHPC for PCIE-PCI bridge

[Aleksandr Bezzubikov]

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/pci-bridge/pcie_pci_bridge.c | 63 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
index 0991a7b..38f665f 100644
--- a/hw/pci-bridge/pcie_pci_bridge.c
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -28,6 +28,7 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/msi.h"
+#include "hw/pci/shpc.h"
 #include "hw/pci/slotid_cap.h"
 
 typedef struct PCIEPCIBridge {
@@ -35,6 +36,7 @@ typedef struct PCIEPCIBridge {
     PCIBridge parent_obj;
     uint32_t flags;
 
+    MemoryRegion bar;
     /*< public >*/
 } PCIEPCIBridge;
 
@@ -44,11 +46,22 @@ typedef struct PCIEPCIBridge {
 
 static void pciepci_bridge_realize(PCIDevice *d, Error **errp)
 {
+    PCIBridge *br = PCI_BRIDGE(d);
+    PCIEPCIBridge *bridge_dev = PCIE_PCI_BRIDGE_DEV(d);
     int rc, pos;
     Error *local_err = NULL;
 
     pci_bridge_initfn(d, TYPE_PCI_BUS);
 
+    d->config[PCI_INTERRUPT_PIN] = 0x1;
+    memory_region_init(&bridge_dev->bar, OBJECT(d), "shpc-bar",
+                       shpc_bar_size(d));
+    rc = shpc_init(d, &br->sec_bus, &bridge_dev->bar, 0, &local_err);
+    if (rc) {
+        error_propagate(errp, local_err);
+        goto error;
+    }
+
     rc = pcie_cap_init(d, 0, PCI_EXP_TYPE_PCI_BRIDGE, 0, &local_err);
     if (rc < 0) {
         error_propagate(errp, local_err);
@@ -78,6 +91,9 @@ static void pciepci_bridge_realize(PCIDevice *d, Error **errp)
         goto error;
     }
 
+    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
+                     PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
+
     return;
 
     error:
@@ -86,7 +102,9 @@ static void pciepci_bridge_realize(PCIDevice *d, Error **errp)
 
 static void pciepci_bridge_exit(PCIDevice *d)
 {
+    PCIEPCIBridge *bridge_dev = PCIE_PCI_BRIDGE_DEV(d);
     pcie_cap_exit(d);
+    shpc_cleanup(d, &bridge_dev->bar);
     pci_bridge_exitfn(d);
 }
 
@@ -95,6 +113,7 @@ static void pciepci_bridge_reset(DeviceState *qdev)
     PCIDevice *d = PCI_DEVICE(qdev);
     pci_bridge_reset(qdev);
     msi_reset(d);
+    shpc_reset(d);
 }
 
 static void pcie_pci_bridge_write_config(PCIDevice *d,
@@ -102,8 +121,15 @@ static void pcie_pci_bridge_write_config(PCIDevice *d,
 {
     pci_bridge_write_config(d, address, val, len);
     msi_write_config(d, address, val, len);
+    shpc_cap_write_config(d, address, val, len);
 }
 
+static bool pci_device_shpc_present(void *opaque, int version_id)
+{
+    PCIDevice *dev = opaque;
+
+    return shpc_present(dev);
+}
 
 static Property pcie_pci_bridge_dev_properties[] = {
         DEFINE_PROP_END_OF_LIST(),
@@ -113,14 +139,43 @@ static const VMStateDescription pciepci_bridge_dev_vmstate = {
         .name = TYPE_PCIE_PCI_BRIDGE_DEV,
         .fields = (VMStateField[]) {
             VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
+            SHPC_VMSTATE(shpc, PCIDevice, pci_device_shpc_present),
             VMSTATE_END_OF_LIST()
         }
 };
 
+static void pcie_pci_bridge_hotplug_cb(HotplugHandler *hotplug_dev,
+                                      DeviceState *dev, Error **errp)
+{
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+
+    if (!shpc_present(pci_hotplug_dev)) {
+        error_setg(errp, "standard hotplug controller has been disabled for "
+                   "this %s", TYPE_PCIE_PCI_BRIDGE_DEV);
+        return;
+    }
+    shpc_device_hotplug_cb(hotplug_dev, dev, errp);
+}
+
+static void pcie_pci_bridge_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                                 DeviceState *dev,
+                                                 Error **errp)
+{
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+
+    if (!shpc_present(pci_hotplug_dev)) {
+        error_setg(errp, "standard hotplug controller has been disabled for "
+                   "this %s", TYPE_PCIE_PCI_BRIDGE_DEV);
+        return;
+    }
+    shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
+}
+
 static void pciepci_bridge_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->is_express = 1;
     k->is_bridge = 1;
@@ -134,13 +189,19 @@ static void pciepci_bridge_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &pciepci_bridge_dev_vmstate;
     dc->reset = &pciepci_bridge_reset;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    hc->plug = pcie_pci_bridge_hotplug_cb;
+    hc->unplug_request = pcie_pci_bridge_hot_unplug_request_cb;
 }
 
 static const TypeInfo pciepci_bridge_info = {
         .name = TYPE_PCIE_PCI_BRIDGE_DEV,
         .parent = TYPE_PCI_BRIDGE,
         .instance_size = sizeof(PCIEPCIBridge),
-        .class_init = pciepci_bridge_class_init
+        .class_init = pciepci_bridge_class_init,
+        .interfaces = (InterfaceInfo[]) {
+            { TYPE_HOTPLUG_HANDLER },
+            { },
+        }
 };
 
 static void pciepci_register(void)
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Aleksandr Bezzubikov]

On PCI init PCI bridges may need some
extra info about bus number to reserve, IO, memory and
prefetchable memory limits. QEMU can provide this
with special vendor-specific PCI capability.

Sizes of limits match ones from
PCI Type 1 Configuration Space Header,
number of buses to reserve occupies only 1 byte 
since it is the size of Subordinate Bus Number register.

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
 include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 720119b..8ec6c2c 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
     br->bus_name = bus_name;
 }
 
+
+int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
+                              uint8_t bus_reserve, uint32_t io_limit,
+                              uint16_t mem_limit, uint64_t pref_limit,
+                              Error **errp)
+{
+    size_t cap_len = sizeof(PCIBridgeQemuCap);
+    PCIBridgeQemuCap cap;
+
+    cap.len = cap_len;
+    cap.bus_res = bus_reserve;
+    cap.io_lim = io_limit & 0xFF;
+    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
+    cap.mem_lim = mem_limit;
+    cap.pref_lim = pref_limit & 0xFFFF;
+    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
+
+    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
+                                    cap_offset, cap_len, errp);
+    if (offset < 0) {
+        return offset;
+    }
+
+    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
+    return 0;
+}
+
 static const TypeInfo pci_bridge_type_info = {
     .name = TYPE_PCI_BRIDGE,
     .parent = TYPE_PCI_DEVICE,
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index ff7cbaa..c9f642c 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
 #define  PCI_BRIDGE_CTL_DISCARD_STATUS	0x400	/* Discard timer status */
 #define  PCI_BRIDGE_CTL_DISCARD_SERR	0x800	/* Discard timer SERR# enable */
 
+typedef struct PCIBridgeQemuCap {
+    uint8_t id;     /* Standard PCI capability header field */
+    uint8_t next;   /* Standard PCI capability header field */
+    uint8_t len;    /* Standard PCI vendor-specific capability header field */
+    uint8_t bus_res;
+    uint32_t pref_lim_upper;
+    uint16_t pref_lim;
+    uint16_t mem_lim;
+    uint16_t io_lim_upper;
+    uint8_t io_lim;
+    uint8_t padding;
+} PCIBridgeQemuCap;
+
+int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
+                              uint8_t bus_reserve, uint32_t io_limit,
+                              uint16_t mem_limit, uint64_t pref_limit,
+                              Error **errp);
+
 #endif /* QEMU_PCI_BRIDGE_H */
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Aleksandr Bezzubikov]

To enable hotplugging of a newly created pcie-pci-bridge,
we need to tell firmware (SeaBIOS in this case) to reserve
additional buses for pcie-root-port, that allows us to 
hotplug pcie-pci-bridge into this root port.
The number of buses to reserve is provided to the device via a corresponding
property, and to the firmware via new PCI capability (next patch).
The property's default value is 1 as we want to hotplug at least 1 bridge.

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/pci-bridge/pcie_root_port.c | 1 +
 include/hw/pci/pcie_port.h     | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 4d588cb..b0e49e1 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
 static Property rp_props[] = {
     DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
                     QEMU_PCIE_SLTCAP_PCP_BITNR, true),
+    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
index 1333266..1b2dd1f 100644
--- a/include/hw/pci/pcie_port.h
+++ b/include/hw/pci/pcie_port.h
@@ -34,6 +34,9 @@ struct PCIEPort {
 
     /* pci express switch port */
     uint8_t     port;
+
+    /* additional buses to reserve on firmware init */
+    uint8_t     bus_reserve;
 };
 
 void pcie_port_init_reg(PCIDevice *d);
-- 
2.7.4

[Qemu-devel] [RFC PATCH v2 6/6] hw/pci: add hint capabilty for additional bus reservation to pcie-root-port

[Aleksandr Bezzubikov]

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/pci-bridge/pcie_root_port.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index b0e49e1..ca92d85 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -106,6 +106,11 @@ static void rp_realize(PCIDevice *d, Error **errp)
     pcie_aer_root_init(d);
     rp_aer_vector_update(d);
 
+    rc = pci_bridge_help_cap_init(d, 0, p->bus_reserve, 0, 0, 0, errp);
+    if (rc < 0) {
+        goto err;
+    }
+
     return;
 
 err:
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Michael S. Tsirkin]

On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
> To enable hotplugging of a newly created pcie-pci-bridge,
> we need to tell firmware (SeaBIOS in this case)

Presumably, EFI would need to support this too?

> to reserve
> additional buses for pcie-root-port, that allows us to 
> hotplug pcie-pci-bridge into this root port.
> The number of buses to reserve is provided to the device via a corresponding
> property, and to the firmware via new PCI capability (next patch).
> The property's default value is 1 as we want to hotplug at least 1 bridge.

If so you should just teach firmware to allocate one bus #
unconditionally.

But why would that be so? What's wrong with a device
directly in the root port?


> 
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> ---
>  hw/pci-bridge/pcie_root_port.c | 1 +
>  include/hw/pci/pcie_port.h     | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 4d588cb..b0e49e1 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
>  static Property rp_props[] = {
>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> index 1333266..1b2dd1f 100644
> --- a/include/hw/pci/pcie_port.h
> +++ b/include/hw/pci/pcie_port.h
> @@ -34,6 +34,9 @@ struct PCIEPort {
>  
>      /* pci express switch port */
>      uint8_t     port;
> +
> +    /* additional buses to reserve on firmware init */
> +    uint8_t     bus_reserve;
>  };
>  
>  void pcie_port_init_reg(PCIDevice *d);

So here is a property and it does not do anything.
It makes it easier to work on series maybe, but review
is harder since we do not see what it does at all.
Please do not split up patches like this - you can maintain
it split up in your branch if you like and merge before sending.

> -- 
> 2.7.4

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Marcel Apfelbaum]

On 23/07/2017 15:22, Michael S. Tsirkin wrote:
> On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
>> To enable hotplugging of a newly created pcie-pci-bridge,
>> we need to tell firmware (SeaBIOS in this case)
> 

Hi Michael,

> Presumably, EFI would need to support this too?
> 

Sure, Eduardo added to CC, but he is in PTO now.

>> to reserve
>> additional buses for pcie-root-port, that allows us to
>> hotplug pcie-pci-bridge into this root port.
>> The number of buses to reserve is provided to the device via a corresponding
>> property, and to the firmware via new PCI capability (next patch).
>> The property's default value is 1 as we want to hotplug at least 1 bridge.
> 
> If so you should just teach firmware to allocate one bus #
> unconditionally.
> 

That would be a problem for the PCIe machines, since each PCIe
devices is plugged in a different bus and we are already
limited to 256 PCIe devices. Allocating an extra-bus always
would really limit the PCIe devices we can use.

> But why would that be so? What's wrong with a device
> directly in the root port?
> 

First, plugging a legacy PCI device into a PCIe Root Port
looks strange at least, and it can;t be done on real HW anyway.
(incompatible slots)

Second (and more important), if we want 2 or more PCI
devices we would loose both IO ports space and bus numbers.

> 
>>
>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>> ---
>>   hw/pci-bridge/pcie_root_port.c | 1 +
>>   include/hw/pci/pcie_port.h     | 3 +++
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>> index 4d588cb..b0e49e1 100644
>> --- a/hw/pci-bridge/pcie_root_port.c
>> +++ b/hw/pci-bridge/pcie_root_port.c
>> @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
>>   static Property rp_props[] = {
>>       DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>>                       QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>> +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
>>       DEFINE_PROP_END_OF_LIST()
>>   };
>>   
>> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
>> index 1333266..1b2dd1f 100644
>> --- a/include/hw/pci/pcie_port.h
>> +++ b/include/hw/pci/pcie_port.h
>> @@ -34,6 +34,9 @@ struct PCIEPort {
>>   
>>       /* pci express switch port */
>>       uint8_t     port;
>> +
>> +    /* additional buses to reserve on firmware init */
>> +    uint8_t     bus_reserve;
>>   };
>>   
>>   void pcie_port_init_reg(PCIDevice *d);
> 
> So here is a property and it does not do anything.
> It makes it easier to work on series maybe, but review
> is harder since we do not see what it does at all.
> Please do not split up patches like this - you can maintain
> it split up in your branch if you like and merge before sending.
>

Agreed, Alexandr please merge patches 4-5-6 for your next submission.

Thanks,
Marcel


>> -- 
>> 2.7.4

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Marcel Apfelbaum]

On 23/07/2017 1:15, Aleksandr Bezzubikov wrote:
> On PCI init PCI bridges may need some
> extra info about bus number to reserve, IO, memory and
> prefetchable memory limits. QEMU can provide this
> with special vendor-specific PCI capability.
> 
> Sizes of limits match ones from
> PCI Type 1 Configuration Space Header,
> number of buses to reserve occupies only 1 byte
> since it is the size of Subordinate Bus Number register.
> 

Hi Alexandr,

> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> ---
>   hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
>   include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
>   2 files changed, 45 insertions(+)
> 
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 720119b..8ec6c2c 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>       br->bus_name = bus_name;
>   }
>   
> +
> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,

Can you please rename to something like
'pci_bridge_qemu_cap_init' to be more specific?

> +                              uint8_t bus_reserve, uint32_t io_limit,
> +                              uint16_t mem_limit, uint64_t pref_limit,


I am not sure regarding "limit" suffix, this
is a reservation, not a limitation.

> +                              Error **errp)
> +{
> +    size_t cap_len = sizeof(PCIBridgeQemuCap);
> +    PCIBridgeQemuCap cap;
> +
> +    cap.len = cap_len;
> +    cap.bus_res = bus_reserve;
> +    cap.io_lim = io_limit & 0xFF;
> +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
> +    cap.mem_lim = mem_limit;
> +    cap.pref_lim = pref_limit & 0xFFFF;
> +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
> +
> +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
> +                                    cap_offset, cap_len, errp);
> +    if (offset < 0) {
> +        return offset;
> +    }
> +
> +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
> +    return 0;
> +}
> +
>   static const TypeInfo pci_bridge_type_info = {
>       .name = TYPE_PCI_BRIDGE,
>       .parent = TYPE_PCI_DEVICE,
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index ff7cbaa..c9f642c 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>   #define  PCI_BRIDGE_CTL_DISCARD_STATUS	0x400	/* Discard timer status */
>   #define  PCI_BRIDGE_CTL_DISCARD_SERR	0x800	/* Discard timer SERR# enable */
>   
> +typedef struct PCIBridgeQemuCap {
> +    uint8_t id;     /* Standard PCI capability header field */
> +    uint8_t next;   /* Standard PCI capability header field */
> +    uint8_t len;    /* Standard PCI vendor-specific capability header field */
> +    uint8_t bus_res;
> +    uint32_t pref_lim_upper;
> +    uint16_t pref_lim;
> +    uint16_t mem_lim;

This 32bit IOMEM, right?

> +    uint16_t io_lim_upper;
> +    uint8_t io_lim;

Why do we need io_lim and io_lim_upper?

Thanks,
Marcel

> +    uint8_t padding;
> +} PCIBridgeQemuCap;
> +
> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> +                              uint8_t bus_reserve, uint32_t io_limit,
> +                              uint16_t mem_limit, uint64_t pref_limit,
> +                              Error **errp);
> +
>   #endif /* QEMU_PCI_BRIDGE_H */
> 

Re: [Qemu-devel] [RFC PATCH v2 2/6] hw/i386: allow SHPC for Q35 machine

[Marcel Apfelbaum]

On 23/07/2017 1:15, Aleksandr Bezzubikov wrote:
> Unmask previously masked SHPC feature in _OSC method.
> 
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> ---
>   hw/i386/acpi-build.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6b7bade..0d99585 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1850,7 +1850,7 @@ static Aml *build_q35_osc_method(void)
>        * Always allow native PME, AER (no dependencies)
>        * Never allow SHPC (no SHPC controller in this system)

Seems the above comment is not longer correct :)

Thanks,
Marcel

>        */
> -    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1D), a_ctrl));
> +    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
>   
>       if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
>       /* Unknown revision */
> 

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Alexander Bezzubikov]

2017-07-23 18:57 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>:

> On 23/07/2017 1:15, Aleksandr Bezzubikov wrote:
>
>> On PCI init PCI bridges may need some
>> extra info about bus number to reserve, IO, memory and
>> prefetchable memory limits. QEMU can provide this
>> with special vendor-specific PCI capability.
>>
>> Sizes of limits match ones from
>> PCI Type 1 Configuration Space Header,
>> number of buses to reserve occupies only 1 byte
>> since it is the size of Subordinate Bus Number register.
>>
>>
> Hi Alexandr,
>
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>> ---
>>   hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
>>   include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>> index 720119b..8ec6c2c 100644
>> --- a/hw/pci/pci_bridge.c
>> +++ b/hw/pci/pci_bridge.c
>> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char*
>> bus_name,
>>       br->bus_name = bus_name;
>>   }
>>   +
>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>>
>
> Can you please rename to something like
> 'pci_bridge_qemu_cap_init' to be more specific?
>
> +                              uint8_t bus_reserve, uint32_t io_limit,
>> +                              uint16_t mem_limit, uint64_t pref_limit,
>>
>
>
> I am not sure regarding "limit" suffix, this
> is a reservation, not a limitation.


I'd like this fields names to match actual registers which
are going to get the values.


>
>
> +                              Error **errp)
>> +{
>> +    size_t cap_len = sizeof(PCIBridgeQemuCap);
>> +    PCIBridgeQemuCap cap;
>> +
>> +    cap.len = cap_len;
>> +    cap.bus_res = bus_reserve;
>> +    cap.io_lim = io_limit & 0xFF;
>> +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
>> +    cap.mem_lim = mem_limit;
>> +    cap.pref_lim = pref_limit & 0xFFFF;
>> +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
>> +
>> +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
>> +                                    cap_offset, cap_len, errp);
>> +    if (offset < 0) {
>> +        return offset;
>> +    }
>> +
>> +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
>> +    return 0;
>> +}
>> +
>>   static const TypeInfo pci_bridge_type_info = {
>>       .name = TYPE_PCI_BRIDGE,
>>       .parent = TYPE_PCI_DEVICE,
>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
>> index ff7cbaa..c9f642c 100644
>> --- a/include/hw/pci/pci_bridge.h
>> +++ b/include/hw/pci/pci_bridge.h
>> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char*
>> bus_name,
>>   #define  PCI_BRIDGE_CTL_DISCARD_STATUS        0x400   /* Discard timer
>> status */
>>   #define  PCI_BRIDGE_CTL_DISCARD_SERR  0x800   /* Discard timer SERR#
>> enable */
>>   +typedef struct PCIBridgeQemuCap {
>> +    uint8_t id;     /* Standard PCI capability header field */
>> +    uint8_t next;   /* Standard PCI capability header field */
>> +    uint8_t len;    /* Standard PCI vendor-specific capability header
>> field */
>> +    uint8_t bus_res;
>> +    uint32_t pref_lim_upper;
>> +    uint16_t pref_lim;
>> +    uint16_t mem_lim;
>>
>
> This 32bit IOMEM, right?
>
> +    uint16_t io_lim_upper;
>> +    uint8_t io_lim;
>>
>
> Why do we need io_lim and io_lim_upper?
>

The idea was to be able to directly move the capability fields values
to the registers when actually using it (in firmware) code.
Secondly, it saves a little space by avoding usage 32-bit types
when 24-bit ones are desired. And the same thing with prefetchable (48->64).
But if it's more convenient no to split this value, I can do that.



>
> Thanks,
> Marcel
>
>
> +    uint8_t padding;
>> +} PCIBridgeQemuCap;
>> +
>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>> +                              uint8_t bus_reserve, uint32_t io_limit,
>> +                              uint16_t mem_limit, uint64_t pref_limit,
>> +                              Error **errp);
>> +
>>   #endif /* QEMU_PCI_BRIDGE_H */
>>
>>
>


-- 
Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Marcel Apfelbaum]

On 23/07/2017 19:19, Alexander Bezzubikov wrote:
> 2017-07-23 18:57 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com 
> <mailto:marcel@redhat.com>>:
> 
>     On 23/07/2017 1:15, Aleksandr Bezzubikov wrote:
> 
>         On PCI init PCI bridges may need some
>         extra info about bus number to reserve, IO, memory and
>         prefetchable memory limits. QEMU can provide this
>         with special vendor-specific PCI capability.
> 
>         Sizes of limits match ones from
>         PCI Type 1 Configuration Space Header,
>         number of buses to reserve occupies only 1 byte
>         since it is the size of Subordinate Bus Number register.
> 
> 
>     Hi Alexandr,
> 
>         Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com
>         <mailto:zuban32s@gmail.com>>
>         ---
>            hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
>            include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
>            2 files changed, 45 insertions(+)
> 
>         diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>         index 720119b..8ec6c2c 100644
>         --- a/hw/pci/pci_bridge.c
>         +++ b/hw/pci/pci_bridge.c
>         @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br,
>         const char* bus_name,
>                br->bus_name = bus_name;
>            }
>            +
>         +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> 
> 
>     Can you please rename to something like
>     'pci_bridge_qemu_cap_init' to be more specific?
> 
>         +                              uint8_t bus_reserve, uint32_t
>         io_limit,
>         +                              uint16_t mem_limit, uint64_t
>         pref_limit,
> 
> 
> 
>     I am not sure regarding "limit" suffix, this
>     is a reservation, not a limitation.
> 
> 
> I'd like this fields names to match actual registers which
> are going to get the values.
> 

For this case I think is better to have io_res..., to describe
the parameter rather than match the registers.

> 
> 
>         +                              Error **errp)
>         +{
>         +    size_t cap_len = sizeof(PCIBridgeQemuCap);
>         +    PCIBridgeQemuCap cap;
>         +
>         +    cap.len = cap_len;
>         +    cap.bus_res = bus_reserve;
>         +    cap.io_lim = io_limit & 0xFF;
>         +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
>         +    cap.mem_lim = mem_limit;
>         +    cap.pref_lim = pref_limit & 0xFFFF;
>         +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
>         +
>         +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
>         +                                    cap_offset, cap_len, errp);
>         +    if (offset < 0) {
>         +        return offset;
>         +    }
>         +
>         +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len
>         - 2);
>         +    return 0;
>         +}
>         +
>            static const TypeInfo pci_bridge_type_info = {
>                .name = TYPE_PCI_BRIDGE,
>                .parent = TYPE_PCI_DEVICE,
>         diff --git a/include/hw/pci/pci_bridge.h
>         b/include/hw/pci/pci_bridge.h
>         index ff7cbaa..c9f642c 100644
>         --- a/include/hw/pci/pci_bridge.h
>         +++ b/include/hw/pci/pci_bridge.h
>         @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const
>         char* bus_name,
>            #define  PCI_BRIDGE_CTL_DISCARD_STATUS        0x400   /*
>         Discard timer status */
>            #define  PCI_BRIDGE_CTL_DISCARD_SERR  0x800   /* Discard
>         timer SERR# enable */
>            +typedef struct PCIBridgeQemuCap {
>         +    uint8_t id;     /* Standard PCI capability header field */
>         +    uint8_t next;   /* Standard PCI capability header field */
>         +    uint8_t len;    /* Standard PCI vendor-specific capability
>         header field */
>         +    uint8_t bus_res;
>         +    uint32_t pref_lim_upper;
>         +    uint16_t pref_lim;
>         +    uint16_t mem_lim;
> 
> 
>     This 32bit IOMEM, right?
> 
>         +    uint16_t io_lim_upper;
>         +    uint8_t io_lim;
> 
> 
>     Why do we need io_lim and io_lim_upper?
> 
> 
> The idea was to be able to directly move the capability fields values
> to the registers when actually using it (in firmware) code.
> Secondly, it saves a little space by avoding usage 32-bit types
> when 24-bit ones are desired. And the same thing with prefetchable (48->64).
> But if it's more convenient no to split this value, I can do that.

With a clear explanation (Mimic of the <PCI config space field>)
I personally don't mind keeping it like that.

Thanks,
Marcel

> 
> 
>     Thanks,
>     Marcel
> 
> 
>         +    uint8_t padding;
>         +} PCIBridgeQemuCap;
>         +
>         +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>         +                              uint8_t bus_reserve, uint32_t
>         io_limit,
>         +                              uint16_t mem_limit, uint64_t
>         pref_limit,
>         +                              Error **errp);
>         +
>            #endif /* QEMU_PCI_BRIDGE_H */
> 
> 
> 
> 
> 
> -- 
> Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 2/6] hw/i386: allow SHPC for Q35 machine

[Alexander Bezzubikov]

2017-07-23 18:59 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>:

> On 23/07/2017 1:15, Aleksandr Bezzubikov wrote:
>
>> Unmask previously masked SHPC feature in _OSC method.
>>
>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>> ---
>>   hw/i386/acpi-build.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 6b7bade..0d99585 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -1850,7 +1850,7 @@ static Aml *build_q35_osc_method(void)
>>        * Always allow native PME, AER (no dependencies)
>>        * Never allow SHPC (no SHPC controller in this system)
>>
>
> Seems the above comment is not longer correct :)
>
Just missed it, will fix in v3.


>
> Thanks,
> Marcel
>
>
>        */
>> -    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1D), a_ctrl));
>> +    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
>>         if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
>>       /* Unknown revision */
>>
>>
>


-- 
Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 6/6] hw/pci: add hint capabilty for additional bus reservation to pcie-root-port

[Michael S. Tsirkin]

On Sun, Jul 23, 2017 at 01:15:43AM +0300, Aleksandr Bezzubikov wrote:
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> ---
>  hw/pci-bridge/pcie_root_port.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index b0e49e1..ca92d85 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -106,6 +106,11 @@ static void rp_realize(PCIDevice *d, Error **errp)
>      pcie_aer_root_init(d);
>      rp_aer_vector_update(d);
>  
> +    rc = pci_bridge_help_cap_init(d, 0, p->bus_reserve, 0, 0, 0, errp);
> +    if (rc < 0) {
> +        goto err;
> +    }
> +
>      return;
>  
>  err:

It looks like this will add the capability unconditionally to all
pcie root ports. Two issues with it:
1. you can't add vendor properties to devices where vendor is
   not qemu as they might have their own concept of what it does.
2. this will break compatibility with old machine types,
   need to disable for these

> -- 
> 2.7.4

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Michael S. Tsirkin]

On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
> > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
> > > To enable hotplugging of a newly created pcie-pci-bridge,
> > > we need to tell firmware (SeaBIOS in this case)
> > 
> 
> Hi Michael,
> 
> > Presumably, EFI would need to support this too?
> > 
> 
> Sure, Eduardo added to CC, but he is in PTO now.
> 
> > > to reserve
> > > additional buses for pcie-root-port, that allows us to
> > > hotplug pcie-pci-bridge into this root port.
> > > The number of buses to reserve is provided to the device via a corresponding
> > > property, and to the firmware via new PCI capability (next patch).
> > > The property's default value is 1 as we want to hotplug at least 1 bridge.
> > 
> > If so you should just teach firmware to allocate one bus #
> > unconditionally.
> > 
> 
> That would be a problem for the PCIe machines, since each PCIe
> devices is plugged in a different bus and we are already
> limited to 256 PCIe devices. Allocating an extra-bus always
> would really limit the PCIe devices we can use.

But this is exactly what this patch does as the property is added to all
buses and default to 1 (1 extra bus).

> > But why would that be so? What's wrong with a device
> > directly in the root port?
> > 
> 
> First, plugging a legacy PCI device into a PCIe Root Port
> looks strange at least, and it can;t be done on real HW anyway.
> (incompatible slots)

You can still plug in PCIe devices there.

> Second (and more important), if we want 2 or more PCI
> devices we would loose both IO ports space and bus numbers.

What I am saying is maybe default should not be 1.

> > 
> > > 
> > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> > > ---
> > >   hw/pci-bridge/pcie_root_port.c | 1 +
> > >   include/hw/pci/pcie_port.h     | 3 +++
> > >   2 files changed, 4 insertions(+)
> > > 
> > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> > > index 4d588cb..b0e49e1 100644
> > > --- a/hw/pci-bridge/pcie_root_port.c
> > > +++ b/hw/pci-bridge/pcie_root_port.c
> > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
> > >   static Property rp_props[] = {
> > >       DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
> > >                       QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> > > +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
> > >       DEFINE_PROP_END_OF_LIST()
> > >   };
> > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> > > index 1333266..1b2dd1f 100644
> > > --- a/include/hw/pci/pcie_port.h
> > > +++ b/include/hw/pci/pcie_port.h
> > > @@ -34,6 +34,9 @@ struct PCIEPort {
> > >       /* pci express switch port */
> > >       uint8_t     port;
> > > +
> > > +    /* additional buses to reserve on firmware init */
> > > +    uint8_t     bus_reserve;
> > >   };
> > >   void pcie_port_init_reg(PCIDevice *d);
> > 
> > So here is a property and it does not do anything.
> > It makes it easier to work on series maybe, but review
> > is harder since we do not see what it does at all.
> > Please do not split up patches like this - you can maintain
> > it split up in your branch if you like and merge before sending.
> > 
> 
> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
> 
> Thanks,
> Marcel
> 
> 
> > > -- 
> > > 2.7.4

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Alexander Bezzubikov]

2017-07-24 23:46 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
>> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
>> > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
>> > > To enable hotplugging of a newly created pcie-pci-bridge,
>> > > we need to tell firmware (SeaBIOS in this case)
>> >
>>
>> Hi Michael,
>>
>> > Presumably, EFI would need to support this too?
>> >
>>
>> Sure, Eduardo added to CC, but he is in PTO now.
>>
>> > > to reserve
>> > > additional buses for pcie-root-port, that allows us to
>> > > hotplug pcie-pci-bridge into this root port.
>> > > The number of buses to reserve is provided to the device via a corresponding
>> > > property, and to the firmware via new PCI capability (next patch).
>> > > The property's default value is 1 as we want to hotplug at least 1 bridge.
>> >
>> > If so you should just teach firmware to allocate one bus #
>> > unconditionally.
>> >
>>
>> That would be a problem for the PCIe machines, since each PCIe
>> devices is plugged in a different bus and we are already
>> limited to 256 PCIe devices. Allocating an extra-bus always
>> would really limit the PCIe devices we can use.
>
> But this is exactly what this patch does as the property is added to all
> buses and default to 1 (1 extra bus).
>
>> > But why would that be so? What's wrong with a device
>> > directly in the root port?
>> >
>>
>> First, plugging a legacy PCI device into a PCIe Root Port
>> looks strange at least, and it can;t be done on real HW anyway.
>> (incompatible slots)
>
> You can still plug in PCIe devices there.
>
>> Second (and more important), if we want 2 or more PCI
>> devices we would loose both IO ports space and bus numbers.
>
> What I am saying is maybe default should not be 1.

The only sensible variant left is 0.
But as we want pcie-pci-bridge to be used for every legacy PCI device
on q35 machine, every time one hotplugs the bridge into the root port,
he must be sure rp's prop value >0 (for Linux). I'm not sure
that it is a very convenient way to utilize the bridge - always remember
to set property.
Another way - we can set this to 0 by default, and to 1 for pcie-root-port,
and recommend to use it for hotplugging of the pcie-pci-bridge itself.

>
>> >
>> > >
>> > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>> > > ---
>> > >   hw/pci-bridge/pcie_root_port.c | 1 +
>> > >   include/hw/pci/pcie_port.h     | 3 +++
>> > >   2 files changed, 4 insertions(+)
>> > >
>> > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>> > > index 4d588cb..b0e49e1 100644
>> > > --- a/hw/pci-bridge/pcie_root_port.c
>> > > +++ b/hw/pci-bridge/pcie_root_port.c
>> > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
>> > >   static Property rp_props[] = {
>> > >       DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>> > >                       QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>> > > +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
>> > >       DEFINE_PROP_END_OF_LIST()
>> > >   };
>> > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
>> > > index 1333266..1b2dd1f 100644
>> > > --- a/include/hw/pci/pcie_port.h
>> > > +++ b/include/hw/pci/pcie_port.h
>> > > @@ -34,6 +34,9 @@ struct PCIEPort {
>> > >       /* pci express switch port */
>> > >       uint8_t     port;
>> > > +
>> > > +    /* additional buses to reserve on firmware init */
>> > > +    uint8_t     bus_reserve;
>> > >   };
>> > >   void pcie_port_init_reg(PCIDevice *d);
>> >
>> > So here is a property and it does not do anything.
>> > It makes it easier to work on series maybe, but review
>> > is harder since we do not see what it does at all.
>> > Please do not split up patches like this - you can maintain
>> > it split up in your branch if you like and merge before sending.
>> >
>>
>> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
>>
>> Thanks,
>> Marcel
>>
>>
>> > > --
>> > > 2.7.4



-- 
Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 6/6] hw/pci: add hint capabilty for additional bus reservation to pcie-root-port

[Alexander Bezzubikov]

2017-07-24 23:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> On Sun, Jul 23, 2017 at 01:15:43AM +0300, Aleksandr Bezzubikov wrote:
>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>> ---
>>  hw/pci-bridge/pcie_root_port.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>> index b0e49e1..ca92d85 100644
>> --- a/hw/pci-bridge/pcie_root_port.c
>> +++ b/hw/pci-bridge/pcie_root_port.c
>> @@ -106,6 +106,11 @@ static void rp_realize(PCIDevice *d, Error **errp)
>>      pcie_aer_root_init(d);
>>      rp_aer_vector_update(d);
>>
>> +    rc = pci_bridge_help_cap_init(d, 0, p->bus_reserve, 0, 0, 0, errp);
>> +    if (rc < 0) {
>> +        goto err;
>> +    }
>> +
>>      return;
>>
>>  err:
>
> It looks like this will add the capability unconditionally to all
> pcie root ports. Two issues with it:
> 1. you can't add vendor properties to devices where vendor is
>    not qemu as they might have their own concept of what it does.
> 2. this will break compatibility with old machine types,
>    need to disable for these
>

Actually the original idea was to add it for pcie-root-port excusively
(for now at least), looks like I've confused a little with files naming.
Will add it for v3.

>> --
>> 2.7.4



-- 
Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Michael S. Tsirkin]

On Tue, Jul 25, 2017 at 12:41:12AM +0300, Alexander Bezzubikov wrote:
> 2017-07-24 23:46 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> > On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
> >> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
> >> > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
> >> > > To enable hotplugging of a newly created pcie-pci-bridge,
> >> > > we need to tell firmware (SeaBIOS in this case)
> >> >
> >>
> >> Hi Michael,
> >>
> >> > Presumably, EFI would need to support this too?
> >> >
> >>
> >> Sure, Eduardo added to CC, but he is in PTO now.
> >>
> >> > > to reserve
> >> > > additional buses for pcie-root-port, that allows us to
> >> > > hotplug pcie-pci-bridge into this root port.
> >> > > The number of buses to reserve is provided to the device via a corresponding
> >> > > property, and to the firmware via new PCI capability (next patch).
> >> > > The property's default value is 1 as we want to hotplug at least 1 bridge.
> >> >
> >> > If so you should just teach firmware to allocate one bus #
> >> > unconditionally.
> >> >
> >>
> >> That would be a problem for the PCIe machines, since each PCIe
> >> devices is plugged in a different bus and we are already
> >> limited to 256 PCIe devices. Allocating an extra-bus always
> >> would really limit the PCIe devices we can use.
> >
> > But this is exactly what this patch does as the property is added to all
> > buses and default to 1 (1 extra bus).
> >
> >> > But why would that be so? What's wrong with a device
> >> > directly in the root port?
> >> >
> >>
> >> First, plugging a legacy PCI device into a PCIe Root Port
> >> looks strange at least, and it can;t be done on real HW anyway.
> >> (incompatible slots)
> >
> > You can still plug in PCIe devices there.
> >
> >> Second (and more important), if we want 2 or more PCI
> >> devices we would loose both IO ports space and bus numbers.
> >
> > What I am saying is maybe default should not be 1.
> 
> The only sensible variant left is 0.
> But as we want pcie-pci-bridge to be used for every legacy PCI device
> on q35 machine, every time one hotplugs the bridge into the root port,
> he must be sure rp's prop value >0 (for Linux). I'm not sure
> that it is a very convenient way to utilize the bridge - always remember
> to set property.

That's what I'm saying then - if in your opinion default is >0 anyway,
tweak firmware to do it by default.

> Another way - we can set this to 0 by default, and to 1 for pcie-root-port,
> and recommend to use it for hotplugging of the pcie-pci-bridge itself.

I wonder about something: imagine hotplugging a hierarchy of bridges
below a root port. It seems that nothing prevents guest from finding a
free range of buses to cover this hierarchy and setting that as
secondary/subordinate bus for this bridge.

This does need support on QEMU side to hotplug a hierarchy at once,
and might need some fixes in Linux, on the plus side you can defer
management decision on how many are needed until you are actually
adding something, and you don't need vendor specific patches.


> >
> >> >
> >> > >
> >> > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> >> > > ---
> >> > >   hw/pci-bridge/pcie_root_port.c | 1 +
> >> > >   include/hw/pci/pcie_port.h     | 3 +++
> >> > >   2 files changed, 4 insertions(+)
> >> > >
> >> > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> >> > > index 4d588cb..b0e49e1 100644
> >> > > --- a/hw/pci-bridge/pcie_root_port.c
> >> > > +++ b/hw/pci-bridge/pcie_root_port.c
> >> > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
> >> > >   static Property rp_props[] = {
> >> > >       DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
> >> > >                       QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> >> > > +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
> >> > >       DEFINE_PROP_END_OF_LIST()
> >> > >   };
> >> > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> >> > > index 1333266..1b2dd1f 100644
> >> > > --- a/include/hw/pci/pcie_port.h
> >> > > +++ b/include/hw/pci/pcie_port.h
> >> > > @@ -34,6 +34,9 @@ struct PCIEPort {
> >> > >       /* pci express switch port */
> >> > >       uint8_t     port;
> >> > > +
> >> > > +    /* additional buses to reserve on firmware init */
> >> > > +    uint8_t     bus_reserve;
> >> > >   };
> >> > >   void pcie_port_init_reg(PCIDevice *d);
> >> >
> >> > So here is a property and it does not do anything.
> >> > It makes it easier to work on series maybe, but review
> >> > is harder since we do not see what it does at all.
> >> > Please do not split up patches like this - you can maintain
> >> > it split up in your branch if you like and merge before sending.
> >> >
> >>
> >> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
> >>
> >> Thanks,
> >> Marcel
> >>
> >>
> >> > > --
> >> > > 2.7.4
> 
> 
> 
> -- 
> Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Marcel Apfelbaum]

On 25/07/2017 0:58, Michael S. Tsirkin wrote:
> On Tue, Jul 25, 2017 at 12:41:12AM +0300, Alexander Bezzubikov wrote:
>> 2017-07-24 23:46 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>>> On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
>>>> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
>>>>> On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
>>>>>> To enable hotplugging of a newly created pcie-pci-bridge,
>>>>>> we need to tell firmware (SeaBIOS in this case)
>>>>>
>>>>
>>>> Hi Michael,
>>>>
>>>>> Presumably, EFI would need to support this too?
>>>>>
>>>>
>>>> Sure, Eduardo added to CC, but he is in PTO now.
>>>>
>>>>>> to reserve
>>>>>> additional buses for pcie-root-port, that allows us to
>>>>>> hotplug pcie-pci-bridge into this root port.
>>>>>> The number of buses to reserve is provided to the device via a corresponding
>>>>>> property, and to the firmware via new PCI capability (next patch).
>>>>>> The property's default value is 1 as we want to hotplug at least 1 bridge.
>>>>>
>>>>> If so you should just teach firmware to allocate one bus #
>>>>> unconditionally.
>>>>>
>>>>
>>>> That would be a problem for the PCIe machines, since each PCIe
>>>> devices is plugged in a different bus and we are already
>>>> limited to 256 PCIe devices. Allocating an extra-bus always
>>>> would really limit the PCIe devices we can use.
>>>
>>> But this is exactly what this patch does as the property is added to all
>>> buses and default to 1 (1 extra bus).
>>>
>>>>> But why would that be so? What's wrong with a device
>>>>> directly in the root port?
>>>>>
>>>>
>>>> First, plugging a legacy PCI device into a PCIe Root Port
>>>> looks strange at least, and it can;t be done on real HW anyway.
>>>> (incompatible slots)
>>>
>>> You can still plug in PCIe devices there.
>>>
>>>> Second (and more important), if we want 2 or more PCI
>>>> devices we would loose both IO ports space and bus numbers.
>>>
>>> What I am saying is maybe default should not be 1.
>>

Hi Michael, Alexander

>> The only sensible variant left is 0.
>> But as we want pcie-pci-bridge to be used for every legacy PCI device
>> on q35 machine, every time one hotplugs the bridge into the root port,
>> he must be sure rp's prop value >0 (for Linux). I'm not sure
>> that it is a very convenient way to utilize the bridge - always remember
>> to set property.
> 

Is not for Linux only, is for all guest OSes.
I also think setting the property is OK, libvirt can always
add a single PCIe Root Port port with this property set,
while upper layers can create flavors (if the feature needed
or not for the current setup)

> That's what I'm saying then - if in your opinion default is >0 anyway,
> tweak firmware to do it by default.
>

Default should be 0 for sure - because of the hard limitation
on the number of PCIe devices for single PCI domain (the same
as the number of buses, 256).

For a positive value we will should add a property "buses-reserve = x".

>> Another way - we can set this to 0 by default, and to 1 for pcie-root-port,
>> and recommend to use it for hotplugging of the pcie-pci-bridge itself.
> 
> I wonder about something: imagine hotplugging a hierarchy of bridges
> below a root port. It seems that nothing prevents guest from finding a
> free range of buses to cover this hierarchy and setting that as
> secondary/subordinate bus for this bridge.
>  > This does need support on QEMU side to hotplug a hierarchy at once,
> and might need some fixes in Linux, on the plus side you can defer
> management decision on how many are needed until you are actually
> adding something, and you don't need vendor specific patches.
> 

We can teach Linux kernel, that's for sure (OK, almost sure...)
but what we don't want is to be dependent on specific guest Operating
Systems. For example, most configurations are not supported
by Windows guests.

Also is a great opportunity adding PCI IO resources hints
to guest FW, something we wanted to do for some time.

Thanks,
Marcel

> 
>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>>>>>> ---
>>>>>>    hw/pci-bridge/pcie_root_port.c | 1 +
>>>>>>    include/hw/pci/pcie_port.h     | 3 +++
>>>>>>    2 files changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>>>>>> index 4d588cb..b0e49e1 100644
>>>>>> --- a/hw/pci-bridge/pcie_root_port.c
>>>>>> +++ b/hw/pci-bridge/pcie_root_port.c
>>>>>> @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
>>>>>>    static Property rp_props[] = {
>>>>>>        DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>>>>>>                        QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>>>>>> +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
>>>>>>        DEFINE_PROP_END_OF_LIST()
>>>>>>    };
>>>>>> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
>>>>>> index 1333266..1b2dd1f 100644
>>>>>> --- a/include/hw/pci/pcie_port.h
>>>>>> +++ b/include/hw/pci/pcie_port.h
>>>>>> @@ -34,6 +34,9 @@ struct PCIEPort {
>>>>>>        /* pci express switch port */
>>>>>>        uint8_t     port;
>>>>>> +
>>>>>> +    /* additional buses to reserve on firmware init */
>>>>>> +    uint8_t     bus_reserve;
>>>>>>    };
>>>>>>    void pcie_port_init_reg(PCIDevice *d);
>>>>>
>>>>> So here is a property and it does not do anything.
>>>>> It makes it easier to work on series maybe, but review
>>>>> is harder since we do not see what it does at all.
>>>>> Please do not split up patches like this - you can maintain
>>>>> it split up in your branch if you like and merge before sending.
>>>>>
>>>>
>>>> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
>>>>
>>>> Thanks,
>>>> Marcel
>>>>
>>>>
>>>>>> --
>>>>>> 2.7.4
>>
>>
>>
>> -- 
>> Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 6/6] hw/pci: add hint capabilty for additional bus reservation to pcie-root-port

[Marcel Apfelbaum]

On 25/07/2017 0:43, Alexander Bezzubikov wrote:
> 2017-07-24 23:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>> On Sun, Jul 23, 2017 at 01:15:43AM +0300, Aleksandr Bezzubikov wrote:
>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>>> ---
>>>   hw/pci-bridge/pcie_root_port.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>>> index b0e49e1..ca92d85 100644
>>> --- a/hw/pci-bridge/pcie_root_port.c
>>> +++ b/hw/pci-bridge/pcie_root_port.c
>>> @@ -106,6 +106,11 @@ static void rp_realize(PCIDevice *d, Error **errp)
>>>       pcie_aer_root_init(d);
>>>       rp_aer_vector_update(d);
>>>
>>> +    rc = pci_bridge_help_cap_init(d, 0, p->bus_reserve, 0, 0, 0, errp);
>>> +    if (rc < 0) {
>>> +        goto err;
>>> +    }
>>> +
>>>       return;
>>>
>>>   err:
>>
>> It looks like this will add the capability unconditionally to all
>> pcie root ports. Two issues with it:
>> 1. you can't add vendor properties to devices where vendor is
>>     not qemu as they might have their own concept of what it does.
>> 2. this will break compatibility with old machine types,
>>     need to disable for these
>>
> 
> Actually the original idea was to add it for pcie-root-port excusively
> (for now at least), looks like I've confused a little with files naming.

Right, for the Generic PCIe Root Port and not for all the root ports.
In the future we may want to add it to the PCI-brigde so we can have
nested bridges, but we are not there yet.

> Will add it for v3.
> 

Thanks,
Marcel

>>> --
>>> 2.7.4
> 
> 
> 

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Michael S. Tsirkin]

On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
> > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
> > > To enable hotplugging of a newly created pcie-pci-bridge,
> > > we need to tell firmware (SeaBIOS in this case)
> > 
> 
> Hi Michael,
> 
> > Presumably, EFI would need to support this too?
> > 
> 
> Sure, Eduardo added to CC, but he is in PTO now.
> 
> > > to reserve
> > > additional buses for pcie-root-port, that allows us to
> > > hotplug pcie-pci-bridge into this root port.
> > > The number of buses to reserve is provided to the device via a corresponding
> > > property, and to the firmware via new PCI capability (next patch).
> > > The property's default value is 1 as we want to hotplug at least 1 bridge.
> > 
> > If so you should just teach firmware to allocate one bus #
> > unconditionally.
> > 
> 
> That would be a problem for the PCIe machines, since each PCIe
> devices is plugged in a different bus and we are already
> limited to 256 PCIe devices. Allocating an extra-bus always
> would really limit the PCIe devices we can use.

One of the declared advantages of PCIe is easy support for multiple roots.
We really should look at that IMHO so we do not need to pile up hacks.

> > But why would that be so? What's wrong with a device
> > directly in the root port?
> > 

To clarify, my point is we might be wasting bus numbers by reservation
since someone might just want to put pcie devices there.

> First, plugging a legacy PCI device into a PCIe Root Port
> looks strange at least, and it can;t be done on real HW anyway.
> (incompatible slots)
> 
> Second (and more important), if we want 2 or more PCI
> devices we would loose both IO ports space and bus numbers.
> 
> > 
> > > 
> > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> > > ---
> > >   hw/pci-bridge/pcie_root_port.c | 1 +
> > >   include/hw/pci/pcie_port.h     | 3 +++
> > >   2 files changed, 4 insertions(+)
> > > 
> > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> > > index 4d588cb..b0e49e1 100644
> > > --- a/hw/pci-bridge/pcie_root_port.c
> > > +++ b/hw/pci-bridge/pcie_root_port.c
> > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
> > >   static Property rp_props[] = {
> > >       DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
> > >                       QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> > > +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
> > >       DEFINE_PROP_END_OF_LIST()
> > >   };
> > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> > > index 1333266..1b2dd1f 100644
> > > --- a/include/hw/pci/pcie_port.h
> > > +++ b/include/hw/pci/pcie_port.h
> > > @@ -34,6 +34,9 @@ struct PCIEPort {
> > >       /* pci express switch port */
> > >       uint8_t     port;
> > > +
> > > +    /* additional buses to reserve on firmware init */
> > > +    uint8_t     bus_reserve;
> > >   };
> > >   void pcie_port_init_reg(PCIDevice *d);
> > 
> > So here is a property and it does not do anything.
> > It makes it easier to work on series maybe, but review
> > is harder since we do not see what it does at all.
> > Please do not split up patches like this - you can maintain
> > it split up in your branch if you like and merge before sending.
> > 
> 
> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
> 
> Thanks,
> Marcel
> 
> 
> > > -- 
> > > 2.7.4

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Alexander Bezzubikov]

2017-07-25 16:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
>> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
>> > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
>> > > To enable hotplugging of a newly created pcie-pci-bridge,
>> > > we need to tell firmware (SeaBIOS in this case)
>> >
>>
>> Hi Michael,
>>
>> > Presumably, EFI would need to support this too?
>> >
>>
>> Sure, Eduardo added to CC, but he is in PTO now.
>>
>> > > to reserve
>> > > additional buses for pcie-root-port, that allows us to
>> > > hotplug pcie-pci-bridge into this root port.
>> > > The number of buses to reserve is provided to the device via a corresponding
>> > > property, and to the firmware via new PCI capability (next patch).
>> > > The property's default value is 1 as we want to hotplug at least 1 bridge.
>> >
>> > If so you should just teach firmware to allocate one bus #
>> > unconditionally.
>> >
>>
>> That would be a problem for the PCIe machines, since each PCIe
>> devices is plugged in a different bus and we are already
>> limited to 256 PCIe devices. Allocating an extra-bus always
>> would really limit the PCIe devices we can use.
>
> One of the declared advantages of PCIe is easy support for multiple roots.
> We really should look at that IMHO so we do not need to pile up hacks.
>
>> > But why would that be so? What's wrong with a device
>> > directly in the root port?
>> >
>
> To clarify, my point is we might be wasting bus numbers by reservation
> since someone might just want to put pcie devices there.

I think, changing default value to 0 can help us avoid this,
as no bus reservation by default. If one's surely wants
to hotplug pcie-pci-bridge into this root port in future,
the property gives him such an opportunity.
So, sure need pcie-pci-bridge hotplug -> creating a root port with
bus_reserve > 0. Otherwise (and default) - just as now, no changes
in bus topology.

>
>> First, plugging a legacy PCI device into a PCIe Root Port
>> looks strange at least, and it can;t be done on real HW anyway.
>> (incompatible slots)
>>
>> Second (and more important), if we want 2 or more PCI
>> devices we would loose both IO ports space and bus numbers.
>>
>> >
>> > >
>> > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>> > > ---
>> > >   hw/pci-bridge/pcie_root_port.c | 1 +
>> > >   include/hw/pci/pcie_port.h     | 3 +++
>> > >   2 files changed, 4 insertions(+)
>> > >
>> > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>> > > index 4d588cb..b0e49e1 100644
>> > > --- a/hw/pci-bridge/pcie_root_port.c
>> > > +++ b/hw/pci-bridge/pcie_root_port.c
>> > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
>> > >   static Property rp_props[] = {
>> > >       DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>> > >                       QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>> > > +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
>> > >       DEFINE_PROP_END_OF_LIST()
>> > >   };
>> > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
>> > > index 1333266..1b2dd1f 100644
>> > > --- a/include/hw/pci/pcie_port.h
>> > > +++ b/include/hw/pci/pcie_port.h
>> > > @@ -34,6 +34,9 @@ struct PCIEPort {
>> > >       /* pci express switch port */
>> > >       uint8_t     port;
>> > > +
>> > > +    /* additional buses to reserve on firmware init */
>> > > +    uint8_t     bus_reserve;
>> > >   };
>> > >   void pcie_port_init_reg(PCIDevice *d);
>> >
>> > So here is a property and it does not do anything.
>> > It makes it easier to work on series maybe, but review
>> > is harder since we do not see what it does at all.
>> > Please do not split up patches like this - you can maintain
>> > it split up in your branch if you like and merge before sending.
>> >
>>
>> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
>>
>> Thanks,
>> Marcel
>>
>>
>> > > --
>> > > 2.7.4



-- 
Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Michael S. Tsirkin]

On Tue, Jul 25, 2017 at 04:50:49PM +0300, Alexander Bezzubikov wrote:
> 2017-07-25 16:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> > On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
> >> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
> >> > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
> >> > > To enable hotplugging of a newly created pcie-pci-bridge,
> >> > > we need to tell firmware (SeaBIOS in this case)
> >> >
> >>
> >> Hi Michael,
> >>
> >> > Presumably, EFI would need to support this too?
> >> >
> >>
> >> Sure, Eduardo added to CC, but he is in PTO now.
> >>
> >> > > to reserve
> >> > > additional buses for pcie-root-port, that allows us to
> >> > > hotplug pcie-pci-bridge into this root port.
> >> > > The number of buses to reserve is provided to the device via a corresponding
> >> > > property, and to the firmware via new PCI capability (next patch).
> >> > > The property's default value is 1 as we want to hotplug at least 1 bridge.
> >> >
> >> > If so you should just teach firmware to allocate one bus #
> >> > unconditionally.
> >> >
> >>
> >> That would be a problem for the PCIe machines, since each PCIe
> >> devices is plugged in a different bus and we are already
> >> limited to 256 PCIe devices. Allocating an extra-bus always
> >> would really limit the PCIe devices we can use.
> >
> > One of the declared advantages of PCIe is easy support for multiple roots.
> > We really should look at that IMHO so we do not need to pile up hacks.
> >
> >> > But why would that be so? What's wrong with a device
> >> > directly in the root port?
> >> >
> >
> > To clarify, my point is we might be wasting bus numbers by reservation
> > since someone might just want to put pcie devices there.
> 
> I think, changing default value to 0 can help us avoid this,
> as no bus reservation by default. If one's surely wants
> to hotplug pcie-pci-bridge into this root port in future,
> the property gives him such an opportunity.
> So, sure need pcie-pci-bridge hotplug -> creating a root port with
> bus_reserve > 0. Otherwise (and default) - just as now, no changes
> in bus topology.

I guess 0 should mean "do not reserve any buses".  So I think we also
need a flag to just avoid the capability altogether.  Maybe -1?  *That*
should be the default.

> >
> >> First, plugging a legacy PCI device into a PCIe Root Port
> >> looks strange at least, and it can;t be done on real HW anyway.
> >> (incompatible slots)
> >>
> >> Second (and more important), if we want 2 or more PCI
> >> devices we would loose both IO ports space and bus numbers.
> >>
> >> >
> >> > >
> >> > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> >> > > ---
> >> > >   hw/pci-bridge/pcie_root_port.c | 1 +
> >> > >   include/hw/pci/pcie_port.h     | 3 +++
> >> > >   2 files changed, 4 insertions(+)
> >> > >
> >> > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> >> > > index 4d588cb..b0e49e1 100644
> >> > > --- a/hw/pci-bridge/pcie_root_port.c
> >> > > +++ b/hw/pci-bridge/pcie_root_port.c
> >> > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
> >> > >   static Property rp_props[] = {
> >> > >       DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
> >> > >                       QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> >> > > +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
> >> > >       DEFINE_PROP_END_OF_LIST()
> >> > >   };
> >> > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> >> > > index 1333266..1b2dd1f 100644
> >> > > --- a/include/hw/pci/pcie_port.h
> >> > > +++ b/include/hw/pci/pcie_port.h
> >> > > @@ -34,6 +34,9 @@ struct PCIEPort {
> >> > >       /* pci express switch port */
> >> > >       uint8_t     port;
> >> > > +
> >> > > +    /* additional buses to reserve on firmware init */
> >> > > +    uint8_t     bus_reserve;
> >> > >   };
> >> > >   void pcie_port_init_reg(PCIDevice *d);
> >> >
> >> > So here is a property and it does not do anything.
> >> > It makes it easier to work on series maybe, but review
> >> > is harder since we do not see what it does at all.
> >> > Please do not split up patches like this - you can maintain
> >> > it split up in your branch if you like and merge before sending.
> >> >
> >>
> >> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
> >>
> >> Thanks,
> >> Marcel
> >>
> >>
> >> > > --
> >> > > 2.7.4
> 
> 
> 
> -- 
> Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Alexander Bezzubikov]

2017-07-25 16:53 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> On Tue, Jul 25, 2017 at 04:50:49PM +0300, Alexander Bezzubikov wrote:
>> 2017-07-25 16:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>> > On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
>> >> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
>> >> > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
>> >> > > To enable hotplugging of a newly created pcie-pci-bridge,
>> >> > > we need to tell firmware (SeaBIOS in this case)
>> >> >
>> >>
>> >> Hi Michael,
>> >>
>> >> > Presumably, EFI would need to support this too?
>> >> >
>> >>
>> >> Sure, Eduardo added to CC, but he is in PTO now.
>> >>
>> >> > > to reserve
>> >> > > additional buses for pcie-root-port, that allows us to
>> >> > > hotplug pcie-pci-bridge into this root port.
>> >> > > The number of buses to reserve is provided to the device via a corresponding
>> >> > > property, and to the firmware via new PCI capability (next patch).
>> >> > > The property's default value is 1 as we want to hotplug at least 1 bridge.
>> >> >
>> >> > If so you should just teach firmware to allocate one bus #
>> >> > unconditionally.
>> >> >
>> >>
>> >> That would be a problem for the PCIe machines, since each PCIe
>> >> devices is plugged in a different bus and we are already
>> >> limited to 256 PCIe devices. Allocating an extra-bus always
>> >> would really limit the PCIe devices we can use.
>> >
>> > One of the declared advantages of PCIe is easy support for multiple roots.
>> > We really should look at that IMHO so we do not need to pile up hacks.
>> >
>> >> > But why would that be so? What's wrong with a device
>> >> > directly in the root port?
>> >> >
>> >
>> > To clarify, my point is we might be wasting bus numbers by reservation
>> > since someone might just want to put pcie devices there.
>>
>> I think, changing default value to 0 can help us avoid this,
>> as no bus reservation by default. If one's surely wants
>> to hotplug pcie-pci-bridge into this root port in future,
>> the property gives him such an opportunity.
>> So, sure need pcie-pci-bridge hotplug -> creating a root port with
>> bus_reserve > 0. Otherwise (and default) - just as now, no changes
>> in bus topology.
>
> I guess 0 should mean "do not reserve any buses".  So I think we also
> need a flag to just avoid the capability altogether.  Maybe -1?  *That*
> should be the default.

-1 might be useful if any limit value 0 is legal, but is it?
If not, we can set every field to 0 and
this is a sign of avoiding capability since none legal
values are provided.

>
>> >
>> >> First, plugging a legacy PCI device into a PCIe Root Port
>> >> looks strange at least, and it can;t be done on real HW anyway.
>> >> (incompatible slots)
>> >>
>> >> Second (and more important), if we want 2 or more PCI
>> >> devices we would loose both IO ports space and bus numbers.
>> >>
>> >> >
>> >> > >
>> >> > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>> >> > > ---
>> >> > >   hw/pci-bridge/pcie_root_port.c | 1 +
>> >> > >   include/hw/pci/pcie_port.h     | 3 +++
>> >> > >   2 files changed, 4 insertions(+)
>> >> > >
>> >> > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>> >> > > index 4d588cb..b0e49e1 100644
>> >> > > --- a/hw/pci-bridge/pcie_root_port.c
>> >> > > +++ b/hw/pci-bridge/pcie_root_port.c
>> >> > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
>> >> > >   static Property rp_props[] = {
>> >> > >       DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>> >> > >                       QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>> >> > > +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
>> >> > >       DEFINE_PROP_END_OF_LIST()
>> >> > >   };
>> >> > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
>> >> > > index 1333266..1b2dd1f 100644
>> >> > > --- a/include/hw/pci/pcie_port.h
>> >> > > +++ b/include/hw/pci/pcie_port.h
>> >> > > @@ -34,6 +34,9 @@ struct PCIEPort {
>> >> > >       /* pci express switch port */
>> >> > >       uint8_t     port;
>> >> > > +
>> >> > > +    /* additional buses to reserve on firmware init */
>> >> > > +    uint8_t     bus_reserve;
>> >> > >   };
>> >> > >   void pcie_port_init_reg(PCIDevice *d);
>> >> >
>> >> > So here is a property and it does not do anything.
>> >> > It makes it easier to work on series maybe, but review
>> >> > is harder since we do not see what it does at all.
>> >> > Please do not split up patches like this - you can maintain
>> >> > it split up in your branch if you like and merge before sending.
>> >> >
>> >>
>> >> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
>> >>
>> >> Thanks,
>> >> Marcel
>> >>
>> >>
>> >> > > --
>> >> > > 2.7.4
>>
>>
>>
>> --
>> Alexander Bezzubikov



-- 
Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Marcel Apfelbaum]

On 25/07/2017 17:09, Alexander Bezzubikov wrote:
> 2017-07-25 16:53 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>> On Tue, Jul 25, 2017 at 04:50:49PM +0300, Alexander Bezzubikov wrote:
>>> 2017-07-25 16:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>>>> On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
>>>>> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
>>>>>> On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
>>>>>>> To enable hotplugging of a newly created pcie-pci-bridge,
>>>>>>> we need to tell firmware (SeaBIOS in this case)
>>>>>>
>>>>>
>>>>> Hi Michael,
>>>>>
>>>>>> Presumably, EFI would need to support this too?
>>>>>>
>>>>>
>>>>> Sure, Eduardo added to CC, but he is in PTO now.
>>>>>
>>>>>>> to reserve
>>>>>>> additional buses for pcie-root-port, that allows us to
>>>>>>> hotplug pcie-pci-bridge into this root port.
>>>>>>> The number of buses to reserve is provided to the device via a corresponding
>>>>>>> property, and to the firmware via new PCI capability (next patch).
>>>>>>> The property's default value is 1 as we want to hotplug at least 1 bridge.
>>>>>>
>>>>>> If so you should just teach firmware to allocate one bus #
>>>>>> unconditionally.
>>>>>>
>>>>>
>>>>> That would be a problem for the PCIe machines, since each PCIe
>>>>> devices is plugged in a different bus and we are already
>>>>> limited to 256 PCIe devices. Allocating an extra-bus always
>>>>> would really limit the PCIe devices we can use.
>>>>
>>>> One of the declared advantages of PCIe is easy support for multiple roots.
>>>> We really should look at that IMHO so we do not need to pile up hacks.
>>>>
>>>>>> But why would that be so? What's wrong with a device
>>>>>> directly in the root port?
>>>>>>
>>>>
>>>> To clarify, my point is we might be wasting bus numbers by reservation
>>>> since someone might just want to put pcie devices there.
>>>
>>> I think, changing default value to 0 can help us avoid this,
>>> as no bus reservation by default. If one's surely wants
>>> to hotplug pcie-pci-bridge into this root port in future,
>>> the property gives him such an opportunity.
>>> So, sure need pcie-pci-bridge hotplug -> creating a root port with
>>> bus_reserve > 0. Otherwise (and default) - just as now, no changes
>>> in bus topology.
>>
>> I guess 0 should mean "do not reserve any buses".  So I think we also
>> need a flag to just avoid the capability altogether.  Maybe -1?  *That*
>> should be the default.
> 
> -1 might be useful if any limit value 0 is legal, but is it?
> If not, we can set every field to 0 and
> this is a sign of avoiding capability since none legal
> values are provided.
> 

As Gerd suggested, this value is not a "delta" but the number
of buses to be reserved behind the bridge. If I got it right,
0 is not a valid value, since the bridge by definition
has a list one bus behind.

Michael, would you be OK with that?

Thanks,
Marcel

>>
>>>>
>>>>> First, plugging a legacy PCI device into a PCIe Root Port
>>>>> looks strange at least, and it can;t be done on real HW anyway.
>>>>> (incompatible slots)
>>>>>
>>>>> Second (and more important), if we want 2 or more PCI
>>>>> devices we would loose both IO ports space and bus numbers.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>>>>>>> ---
>>>>>>>    hw/pci-bridge/pcie_root_port.c | 1 +
>>>>>>>    include/hw/pci/pcie_port.h     | 3 +++
>>>>>>>    2 files changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>>>>>>> index 4d588cb..b0e49e1 100644
>>>>>>> --- a/hw/pci-bridge/pcie_root_port.c
>>>>>>> +++ b/hw/pci-bridge/pcie_root_port.c
>>>>>>> @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
>>>>>>>    static Property rp_props[] = {
>>>>>>>        DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>>>>>>>                        QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>>>>>>> +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
>>>>>>>        DEFINE_PROP_END_OF_LIST()
>>>>>>>    };
>>>>>>> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
>>>>>>> index 1333266..1b2dd1f 100644
>>>>>>> --- a/include/hw/pci/pcie_port.h
>>>>>>> +++ b/include/hw/pci/pcie_port.h
>>>>>>> @@ -34,6 +34,9 @@ struct PCIEPort {
>>>>>>>        /* pci express switch port */
>>>>>>>        uint8_t     port;
>>>>>>> +
>>>>>>> +    /* additional buses to reserve on firmware init */
>>>>>>> +    uint8_t     bus_reserve;
>>>>>>>    };
>>>>>>>    void pcie_port_init_reg(PCIDevice *d);
>>>>>>
>>>>>> So here is a property and it does not do anything.
>>>>>> It makes it easier to work on series maybe, but review
>>>>>> is harder since we do not see what it does at all.
>>>>>> Please do not split up patches like this - you can maintain
>>>>>> it split up in your branch if you like and merge before sending.
>>>>>>
>>>>>
>>>>> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
>>>>>
>>>>> Thanks,
>>>>> Marcel
>>>>>
>>>>>
>>>>>>> --
>>>>>>> 2.7.4
>>>
>>>
>>>
>>> --
>>> Alexander Bezzubikov
> 
> 
> 

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Alexander Bezzubikov]

вт, 25 июля 2017 г. в 19:10, Marcel Apfelbaum <marcel@redhat.com>:

> On 25/07/2017 17:09, Alexander Bezzubikov wrote:
> > 2017-07-25 16:53 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> >> On Tue, Jul 25, 2017 at 04:50:49PM +0300, Alexander Bezzubikov wrote:
> >>> 2017-07-25 16:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> >>>> On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
> >>>>> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
> >>>>>> On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov
> wrote:
> >>>>>>> To enable hotplugging of a newly created pcie-pci-bridge,
> >>>>>>> we need to tell firmware (SeaBIOS in this case)
> >>>>>>
> >>>>>
> >>>>> Hi Michael,
> >>>>>
> >>>>>> Presumably, EFI would need to support this too?
> >>>>>>
> >>>>>
> >>>>> Sure, Eduardo added to CC, but he is in PTO now.
> >>>>>
> >>>>>>> to reserve
> >>>>>>> additional buses for pcie-root-port, that allows us to
> >>>>>>> hotplug pcie-pci-bridge into this root port.
> >>>>>>> The number of buses to reserve is provided to the device via a
> corresponding
> >>>>>>> property, and to the firmware via new PCI capability (next patch).
> >>>>>>> The property's default value is 1 as we want to hotplug at least 1
> bridge.
> >>>>>>
> >>>>>> If so you should just teach firmware to allocate one bus #
> >>>>>> unconditionally.
> >>>>>>
> >>>>>
> >>>>> That would be a problem for the PCIe machines, since each PCIe
> >>>>> devices is plugged in a different bus and we are already
> >>>>> limited to 256 PCIe devices. Allocating an extra-bus always
> >>>>> would really limit the PCIe devices we can use.
> >>>>
> >>>> One of the declared advantages of PCIe is easy support for multiple
> roots.
> >>>> We really should look at that IMHO so we do not need to pile up hacks.
> >>>>
> >>>>>> But why would that be so? What's wrong with a device
> >>>>>> directly in the root port?
> >>>>>>
> >>>>
> >>>> To clarify, my point is we might be wasting bus numbers by reservation
> >>>> since someone might just want to put pcie devices there.
> >>>
> >>> I think, changing default value to 0 can help us avoid this,
> >>> as no bus reservation by default. If one's surely wants
> >>> to hotplug pcie-pci-bridge into this root port in future,
> >>> the property gives him such an opportunity.
> >>> So, sure need pcie-pci-bridge hotplug -> creating a root port with
> >>> bus_reserve > 0. Otherwise (and default) - just as now, no changes
> >>> in bus topology.
> >>
> >> I guess 0 should mean "do not reserve any buses".  So I think we also
> >> need a flag to just avoid the capability altogether.  Maybe -1?  *That*
> >> should be the default.
> >
> > -1 might be useful if any limit value 0 is legal, but is it?
> > If not, we can set every field to 0 and
> > this is a sign of avoiding capability since none legal
> > values are provided.
> >
>
> As Gerd suggested, this value is not a "delta" but the number
> of buses to be reserved behind the bridge. If I got it right,
> 0 is not a valid value, since the bridge by definition
> has a list one bus behind.


Gerd's suggestion was to set min(cap_value, children_found). From such
point of view 0 can be a valid value.


>
> Michael, would you be OK with that?
>
> Thanks,
> Marcel
>
> >>
> >>>>
> >>>>> First, plugging a legacy PCI device into a PCIe Root Port
> >>>>> looks strange at least, and it can;t be done on real HW anyway.
> >>>>> (incompatible slots)
> >>>>>
> >>>>> Second (and more important), if we want 2 or more PCI
> >>>>> devices we would loose both IO ports space and bus numbers.
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> >>>>>>> ---
> >>>>>>>    hw/pci-bridge/pcie_root_port.c | 1 +
> >>>>>>>    include/hw/pci/pcie_port.h     | 3 +++
> >>>>>>>    2 files changed, 4 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/hw/pci-bridge/pcie_root_port.c
> b/hw/pci-bridge/pcie_root_port.c
> >>>>>>> index 4d588cb..b0e49e1 100644
> >>>>>>> --- a/hw/pci-bridge/pcie_root_port.c
> >>>>>>> +++ b/hw/pci-bridge/pcie_root_port.c
> >>>>>>> @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
> >>>>>>>    static Property rp_props[] = {
> >>>>>>>        DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
> >>>>>>>                        QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> >>>>>>> +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
> >>>>>>>        DEFINE_PROP_END_OF_LIST()
> >>>>>>>    };
> >>>>>>> diff --git a/include/hw/pci/pcie_port.h
> b/include/hw/pci/pcie_port.h
> >>>>>>> index 1333266..1b2dd1f 100644
> >>>>>>> --- a/include/hw/pci/pcie_port.h
> >>>>>>> +++ b/include/hw/pci/pcie_port.h
> >>>>>>> @@ -34,6 +34,9 @@ struct PCIEPort {
> >>>>>>>        /* pci express switch port */
> >>>>>>>        uint8_t     port;
> >>>>>>> +
> >>>>>>> +    /* additional buses to reserve on firmware init */
> >>>>>>> +    uint8_t     bus_reserve;
> >>>>>>>    };
> >>>>>>>    void pcie_port_init_reg(PCIDevice *d);
> >>>>>>
> >>>>>> So here is a property and it does not do anything.
> >>>>>> It makes it easier to work on series maybe, but review
> >>>>>> is harder since we do not see what it does at all.
> >>>>>> Please do not split up patches like this - you can maintain
> >>>>>> it split up in your branch if you like and merge before sending.
> >>>>>>
> >>>>>
> >>>>> Agreed, Alexandr please merge patches 4-5-6 for your next submission.
> >>>>>
> >>>>> Thanks,
> >>>>> Marcel
> >>>>>
> >>>>>
> >>>>>>> --
> >>>>>>> 2.7.4
> >>>
> >>>
> >>>
> >>> --
> >>> Alexander Bezzubikov
> >
> >
> >
>
> --
Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Marcel Apfelbaum]

On 25/07/2017 20:11, Alexander Bezzubikov wrote:
> 
> вт, 25 июля 2017 г. в 19:10, Marcel Apfelbaum <marcel@redhat.com 
> <mailto:marcel@redhat.com>>:
> 
>     On 25/07/2017 17:09, Alexander Bezzubikov wrote:
>      > 2017-07-25 16:53 GMT+03:00 Michael S. Tsirkin <mst@redhat.com
>     <mailto:mst@redhat.com>>:
>      >> On Tue, Jul 25, 2017 at 04:50:49PM +0300, Alexander Bezzubikov
>     wrote:
>      >>> 2017-07-25 16:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com
>     <mailto:mst@redhat.com>>:
>      >>>> On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
>      >>>>> On 23/07/2017 15:22, Michael S. Tsirkin wrote:
>      >>>>>> On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr
>     Bezzubikov wrote:
>      >>>>>>> To enable hotplugging of a newly created pcie-pci-bridge,
>      >>>>>>> we need to tell firmware (SeaBIOS in this case)
>      >>>>>>
>      >>>>>
>      >>>>> Hi Michael,
>      >>>>>
>      >>>>>> Presumably, EFI would need to support this too?
>      >>>>>>
>      >>>>>
>      >>>>> Sure, Eduardo added to CC, but he is in PTO now.
>      >>>>>
>      >>>>>>> to reserve
>      >>>>>>> additional buses for pcie-root-port, that allows us to
>      >>>>>>> hotplug pcie-pci-bridge into this root port.
>      >>>>>>> The number of buses to reserve is provided to the device
>     via a corresponding
>      >>>>>>> property, and to the firmware via new PCI capability (next
>     patch).
>      >>>>>>> The property's default value is 1 as we want to hotplug at
>     least 1 bridge.
>      >>>>>>
>      >>>>>> If so you should just teach firmware to allocate one bus #
>      >>>>>> unconditionally.
>      >>>>>>
>      >>>>>
>      >>>>> That would be a problem for the PCIe machines, since each PCIe
>      >>>>> devices is plugged in a different bus and we are already
>      >>>>> limited to 256 PCIe devices. Allocating an extra-bus always
>      >>>>> would really limit the PCIe devices we can use.
>      >>>>
>      >>>> One of the declared advantages of PCIe is easy support for
>     multiple roots.
>      >>>> We really should look at that IMHO so we do not need to pile
>     up hacks.
>      >>>>
>      >>>>>> But why would that be so? What's wrong with a device
>      >>>>>> directly in the root port?
>      >>>>>>
>      >>>>
>      >>>> To clarify, my point is we might be wasting bus numbers by
>     reservation
>      >>>> since someone might just want to put pcie devices there.
>      >>>
>      >>> I think, changing default value to 0 can help us avoid this,
>      >>> as no bus reservation by default. If one's surely wants
>      >>> to hotplug pcie-pci-bridge into this root port in future,
>      >>> the property gives him such an opportunity.
>      >>> So, sure need pcie-pci-bridge hotplug -> creating a root port with
>      >>> bus_reserve > 0. Otherwise (and default) - just as now, no changes
>      >>> in bus topology.
>      >>
>      >> I guess 0 should mean "do not reserve any buses".  So I think we
>     also
>      >> need a flag to just avoid the capability altogether.  Maybe -1? 
>     *That*
>      >> should be the default.
>      >
>      > -1 might be useful if any limit value 0 is legal, but is it?
>      > If not, we can set every field to 0 and
>      > this is a sign of avoiding capability since none legal
>      > values are provided.
>      >
> 
>     As Gerd suggested, this value is not a "delta" but the number
>     of buses to be reserved behind the bridge. If I got it right,
>     0 is not a valid value, since the bridge by definition
>     has a list one bus behind.
> 
> 
> Gerd's suggestion was to set min(cap_value, children_found). From such 
> point of view 0 can be a valid value.
> 

I am lost now :)
How can we use the capability to reserve "more" buses since
children-found will be always the smaller value?

I think you should use max(cap_value, children_found) to
ensure you always reserve enough buses for existing children.

In this case 0 is actually an invalid value since
children_found > 0 for a bridge.

Thanks,
Marcel

> 
> 
>     Michael, would you be OK with that?
> 
>     Thanks,
>     Marcel
> 
>      >>
>      >>>>
>      >>>>> First, plugging a legacy PCI device into a PCIe Root Port
>      >>>>> looks strange at least, and it can;t be done on real HW anyway.
>      >>>>> (incompatible slots)
>      >>>>>
>      >>>>> Second (and more important), if we want 2 or more PCI
>      >>>>> devices we would loose both IO ports space and bus numbers.
>      >>>>>
>      >>>>>>
>      >>>>>>>
>      >>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com
>     <mailto:zuban32s@gmail.com>>
>      >>>>>>> ---
>      >>>>>>>    hw/pci-bridge/pcie_root_port.c | 1 +
>      >>>>>>>    include/hw/pci/pcie_port.h     | 3 +++
>      >>>>>>>    2 files changed, 4 insertions(+)
>      >>>>>>>
>      >>>>>>> diff --git a/hw/pci-bridge/pcie_root_port.c
>     b/hw/pci-bridge/pcie_root_port.c
>      >>>>>>> index 4d588cb..b0e49e1 100644
>      >>>>>>> --- a/hw/pci-bridge/pcie_root_port.c
>      >>>>>>> +++ b/hw/pci-bridge/pcie_root_port.c
>      >>>>>>> @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
>      >>>>>>>    static Property rp_props[] = {
>      >>>>>>>        DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>      >>>>>>>                        QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>      >>>>>>> +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort,
>     bus_reserve, 1),
>      >>>>>>>        DEFINE_PROP_END_OF_LIST()
>      >>>>>>>    };
>      >>>>>>> diff --git a/include/hw/pci/pcie_port.h
>     b/include/hw/pci/pcie_port.h
>      >>>>>>> index 1333266..1b2dd1f 100644
>      >>>>>>> --- a/include/hw/pci/pcie_port.h
>      >>>>>>> +++ b/include/hw/pci/pcie_port.h
>      >>>>>>> @@ -34,6 +34,9 @@ struct PCIEPort {
>      >>>>>>>        /* pci express switch port */
>      >>>>>>>        uint8_t     port;
>      >>>>>>> +
>      >>>>>>> +    /* additional buses to reserve on firmware init */
>      >>>>>>> +    uint8_t     bus_reserve;
>      >>>>>>>    };
>      >>>>>>>    void pcie_port_init_reg(PCIDevice *d);
>      >>>>>>
>      >>>>>> So here is a property and it does not do anything.
>      >>>>>> It makes it easier to work on series maybe, but review
>      >>>>>> is harder since we do not see what it does at all.
>      >>>>>> Please do not split up patches like this - you can maintain
>      >>>>>> it split up in your branch if you like and merge before sending.
>      >>>>>>
>      >>>>>
>      >>>>> Agreed, Alexandr please merge patches 4-5-6 for your next
>     submission.
>      >>>>>
>      >>>>> Thanks,
>      >>>>> Marcel
>      >>>>>
>      >>>>>
>      >>>>>>> --
>      >>>>>>> 2.7.4
>      >>>
>      >>>
>      >>>
>      >>> --
>      >>> Alexander Bezzubikov
>      >
>      >
>      >
> 
> -- 
> Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Gerd Hoffmann]

  Hi,

> > Gerd's suggestion was to set min(cap_value, children_found). From
> > such 
> > point of view 0 can be a valid value.
> > 
> 
> I am lost now :)
> How can we use the capability to reserve "more" buses since
> children-found will be always the smaller value?
> 
> I think you should use max(cap_value, children_found) to
> ensure you always reserve enough buses for existing children.

Was about to point out the same ;)

Yes, should be max(reserve-buses, child-buses-found).  And reserve-
buses is actually a minimum, because the result can't be smaller than
reserve-buses.

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Michael S. Tsirkin]

On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
> On PCI init PCI bridges may need some
> extra info about bus number to reserve, IO, memory and
> prefetchable memory limits. QEMU can provide this
> with special

with a special

> vendor-specific PCI capability.
> 
> Sizes of limits match ones from
> PCI Type 1 Configuration Space Header,
> number of buses to reserve occupies only 1 byte 
> since it is the size of Subordinate Bus Number register.
> 
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> ---
>  hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
>  include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 720119b..8ec6c2c 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>      br->bus_name = bus_name;
>  }
>  
> +
> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,

help? should be qemu_cap_init?

> +                              uint8_t bus_reserve, uint32_t io_limit,
> +                              uint16_t mem_limit, uint64_t pref_limit,
> +                              Error **errp)
> +{
> +    size_t cap_len = sizeof(PCIBridgeQemuCap);
> +    PCIBridgeQemuCap cap;

This leaks info to guest. You want to init all fields here:

cap = {
 .len = ....
};

> +
> +    cap.len = cap_len;
> +    cap.bus_res = bus_reserve;
> +    cap.io_lim = io_limit & 0xFF;
> +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
> +    cap.mem_lim = mem_limit;
> +    cap.pref_lim = pref_limit & 0xFFFF;
> +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;

Please use pci_set_word etc or cpu_to_leXX.

I think it's easiest to replace struct with a set of macros then
pci_set_word does the work for you.


> +
> +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
> +                                    cap_offset, cap_len, errp);
> +    if (offset < 0) {
> +        return offset;
> +    }
> +
> +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);

+2 is yacky. See how virtio does it:

    memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
           cap->cap_len - PCI_CAP_FLAGS);


> +    return 0;
> +}
> +
>  static const TypeInfo pci_bridge_type_info = {
>      .name = TYPE_PCI_BRIDGE,
>      .parent = TYPE_PCI_DEVICE,
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index ff7cbaa..c9f642c 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>  #define  PCI_BRIDGE_CTL_DISCARD_STATUS	0x400	/* Discard timer status */
>  #define  PCI_BRIDGE_CTL_DISCARD_SERR	0x800	/* Discard timer SERR# enable */
>  
> +typedef struct PCIBridgeQemuCap {
> +    uint8_t id;     /* Standard PCI capability header field */
> +    uint8_t next;   /* Standard PCI capability header field */
> +    uint8_t len;    /* Standard PCI vendor-specific capability header field */
> +    uint8_t bus_res;
> +    uint32_t pref_lim_upper;

Big endian? Ugh.

> +    uint16_t pref_lim;
> +    uint16_t mem_lim;

I'd say we need 64 bit for memory.

> +    uint16_t io_lim_upper;
> +    uint8_t io_lim;
> +    uint8_t padding;

IMHO each type should have a special "don't care" flag
that would mean "I don't know".


> +} PCIBridgeQemuCap;

You don't really need this struct in the header. And pls document all fields.

> +
> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> +                              uint8_t bus_reserve, uint32_t io_limit,
> +                              uint16_t mem_limit, uint64_t pref_limit,
> +                              Error **errp);
> +
>  #endif /* QEMU_PCI_BRIDGE_H */
> -- 
> 2.7.4

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Alexander Bezzubikov]

2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
>> On PCI init PCI bridges may need some
>> extra info about bus number to reserve, IO, memory and
>> prefetchable memory limits. QEMU can provide this
>> with special
>
> with a special
>
>> vendor-specific PCI capability.
>>
>> Sizes of limits match ones from
>> PCI Type 1 Configuration Space Header,
>> number of buses to reserve occupies only 1 byte
>> since it is the size of Subordinate Bus Number register.
>>
>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>> ---
>>  hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
>>  include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>> index 720119b..8ec6c2c 100644
>> --- a/hw/pci/pci_bridge.c
>> +++ b/hw/pci/pci_bridge.c
>> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>>      br->bus_name = bus_name;
>>  }
>>
>> +
>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>
> help? should be qemu_cap_init?
>
>> +                              uint8_t bus_reserve, uint32_t io_limit,
>> +                              uint16_t mem_limit, uint64_t pref_limit,
>> +                              Error **errp)
>> +{
>> +    size_t cap_len = sizeof(PCIBridgeQemuCap);
>> +    PCIBridgeQemuCap cap;
>
> This leaks info to guest. You want to init all fields here:
>
> cap = {
>  .len = ....
> };

I surely can do this for len field, but as Laszlo proposed
we can use mutually exclusive fields,
e.g. pref_32 and pref_64, the only way I have left
is to use ternary operator (if we surely need this
big initializer). Keeping some if's would look better,
I think.

>
>> +
>> +    cap.len = cap_len;
>> +    cap.bus_res = bus_reserve;
>> +    cap.io_lim = io_limit & 0xFF;
>> +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
>> +    cap.mem_lim = mem_limit;
>> +    cap.pref_lim = pref_limit & 0xFFFF;
>> +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
>
> Please use pci_set_word etc or cpu_to_leXX.
>

Since now we've decided to avoid fields separation into <field> + <field_upper>,
this bitmask along with pci_set_word are no longer needed.

> I think it's easiest to replace struct with a set of macros then
> pci_set_word does the work for you.
>

I don't really want to use macros here because structure
show us the whole capability layout and this can
decrease documenting efforts. More than that,
memcpy usage is very convenient here, and I wouldn't like
to lose it.

>
>> +
>> +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
>> +                                    cap_offset, cap_len, errp);
>> +    if (offset < 0) {
>> +        return offset;
>> +    }
>> +
>> +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
>
> +2 is yacky. See how virtio does it:
>
>     memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
>            cap->cap_len - PCI_CAP_FLAGS);
>
>

OK.

>> +    return 0;
>> +}
>> +
>>  static const TypeInfo pci_bridge_type_info = {
>>      .name = TYPE_PCI_BRIDGE,
>>      .parent = TYPE_PCI_DEVICE,
>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
>> index ff7cbaa..c9f642c 100644
>> --- a/include/hw/pci/pci_bridge.h
>> +++ b/include/hw/pci/pci_bridge.h
>> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>>  #define  PCI_BRIDGE_CTL_DISCARD_STATUS       0x400   /* Discard timer status */
>>  #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer SERR# enable */
>>
>> +typedef struct PCIBridgeQemuCap {
>> +    uint8_t id;     /* Standard PCI capability header field */
>> +    uint8_t next;   /* Standard PCI capability header field */
>> +    uint8_t len;    /* Standard PCI vendor-specific capability header field */
>> +    uint8_t bus_res;
>> +    uint32_t pref_lim_upper;
>
> Big endian? Ugh.
>

Agreed, and this's gonna to disappear with
the new layout.

>> +    uint16_t pref_lim;
>> +    uint16_t mem_lim;
>
> I'd say we need 64 bit for memory.
>

Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.

>> +    uint16_t io_lim_upper;
>> +    uint8_t io_lim;
>> +    uint8_t padding;
>
> IMHO each type should have a special "don't care" flag
> that would mean "I don't know".
>
>

Don't know what? Now 0 is an indicator to do nothing with this field.

>> +} PCIBridgeQemuCap;
>
> You don't really need this struct in the header. And pls document all fields.
>
>> +
>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>> +                              uint8_t bus_reserve, uint32_t io_limit,
>> +                              uint16_t mem_limit, uint64_t pref_limit,
>> +                              Error **errp);
>> +
>>  #endif /* QEMU_PCI_BRIDGE_H */
>> --
>> 2.7.4



--
Alexander Bezzubikov

Re: [Qemu-devel] [SeaBIOS] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Laszlo Ersek]

On 07/26/17 23:54, Alexander Bezzubikov wrote:
> 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>> On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:

>>> +    PCIBridgeQemuCap cap;
>>
>> This leaks info to guest. You want to init all fields here:
>>
>> cap = {
>>  .len = ....
>> };
> 
> I surely can do this for len field, but as Laszlo proposed
> we can use mutually exclusive fields,
> e.g. pref_32 and pref_64, the only way I have left
> is to use ternary operator (if we surely need this
> big initializer). Keeping some if's would look better,
> I think.

I think it's fine to use "if"s in order to set up the structure
partially / gradually, but then please clear the structure up-front:


  PCIBridgeQemuCap cap = { 0 };

(In general "{ 0 }" is the best initializer ever, because it can
zero-init a variable of *any* type at all. Gcc might complain about the
inexact depth of {} nesting of course, but it's nonetheless valid C.)

Or else add a memset-to-zero.

Or else, do just

  PCIBridgeQemuCap cap = { .len = ... };

which will zero-fill every other field. ("[...] all subobjects that are
not initialized explicitly shall be initialized implicitly the same as
objects that have static storage duration").

Thanks
Laszlo

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Michael S. Tsirkin]

On Thu, Jul 27, 2017 at 12:54:07AM +0300, Alexander Bezzubikov wrote:
> 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> > On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
> >> On PCI init PCI bridges may need some
> >> extra info about bus number to reserve, IO, memory and
> >> prefetchable memory limits. QEMU can provide this
> >> with special
> >
> > with a special
> >
> >> vendor-specific PCI capability.
> >>
> >> Sizes of limits match ones from
> >> PCI Type 1 Configuration Space Header,
> >> number of buses to reserve occupies only 1 byte
> >> since it is the size of Subordinate Bus Number register.
> >>
> >> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> >> ---
> >>  hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
> >>  include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
> >>  2 files changed, 45 insertions(+)
> >>
> >> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> >> index 720119b..8ec6c2c 100644
> >> --- a/hw/pci/pci_bridge.c
> >> +++ b/hw/pci/pci_bridge.c
> >> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
> >>      br->bus_name = bus_name;
> >>  }
> >>
> >> +
> >> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> >
> > help? should be qemu_cap_init?
> >
> >> +                              uint8_t bus_reserve, uint32_t io_limit,
> >> +                              uint16_t mem_limit, uint64_t pref_limit,
> >> +                              Error **errp)
> >> +{
> >> +    size_t cap_len = sizeof(PCIBridgeQemuCap);
> >> +    PCIBridgeQemuCap cap;
> >
> > This leaks info to guest. You want to init all fields here:
> >
> > cap = {
> >  .len = ....
> > };
> 
> I surely can do this for len field, but as Laszlo proposed
> we can use mutually exclusive fields,
> e.g. pref_32 and pref_64, the only way I have left
> is to use ternary operator (if we surely need this
> big initializer). Keeping some if's would look better,
> I think.
> 
> >
> >> +
> >> +    cap.len = cap_len;
> >> +    cap.bus_res = bus_reserve;
> >> +    cap.io_lim = io_limit & 0xFF;
> >> +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
> >> +    cap.mem_lim = mem_limit;
> >> +    cap.pref_lim = pref_limit & 0xFFFF;
> >> +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
> >
> > Please use pci_set_word etc or cpu_to_leXX.
> >
> 
> Since now we've decided to avoid fields separation into <field> + <field_upper>,
> this bitmask along with pci_set_word are no longer needed.
> 
> > I think it's easiest to replace struct with a set of macros then
> > pci_set_word does the work for you.
> >
> 
> I don't really want to use macros here because structure
> show us the whole capability layout and this can
> decrease documenting efforts. More than that,
> memcpy usage is very convenient here, and I wouldn't like
> to lose it.
> 
> >
> >> +
> >> +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
> >> +                                    cap_offset, cap_len, errp);
> >> +    if (offset < 0) {
> >> +        return offset;
> >> +    }
> >> +
> >> +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
> >
> > +2 is yacky. See how virtio does it:
> >
> >     memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
> >            cap->cap_len - PCI_CAP_FLAGS);
> >
> >
> 
> OK.
> 
> >> +    return 0;
> >> +}
> >> +
> >>  static const TypeInfo pci_bridge_type_info = {
> >>      .name = TYPE_PCI_BRIDGE,
> >>      .parent = TYPE_PCI_DEVICE,
> >> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> >> index ff7cbaa..c9f642c 100644
> >> --- a/include/hw/pci/pci_bridge.h
> >> +++ b/include/hw/pci/pci_bridge.h
> >> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
> >>  #define  PCI_BRIDGE_CTL_DISCARD_STATUS       0x400   /* Discard timer status */
> >>  #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer SERR# enable */
> >>
> >> +typedef struct PCIBridgeQemuCap {
> >> +    uint8_t id;     /* Standard PCI capability header field */
> >> +    uint8_t next;   /* Standard PCI capability header field */
> >> +    uint8_t len;    /* Standard PCI vendor-specific capability header field */
> >> +    uint8_t bus_res;
> >> +    uint32_t pref_lim_upper;
> >
> > Big endian? Ugh.
> >
> 
> Agreed, and this's gonna to disappear with
> the new layout.
> 
> >> +    uint16_t pref_lim;
> >> +    uint16_t mem_lim;
> >
> > I'd say we need 64 bit for memory.
> >
> 
> Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.

Hmm ok, but e.g. for io there are bridges that have extra registers
to specify non-standard non-aligned registers.

> >> +    uint16_t io_lim_upper;
> >> +    uint8_t io_lim;
> >> +    uint8_t padding;
> >
> > IMHO each type should have a special "don't care" flag
> > that would mean "I don't know".
> >
> >
> 
> Don't know what? Now 0 is an indicator to do nothing with this field.

In that case how do you say "don't allocate any memory"?


> >> +} PCIBridgeQemuCap;
> >
> > You don't really need this struct in the header. And pls document all fields.
> >
> >> +
> >> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> >> +                              uint8_t bus_reserve, uint32_t io_limit,
> >> +                              uint16_t mem_limit, uint64_t pref_limit,
> >> +                              Error **errp);
> >> +
> >>  #endif /* QEMU_PCI_BRIDGE_H */
> >> --
> >> 2.7.4
> 
> 
> 
> --
> Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Marcel Apfelbaum]

On 27/07/2017 2:28, Michael S. Tsirkin wrote:
> On Thu, Jul 27, 2017 at 12:54:07AM +0300, Alexander Bezzubikov wrote:
>> 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>>> On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
>>>> On PCI init PCI bridges may need some
>>>> extra info about bus number to reserve, IO, memory and
>>>> prefetchable memory limits. QEMU can provide this
>>>> with special
>>>
>>> with a special
>>>
>>>> vendor-specific PCI capability.
>>>>
>>>> Sizes of limits match ones from
>>>> PCI Type 1 Configuration Space Header,
>>>> number of buses to reserve occupies only 1 byte
>>>> since it is the size of Subordinate Bus Number register.
>>>>
>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>>>> ---
>>>>   hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
>>>>   include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
>>>>   2 files changed, 45 insertions(+)
>>>>
>>>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>>>> index 720119b..8ec6c2c 100644
>>>> --- a/hw/pci/pci_bridge.c
>>>> +++ b/hw/pci/pci_bridge.c
>>>> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>>>>       br->bus_name = bus_name;
>>>>   }
>>>>
>>>> +
>>>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>>>
>>> help? should be qemu_cap_init?
>>>
>>>> +                              uint8_t bus_reserve, uint32_t io_limit,
>>>> +                              uint16_t mem_limit, uint64_t pref_limit,
>>>> +                              Error **errp)
>>>> +{
>>>> +    size_t cap_len = sizeof(PCIBridgeQemuCap);
>>>> +    PCIBridgeQemuCap cap;
>>>
>>> This leaks info to guest. You want to init all fields here:
>>>
>>> cap = {
>>>   .len = ....
>>> };
>>
>> I surely can do this for len field, but as Laszlo proposed
>> we can use mutually exclusive fields,
>> e.g. pref_32 and pref_64, the only way I have left
>> is to use ternary operator (if we surely need this
>> big initializer). Keeping some if's would look better,
>> I think.
>>
>>>
>>>> +
>>>> +    cap.len = cap_len;
>>>> +    cap.bus_res = bus_reserve;
>>>> +    cap.io_lim = io_limit & 0xFF;
>>>> +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
>>>> +    cap.mem_lim = mem_limit;
>>>> +    cap.pref_lim = pref_limit & 0xFFFF;
>>>> +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
>>>
>>> Please use pci_set_word etc or cpu_to_leXX.
>>>
>>
>> Since now we've decided to avoid fields separation into <field> + <field_upper>,
>> this bitmask along with pci_set_word are no longer needed.
>>
>>> I think it's easiest to replace struct with a set of macros then
>>> pci_set_word does the work for you.
>>>
>>
>> I don't really want to use macros here because structure
>> show us the whole capability layout and this can
>> decrease documenting efforts. More than that,
>> memcpy usage is very convenient here, and I wouldn't like
>> to lose it.
>>
>>>
>>>> +
>>>> +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
>>>> +                                    cap_offset, cap_len, errp);
>>>> +    if (offset < 0) {
>>>> +        return offset;
>>>> +    }
>>>> +
>>>> +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
>>>
>>> +2 is yacky. See how virtio does it:
>>>
>>>      memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
>>>             cap->cap_len - PCI_CAP_FLAGS);
>>>
>>>
>>
>> OK.
>>
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static const TypeInfo pci_bridge_type_info = {
>>>>       .name = TYPE_PCI_BRIDGE,
>>>>       .parent = TYPE_PCI_DEVICE,
>>>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
>>>> index ff7cbaa..c9f642c 100644
>>>> --- a/include/hw/pci/pci_bridge.h
>>>> +++ b/include/hw/pci/pci_bridge.h
>>>> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>>>>   #define  PCI_BRIDGE_CTL_DISCARD_STATUS       0x400   /* Discard timer status */
>>>>   #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer SERR# enable */
>>>>
>>>> +typedef struct PCIBridgeQemuCap {
>>>> +    uint8_t id;     /* Standard PCI capability header field */
>>>> +    uint8_t next;   /* Standard PCI capability header field */
>>>> +    uint8_t len;    /* Standard PCI vendor-specific capability header field */
>>>> +    uint8_t bus_res;
>>>> +    uint32_t pref_lim_upper;
>>>
>>> Big endian? Ugh.
>>>
>>
>> Agreed, and this's gonna to disappear with
>> the new layout.
>>
>>>> +    uint16_t pref_lim;
>>>> +    uint16_t mem_lim;
>>>
>>> I'd say we need 64 bit for memory.
>>>
>>
>> Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.
> 
> Hmm ok, but e.g. for io there are bridges that have extra registers
> to specify non-standard non-aligned registers.
> 
>>>> +    uint16_t io_lim_upper;
>>>> +    uint8_t io_lim;
>>>> +    uint8_t padding;
>>>
>>> IMHO each type should have a special "don't care" flag
>>> that would mean "I don't know".
>>>
>>>
>>
>> Don't know what? Now 0 is an indicator to do nothing with this field.
> 
> In that case how do you say "don't allocate any memory"?
> 

We can keep the MEM/Limit registers read-only for such cases,
as they are optional registers.

Thanks,
Marcel

> 
>>>> +} PCIBridgeQemuCap;
>>>
>>> You don't really need this struct in the header. And pls document all fields.
>>>
>>>> +
>>>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>>>> +                              uint8_t bus_reserve, uint32_t io_limit,
>>>> +                              uint16_t mem_limit, uint64_t pref_limit,
>>>> +                              Error **errp);
>>>> +
>>>>   #endif /* QEMU_PCI_BRIDGE_H */
>>>> --
>>>> 2.7.4
>>
>>
>>
>> --
>> Alexander Bezzubikov

Re: [Qemu-devel] [SeaBIOS] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Laszlo Ersek]

On 07/27/17 11:39, Marcel Apfelbaum wrote:
> On 27/07/2017 2:28, Michael S. Tsirkin wrote:
>> On Thu, Jul 27, 2017 at 12:54:07AM +0300, Alexander Bezzubikov wrote:
>>> 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>>>> On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
>>>>> On PCI init PCI bridges may need some
>>>>> extra info about bus number to reserve, IO, memory and
>>>>> prefetchable memory limits. QEMU can provide this
>>>>> with special
>>>>
>>>> with a special
>>>>
>>>>> vendor-specific PCI capability.
>>>>>
>>>>> Sizes of limits match ones from
>>>>> PCI Type 1 Configuration Space Header,
>>>>> number of buses to reserve occupies only 1 byte
>>>>> since it is the size of Subordinate Bus Number register.
>>>>>
>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>>>>> ---
>>>>>   hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
>>>>>   include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
>>>>>   2 files changed, 45 insertions(+)
>>>>>
>>>>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>>>>> index 720119b..8ec6c2c 100644
>>>>> --- a/hw/pci/pci_bridge.c
>>>>> +++ b/hw/pci/pci_bridge.c
>>>>> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const
>>>>> char* bus_name,
>>>>>       br->bus_name = bus_name;
>>>>>   }
>>>>>
>>>>> +
>>>>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>>>>
>>>> help? should be qemu_cap_init?
>>>>
>>>>> +                              uint8_t bus_reserve, uint32_t io_limit,
>>>>> +                              uint16_t mem_limit, uint64_t
>>>>> pref_limit,
>>>>> +                              Error **errp)
>>>>> +{
>>>>> +    size_t cap_len = sizeof(PCIBridgeQemuCap);
>>>>> +    PCIBridgeQemuCap cap;
>>>>
>>>> This leaks info to guest. You want to init all fields here:
>>>>
>>>> cap = {
>>>>   .len = ....
>>>> };
>>>
>>> I surely can do this for len field, but as Laszlo proposed
>>> we can use mutually exclusive fields,
>>> e.g. pref_32 and pref_64, the only way I have left
>>> is to use ternary operator (if we surely need this
>>> big initializer). Keeping some if's would look better,
>>> I think.
>>>
>>>>
>>>>> +
>>>>> +    cap.len = cap_len;
>>>>> +    cap.bus_res = bus_reserve;
>>>>> +    cap.io_lim = io_limit & 0xFF;
>>>>> +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
>>>>> +    cap.mem_lim = mem_limit;
>>>>> +    cap.pref_lim = pref_limit & 0xFFFF;
>>>>> +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
>>>>
>>>> Please use pci_set_word etc or cpu_to_leXX.
>>>>
>>>
>>> Since now we've decided to avoid fields separation into <field> +
>>> <field_upper>,
>>> this bitmask along with pci_set_word are no longer needed.
>>>
>>>> I think it's easiest to replace struct with a set of macros then
>>>> pci_set_word does the work for you.
>>>>
>>>
>>> I don't really want to use macros here because structure
>>> show us the whole capability layout and this can
>>> decrease documenting efforts. More than that,
>>> memcpy usage is very convenient here, and I wouldn't like
>>> to lose it.
>>>
>>>>
>>>>> +
>>>>> +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
>>>>> +                                    cap_offset, cap_len, errp);
>>>>> +    if (offset < 0) {
>>>>> +        return offset;
>>>>> +    }
>>>>> +
>>>>> +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
>>>>
>>>> +2 is yacky. See how virtio does it:
>>>>
>>>>      memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
>>>>             cap->cap_len - PCI_CAP_FLAGS);
>>>>
>>>>
>>>
>>> OK.
>>>
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>   static const TypeInfo pci_bridge_type_info = {
>>>>>       .name = TYPE_PCI_BRIDGE,
>>>>>       .parent = TYPE_PCI_DEVICE,
>>>>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
>>>>> index ff7cbaa..c9f642c 100644
>>>>> --- a/include/hw/pci/pci_bridge.h
>>>>> +++ b/include/hw/pci/pci_bridge.h
>>>>> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const
>>>>> char* bus_name,
>>>>>   #define  PCI_BRIDGE_CTL_DISCARD_STATUS       0x400   /* Discard
>>>>> timer status */
>>>>>   #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer
>>>>> SERR# enable */
>>>>>
>>>>> +typedef struct PCIBridgeQemuCap {
>>>>> +    uint8_t id;     /* Standard PCI capability header field */
>>>>> +    uint8_t next;   /* Standard PCI capability header field */
>>>>> +    uint8_t len;    /* Standard PCI vendor-specific capability
>>>>> header field */
>>>>> +    uint8_t bus_res;
>>>>> +    uint32_t pref_lim_upper;
>>>>
>>>> Big endian? Ugh.
>>>>
>>>
>>> Agreed, and this's gonna to disappear with
>>> the new layout.
>>>
>>>>> +    uint16_t pref_lim;
>>>>> +    uint16_t mem_lim;
>>>>
>>>> I'd say we need 64 bit for memory.
>>>>
>>>
>>> Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.
>>
>> Hmm ok, but e.g. for io there are bridges that have extra registers
>> to specify non-standard non-aligned registers.
>>
>>>>> +    uint16_t io_lim_upper;
>>>>> +    uint8_t io_lim;
>>>>> +    uint8_t padding;
>>>>
>>>> IMHO each type should have a special "don't care" flag
>>>> that would mean "I don't know".
>>>>
>>>>
>>>
>>> Don't know what? Now 0 is an indicator to do nothing with this field.
>>
>> In that case how do you say "don't allocate any memory"?
>>
> 
> We can keep the MEM/Limit registers read-only for such cases,
> as they are optional registers.

For OVMF, it would be really nice if OVMF could gather the reservation
numbers with
- read-only accesses
- to the one exact hotplug controller (root port, bridge, etc) that's
  being queried for reservation.

Deciding whether some registers in config space are r/o would require
OVMF to attempt a write and check the result (and if it succeeded, to
restore the original value). I'm not too attracted to doing this in a
platform hook, while PciBusDxe is in the middle of enumerating /
configuring the PCI(e) hierarchy :)

Thanks
Laszlo

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Michael S. Tsirkin]

On Thu, Jul 27, 2017 at 12:39:54PM +0300, Marcel Apfelbaum wrote:
> On 27/07/2017 2:28, Michael S. Tsirkin wrote:
> > On Thu, Jul 27, 2017 at 12:54:07AM +0300, Alexander Bezzubikov wrote:
> > > 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> > > > On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
> > > > > On PCI init PCI bridges may need some
> > > > > extra info about bus number to reserve, IO, memory and
> > > > > prefetchable memory limits. QEMU can provide this
> > > > > with special
> > > > 
> > > > with a special
> > > > 
> > > > > vendor-specific PCI capability.
> > > > > 
> > > > > Sizes of limits match ones from
> > > > > PCI Type 1 Configuration Space Header,
> > > > > number of buses to reserve occupies only 1 byte
> > > > > since it is the size of Subordinate Bus Number register.
> > > > > 
> > > > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> > > > > ---
> > > > >   hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
> > > > >   include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
> > > > >   2 files changed, 45 insertions(+)
> > > > > 
> > > > > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > > > > index 720119b..8ec6c2c 100644
> > > > > --- a/hw/pci/pci_bridge.c
> > > > > +++ b/hw/pci/pci_bridge.c
> > > > > @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
> > > > >       br->bus_name = bus_name;
> > > > >   }
> > > > > 
> > > > > +
> > > > > +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> > > > 
> > > > help? should be qemu_cap_init?
> > > > 
> > > > > +                              uint8_t bus_reserve, uint32_t io_limit,
> > > > > +                              uint16_t mem_limit, uint64_t pref_limit,
> > > > > +                              Error **errp)
> > > > > +{
> > > > > +    size_t cap_len = sizeof(PCIBridgeQemuCap);
> > > > > +    PCIBridgeQemuCap cap;
> > > > 
> > > > This leaks info to guest. You want to init all fields here:
> > > > 
> > > > cap = {
> > > >   .len = ....
> > > > };
> > > 
> > > I surely can do this for len field, but as Laszlo proposed
> > > we can use mutually exclusive fields,
> > > e.g. pref_32 and pref_64, the only way I have left
> > > is to use ternary operator (if we surely need this
> > > big initializer). Keeping some if's would look better,
> > > I think.
> > > 
> > > > 
> > > > > +
> > > > > +    cap.len = cap_len;
> > > > > +    cap.bus_res = bus_reserve;
> > > > > +    cap.io_lim = io_limit & 0xFF;
> > > > > +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
> > > > > +    cap.mem_lim = mem_limit;
> > > > > +    cap.pref_lim = pref_limit & 0xFFFF;
> > > > > +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
> > > > 
> > > > Please use pci_set_word etc or cpu_to_leXX.
> > > > 
> > > 
> > > Since now we've decided to avoid fields separation into <field> + <field_upper>,
> > > this bitmask along with pci_set_word are no longer needed.
> > > 
> > > > I think it's easiest to replace struct with a set of macros then
> > > > pci_set_word does the work for you.
> > > > 
> > > 
> > > I don't really want to use macros here because structure
> > > show us the whole capability layout and this can
> > > decrease documenting efforts. More than that,
> > > memcpy usage is very convenient here, and I wouldn't like
> > > to lose it.
> > > 
> > > > 
> > > > > +
> > > > > +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
> > > > > +                                    cap_offset, cap_len, errp);
> > > > > +    if (offset < 0) {
> > > > > +        return offset;
> > > > > +    }
> > > > > +
> > > > > +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
> > > > 
> > > > +2 is yacky. See how virtio does it:
> > > > 
> > > >      memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
> > > >             cap->cap_len - PCI_CAP_FLAGS);
> > > > 
> > > > 
> > > 
> > > OK.
> > > 
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > >   static const TypeInfo pci_bridge_type_info = {
> > > > >       .name = TYPE_PCI_BRIDGE,
> > > > >       .parent = TYPE_PCI_DEVICE,
> > > > > diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> > > > > index ff7cbaa..c9f642c 100644
> > > > > --- a/include/hw/pci/pci_bridge.h
> > > > > +++ b/include/hw/pci/pci_bridge.h
> > > > > @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
> > > > >   #define  PCI_BRIDGE_CTL_DISCARD_STATUS       0x400   /* Discard timer status */
> > > > >   #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer SERR# enable */
> > > > > 
> > > > > +typedef struct PCIBridgeQemuCap {
> > > > > +    uint8_t id;     /* Standard PCI capability header field */
> > > > > +    uint8_t next;   /* Standard PCI capability header field */
> > > > > +    uint8_t len;    /* Standard PCI vendor-specific capability header field */
> > > > > +    uint8_t bus_res;
> > > > > +    uint32_t pref_lim_upper;
> > > > 
> > > > Big endian? Ugh.
> > > > 
> > > 
> > > Agreed, and this's gonna to disappear with
> > > the new layout.
> > > 
> > > > > +    uint16_t pref_lim;
> > > > > +    uint16_t mem_lim;
> > > > 
> > > > I'd say we need 64 bit for memory.
> > > > 
> > > 
> > > Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.
> > 
> > Hmm ok, but e.g. for io there are bridges that have extra registers
> > to specify non-standard non-aligned registers.
> > 
> > > > > +    uint16_t io_lim_upper;
> > > > > +    uint8_t io_lim;
> > > > > +    uint8_t padding;
> > > > 
> > > > IMHO each type should have a special "don't care" flag
> > > > that would mean "I don't know".
> > > > 
> > > > 
> > > 
> > > Don't know what? Now 0 is an indicator to do nothing with this field.
> > 
> > In that case how do you say "don't allocate any memory"?
> > 
> 
> We can keep the MEM/Limit registers read-only for such cases,
> as they are optional registers.
> 
> Thanks,
> Marcel

I don't believe they are - from the spec (1.2):
	The Memory Base and Memory Limit registers are both required registers

Rest of ranges are indeed optional.


> > 
> > > > > +} PCIBridgeQemuCap;
> > > > 
> > > > You don't really need this struct in the header. And pls document all fields.
> > > > 
> > > > > +
> > > > > +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> > > > > +                              uint8_t bus_reserve, uint32_t io_limit,
> > > > > +                              uint16_t mem_limit, uint64_t pref_limit,
> > > > > +                              Error **errp);
> > > > > +
> > > > >   #endif /* QEMU_PCI_BRIDGE_H */
> > > > > --
> > > > > 2.7.4
> > > 
> > > 
> > > 
> > > --
> > > Alexander Bezzubikov

Re: [Qemu-devel] [SeaBIOS] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Michael S. Tsirkin]

On Thu, Jul 27, 2017 at 03:58:58PM +0200, Laszlo Ersek wrote:
> On 07/27/17 11:39, Marcel Apfelbaum wrote:
> > On 27/07/2017 2:28, Michael S. Tsirkin wrote:
> >> On Thu, Jul 27, 2017 at 12:54:07AM +0300, Alexander Bezzubikov wrote:
> >>> 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> >>>> On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
> >>>>> On PCI init PCI bridges may need some
> >>>>> extra info about bus number to reserve, IO, memory and
> >>>>> prefetchable memory limits. QEMU can provide this
> >>>>> with special
> >>>>
> >>>> with a special
> >>>>
> >>>>> vendor-specific PCI capability.
> >>>>>
> >>>>> Sizes of limits match ones from
> >>>>> PCI Type 1 Configuration Space Header,
> >>>>> number of buses to reserve occupies only 1 byte
> >>>>> since it is the size of Subordinate Bus Number register.
> >>>>>
> >>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> >>>>> ---
> >>>>>   hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
> >>>>>   include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
> >>>>>   2 files changed, 45 insertions(+)
> >>>>>
> >>>>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> >>>>> index 720119b..8ec6c2c 100644
> >>>>> --- a/hw/pci/pci_bridge.c
> >>>>> +++ b/hw/pci/pci_bridge.c
> >>>>> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const
> >>>>> char* bus_name,
> >>>>>       br->bus_name = bus_name;
> >>>>>   }
> >>>>>
> >>>>> +
> >>>>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> >>>>
> >>>> help? should be qemu_cap_init?
> >>>>
> >>>>> +                              uint8_t bus_reserve, uint32_t io_limit,
> >>>>> +                              uint16_t mem_limit, uint64_t
> >>>>> pref_limit,
> >>>>> +                              Error **errp)
> >>>>> +{
> >>>>> +    size_t cap_len = sizeof(PCIBridgeQemuCap);
> >>>>> +    PCIBridgeQemuCap cap;
> >>>>
> >>>> This leaks info to guest. You want to init all fields here:
> >>>>
> >>>> cap = {
> >>>>   .len = ....
> >>>> };
> >>>
> >>> I surely can do this for len field, but as Laszlo proposed
> >>> we can use mutually exclusive fields,
> >>> e.g. pref_32 and pref_64, the only way I have left
> >>> is to use ternary operator (if we surely need this
> >>> big initializer). Keeping some if's would look better,
> >>> I think.
> >>>
> >>>>
> >>>>> +
> >>>>> +    cap.len = cap_len;
> >>>>> +    cap.bus_res = bus_reserve;
> >>>>> +    cap.io_lim = io_limit & 0xFF;
> >>>>> +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
> >>>>> +    cap.mem_lim = mem_limit;
> >>>>> +    cap.pref_lim = pref_limit & 0xFFFF;
> >>>>> +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
> >>>>
> >>>> Please use pci_set_word etc or cpu_to_leXX.
> >>>>
> >>>
> >>> Since now we've decided to avoid fields separation into <field> +
> >>> <field_upper>,
> >>> this bitmask along with pci_set_word are no longer needed.
> >>>
> >>>> I think it's easiest to replace struct with a set of macros then
> >>>> pci_set_word does the work for you.
> >>>>
> >>>
> >>> I don't really want to use macros here because structure
> >>> show us the whole capability layout and this can
> >>> decrease documenting efforts. More than that,
> >>> memcpy usage is very convenient here, and I wouldn't like
> >>> to lose it.
> >>>
> >>>>
> >>>>> +
> >>>>> +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
> >>>>> +                                    cap_offset, cap_len, errp);
> >>>>> +    if (offset < 0) {
> >>>>> +        return offset;
> >>>>> +    }
> >>>>> +
> >>>>> +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
> >>>>
> >>>> +2 is yacky. See how virtio does it:
> >>>>
> >>>>      memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
> >>>>             cap->cap_len - PCI_CAP_FLAGS);
> >>>>
> >>>>
> >>>
> >>> OK.
> >>>
> >>>>> +    return 0;
> >>>>> +}
> >>>>> +
> >>>>>   static const TypeInfo pci_bridge_type_info = {
> >>>>>       .name = TYPE_PCI_BRIDGE,
> >>>>>       .parent = TYPE_PCI_DEVICE,
> >>>>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> >>>>> index ff7cbaa..c9f642c 100644
> >>>>> --- a/include/hw/pci/pci_bridge.h
> >>>>> +++ b/include/hw/pci/pci_bridge.h
> >>>>> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const
> >>>>> char* bus_name,
> >>>>>   #define  PCI_BRIDGE_CTL_DISCARD_STATUS       0x400   /* Discard
> >>>>> timer status */
> >>>>>   #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer
> >>>>> SERR# enable */
> >>>>>
> >>>>> +typedef struct PCIBridgeQemuCap {
> >>>>> +    uint8_t id;     /* Standard PCI capability header field */
> >>>>> +    uint8_t next;   /* Standard PCI capability header field */
> >>>>> +    uint8_t len;    /* Standard PCI vendor-specific capability
> >>>>> header field */
> >>>>> +    uint8_t bus_res;
> >>>>> +    uint32_t pref_lim_upper;
> >>>>
> >>>> Big endian? Ugh.
> >>>>
> >>>
> >>> Agreed, and this's gonna to disappear with
> >>> the new layout.
> >>>
> >>>>> +    uint16_t pref_lim;
> >>>>> +    uint16_t mem_lim;
> >>>>
> >>>> I'd say we need 64 bit for memory.
> >>>>
> >>>
> >>> Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.
> >>
> >> Hmm ok, but e.g. for io there are bridges that have extra registers
> >> to specify non-standard non-aligned registers.
> >>
> >>>>> +    uint16_t io_lim_upper;
> >>>>> +    uint8_t io_lim;
> >>>>> +    uint8_t padding;
> >>>>
> >>>> IMHO each type should have a special "don't care" flag
> >>>> that would mean "I don't know".
> >>>>
> >>>>
> >>>
> >>> Don't know what? Now 0 is an indicator to do nothing with this field.
> >>
> >> In that case how do you say "don't allocate any memory"?
> >>
> > 
> > We can keep the MEM/Limit registers read-only for such cases,
> > as they are optional registers.
> 
> For OVMF, it would be really nice if OVMF could gather the reservation
> numbers with
> - read-only accesses
> - to the one exact hotplug controller (root port, bridge, etc) that's
>   being queried for reservation.
> 
> Deciding whether some registers in config space are r/o would require
> OVMF to attempt a write and check the result (and if it succeeded, to
> restore the original value). I'm not too attracted to doing this in a
> platform hook, while PciBusDxe is in the middle of enumerating /
> configuring the PCI(e) hierarchy :)
> 
> Thanks
> Laszlo

Well that's the PCI spec, I don't think there's a choice for
regular bridges. If we have this capability this is a possible
optimization.
But without it, if you assign ranges without checking whether they are
available they won't have any effect practically.
It's mostly harmless, but you are going to waste resources.

-- 
MST

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Michael S. Tsirkin]

On Tue, Jul 25, 2017 at 02:49:49PM +0300, Marcel Apfelbaum wrote:
> On 25/07/2017 0:58, Michael S. Tsirkin wrote:
> > On Tue, Jul 25, 2017 at 12:41:12AM +0300, Alexander Bezzubikov wrote:
> > > 2017-07-24 23:46 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> > > > On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
> > > > > On 23/07/2017 15:22, Michael S. Tsirkin wrote:
> > > > > > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
> > > > > > > To enable hotplugging of a newly created pcie-pci-bridge,
> > > > > > > we need to tell firmware (SeaBIOS in this case)
> > > > > > 
> > > > > 
> > > > > Hi Michael,
> > > > > 
> > > > > > Presumably, EFI would need to support this too?
> > > > > > 
> > > > > 
> > > > > Sure, Eduardo added to CC, but he is in PTO now.
> > > > > 
> > > > > > > to reserve
> > > > > > > additional buses for pcie-root-port, that allows us to
> > > > > > > hotplug pcie-pci-bridge into this root port.
> > > > > > > The number of buses to reserve is provided to the device via a corresponding
> > > > > > > property, and to the firmware via new PCI capability (next patch).
> > > > > > > The property's default value is 1 as we want to hotplug at least 1 bridge.
> > > > > > 
> > > > > > If so you should just teach firmware to allocate one bus #
> > > > > > unconditionally.
> > > > > > 
> > > > > 
> > > > > That would be a problem for the PCIe machines, since each PCIe
> > > > > devices is plugged in a different bus and we are already
> > > > > limited to 256 PCIe devices. Allocating an extra-bus always
> > > > > would really limit the PCIe devices we can use.
> > > > 
> > > > But this is exactly what this patch does as the property is added to all
> > > > buses and default to 1 (1 extra bus).
> > > > 
> > > > > > But why would that be so? What's wrong with a device
> > > > > > directly in the root port?
> > > > > > 
> > > > > 
> > > > > First, plugging a legacy PCI device into a PCIe Root Port
> > > > > looks strange at least, and it can;t be done on real HW anyway.
> > > > > (incompatible slots)
> > > > 
> > > > You can still plug in PCIe devices there.
> > > > 
> > > > > Second (and more important), if we want 2 or more PCI
> > > > > devices we would loose both IO ports space and bus numbers.
> > > > 
> > > > What I am saying is maybe default should not be 1.
> > > 
> 
> Hi Michael, Alexander
> 
> > > The only sensible variant left is 0.
> > > But as we want pcie-pci-bridge to be used for every legacy PCI device
> > > on q35 machine, every time one hotplugs the bridge into the root port,
> > > he must be sure rp's prop value >0 (for Linux). I'm not sure
> > > that it is a very convenient way to utilize the bridge - always remember
> > > to set property.
> > 
> 
> Is not for Linux only, is for all guest OSes.
> I also think setting the property is OK, libvirt can always
> add a single PCIe Root Port port with this property set,
> while upper layers can create flavors (if the feature needed
> or not for the current setup)

If you are going to always do this, it kind of looks like
Laszlo's idea of always cold-plugging a pci bridge.

> > That's what I'm saying then - if in your opinion default is >0 anyway,
> > tweak firmware to do it by default.
> > 
> 
> Default should be 0 for sure - because of the hard limitation
> on the number of PCIe devices for single PCI domain (the same
> as the number of buses, 256).
> 
> For a positive value we will should add a property "buses-reserve = x".

So value here is borderline but if it includes other resources
then value seems clearer.

> > > Another way - we can set this to 0 by default, and to 1 for pcie-root-port,
> > > and recommend to use it for hotplugging of the pcie-pci-bridge itself.
> > 
> > I wonder about something: imagine hotplugging a hierarchy of bridges
> > below a root port. It seems that nothing prevents guest from finding a
> > free range of buses to cover this hierarchy and setting that as
> > secondary/subordinate bus for this bridge.
> >  > This does need support on QEMU side to hotplug a hierarchy at once,
> > and might need some fixes in Linux, on the plus side you can defer
> > management decision on how many are needed until you are actually
> > adding something, and you don't need vendor specific patches.
> > 
> 
> We can teach Linux kernel, that's for sure (OK, almost sure...)
> but what we don't want is to be dependent on specific guest Operating
> Systems. For example, most configurations are not supported
> by Windows guests.

If you fix Linux then windows will not want to be left behind
and will implement this too.

> Also is a great opportunity adding PCI IO resources hints
> to guest FW, something we wanted to do for some time.
> 
> Thanks,
> Marcel

I agree, it's a good reason to add this capability.

> > 
> > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> > > > > > > ---
> > > > > > >    hw/pci-bridge/pcie_root_port.c | 1 +
> > > > > > >    include/hw/pci/pcie_port.h     | 3 +++
> > > > > > >    2 files changed, 4 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> > > > > > > index 4d588cb..b0e49e1 100644
> > > > > > > --- a/hw/pci-bridge/pcie_root_port.c
> > > > > > > +++ b/hw/pci-bridge/pcie_root_port.c
> > > > > > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
> > > > > > >    static Property rp_props[] = {
> > > > > > >        DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
> > > > > > >                        QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> > > > > > > +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
> > > > > > >        DEFINE_PROP_END_OF_LIST()
> > > > > > >    };
> > > > > > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> > > > > > > index 1333266..1b2dd1f 100644
> > > > > > > --- a/include/hw/pci/pcie_port.h
> > > > > > > +++ b/include/hw/pci/pcie_port.h
> > > > > > > @@ -34,6 +34,9 @@ struct PCIEPort {
> > > > > > >        /* pci express switch port */
> > > > > > >        uint8_t     port;
> > > > > > > +
> > > > > > > +    /* additional buses to reserve on firmware init */
> > > > > > > +    uint8_t     bus_reserve;
> > > > > > >    };
> > > > > > >    void pcie_port_init_reg(PCIDevice *d);
> > > > > > 
> > > > > > So here is a property and it does not do anything.
> > > > > > It makes it easier to work on series maybe, but review
> > > > > > is harder since we do not see what it does at all.
> > > > > > Please do not split up patches like this - you can maintain
> > > > > > it split up in your branch if you like and merge before sending.
> > > > > > 
> > > > > 
> > > > > Agreed, Alexandr please merge patches 4-5-6 for your next submission.
> > > > > 
> > > > > Thanks,
> > > > > Marcel
> > > > > 
> > > > > 
> > > > > > > --
> > > > > > > 2.7.4
> > > 
> > > 
> > > 
> > > -- 
> > > Alexander Bezzubikov

Re: [Qemu-devel] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port

[Michael S. Tsirkin]

On Tue, Jul 25, 2017 at 07:10:33PM +0300, Marcel Apfelbaum wrote:
> On 25/07/2017 17:09, Alexander Bezzubikov wrote:
> > 2017-07-25 16:53 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> > > On Tue, Jul 25, 2017 at 04:50:49PM +0300, Alexander Bezzubikov wrote:
> > > > 2017-07-25 16:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> > > > > On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote:
> > > > > > On 23/07/2017 15:22, Michael S. Tsirkin wrote:
> > > > > > > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote:
> > > > > > > > To enable hotplugging of a newly created pcie-pci-bridge,
> > > > > > > > we need to tell firmware (SeaBIOS in this case)
> > > > > > > 
> > > > > > 
> > > > > > Hi Michael,
> > > > > > 
> > > > > > > Presumably, EFI would need to support this too?
> > > > > > > 
> > > > > > 
> > > > > > Sure, Eduardo added to CC, but he is in PTO now.
> > > > > > 
> > > > > > > > to reserve
> > > > > > > > additional buses for pcie-root-port, that allows us to
> > > > > > > > hotplug pcie-pci-bridge into this root port.
> > > > > > > > The number of buses to reserve is provided to the device via a corresponding
> > > > > > > > property, and to the firmware via new PCI capability (next patch).
> > > > > > > > The property's default value is 1 as we want to hotplug at least 1 bridge.
> > > > > > > 
> > > > > > > If so you should just teach firmware to allocate one bus #
> > > > > > > unconditionally.
> > > > > > > 
> > > > > > 
> > > > > > That would be a problem for the PCIe machines, since each PCIe
> > > > > > devices is plugged in a different bus and we are already
> > > > > > limited to 256 PCIe devices. Allocating an extra-bus always
> > > > > > would really limit the PCIe devices we can use.
> > > > > 
> > > > > One of the declared advantages of PCIe is easy support for multiple roots.
> > > > > We really should look at that IMHO so we do not need to pile up hacks.
> > > > > 
> > > > > > > But why would that be so? What's wrong with a device
> > > > > > > directly in the root port?
> > > > > > > 
> > > > > 
> > > > > To clarify, my point is we might be wasting bus numbers by reservation
> > > > > since someone might just want to put pcie devices there.
> > > > 
> > > > I think, changing default value to 0 can help us avoid this,
> > > > as no bus reservation by default. If one's surely wants
> > > > to hotplug pcie-pci-bridge into this root port in future,
> > > > the property gives him such an opportunity.
> > > > So, sure need pcie-pci-bridge hotplug -> creating a root port with
> > > > bus_reserve > 0. Otherwise (and default) - just as now, no changes
> > > > in bus topology.
> > > 
> > > I guess 0 should mean "do not reserve any buses".  So I think we also
> > > need a flag to just avoid the capability altogether.  Maybe -1?  *That*
> > > should be the default.
> > 
> > -1 might be useful if any limit value 0 is legal, but is it?
> > If not, we can set every field to 0 and
> > this is a sign of avoiding capability since none legal
> > values are provided.
> > 
> 
> As Gerd suggested, this value is not a "delta" but the number
> of buses to be reserved behind the bridge. If I got it right,
> 0 is not a valid value, since the bridge by definition
> has a list one bus behind.
> 
> Michael, would you be OK with that?
> 
> Thanks,
> Marcel

IMHO we want an encoding that works for IO and memory too.


> > > 
> > > > > 
> > > > > > First, plugging a legacy PCI device into a PCIe Root Port
> > > > > > looks strange at least, and it can;t be done on real HW anyway.
> > > > > > (incompatible slots)
> > > > > > 
> > > > > > Second (and more important), if we want 2 or more PCI
> > > > > > devices we would loose both IO ports space and bus numbers.
> > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> > > > > > > > ---
> > > > > > > >    hw/pci-bridge/pcie_root_port.c | 1 +
> > > > > > > >    include/hw/pci/pcie_port.h     | 3 +++
> > > > > > > >    2 files changed, 4 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> > > > > > > > index 4d588cb..b0e49e1 100644
> > > > > > > > --- a/hw/pci-bridge/pcie_root_port.c
> > > > > > > > +++ b/hw/pci-bridge/pcie_root_port.c
> > > > > > > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d)
> > > > > > > >    static Property rp_props[] = {
> > > > > > > >        DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
> > > > > > > >                        QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> > > > > > > > +    DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1),
> > > > > > > >        DEFINE_PROP_END_OF_LIST()
> > > > > > > >    };
> > > > > > > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> > > > > > > > index 1333266..1b2dd1f 100644
> > > > > > > > --- a/include/hw/pci/pcie_port.h
> > > > > > > > +++ b/include/hw/pci/pcie_port.h
> > > > > > > > @@ -34,6 +34,9 @@ struct PCIEPort {
> > > > > > > >        /* pci express switch port */
> > > > > > > >        uint8_t     port;
> > > > > > > > +
> > > > > > > > +    /* additional buses to reserve on firmware init */
> > > > > > > > +    uint8_t     bus_reserve;
> > > > > > > >    };
> > > > > > > >    void pcie_port_init_reg(PCIDevice *d);
> > > > > > > 
> > > > > > > So here is a property and it does not do anything.
> > > > > > > It makes it easier to work on series maybe, but review
> > > > > > > is harder since we do not see what it does at all.
> > > > > > > Please do not split up patches like this - you can maintain
> > > > > > > it split up in your branch if you like and merge before sending.
> > > > > > > 
> > > > > > 
> > > > > > Agreed, Alexandr please merge patches 4-5-6 for your next submission.
> > > > > > 
> > > > > > Thanks,
> > > > > > Marcel
> > > > > > 
> > > > > > 
> > > > > > > > --
> > > > > > > > 2.7.4
> > > > 
> > > > 
> > > > 
> > > > --
> > > > Alexander Bezzubikov
> > 
> > 
> > 

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Marcel Apfelbaum]

On 29/07/2017 2:12, Michael S. Tsirkin wrote:
> On Thu, Jul 27, 2017 at 12:39:54PM +0300, Marcel Apfelbaum wrote:
>> On 27/07/2017 2:28, Michael S. Tsirkin wrote:
>>> On Thu, Jul 27, 2017 at 12:54:07AM +0300, Alexander Bezzubikov wrote:
>>>> 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>>>>> On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
>>>>>> On PCI init PCI bridges may need some
>>>>>> extra info about bus number to reserve, IO, memory and
>>>>>> prefetchable memory limits. QEMU can provide this
>>>>>> with special
>>>>>
>>>>> with a special
>>>>>
>>>>>> vendor-specific PCI capability.
>>>>>>
>>>>>> Sizes of limits match ones from
>>>>>> PCI Type 1 Configuration Space Header,
>>>>>> number of buses to reserve occupies only 1 byte
>>>>>> since it is the size of Subordinate Bus Number register.
>>>>>>
>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>>>>>> ---
>>>>>>    hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
>>>>>>    include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
>>>>>>    2 files changed, 45 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>>>>>> index 720119b..8ec6c2c 100644
>>>>>> --- a/hw/pci/pci_bridge.c
>>>>>> +++ b/hw/pci/pci_bridge.c
>>>>>> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>>>>>>        br->bus_name = bus_name;
>>>>>>    }
>>>>>>
>>>>>> +
>>>>>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>>>>>
>>>>> help? should be qemu_cap_init?
>>>>>
>>>>>> +                              uint8_t bus_reserve, uint32_t io_limit,
>>>>>> +                              uint16_t mem_limit, uint64_t pref_limit,
>>>>>> +                              Error **errp)
>>>>>> +{
>>>>>> +    size_t cap_len = sizeof(PCIBridgeQemuCap);
>>>>>> +    PCIBridgeQemuCap cap;
>>>>>
>>>>> This leaks info to guest. You want to init all fields here:
>>>>>
>>>>> cap = {
>>>>>    .len = ....
>>>>> };
>>>>
>>>> I surely can do this for len field, but as Laszlo proposed
>>>> we can use mutually exclusive fields,
>>>> e.g. pref_32 and pref_64, the only way I have left
>>>> is to use ternary operator (if we surely need this
>>>> big initializer). Keeping some if's would look better,
>>>> I think.
>>>>
>>>>>
>>>>>> +
>>>>>> +    cap.len = cap_len;
>>>>>> +    cap.bus_res = bus_reserve;
>>>>>> +    cap.io_lim = io_limit & 0xFF;
>>>>>> +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
>>>>>> +    cap.mem_lim = mem_limit;
>>>>>> +    cap.pref_lim = pref_limit & 0xFFFF;
>>>>>> +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
>>>>>
>>>>> Please use pci_set_word etc or cpu_to_leXX.
>>>>>
>>>>
>>>> Since now we've decided to avoid fields separation into <field> + <field_upper>,
>>>> this bitmask along with pci_set_word are no longer needed.
>>>>
>>>>> I think it's easiest to replace struct with a set of macros then
>>>>> pci_set_word does the work for you.
>>>>>
>>>>
>>>> I don't really want to use macros here because structure
>>>> show us the whole capability layout and this can
>>>> decrease documenting efforts. More than that,
>>>> memcpy usage is very convenient here, and I wouldn't like
>>>> to lose it.
>>>>
>>>>>
>>>>>> +
>>>>>> +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
>>>>>> +                                    cap_offset, cap_len, errp);
>>>>>> +    if (offset < 0) {
>>>>>> +        return offset;
>>>>>> +    }
>>>>>> +
>>>>>> +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
>>>>>
>>>>> +2 is yacky. See how virtio does it:
>>>>>
>>>>>       memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
>>>>>              cap->cap_len - PCI_CAP_FLAGS);
>>>>>
>>>>>
>>>>
>>>> OK.
>>>>
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>    static const TypeInfo pci_bridge_type_info = {
>>>>>>        .name = TYPE_PCI_BRIDGE,
>>>>>>        .parent = TYPE_PCI_DEVICE,
>>>>>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
>>>>>> index ff7cbaa..c9f642c 100644
>>>>>> --- a/include/hw/pci/pci_bridge.h
>>>>>> +++ b/include/hw/pci/pci_bridge.h
>>>>>> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
>>>>>>    #define  PCI_BRIDGE_CTL_DISCARD_STATUS       0x400   /* Discard timer status */
>>>>>>    #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer SERR# enable */
>>>>>>
>>>>>> +typedef struct PCIBridgeQemuCap {
>>>>>> +    uint8_t id;     /* Standard PCI capability header field */
>>>>>> +    uint8_t next;   /* Standard PCI capability header field */
>>>>>> +    uint8_t len;    /* Standard PCI vendor-specific capability header field */
>>>>>> +    uint8_t bus_res;
>>>>>> +    uint32_t pref_lim_upper;
>>>>>
>>>>> Big endian? Ugh.
>>>>>
>>>>
>>>> Agreed, and this's gonna to disappear with
>>>> the new layout.
>>>>
>>>>>> +    uint16_t pref_lim;
>>>>>> +    uint16_t mem_lim;
>>>>>
>>>>> I'd say we need 64 bit for memory.
>>>>>
>>>>
>>>> Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.
>>>
>>> Hmm ok, but e.g. for io there are bridges that have extra registers
>>> to specify non-standard non-aligned registers.
>>>
>>>>>> +    uint16_t io_lim_upper;
>>>>>> +    uint8_t io_lim;
>>>>>> +    uint8_t padding;
>>>>>
>>>>> IMHO each type should have a special "don't care" flag
>>>>> that would mean "I don't know".
>>>>>
>>>>>
>>>>
>>>> Don't know what? Now 0 is an indicator to do nothing with this field.
>>>
>>> In that case how do you say "don't allocate any memory"?
>>>
>>
>> We can keep the MEM/Limit registers read-only for such cases,
>> as they are optional registers.
>>
>> Thanks,
>> Marcel
>

Hi Michael,


> I don't believe they are - from the spec (1.2):
> 	The Memory Base and Memory Limit registers are both required registers
> 
> Rest of ranges are indeed optional.
> 


Oops, my mistake, I looked at prefetchable memory, sorry for the mix-up.
I wonder what would be the use case to disable IOMEM.

Anyway, we can have a "-1" default value giving the hint: do not reserve
anything, but in this case we should use it also for IO/prefetch mem.
The problem is, the guest OS can try to allocate IO space even if
the firmware didn't; Linux does it today.
Having IO-base/IO-limit registers read-only will solve the problem.

What am I getting at is we would have now 2 ways to solve the same
problem and the hint would be inferior.

I would use the hint to reserve a specific range and read only registers
to disable the range. IOMEM cannot be disabled in this way, but
should not bother us.

Thanks,
Marcel

> 
>>>
>>>>>> +} PCIBridgeQemuCap;
>>>>>
>>>>> You don't really need this struct in the header. And pls document all fields.
>>>>>
>>>>>> +
>>>>>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>>>>>> +                              uint8_t bus_reserve, uint32_t io_limit,
>>>>>> +                              uint16_t mem_limit, uint64_t pref_limit,
>>>>>> +                              Error **errp);
>>>>>> +
>>>>>>    #endif /* QEMU_PCI_BRIDGE_H */
>>>>>> --
>>>>>> 2.7.4
>>>>
>>>>
>>>>
>>>> --
>>>> Alexander Bezzubikov

Re: [Qemu-devel] [SeaBIOS] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Laszlo Ersek]

On 07/29/17 01:15, Michael S. Tsirkin wrote:
> On Thu, Jul 27, 2017 at 03:58:58PM +0200, Laszlo Ersek wrote:
>> On 07/27/17 11:39, Marcel Apfelbaum wrote:
>>> On 27/07/2017 2:28, Michael S. Tsirkin wrote:
>>>> On Thu, Jul 27, 2017 at 12:54:07AM +0300, Alexander Bezzubikov wrote:
>>>>> 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>>>>>> On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
>>>>>>> On PCI init PCI bridges may need some
>>>>>>> extra info about bus number to reserve, IO, memory and
>>>>>>> prefetchable memory limits. QEMU can provide this
>>>>>>> with special
>>>>>>
>>>>>> with a special
>>>>>>
>>>>>>> vendor-specific PCI capability.
>>>>>>>
>>>>>>> Sizes of limits match ones from
>>>>>>> PCI Type 1 Configuration Space Header,
>>>>>>> number of buses to reserve occupies only 1 byte
>>>>>>> since it is the size of Subordinate Bus Number register.
>>>>>>>
>>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
>>>>>>> ---
>>>>>>>   hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
>>>>>>>   include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
>>>>>>>   2 files changed, 45 insertions(+)
>>>>>>>
>>>>>>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>>>>>>> index 720119b..8ec6c2c 100644
>>>>>>> --- a/hw/pci/pci_bridge.c
>>>>>>> +++ b/hw/pci/pci_bridge.c
>>>>>>> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const
>>>>>>> char* bus_name,
>>>>>>>       br->bus_name = bus_name;
>>>>>>>   }
>>>>>>>
>>>>>>> +
>>>>>>> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
>>>>>>
>>>>>> help? should be qemu_cap_init?
>>>>>>
>>>>>>> +                              uint8_t bus_reserve, uint32_t io_limit,
>>>>>>> +                              uint16_t mem_limit, uint64_t
>>>>>>> pref_limit,
>>>>>>> +                              Error **errp)
>>>>>>> +{
>>>>>>> +    size_t cap_len = sizeof(PCIBridgeQemuCap);
>>>>>>> +    PCIBridgeQemuCap cap;
>>>>>>
>>>>>> This leaks info to guest. You want to init all fields here:
>>>>>>
>>>>>> cap = {
>>>>>>   .len = ....
>>>>>> };
>>>>>
>>>>> I surely can do this for len field, but as Laszlo proposed
>>>>> we can use mutually exclusive fields,
>>>>> e.g. pref_32 and pref_64, the only way I have left
>>>>> is to use ternary operator (if we surely need this
>>>>> big initializer). Keeping some if's would look better,
>>>>> I think.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +    cap.len = cap_len;
>>>>>>> +    cap.bus_res = bus_reserve;
>>>>>>> +    cap.io_lim = io_limit & 0xFF;
>>>>>>> +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
>>>>>>> +    cap.mem_lim = mem_limit;
>>>>>>> +    cap.pref_lim = pref_limit & 0xFFFF;
>>>>>>> +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
>>>>>>
>>>>>> Please use pci_set_word etc or cpu_to_leXX.
>>>>>>
>>>>>
>>>>> Since now we've decided to avoid fields separation into <field> +
>>>>> <field_upper>,
>>>>> this bitmask along with pci_set_word are no longer needed.
>>>>>
>>>>>> I think it's easiest to replace struct with a set of macros then
>>>>>> pci_set_word does the work for you.
>>>>>>
>>>>>
>>>>> I don't really want to use macros here because structure
>>>>> show us the whole capability layout and this can
>>>>> decrease documenting efforts. More than that,
>>>>> memcpy usage is very convenient here, and I wouldn't like
>>>>> to lose it.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
>>>>>>> +                                    cap_offset, cap_len, errp);
>>>>>>> +    if (offset < 0) {
>>>>>>> +        return offset;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
>>>>>>
>>>>>> +2 is yacky. See how virtio does it:
>>>>>>
>>>>>>      memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
>>>>>>             cap->cap_len - PCI_CAP_FLAGS);
>>>>>>
>>>>>>
>>>>>
>>>>> OK.
>>>>>
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>>   static const TypeInfo pci_bridge_type_info = {
>>>>>>>       .name = TYPE_PCI_BRIDGE,
>>>>>>>       .parent = TYPE_PCI_DEVICE,
>>>>>>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
>>>>>>> index ff7cbaa..c9f642c 100644
>>>>>>> --- a/include/hw/pci/pci_bridge.h
>>>>>>> +++ b/include/hw/pci/pci_bridge.h
>>>>>>> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const
>>>>>>> char* bus_name,
>>>>>>>   #define  PCI_BRIDGE_CTL_DISCARD_STATUS       0x400   /* Discard
>>>>>>> timer status */
>>>>>>>   #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer
>>>>>>> SERR# enable */
>>>>>>>
>>>>>>> +typedef struct PCIBridgeQemuCap {
>>>>>>> +    uint8_t id;     /* Standard PCI capability header field */
>>>>>>> +    uint8_t next;   /* Standard PCI capability header field */
>>>>>>> +    uint8_t len;    /* Standard PCI vendor-specific capability
>>>>>>> header field */
>>>>>>> +    uint8_t bus_res;
>>>>>>> +    uint32_t pref_lim_upper;
>>>>>>
>>>>>> Big endian? Ugh.
>>>>>>
>>>>>
>>>>> Agreed, and this's gonna to disappear with
>>>>> the new layout.
>>>>>
>>>>>>> +    uint16_t pref_lim;
>>>>>>> +    uint16_t mem_lim;
>>>>>>
>>>>>> I'd say we need 64 bit for memory.
>>>>>>
>>>>>
>>>>> Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.
>>>>
>>>> Hmm ok, but e.g. for io there are bridges that have extra registers
>>>> to specify non-standard non-aligned registers.
>>>>
>>>>>>> +    uint16_t io_lim_upper;
>>>>>>> +    uint8_t io_lim;
>>>>>>> +    uint8_t padding;
>>>>>>
>>>>>> IMHO each type should have a special "don't care" flag
>>>>>> that would mean "I don't know".
>>>>>>
>>>>>>
>>>>>
>>>>> Don't know what? Now 0 is an indicator to do nothing with this field.
>>>>
>>>> In that case how do you say "don't allocate any memory"?
>>>>
>>>
>>> We can keep the MEM/Limit registers read-only for such cases,
>>> as they are optional registers.
>>
>> For OVMF, it would be really nice if OVMF could gather the reservation
>> numbers with
>> - read-only accesses
>> - to the one exact hotplug controller (root port, bridge, etc) that's
>>   being queried for reservation.
>>
>> Deciding whether some registers in config space are r/o would require
>> OVMF to attempt a write and check the result (and if it succeeded, to
>> restore the original value). I'm not too attracted to doing this in a
>> platform hook, while PciBusDxe is in the middle of enumerating /
>> configuring the PCI(e) hierarchy :)
>>
>> Thanks
>> Laszlo
> 
> Well that's the PCI spec, I don't think there's a choice for
> regular bridges. If we have this capability this is a possible
> optimization.
> But without it, if you assign ranges without checking whether they are
> available they won't have any effect practically.
> It's mostly harmless, but you are going to waste resources.
> 

OK. If the proposed solution with the r/o mem base/limit registers is
rooted in the spec (and I think it indeed must be; apparently this would
be the same as what we're already planning for IO disablement), then
that's a strong argument for PciBusDxe to accommodate this probing in
the platform hook.

Thanks
Laszlo

Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Michael S. Tsirkin]

On Mon, Jul 31, 2017 at 01:06:23PM +0300, Marcel Apfelbaum wrote:
> On 29/07/2017 2:12, Michael S. Tsirkin wrote:
> > On Thu, Jul 27, 2017 at 12:39:54PM +0300, Marcel Apfelbaum wrote:
> > > On 27/07/2017 2:28, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 27, 2017 at 12:54:07AM +0300, Alexander Bezzubikov wrote:
> > > > > 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
> > > > > > On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote:
> > > > > > > On PCI init PCI bridges may need some
> > > > > > > extra info about bus number to reserve, IO, memory and
> > > > > > > prefetchable memory limits. QEMU can provide this
> > > > > > > with special
> > > > > > 
> > > > > > with a special
> > > > > > 
> > > > > > > vendor-specific PCI capability.
> > > > > > > 
> > > > > > > Sizes of limits match ones from
> > > > > > > PCI Type 1 Configuration Space Header,
> > > > > > > number of buses to reserve occupies only 1 byte
> > > > > > > since it is the size of Subordinate Bus Number register.
> > > > > > > 
> > > > > > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
> > > > > > > ---
> > > > > > >    hw/pci/pci_bridge.c         | 27 +++++++++++++++++++++++++++
> > > > > > >    include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++
> > > > > > >    2 files changed, 45 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > > > > > > index 720119b..8ec6c2c 100644
> > > > > > > --- a/hw/pci/pci_bridge.c
> > > > > > > +++ b/hw/pci/pci_bridge.c
> > > > > > > @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
> > > > > > >        br->bus_name = bus_name;
> > > > > > >    }
> > > > > > > 
> > > > > > > +
> > > > > > > +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> > > > > > 
> > > > > > help? should be qemu_cap_init?
> > > > > > 
> > > > > > > +                              uint8_t bus_reserve, uint32_t io_limit,
> > > > > > > +                              uint16_t mem_limit, uint64_t pref_limit,
> > > > > > > +                              Error **errp)
> > > > > > > +{
> > > > > > > +    size_t cap_len = sizeof(PCIBridgeQemuCap);
> > > > > > > +    PCIBridgeQemuCap cap;
> > > > > > 
> > > > > > This leaks info to guest. You want to init all fields here:
> > > > > > 
> > > > > > cap = {
> > > > > >    .len = ....
> > > > > > };
> > > > > 
> > > > > I surely can do this for len field, but as Laszlo proposed
> > > > > we can use mutually exclusive fields,
> > > > > e.g. pref_32 and pref_64, the only way I have left
> > > > > is to use ternary operator (if we surely need this
> > > > > big initializer). Keeping some if's would look better,
> > > > > I think.
> > > > > 
> > > > > > 
> > > > > > > +
> > > > > > > +    cap.len = cap_len;
> > > > > > > +    cap.bus_res = bus_reserve;
> > > > > > > +    cap.io_lim = io_limit & 0xFF;
> > > > > > > +    cap.io_lim_upper = io_limit >> 8 & 0xFFFF;
> > > > > > > +    cap.mem_lim = mem_limit;
> > > > > > > +    cap.pref_lim = pref_limit & 0xFFFF;
> > > > > > > +    cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF;
> > > > > > 
> > > > > > Please use pci_set_word etc or cpu_to_leXX.
> > > > > > 
> > > > > 
> > > > > Since now we've decided to avoid fields separation into <field> + <field_upper>,
> > > > > this bitmask along with pci_set_word are no longer needed.
> > > > > 
> > > > > > I think it's easiest to replace struct with a set of macros then
> > > > > > pci_set_word does the work for you.
> > > > > > 
> > > > > 
> > > > > I don't really want to use macros here because structure
> > > > > show us the whole capability layout and this can
> > > > > decrease documenting efforts. More than that,
> > > > > memcpy usage is very convenient here, and I wouldn't like
> > > > > to lose it.
> > > > > 
> > > > > > 
> > > > > > > +
> > > > > > > +    int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
> > > > > > > +                                    cap_offset, cap_len, errp);
> > > > > > > +    if (offset < 0) {
> > > > > > > +        return offset;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2);
> > > > > > 
> > > > > > +2 is yacky. See how virtio does it:
> > > > > > 
> > > > > >       memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
> > > > > >              cap->cap_len - PCI_CAP_FLAGS);
> > > > > > 
> > > > > > 
> > > > > 
> > > > > OK.
> > > > > 
> > > > > > > +    return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >    static const TypeInfo pci_bridge_type_info = {
> > > > > > >        .name = TYPE_PCI_BRIDGE,
> > > > > > >        .parent = TYPE_PCI_DEVICE,
> > > > > > > diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> > > > > > > index ff7cbaa..c9f642c 100644
> > > > > > > --- a/include/hw/pci/pci_bridge.h
> > > > > > > +++ b/include/hw/pci/pci_bridge.h
> > > > > > > @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name,
> > > > > > >    #define  PCI_BRIDGE_CTL_DISCARD_STATUS       0x400   /* Discard timer status */
> > > > > > >    #define  PCI_BRIDGE_CTL_DISCARD_SERR 0x800   /* Discard timer SERR# enable */
> > > > > > > 
> > > > > > > +typedef struct PCIBridgeQemuCap {
> > > > > > > +    uint8_t id;     /* Standard PCI capability header field */
> > > > > > > +    uint8_t next;   /* Standard PCI capability header field */
> > > > > > > +    uint8_t len;    /* Standard PCI vendor-specific capability header field */
> > > > > > > +    uint8_t bus_res;
> > > > > > > +    uint32_t pref_lim_upper;
> > > > > > 
> > > > > > Big endian? Ugh.
> > > > > > 
> > > > > 
> > > > > Agreed, and this's gonna to disappear with
> > > > > the new layout.
> > > > > 
> > > > > > > +    uint16_t pref_lim;
> > > > > > > +    uint16_t mem_lim;
> > > > > > 
> > > > > > I'd say we need 64 bit for memory.
> > > > > > 
> > > > > 
> > > > > Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long.
> > > > 
> > > > Hmm ok, but e.g. for io there are bridges that have extra registers
> > > > to specify non-standard non-aligned registers.
> > > > 
> > > > > > > +    uint16_t io_lim_upper;
> > > > > > > +    uint8_t io_lim;
> > > > > > > +    uint8_t padding;
> > > > > > 
> > > > > > IMHO each type should have a special "don't care" flag
> > > > > > that would mean "I don't know".
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Don't know what? Now 0 is an indicator to do nothing with this field.
> > > > 
> > > > In that case how do you say "don't allocate any memory"?
> > > > 
> > > 
> > > We can keep the MEM/Limit registers read-only for such cases,
> > > as they are optional registers.
> > > 
> > > Thanks,
> > > Marcel
> > 
> 
> Hi Michael,
> 
> 
> > I don't believe they are - from the spec (1.2):
> > 	The Memory Base and Memory Limit registers are both required registers
> > 
> > Rest of ranges are indeed optional.
> > 
> 
> 
> Oops, my mistake, I looked at prefetchable memory, sorry for the mix-up.
> I wonder what would be the use case to disable IOMEM.
> 
> Anyway, we can have a "-1" default value giving the hint: do not reserve
> anything, but in this case we should use it also for IO/prefetch mem.
> The problem is, the guest OS can try to allocate IO space even if
> the firmware didn't; Linux does it today.
> Having IO-base/IO-limit registers read-only will solve the problem.

Makes sense.

> What am I getting at is we would have now 2 ways to solve the same
> problem and the hint would be inferior.
> 
> I would use the hint to reserve a specific range and read only registers
> to disable the range. IOMEM cannot be disabled in this way, but
> should not bother us.
> 
> Thanks,
> Marcel

I think Laszlo said that RO as an interface to indicate optional
range is tricky to use.

> > 
> > > > 
> > > > > > > +} PCIBridgeQemuCap;
> > > > > > 
> > > > > > You don't really need this struct in the header. And pls document all fields.
> > > > > > 
> > > > > > > +
> > > > > > > +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset,
> > > > > > > +                              uint8_t bus_reserve, uint32_t io_limit,
> > > > > > > +                              uint16_t mem_limit, uint64_t pref_limit,
> > > > > > > +                              Error **errp);
> > > > > > > +
> > > > > > >    #endif /* QEMU_PCI_BRIDGE_H */
> > > > > > > --
> > > > > > > 2.7.4
> > > > > 
> > > > > 
> > > > > 
> > > > > --
> > > > > Alexander Bezzubikov

Re: [Qemu-devel] [SeaBIOS] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Michael S. Tsirkin]

On Mon, Jul 31, 2017 at 08:16:49PM +0200, Laszlo Ersek wrote:
> OK. If the proposed solution with the r/o mem base/limit registers is
> rooted in the spec (and I think it indeed must be; apparently this would
> be the same as what we're already planning for IO disablement), then
> that's a strong argument for PciBusDxe to accommodate this probing in
> the platform hook.
> 
> Thanks
> Laszlo

Do you mean making base/limit read-only?

Re: [Qemu-devel] [SeaBIOS] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware

[Laszlo Ersek]

On 07/31/17 20:55, Michael S. Tsirkin wrote:
> On Mon, Jul 31, 2017 at 08:16:49PM +0200, Laszlo Ersek wrote:
>> OK. If the proposed solution with the r/o mem base/limit registers is
>> rooted in the spec (and I think it indeed must be; apparently this would
>> be the same as what we're already planning for IO disablement), then
>> that's a strong argument for PciBusDxe to accommodate this probing in
>> the platform hook.
>>
>> Thanks
>> Laszlo
> 
> Do you mean making base/limit read-only?

Yes, I do. (Perhaps writing "r/o" was too terse.)

Thanks
Laszlo