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
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
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 */
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 */
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 */
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)
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 */
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 */
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 */
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
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
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);
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
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
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
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
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
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
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:
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
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:
- you can't add vendor properties to devices where vendor is not qemu as they might have their own concept of what it does.
- 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
-
Aleksandr Bezzubikov -
Alexander Bezzubikov
-
Marcel Apfelbaum -
Michael S. Tsirkin