SeaBIOS

seabios@seabios.org

3783 discussions

[PATCH v4 0/5] Generic PCIE-PCI Bridge
by Aleksandr Bezzubikov 09 Aug '17

09 Aug '17

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. Changes v3->v4: 1. PCIE-PCI Bridge device: "msi_enable"->"msi", "shpc"->"shpc_bar", remove local_err, make "msi" property OnOffAuto, shpc_present() is still here to avoid SHPC_VMSTATE refactoring (address Marcel's comments). 2. Change QEMU PCI capability layout (SeaBIOS side has the same changes): - change reservation fields types: bus_res - uint32_t, others - uint64_t - rename 'non_pref' and 'pref' fields - interpret -1 value as 'ignore' 3. Use parent_realize in Generic PCI Express Root Port properly. 4. Fix documentation: fully replace the DMI-PCI bridge references with the new PCIE-PCI bridge, "PCIE"->"PCI Express", small mistakes and typos - address Laszlo's and Marcel's comments. 5. Rename QEMU PCI cap creation fucntion - addresses Marcel's comment. Changes v2->v3: (0). 'do_not_use' capability field flag is still _not_ in here since we haven't come to consesus on it yet. 1. Merge commits 5 (bus_reserve property creation) and 6 (property usage) together - addresses Michael's comment. 2. Add 'bus_reserve' property and QEMU PCI capability only to Generic PCIE Root Port - addresses Michael's and Marcel's comments. 3. Change 'bus_reserve' property's default value to 0 - addresses Michael's comment. 4. Rename QEMU bridge-specific PCI capability creation function - addresses Michael's comment. 5. Init the whole QEMU PCI capability with zeroes - addresses Michael's and Laszlo's comments. 6. Change QEMU PCI capability layout (SeaBIOS side has the same changes) - add 'type' field to distinguish multiple RedHat-specific capabilities - addresses Michael's comment - do not mimiс PCI Config space register layout, but use mutually exclusive differently sized fields for IO and prefetchable memory limits - addresses Laszlo's comment 7. Correct error handling in PCIE-PCI bridge realize function. 8. Replace a '2' constant with PCI_CAP_FLAGS in the capability creation function - addresses Michael's comment. 9. Remove a comment on _OSC which isn't correct anymore - address Marcel's comment. 10. Add documentation for the Generic PCIE-PCI Bridge and QEMU PCI capability - addresses Michael's comment. 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 "Allow RedHat PCI bridges reserve more buses than necessary during init". Aleksandr Bezzubikov (5): hw/i386: allow SHPC for Q35 machine hw/pci: introduce pcie-pci-bridge device hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware hw/pci: add QEMU-specific PCI capability to the Generic PCI Express Root Port docs: update documentation considering PCIE-PCI bridge docs/pcie.txt | 49 +++++---- docs/pcie_pci_bridge.txt | 110 +++++++++++++++++++ hw/i386/acpi-build.c | 4 +- hw/pci-bridge/Makefile.objs | 2 +- hw/pci-bridge/gen_pcie_root_port.c | 33 ++++++ hw/pci-bridge/pcie_pci_bridge.c | 212 +++++++++++++++++++++++++++++++++++++ hw/pci/pci_bridge.c | 29 +++++ include/hw/pci/pci.h | 1 + include/hw/pci/pci_bridge.h | 21 ++++ include/hw/pci/pcie_port.h | 1 + 10 files changed, 436 insertions(+), 26 deletions(-) create mode 100644 docs/pcie_pci_bridge.txt create mode 100644 hw/pci-bridge/pcie_pci_bridge.c -- 2.7.4

5 13

[PATCH v4 0/3] Allow RedHat PCI bridges reserve more buses than necessary during init
by Aleksandr Bezzubikov 09 Aug '17

09 Aug '17

Now PCI bridges get a bus range number on a system init, basing on currently plugged devices. That's why when one wants to hotplug another bridge, it needs his child bus, which the parent is unable to provide (speaking about virtual device). The suggested workaround is to have vendor-specific capability in Red Hat PCI bridges that contains number of additional bus to reserve (as well as IO/MEM/PREF space limit hints) on BIOS PCI init. So this capability is intented only for pure QEMU->SeaBIOS usage. Considering all aforesaid, this series is directly connected with QEMU series "Generic PCIE-PCI Bridge". Although the new PCI capability is supposed to contain various limits along with bus number to reserve, now only its full layout is proposed. And only bus_reserve field is used in QEMU and BIOS. Limits usage is still a subject for implementation as now the main goal of this series to provide necessary support from the firmware side to PCIE-PCI bridge hotplug. Changes v3->v4: 1. Use all QEMU PCI capability fields - addresses Michael's comment 2. Changes of the capability layout (QEMU side has the same changes): - change reservation fields types: bus_res - uint32_t, others - uint64_t - interpret -1 value as 'ignore' Changes v2->v3: 1. Merge commit 2 (Red Hat vendor ID) into commit 4 - addresses Marcel's comment, and add Generic PCIE Root Port device ID - addresses Michael's comment. 2. Changes of the capability layout (QEMU side has the same changes): - add 'type' field to distinguish multiple RedHat-specific capabilities - addresses Michael's comment - do not mimiс PCI Config space register layout, but use mutually exclusive differently sized fields for IO and prefetchable memory limits - addresses Laszlo's comment - use defines instead of structure and offsetof - addresses Michael's comment 3. Interpret 'bus_reserve' field as a minimum necessary range to reserve - addresses Gerd's comment 4. pci_find_capability moved to pci.c - addresses Kevin's comment 5. Move capability layout header to src/fw/dev-pci.h - addresses Kevin's comment 6. Add the capability documentation - addresses Michael's comment 7. Add capability length and bus_reserve field sanity checks - addresses Michael's comment Changes v1->v2: 1. New #define for Red Hat vendor added (addresses Konrad's comment). 2. Refactored pci_find_capability function (addresses Marcel's comment). 3. Capability reworked: - data type added; - reserve space in a structure for IO, memory and prefetchable memory limits. Aleksandr Bezzubikov (3): pci: refactor pci_find_capapibilty to get bdf as the first argument instead of the whole pci_device pci: add QEMU-specific PCI capability structure pci: enable RedHat PCI bridges to reserve additional buses on PCI init src/fw/dev-pci.h | 50 ++++++++++++++++++++++++++++++++++++ src/fw/pciinit.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++----- src/hw/pci.c | 25 ++++++++++++++++++ src/hw/pci.h | 1 + src/hw/pci_ids.h | 3 +++ src/hw/pcidevice.c | 24 ------------------ src/hw/pcidevice.h | 1 - src/hw/virtio-pci.c | 6 ++--- 8 files changed, 149 insertions(+), 34 deletions(-) create mode 100644 src/fw/dev-pci.h -- 2.7.4

4 7

Re: [SeaBIOS] >256 Virtio-net-pci hotplug Devices
by Marcel Apfelbaum 08 Aug '17

08 Aug '17

On 24/07/2017 7:53, Kinsella, Ray wrote: > Hi Ray, Thank you for the details, > So as it turns out at 512 devices, it is nothing to do SeaBIOS, it was the Kernel again. > It is taking quite a while to startup, a little over two hours (7489 seconds). > The main culprits appear to be enumerating/initializing the PCI Express ports and enabling interrupts. > > The PCI Express Root Ports are taking a long time to enumerate/ initializing. > 42 minutes in total=2579/60=64 ports in total, 40 seconds each. > Even if I am not aware of how much time would take to init a bare-metal PCIe Root Port, it seems too much. > [ 50.612822] pci_bus 0000:80: root bus resource [bus 80-c1] > [ 172.345361] pci 0000:80:00.0: PCI bridge to [bus 81] > ... > [ 2724.734240] pci 0000:80:08.0: PCI bridge to [bus c1] > [ 2751.154702] ACPI: Enabled 2 GPEs in block 00 to 3F > > I assume the 1 hour (3827 seconds) below is being spent enabling interrupts. > Assuming you are referring to legacy interrupts, maybe is possible to disable them and use only MSI/MSI-X for PCIe Root Ports (based on user input, we can't disable INTx for all the ports) > [ 2899.394288] ACPI: PCI Interrupt Link [GSIG] enabled at IRQ 22 > [ 2899.531324] ACPI: PCI Interrupt Link [GSIH] enabled at IRQ 23 > [ 2899.534778] ACPI: PCI Interrupt Link [GSIE] enabled at IRQ 20 > [ 6726.914388] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled > [ 6726.937932] 00:04: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A > [ 6726.964699] Linux agpgart interface v0.103 > > There finally there is another 20 minutes to find in the boot. > > [ 7489.202589] virtio_net virtio515 enp193s0f0: renamed from eth513 > > Poky (Yocto Project Reference Distro) 2.3 qemux86-64 ttyS0 > > qemux86-64 login: root > > I will remove the virtio-net-pci devices and hotplug them instead. > In theory it should improve boot time, at expense of incurring some of these costs at runtime. > I would appreciate if you can share the results. Thanks, Marcel > Ray K > > -----Original Message----- > From: Kevin O'Connor [mailto:kevin@koconnor.net] > Sent: Sunday, July 23, 2017 1:05 PM > To: Marcel Apfelbaum <marcel(a)redhat.com>; Kinsella, Ray <ray.kinsella(a)intel.com> > Cc: qemu-devel(a)nongnu.org; seabios(a)seabios.org; Gerd Hoffmann <kraxel(a)redhat.com>; Michael Tsirkin <mst(a)redhat.com> > Subject: Re: >256 Virtio-net-pci hotplug Devices > > On Sun, Jul 23, 2017 at 07:28:01PM +0300, Marcel Apfelbaum wrote: >> On 22/07/2017 2:57, Kinsella, Ray wrote: >>> When scaling up to 512 Virtio-net devices SeaBIOS appears to really >>> slow down when configuring PCI Config space - haven't manage to get >>> this to work yet. > > If there is a slowdown in SeaBIOS, it would help to produce a log with timing information - see: > https://www.seabios.org/Debugging#Timing_debug_messages > > It may also help to increase the debug level in SeaBIOS to get more fine grained timing reports. > > -Kevin >

2 3

Re: [SeaBIOS] [PATCH v4 2/5] hw/pci: introduce pcie-pci-bridge device
by Marcel Apfelbaum 08 Aug '17

08 Aug '17

On 07/08/2017 19:42, Alexander Bezzubikov wrote: > 2017-08-07 19:39 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com>: >> On 05/08/2017 23:27, Aleksandr Bezzubikov wrote: >>> >>> Introduce a new PCIExpress-to-PCI Bridge device, >>> which is a hot-pluggable PCI Express device and >>> supports devices hot-plug with SHPC. >>> >> >> >> Hi Aleksandr, >> >>> This device is intended to replace the DMI-to-PCI >>> Bridge in an overwhelming majority of use-cases. >> >> >> Please drop the last line ( ... majority...) >> It simply replaces it. >> >> >> >> >> >>> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> >>> --- >>> hw/pci-bridge/Makefile.objs | 2 +- >>> hw/pci-bridge/pcie_pci_bridge.c | 212 >>> ++++++++++++++++++++++++++++++++++++++++ >>> include/hw/pci/pci.h | 1 + >>> 3 files changed, 214 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..4127725 >>> --- /dev/null >>> +++ b/hw/pci-bridge/pcie_pci_bridge.c >>> @@ -0,0 +1,212 @@ >>> +/* >>> + * QEMU Generic PCIE-PCI Bridge >>> + * >>> + * Copyright (c) 2017 Aleksandr Bezzubikov >>> + * >> >> >> Please replace the below license with: >> >> " This work is licensed under the terms of the GNU GPL, version 2 >> or later. See the COPYING file in the top-level directory." >> >> to be sure it matches QEMU's license. >> >> >> >>> + * 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/shpc.h" >>> +#include "hw/pci/slotid_cap.h" >>> + >>> +typedef struct PCIEPCIBridge { >>> + /*< private >*/ >>> + PCIBridge parent_obj; >>> + >>> + OnOffAuto msi; >>> + MemoryRegion shpc_bar; >>> + /*< 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 pcie_pci_bridge_realize(PCIDevice *d, Error **errp) >>> +{ >>> + PCIBridge *br = PCI_BRIDGE(d); >>> + PCIEPCIBridge *pcie_br = PCIE_PCI_BRIDGE_DEV(d); >>> + int rc, pos; >>> + >>> + pci_bridge_initfn(d, TYPE_PCI_BUS); >>> + >>> + d->config[PCI_INTERRUPT_PIN] = 0x1; >>> + memory_region_init(&pcie_br->shpc_bar, OBJECT(d), "shpc-bar", >>> + shpc_bar_size(d)); >>> + rc = shpc_init(d, &br->sec_bus, &pcie_br->shpc_bar, 0, errp); >>> + if (rc) { >>> + goto error; >>> + } >>> + >>> + rc = pcie_cap_init(d, 0, PCI_EXP_TYPE_PCI_BRIDGE, 0, errp); >>> + if (rc < 0) { >>> + goto cap_error; >>> + } >>> + >>> + pos = pci_add_capability(d, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF, errp); >>> + if (pos < 0) { >>> + goto pm_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, errp); >>> + if (rc < 0) { >>> + goto aer_error; >>> + } >>> + >>> + if (pcie_br->msi != ON_OFF_AUTO_OFF) { >>> + rc = msi_init(d, 0, 1, true, true, errp); >>> + if (rc < 0) { >>> + goto msi_error; >>> + } >>> + } >>> + pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY | >>> + PCI_BASE_ADDRESS_MEM_TYPE_64, &pcie_br->shpc_bar); >>> + return; >>> + >>> +msi_error: >>> + pcie_aer_exit(d); >>> +aer_error: >>> +pm_error: >>> + pcie_cap_exit(d); >>> +cap_error: >>> + shpc_free(d); >>> +error: >>> + pci_bridge_exitfn(d); >>> +} >>> + >>> +static void pcie_pci_bridge_exit(PCIDevice *d) >>> +{ >>> + PCIEPCIBridge *bridge_dev = PCIE_PCI_BRIDGE_DEV(d); >>> + pcie_cap_exit(d); >>> + shpc_cleanup(d, &bridge_dev->shpc_bar); >>> + pci_bridge_exitfn(d); >>> +} >>> + >>> +static void pcie_pci_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, >>> + uint32_t address, uint32_t val, int len) >>> +{ >>> + pci_bridge_write_config(d, address, val, len); >>> + msi_write_config(d, address, val, len); >>> + shpc_cap_write_config(d, address, val, len); >>> +} >>> + >>> +static Property pcie_pci_bridge_dev_properties[] = { >>> + DEFINE_PROP_ON_OFF_AUTO("msi", PCIEPCIBridge, msi, >>> ON_OFF_AUTO_ON), >>> + DEFINE_PROP_END_OF_LIST(), >>> +}; >>> + >>> +static bool pcie_pci_bridge_shpc_present(void *opaque, int version_id) >>> +{ >>> + return true; >>> +} >> >> >> Already commented on this, you don't need the shpc >> present check, it is always there (you don't have code to disable >> it). >> >> Please check previous comments before posting a new version. >> It makes it hard to follow. > > I've seen it, and in the cover letter I've posted the reason why I still > have this function - SHPC_VMSTATE takes function as the third arg, > so either I refactor SHPC_VMSTATE (and all its usages, and it needs > new SHPC_VMSTATE and SHPC_VMSTATE_TEST macros) or > I leave it as it is. > You can just pass NULL to SHPC_VMSTATE instead of a function that returns TRUE Sorry for not paying attention. Thanks, Marcel >> >> Thanks, >> Marcel >> >> >>> + >>> +static const VMStateDescription pcie_pci_bridge_dev_vmstate = { >>> + .name = TYPE_PCIE_PCI_BRIDGE_DEV, >>> + .fields = (VMStateField[]) { >>> + VMSTATE_PCI_DEVICE(parent_obj, PCIBridge), >>> + SHPC_VMSTATE(shpc, PCIDevice, pcie_pci_bridge_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 pcie_pci_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; >>> + k->vendor_id = PCI_VENDOR_ID_REDHAT; >>> + k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE; >>> + k->realize = pcie_pci_bridge_realize; >>> + k->exit = pcie_pci_bridge_exit; >>> + k->config_write = pcie_pci_bridge_write_config; >>> + dc->vmsd = &pcie_pci_bridge_dev_vmstate; >>> + dc->props = pcie_pci_bridge_dev_properties; >>> + dc->vmsd = &pcie_pci_bridge_dev_vmstate; >>> + dc->reset = &pcie_pci_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 pcie_pci_bridge_info = { >>> + .name = TYPE_PCIE_PCI_BRIDGE_DEV, >>> + .parent = TYPE_PCI_BRIDGE, >>> + .instance_size = sizeof(PCIEPCIBridge), >>> + .class_init = pcie_pci_bridge_class_init, >>> + .interfaces = (InterfaceInfo[]) { >>> + { TYPE_HOTPLUG_HANDLER }, >>> + { }, >>> + } >>> +}; >>> + >>> +static void pciepci_register(void) >>> +{ >>> + type_register_static(&pcie_pci_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 >>> >> > > >

1 0

Re: [SeaBIOS] [PATCH v4 2/3] pci: add QEMU-specific PCI capability structure
by Marcel Apfelbaum 08 Aug '17

08 Aug '17

On 07/08/2017 19:02, Alexander Bezzubikov wrote: > 2017-08-07 18:52 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com>: >> On 05/08/2017 23:29, Aleksandr Bezzubikov wrote: >>> >>> On PCI init PCI bridge devices 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. >>> >>> This capability is intended to be used only >>> for Red Hat PCI bridges, i.e. QEMU cooperation. >>> >>> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> >>> --- >>> src/fw/dev-pci.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 50 insertions(+) >>> create mode 100644 src/fw/dev-pci.h >>> >>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h >>> new file mode 100644 >>> index 0000000..2c8ddb0 >> >> >> Hi Aleksandr, >> > > Hi Marcel, > >>> --- /dev/null >>> +++ b/src/fw/dev-pci.h >>> @@ -0,0 +1,50 @@ >>> +#ifndef _PCI_CAP_H >>> +#define _PCI_CAP_H >>> + >>> +#include "types.h" >>> + >>> +/* >>> + >> >> >> Please use the standard comment: >> >> /* >> * >> * >> */ >> >> >>> +QEMU-specific vendor(Red Hat)-specific capability. >>> +It's intended to provide some hints for firmware to init PCI devices. >>> + >>> +Its structure is shown below: >>> + >>> +Header: >>> + >>> +u8 id; Standard PCI Capability Header field >>> +u8 next; Standard PCI Capability Header field >>> +u8 len; Standard PCI Capability Header field >>> +u8 type; Red Hat vendor-specific capability type: >>> + now only REDHAT_CAP_TYP_QEMU=1 exists >> >> >> Typo o the line before, but I think you don't need it there. >> >>> +Data: >>> + >>> +u32 bus_res; minimum bus number to reserve; >>> + this is necessary for PCI Express Root Ports >>> + to support PCIE-to-PCI bridge hotplug >> >> >> I would add a broader class of usage: >> necessary for nesting PCI bridges hotplug. >> >>> +u64 io; IO space to reserve >>> +u64 mem; non-prefetchable memory space to reserve >>> +u64 prefetchable_mem; prefetchable memory space to reserve >>> + >> >> >> Layout looks good to me. >> >>> +If any field value in Data section is -1, >>> +it means that such kind of reservation >>> +is not needed and must be ignored. >>> + >> >> >> -1 is not a valid value for unsigned fields, you may >> want to say 0xff..f or some other way. > > I meant cast to unsigned here (because we still use unsigned types), > but if it can mislead someone I will change this. > We should not document signed values for unsigned fields, even if the reason is "best practices." >> >>> +*/ >>> + >>> +/* Offset of vendor-specific capability type field */ >>> +#define PCI_CAP_REDHAT_TYPE 3 >> >> >> May I ask why why '3'? I am not against it, I just >> want to understand the number. >> > > This is actually an offset to this field > OK, so it should end with 'offset' to be clear. I was mislead. >>> + >>> +/* List of valid Red Hat vendor-specific capability types */ >>> +#define REDHAT_CAP_TYPE_QEMU 1 >> >> >> I think I pointed this in another thread, the name is >> too vague, please change it to something like: >> REDHAT_CAP_RES_RESERVE_QEMU >> that narrows down the intend. >> > > What does the first 'RES' mean? Resource. I don't mind you change it how you seem fit, just make it clear what it does. Is about resource reserving, not a general capability. Thanks, Marcel > >>> + >>> + >>> +/* Offsets of QEMU capability fields */ >>> +#define QEMU_PCI_CAP_BUS_RES 4 >>> +#define QEMU_PCI_CAP_LIMITS_OFFSET 8 >>> +#define QEMU_PCI_CAP_IO 8 >>> +#define QEMU_PCI_CAP_MEM 16 >>> +#define QEMU_PCI_CAP_PREF_MEM 24 >>> +#define QEMU_PCI_CAP_SIZE 32 >>> + >>> +#endif /* _PCI_CAP_H */ >>> >> >> The layout looks good to me. >> >> Thanks, >> Marcel > > >

1 0

Re: [SeaBIOS] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure
by Marcel Apfelbaum 07 Aug '17

07 Aug '17

On 04/08/2017 23:47, Alexander Bezzubikov wrote: > 2017-08-04 23:28 GMT+03:00 Laszlo Ersek <lersek(a)redhat.com>: >> On 08/04/17 20:59, Alexander Bezzubikov wrote: >>> 2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov <zuban32s(a)gmail.com>: >>>> 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com>: >>>>> On 31/07/2017 22:01, Alexander Bezzubikov wrote: >>>>>> >>>>>> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com>: >>>>>>> >>>>>>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>>>>>>> >>>>>>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com>: >>>>>>>>> >>>>>>>>> On 31/07/2017 17:00, Michael S. Tsirkin wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On PCI init PCI bridge devices 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. >>>>>>>>>>> >>>>>>>>>>> This capability is intended to be used only >>>>>>>>>>> for Red Hat PCI bridges, i.e. QEMU cooperation. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> >>>>>>>>>>> --- >>>>>>>>>>> src/fw/dev-pci.h | 62 >>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>>>>> 1 file changed, 62 insertions(+) >>>>>>>>>>> create mode 100644 src/fw/dev-pci.h >>>>>>>>>>> >>>>>>>>>>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h >>>>>>>>>>> new file mode 100644 >>>>>>>>>>> index 0000000..fbd49ed >>>>>>>>>>> --- /dev/null >>>>>>>>>>> +++ b/src/fw/dev-pci.h >>>>>>>>>>> @@ -0,0 +1,62 @@ >>>>>>>>>>> +#ifndef _PCI_CAP_H >>>>>>>>>>> +#define _PCI_CAP_H >>>>>>>>>>> + >>>>>>>>>>> +#include "types.h" >>>>>>>>>>> + >>>>>>>>>>> +/* >>>>>>>>>>> + >>>>>>>>>>> +QEMU-specific vendor(Red Hat)-specific capability. >>>>>>>>>>> +It's intended to provide some hints for firmware to init PCI >>>>>>>>>>> devices. >>>>>>>>>>> + >>>>>>>>>>> +Its is shown below: >>>>>>>>>>> + >>>>>>>>>>> +Header: >>>>>>>>>>> + >>>>>>>>>>> +u8 id; Standard PCI Capability Header field >>>>>>>>>>> +u8 next; Standard PCI Capability Header field >>>>>>>>>>> +u8 len; Standard PCI Capability Header field >>>>>>>>>>> +u8 type; Red Hat vendor-specific capability type: >>>>>>>>>>> + now only REDHAT_QEMU_CAP 1 exists >>>>>>>>>>> +Data: >>>>>>>>>>> + >>>>>>>>>>> +u16 non_prefetchable_16; non-prefetchable memory limit >>>>>>>>>>> + >>>>>>>>>>> +u8 bus_res; minimum bus number to reserve; >>>>>>>>>>> + this is necessary for PCI Express Root Ports >>>>>>>>>>> + to support PCIE-to-PCI bridge hotplug >>>>>>>>>>> + >>>>>>>>>>> +u8 io_8; IO limit in case of 8-bit limit value >>>>>>>>>>> +u32 io_32; IO limit in case of 16-bit limit value >>>>>>>>>>> + io_8 and io_16 are mutually exclusive, in other words, >>>>>>>>>>> + they can't be non-zero simultaneously >>>>>>>>>>> + >>>>>>>>>>> +u32 prefetchable_32; non-prefetchable memory limit >>>>>>>>>>> + in case of 32-bit limit value >>>>>>>>>>> +u64 prefetchable_64; non-prefetchable memory limit >>>>>>>>>>> + in case of 64-bit limit value >>>>>>>>>>> + prefetachable_32 and prefetchable_64 >>>>>>>>>>> are >>>>>>>>>>> + mutually exclusive, in other words, >>>>>>>>>>> + they can't be non-zero simultaneously >>>>>>>>>>> +If any field in Data section is 0, >>>>>>>>>>> +it means that such kind of reservation >>>>>>>>>>> +is not needed. >>>>>>>> >>>>>>>> >>>>>>>> I really don't like this 'mutually exclusive' fields approach because >>>>>>>> IMHO it increases confusion level when undertanding this capability >>>>>>>> structure. >>>>>>>> But - if we came to consensus on that, then IO fields should be used >>>>>>>> in the same way, >>>>>>>> because as I understand, this 'mutual exclusivity' serves to distinguish >>>>>>>> cases >>>>>>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>>>>>>> registers. >>>>>>>> And this is how both IO and PREFETCHABLE works, isn't it? >>>>>>> >>>>>>> >>>>>>> I would just use simeple 64 bit registers. PCI spec has an ugly format >>>>>>> with fields spread all over the place but that is because of >>>>>>> compatibility concerns. It makes not sense to spend cycles just >>>>>>> to be similarly messy. >>>>>> >>>>>> >>>>>> Then I suggest to use exactly one field of a maximum possible size >>>>>> for each reserving object, and get rid of mutually exclusive fields. >>>>>> Then it can be something like that (order and names can be changed): >>>>>> u8 bus; >>>>>> u16 non_pref; >>>>>> u32 io; >>>>>> u64 pref; >>>>>> >>>>> >>>>> I think Michael suggested: >>>>> >>>>> u64 bus_res; >>>>> u64 mem_res; >>>>> u64 io_res; >>>>> u64 mem_pref_res; >>>>> >>>>> OR: >>>>> u32 bus_res; >>>>> u32 mem_res; >>>>> u32 io_res; >>>>> u64 mem_pref_res; >>>>> >>>>> >>>>> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's >>>>> requests. >>>> >>>> Let's dwell on the second option (with -1 as 'ignore' sign), if no new >>>> objections. >>>> >>> >>> BTW, talking about limit values provided in the capability - do we >>> want to completely override >>> existing PCI resources allocation mechanism being used in SeaBIOS, I >>> mean, to assign capability >>> values hardly, not taking into consideration any existing checks, >>> or somehow make this process soft (not an obvious way, can lead to >>> another big discussion)? >>> >>> In other words, how do we plan to use IO/MEM/PREF limits provided in >>> this capability >>> in application to the PCIE Root Port, what result is this supposed to achieve? >> >> I think Gerd spoke about this earlier: when determining a given kind of >> aperture for a given bridge, pick the maximum of: >> - the actual cumulative need of the devices behind the bridge, and >> - the hint for the given kind of aperture. >> >> So basically, do the same thing as before, but if the hint is larger, >> grow the aperture to that. > > Looks like current SeaBIOS resource allocation implementation is incorrect. > E.g. let's look at IO for pcie-root-port (and ioh3420 too) - we have 2 different > pci_region_entry objects, where one of them have bar = -1 (that > respresent a bridge > region as it as in the comment there) and size = 0. This leads to the > next situation after > the whole BIOS init: root port's IO has base=0x5000,size=0x4FFF. > And then Linux reconfigures this values itself. > Should the size of the pci_region be checked for emptiness before creation or > this isn't an error for some reason? I am not aware of such issue, however there is a patch that prevents SeaBIOS to allocate IO range for PCIe Root Ports if the device attached to it does not require IO ports. (or if a Root Port is empty) Can you provide more info? (a debug log) Or a patch that could fix the issue you see? So we can better understand the problem. Thanks, Marcel > >> >> This is how I recall the discussion anyway. >> >> Thanks >> Laszlo > > >

1 0

Re: [SeaBIOS] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure
by Laszlo Ersek 05 Aug '17

05 Aug '17

On 08/04/17 20:59, Alexander Bezzubikov wrote: > 2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov <zuban32s(a)gmail.com>: >> 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com>: >>> On 31/07/2017 22:01, Alexander Bezzubikov wrote: >>>> >>>> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com>: >>>>> >>>>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>>>>> >>>>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com>: >>>>>>> >>>>>>> On 31/07/2017 17:00, Michael S. Tsirkin wrote: >>>>>>>> >>>>>>>> >>>>>>>> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On PCI init PCI bridge devices 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. >>>>>>>>> >>>>>>>>> This capability is intended to be used only >>>>>>>>> for Red Hat PCI bridges, i.e. QEMU cooperation. >>>>>>>>> >>>>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> >>>>>>>>> --- >>>>>>>>> src/fw/dev-pci.h | 62 >>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>>> 1 file changed, 62 insertions(+) >>>>>>>>> create mode 100644 src/fw/dev-pci.h >>>>>>>>> >>>>>>>>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h >>>>>>>>> new file mode 100644 >>>>>>>>> index 0000000..fbd49ed >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/src/fw/dev-pci.h >>>>>>>>> @@ -0,0 +1,62 @@ >>>>>>>>> +#ifndef _PCI_CAP_H >>>>>>>>> +#define _PCI_CAP_H >>>>>>>>> + >>>>>>>>> +#include "types.h" >>>>>>>>> + >>>>>>>>> +/* >>>>>>>>> + >>>>>>>>> +QEMU-specific vendor(Red Hat)-specific capability. >>>>>>>>> +It's intended to provide some hints for firmware to init PCI >>>>>>>>> devices. >>>>>>>>> + >>>>>>>>> +Its is shown below: >>>>>>>>> + >>>>>>>>> +Header: >>>>>>>>> + >>>>>>>>> +u8 id; Standard PCI Capability Header field >>>>>>>>> +u8 next; Standard PCI Capability Header field >>>>>>>>> +u8 len; Standard PCI Capability Header field >>>>>>>>> +u8 type; Red Hat vendor-specific capability type: >>>>>>>>> + now only REDHAT_QEMU_CAP 1 exists >>>>>>>>> +Data: >>>>>>>>> + >>>>>>>>> +u16 non_prefetchable_16; non-prefetchable memory limit >>>>>>>>> + >>>>>>>>> +u8 bus_res; minimum bus number to reserve; >>>>>>>>> + this is necessary for PCI Express Root Ports >>>>>>>>> + to support PCIE-to-PCI bridge hotplug >>>>>>>>> + >>>>>>>>> +u8 io_8; IO limit in case of 8-bit limit value >>>>>>>>> +u32 io_32; IO limit in case of 16-bit limit value >>>>>>>>> + io_8 and io_16 are mutually exclusive, in other words, >>>>>>>>> + they can't be non-zero simultaneously >>>>>>>>> + >>>>>>>>> +u32 prefetchable_32; non-prefetchable memory limit >>>>>>>>> + in case of 32-bit limit value >>>>>>>>> +u64 prefetchable_64; non-prefetchable memory limit >>>>>>>>> + in case of 64-bit limit value >>>>>>>>> + prefetachable_32 and prefetchable_64 >>>>>>>>> are >>>>>>>>> + mutually exclusive, in other words, >>>>>>>>> + they can't be non-zero simultaneously >>>>>>>>> +If any field in Data section is 0, >>>>>>>>> +it means that such kind of reservation >>>>>>>>> +is not needed. >>>>>> >>>>>> >>>>>> I really don't like this 'mutually exclusive' fields approach because >>>>>> IMHO it increases confusion level when undertanding this capability >>>>>> structure. >>>>>> But - if we came to consensus on that, then IO fields should be used >>>>>> in the same way, >>>>>> because as I understand, this 'mutual exclusivity' serves to distinguish >>>>>> cases >>>>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>>>>> registers. >>>>>> And this is how both IO and PREFETCHABLE works, isn't it? >>>>> >>>>> >>>>> I would just use simeple 64 bit registers. PCI spec has an ugly format >>>>> with fields spread all over the place but that is because of >>>>> compatibility concerns. It makes not sense to spend cycles just >>>>> to be similarly messy. >>>> >>>> >>>> Then I suggest to use exactly one field of a maximum possible size >>>> for each reserving object, and get rid of mutually exclusive fields. >>>> Then it can be something like that (order and names can be changed): >>>> u8 bus; >>>> u16 non_pref; >>>> u32 io; >>>> u64 pref; >>>> >>> >>> I think Michael suggested: >>> >>> u64 bus_res; >>> u64 mem_res; >>> u64 io_res; >>> u64 mem_pref_res; >>> >>> OR: >>> u32 bus_res; >>> u32 mem_res; >>> u32 io_res; >>> u64 mem_pref_res; >>> >>> >>> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's >>> requests. >> >> Let's dwell on the second option (with -1 as 'ignore' sign), if no new >> objections. >> > > BTW, talking about limit values provided in the capability - do we > want to completely override > existing PCI resources allocation mechanism being used in SeaBIOS, I > mean, to assign capability > values hardly, not taking into consideration any existing checks, > or somehow make this process soft (not an obvious way, can lead to > another big discussion)? > > In other words, how do we plan to use IO/MEM/PREF limits provided in > this capability > in application to the PCIE Root Port, what result is this supposed to achieve? I think Gerd spoke about this earlier: when determining a given kind of aperture for a given bridge, pick the maximum of: - the actual cumulative need of the devices behind the bridge, and - the hint for the given kind of aperture. So basically, do the same thing as before, but if the hint is larger, grow the aperture to that. This is how I recall the discussion anyway. Thanks Laszlo

2 1

Re: [SeaBIOS] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure
by Alexander Bezzubikov 05 Aug '17

05 Aug '17

2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov <zuban32s(a)gmail.com>: > 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com>: >> On 31/07/2017 22:01, Alexander Bezzubikov wrote: >>> >>> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com>: >>>> >>>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>>>> >>>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com>: >>>>>> >>>>>> On 31/07/2017 17:00, Michael S. Tsirkin wrote: >>>>>>> >>>>>>> >>>>>>> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote: >>>>>>>> >>>>>>>> >>>>>>>> On PCI init PCI bridge devices 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. >>>>>>>> >>>>>>>> This capability is intended to be used only >>>>>>>> for Red Hat PCI bridges, i.e. QEMU cooperation. >>>>>>>> >>>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> >>>>>>>> --- >>>>>>>> src/fw/dev-pci.h | 62 >>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> 1 file changed, 62 insertions(+) >>>>>>>> create mode 100644 src/fw/dev-pci.h >>>>>>>> >>>>>>>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h >>>>>>>> new file mode 100644 >>>>>>>> index 0000000..fbd49ed >>>>>>>> --- /dev/null >>>>>>>> +++ b/src/fw/dev-pci.h >>>>>>>> @@ -0,0 +1,62 @@ >>>>>>>> +#ifndef _PCI_CAP_H >>>>>>>> +#define _PCI_CAP_H >>>>>>>> + >>>>>>>> +#include "types.h" >>>>>>>> + >>>>>>>> +/* >>>>>>>> + >>>>>>>> +QEMU-specific vendor(Red Hat)-specific capability. >>>>>>>> +It's intended to provide some hints for firmware to init PCI >>>>>>>> devices. >>>>>>>> + >>>>>>>> +Its is shown below: >>>>>>>> + >>>>>>>> +Header: >>>>>>>> + >>>>>>>> +u8 id; Standard PCI Capability Header field >>>>>>>> +u8 next; Standard PCI Capability Header field >>>>>>>> +u8 len; Standard PCI Capability Header field >>>>>>>> +u8 type; Red Hat vendor-specific capability type: >>>>>>>> + now only REDHAT_QEMU_CAP 1 exists >>>>>>>> +Data: >>>>>>>> + >>>>>>>> +u16 non_prefetchable_16; non-prefetchable memory limit >>>>>>>> + >>>>>>>> +u8 bus_res; minimum bus number to reserve; >>>>>>>> + this is necessary for PCI Express Root Ports >>>>>>>> + to support PCIE-to-PCI bridge hotplug >>>>>>>> + >>>>>>>> +u8 io_8; IO limit in case of 8-bit limit value >>>>>>>> +u32 io_32; IO limit in case of 16-bit limit value >>>>>>>> + io_8 and io_16 are mutually exclusive, in other words, >>>>>>>> + they can't be non-zero simultaneously >>>>>>>> + >>>>>>>> +u32 prefetchable_32; non-prefetchable memory limit >>>>>>>> + in case of 32-bit limit value >>>>>>>> +u64 prefetchable_64; non-prefetchable memory limit >>>>>>>> + in case of 64-bit limit value >>>>>>>> + prefetachable_32 and prefetchable_64 >>>>>>>> are >>>>>>>> + mutually exclusive, in other words, >>>>>>>> + they can't be non-zero simultaneously >>>>>>>> +If any field in Data section is 0, >>>>>>>> +it means that such kind of reservation >>>>>>>> +is not needed. >>>>> >>>>> >>>>> I really don't like this 'mutually exclusive' fields approach because >>>>> IMHO it increases confusion level when undertanding this capability >>>>> structure. >>>>> But - if we came to consensus on that, then IO fields should be used >>>>> in the same way, >>>>> because as I understand, this 'mutual exclusivity' serves to distinguish >>>>> cases >>>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>>>> registers. >>>>> And this is how both IO and PREFETCHABLE works, isn't it? >>>> >>>> >>>> I would just use simeple 64 bit registers. PCI spec has an ugly format >>>> with fields spread all over the place but that is because of >>>> compatibility concerns. It makes not sense to spend cycles just >>>> to be similarly messy. >>> >>> >>> Then I suggest to use exactly one field of a maximum possible size >>> for each reserving object, and get rid of mutually exclusive fields. >>> Then it can be something like that (order and names can be changed): >>> u8 bus; >>> u16 non_pref; >>> u32 io; >>> u64 pref; >>> >> >> I think Michael suggested: >> >> u64 bus_res; >> u64 mem_res; >> u64 io_res; >> u64 mem_pref_res; >> >> OR: >> u32 bus_res; >> u32 mem_res; >> u32 io_res; >> u64 mem_pref_res; >> >> >> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's >> requests. > > Let's dwell on the second option (with -1 as 'ignore' sign), if no new > objections. > BTW, talking about limit values provided in the capability - do we want to completely override existing PCI resources allocation mechanism being used in SeaBIOS, I mean, to assign capability values hardly, not taking into consideration any existing checks, or somehow make this process soft (not an obvious way, can lead to another big discussion)? In other words, how do we plan to use IO/MEM/PREF limits provided in this capability in application to the PCIE Root Port, what result is this supposed to achieve? >> >> Thanks, >> Marcel >> >> >>> >>>> >>>> >>>>>>> >>>>>>> >>>>>> >>>>>> Hi Michael, >>>>>> >>>>>>> We also want a way to say "no hint for this type". >>>>>>> >>>>>>> One way to achive this would be to have instead multiple >>>>>>> vendor specific capabilities, one for each of >>>>>>> bus#/io/mem/prefetch. 0 would mean do not reserve anything, >>>>>>> absence of capability would mean "no info, up to firmware". >>>>>>> >>>>>> >>>>>> First version of the series was implemented exactly like you propose, >>>>>> however Gerd preferred only one capability with multiple fields. >>>>>> >>>>>> I personally like the simplicity of vendor cap per io/mem/bus, >>>>>> even if it is on the expense of the limited PCI Config space. >>>>>> >>>>> >>>>> Personally I agree with Marcel since all this fields express >>>>> reservations of some objects. >>>>> >>>>>> We need a consensus here :) >>>>>> >>>>> >>>>> Absolutely :) >>>>> >>>>>> Thanks, >>>>>> Marcel >>>>>> >>>>>> >>>>>>> >>>>>>>> + >>>>>>>> +*/ >>>>>>>> + >>>>>>>> +/* Offset of vendor-specific capability type field */ >>>>>>>> +#define PCI_CAP_VNDR_SPEC_TYPE 3 >>>>>>> >>>>>>> >>>>>>> >>>>>>> This is a QEMU specific thing. Please name it as such. >>>>>>> >>>>>>>> + >>>>>>>> +/* List of valid Red Hat vendor-specific capability types */ >>>>>>>> +#define REDHAT_CAP_TYPE_QEMU 1 >>>>>>>> + >>>>>>>> + >>>>>>>> +/* Offsets of QEMU capability fields */ >>>>>>>> +#define QEMU_PCI_CAP_NON_PREF 4 >>>>>>>> +#define QEMU_PCI_CAP_BUS_RES 6 >>>>>>>> +#define QEMU_PCI_CAP_IO_8 7 >>>>>>>> +#define QEMU_PCI_CAP_IO_32 8 >>>>>>>> +#define QEMU_PCI_CAP_PREF_32 12 >>>>>>>> +#define QEMU_PCI_CAP_PREF_64 16 >>>>>>>> +#define QEMU_PCI_CAP_SIZE 24 >>>>>>>> + >>>>>>>> +#endif /* _PCI_CAP_H */ >>>>>>>> -- >>>>>>>> 2.7.4 >>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Alexander Bezzubikov >>> >>> >>> >>> >> > > > > -- > Aleksandr Bezzubikov -- Aleksandr Bezzubikov

1 0

Re: [SeaBIOS] [Qemu-devel] [PATCH v3 5/5] docs: update documentation considering PCIE-PCI bridge
by Michael S. Tsirkin 04 Aug '17

04 Aug '17

On Wed, Aug 02, 2017 at 12:33:12AM +0300, Alexander Bezzubikov wrote: > 2017-08-01 23:31 GMT+03:00 Laszlo Ersek <lersek(a)redhat.com>: > > (Whenever my comments conflict with Michael's or Marcel's, I defer to them.) > > > > On 07/29/17 01:37, Aleksandr Bezzubikov wrote: > >> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> > >> --- > >> docs/pcie.txt | 46 ++++++++++-------- > >> docs/pcie_pci_bridge.txt | 121 +++++++++++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 147 insertions(+), 20 deletions(-) > >> create mode 100644 docs/pcie_pci_bridge.txt > >> > >> diff --git a/docs/pcie.txt b/docs/pcie.txt > >> index 5bada24..338b50e 100644 > >> --- a/docs/pcie.txt > >> +++ b/docs/pcie.txt > >> @@ -46,7 +46,7 @@ Place only the following kinds of devices directly on the Root Complex: > >> (2) PCI Express Root Ports (ioh3420), for starting exclusively PCI Express > >> hierarchies. > >> > >> - (3) DMI-PCI Bridges (i82801b11-bridge), for starting legacy PCI > >> + (3) PCIE-PCI Bridge (pcie-pci-bridge), for starting legacy PCI > >> hierarchies. > >> > >> (4) Extra Root Complexes (pxb-pcie), if multiple PCI Express Root Buses > > > > When reviewing previous patches modifying / adding this file, I > > requested that we spell out "PCI Express" every single time. I'd like to > > see the same in this patch, if possible. > > OK, I didn't know it. > > > > >> @@ -55,18 +55,18 @@ Place only the following kinds of devices directly on the Root Complex: > >> pcie.0 bus > >> ---------------------------------------------------------------------------- > >> | | | | > >> - ----------- ------------------ ------------------ -------------- > >> - | PCI Dev | | PCIe Root Port | | DMI-PCI Bridge | | pxb-pcie | > >> - ----------- ------------------ ------------------ -------------- > >> + ----------- ------------------ ------------------- -------------- > >> + | PCI Dev | | PCIe Root Port | | PCIE-PCI Bridge | | pxb-pcie | > >> + ----------- ------------------ ------------------- -------------- > >> > >> 2.1.1 To plug a device into pcie.0 as a Root Complex Integrated Endpoint use: > >> -device <dev>[,bus=pcie.0] > >> 2.1.2 To expose a new PCI Express Root Bus use: > >> -device pxb-pcie,id=pcie.1,bus_nr=x[,numa_node=y][,addr=z] > >> - Only PCI Express Root Ports and DMI-PCI bridges can be connected > >> + Only PCI Express Root Ports, PCIE-PCI bridges and DMI-PCI bridges can be connected > > > > It would be nice if we could keep the flowing text wrapped to 80 chars. > > > > Also, here you add the "PCI Express-PCI" bridge to the list of allowed > > controllers (and you keep DMI-PCI as permitted), but above DMI was > > replaced. I think these should be made consistent -- we should make up > > our minds if we continue to recommend the DMI-PCI bridge or not. If not, > > then we should eradicate all traces of it. If we want to keep it at > > least for compatibility, then it should remain as fully documented as it > > is now. > > Now I'm beginning to think that we shouldn't keep the DMI-PCI bridge > even for compatibility and may want to use a new PCIE-PCI bridge > everywhere (of course, except some cases when users are > sure they need exactly DMI-PCI bridge for some reason) Can dmi-pci support shpc? why doesn't it? For compatibility? > > > >> to the pcie.1 bus: > >> -device ioh3420,id=root_port1[,bus=pcie.1][,chassis=x][,slot=y][,addr=z] \ > >> - -device i82801b11-bridge,id=dmi_pci_bridge1,bus=pcie.1 > >> + -device pcie-pci-bridge,id=pcie_pci_bridge1,bus=pcie.1 > >> > >> > >> 2.2 PCI Express only hierarchy > >> @@ -130,21 +130,25 @@ Notes: > >> Legacy PCI devices can be plugged into pcie.0 as Integrated Endpoints, > >> but, as mentioned in section 5, doing so means the legacy PCI > >> device in question will be incapable of hot-unplugging. > >> -Besides that use DMI-PCI Bridges (i82801b11-bridge) in combination > >> +Besides that use PCIE-PCI Bridges (pcie-pci-bridge) in combination > >> with PCI-PCI Bridges (pci-bridge) to start PCI hierarchies. > >> +Instead of the PCIE-PCI Bridge DMI-PCI one can be used, > >> +but it doens't support hot-plug, is not crossplatform and since that > > > > s/doens't/doesn't/ > > > > s/since that/therefore it/ > > > >> +is obsolete and deprecated. Use the PCIE-PCI Bridge if you're not > >> +absolutely sure you need the DMI-PCI Bridge. > >> > >> -Prefer flat hierarchies. For most scenarios a single DMI-PCI Bridge > >> +Prefer flat hierarchies. For most scenarios a single PCIE-PCI Bridge > >> (having 32 slots) and several PCI-PCI Bridges attached to it > >> (each supporting also 32 slots) will support hundreds of legacy devices. > >> -The recommendation is to populate one PCI-PCI Bridge under the DMI-PCI Bridge > >> +The recommendation is to populate one PCI-PCI Bridge under the PCIE-PCI Bridge > >> until is full and then plug a new PCI-PCI Bridge... > >> > >> pcie.0 bus > >> ---------------------------------------------- > >> | | > >> - ----------- ------------------ > >> - | PCI Dev | | DMI-PCI BRIDGE | > >> - ---------- ------------------ > >> + ----------- ------------------- > >> + | PCI Dev | | PCIE-PCI BRIDGE | > >> + ---------- ------------------- > >> | | > >> ------------------ ------------------ > >> | PCI-PCI Bridge | | PCI-PCI Bridge | ... > >> @@ -157,11 +161,11 @@ until is full and then plug a new PCI-PCI Bridge... > >> 2.3.1 To plug a PCI device into pcie.0 as an Integrated Endpoint use: > >> -device <dev>[,bus=pcie.0] > >> 2.3.2 Plugging a PCI device into a PCI-PCI Bridge: > >> - -device i82801b11-bridge,id=dmi_pci_bridge1[,bus=pcie.0] \ > >> - -device pci-bridge,id=pci_bridge1,bus=dmi_pci_bridge1[,chassis_nr=x][,addr=y] \ > >> + -device pcie-pci-bridge,id=pcie_pci_bridge1[,bus=pcie.0] \ > >> + -device pci-bridge,id=pci_bridge1,bus=pcie_pci_bridge1[,chassis_nr=x][,addr=y] \ > >> -device <dev>,bus=pci_bridge1[,addr=x] > >> Note that 'addr' cannot be 0 unless shpc=off parameter is passed to > >> - the PCI Bridge. > >> + the PCI Bridge, and can never be 0 when plugging into the PCIE-PCI Bridge. > >> > >> 3. IO space issues > >> =================== > >> @@ -219,25 +223,27 @@ do not support hot-plug, so any devices plugged into Root Complexes > >> cannot be hot-plugged/hot-unplugged: > >> (1) PCI Express Integrated Endpoints > >> (2) PCI Express Root Ports > >> - (3) DMI-PCI Bridges > >> + (3) PCIE-PCI Bridges > >> (4) pxb-pcie > >> > >> Be aware that PCI Express Downstream Ports can't be hot-plugged into > >> an existing PCI Express Upstream Port. > >> > >> -PCI devices can be hot-plugged into PCI-PCI Bridges. The PCI hot-plug is ACPI > >> -based and can work side by side with the PCI Express native hot-plug. > >> +PCI devices can be hot-plugged into PCIE-PCI and PCI-PCI Bridges. > >> +The PCI hot-plug into PCI-PCI bridge is ACPI based, whereas hot-plug into > >> +PCIE-PCI bridges is SHPC-base. They both can work side by side with the PCI Express native hot-plug. > > > > s/SHPC-base/SHPC-based/ > > > > And, I don't understand the difference between "ACPI based" and "SHPC > > based". I thought the PCI Express - PCI bridge reused the same hotplug > > mechanism of the PCI-PCI bridge. That is, "ACPI based" and "SHPC based" > > were the same thing, and they were both used by the PCI Express-PCI > > bridge, and the PCI-PCI bridge. > > > > I'm basing this on the fact that the "shpc=off" property was always > > mentioned for PCI-PCI bridges in this document (in a different context, > > see above), which implies that the PCI-PCI bridge has a SHPC too. And, > > we've always called that "ACPI based" hotplug in this section. > > Now we don't have ACPI hotplug support for Q35 machines, > which variants are the only valid machines to use PCIE-PCI bridges with. > > > > >> PCI Express devices can be natively hot-plugged/hot-unplugged into/from > >> -PCI Express Root Ports (and PCI Express Downstream Ports). > >> +PCI Express Root Ports (and PCI Express Downstream Ports) and PCIExpress-to-PCI Bridges. > > > > I don't think this is right; PCI Express endpoints should be plugged > > into PCI Express downstream and root ports only. (... See the last > > sentence of "2. Device placement strategy".) > > Agreed. > > > > >> > >> 5.1 Planning for hot-plug: > >> (1) PCI hierarchy > >> Leave enough PCI-PCI Bridge slots empty or add one > >> - or more empty PCI-PCI Bridges to the DMI-PCI Bridge. > >> + or more empty PCI-PCI Bridges to the PCIE-PCI Bridge. > >> > >> For each such PCI-PCI Bridge the Guest Firmware is expected to reserve > >> 4K IO space and 2M MMIO range to be used for all devices behind it. > >> + Appropriate PCI capability is designed, see pcie_pci_bridge.txt. > >> > >> Because of the hard IO limit of around 10 PCI Bridges (~ 40K space) > >> per system don't use more than 9 PCI-PCI Bridges, leaving 4K for the > >> diff --git a/docs/pcie_pci_bridge.txt b/docs/pcie_pci_bridge.txt > >> new file mode 100644 > >> index 0000000..ad392ad > >> --- /dev/null > >> +++ b/docs/pcie_pci_bridge.txt > >> @@ -0,0 +1,121 @@ > >> +Generic PCIExpress-to-PCI Bridge > >> +================================ > >> + > >> +Description > >> +=========== > >> +PCIE-to-PCI bridge is a new method for legacy PCI > >> +hierarchies creation on Q35 machines. > >> + > >> +Previously Intel DMI-to-PCI bridge was used for this purpose. > >> +But due to its strict limitations - no support of hot-plug, > >> +no cross-platform and cross-architecture support - a new generic > >> +PCIE-to-PCI bridge should now be used for any legacy PCI device usage > >> +with PCI Express machine. > >> + > >> +This generic PCIE-PCI bridge is a cross-platform device, > >> +can be hot-plugged into appropriate root port (requires additional actions, > >> +see 'PCIE-PCI bridge hot-plug' section), > >> +and supports devices hot-plug into the bridge itself > >> +(with some limitations, see below). > >> + > >> +Hot-plug of legacy PCI devices into the bridge > >> +is provided by bridge's built-in Standard hot-plug Controller. > >> +Though it still has some limitations, see 'Limitations' below. > >> + > >> +PCIE-PCI bridge hot-plug > >> +======================= > >> +As opposed to Windows, Linux guest requires extra efforts to > >> +enable PCIE-PCI bridge hot-plug. > >> +Motivation - now on init any PCI Express root port which doesn't have > >> +any device plugged in, has no free buses reserved to provide any of them > >> +to a hot-plugged devices in future. > >> + > >> +To solve this problem we reserve additional buses on a firmware level. > >> +Currently only SeaBIOS is supported. > >> +The way of bus number to reserve delivery is special > >> +Red Hat vendor-specific PCI capability, added to the root port > >> +that is planned to have PCIE-PCI bridge hot-plugged in. > >> + > >> +Capability layout (defined in include/hw/pci/pci_bridge.h): > >> + > >> + 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 type; Red Hat vendor-specific capability type > >> + List of currently existing types: > >> + QEMU = 1 > >> + > >> + uint16_t non_pref_16; Non-prefetchable memory limit > >> + > >> + uint8_t bus_res; Minimum number of buses to reserve > >> + > >> + uint8_t io_8; IO space limit in case of 8-bit value > >> + uint32_t io_32; IO space limit in case of 32-bit value > >> + This two values are mutually exclusive, > >> + i.e. they can't both be >0. > >> + > >> + uint32_t pref_32; Prefetchable memory limit in case of 32-bit value > >> + uint64_t pref_64; Prefetchable memory limit in case of 64-bit value > >> + This two values are mutually exclusive (just as IO limit), > >> + i.e. they can't both be >0. > >> + > >> +Memory limits are unused now, in future they are planned > >> +to be used for providing similar hints to the firmware. > >> + > >> +At the moment this capability is used only in > >> +QEMU generic PCIE root port (-device pcie-root-port). > >> +Capability construction function takes bus range value > >> +from root ports' common property 'bus_reserve'. > >> +By default it is set to 0 to leave root port's default > >> +behavior unchanged. > > > > This paragraph looks too narrow. Please fill it / wrap it to 80 chars or > > so (this should apply to all flowing text paragraphs). > > > >> + > >> +Usage > >> +===== > >> +A detailed command line would be: > >> + > >> +[qemu-bin + storage options] > >> +-m 2G > >> +-device ioh3420,bus=pcie.0,id=rp1 > >> +-device ioh3420,bus=pcie.0,id=rp2 > >> +-device pcie-root-port,bus=pcie.0,id=rp3,bus-reserve=1 > >> +-device pcie-pci-bridge,id=br1,bus=rp1 > >> +-device pcie-pci-bridge,id=br2,bus=rp2 > >> +-device e1000,bus=br1,addr=8 > > > > Backslashes seem to be missing. > > I took pci_expander_bridge.txt as an example, and there I found > no backslashes. I understand we need them if we plan to copy-paste this > to the command-line directly, and if so, I will add them. > > > > > Thanks > > Laszlo > > > >> + > >> +Then in monitor it's OK to do: > >> +device_add pcie-pci-bridge,id=br3,bus=rp3 > >> +device_add e1000,bus=br2,addr=1 > >> +device_add e1000,bus=br3,addr=1 > >> + > >> +Here you have: > >> + (1) Cold-plugged: > >> + - Root ports: 1 QEMU generic root port with the capability mentioned above, > >> + 2 ioh3420 root ports; > >> + - 2 PCIE-PCI bridges plugged into 2 different root ports; > >> + - e1000 plugged into the first bridge. > >> + (2) Hot-plugged: > >> + - PCIE-PCI bridge, plugged into QEMU generic root port; > >> + - 2 e1000 cards, one plugged into the cold-plugged PCIE-PCI bridge, > >> + another plugged into the hot-plugged bridge. > >> + > >> +Limitations > >> +=========== > >> +The PCIE-PCI bridge can be hot-plugged only into pcie-root-port that > >> +has proper 'bus_reserve' property value to provide secondary bus for the hot-plugged bridge. > >> + > >> +Windows 7 and older versions don't support hot-plug devices into the PCIE-PCI bridge. > >> +To enable device hot-plug into the bridge on Linux there're 3 ways: > >> +1) Build shpchp module with this patch http://www.spinics.net/lists/linux-pci/msg63052.html > >> +2) Wait until the kernel patch mentioned above get merged into upstream - > >> + it's expected to happen in 4.14. > >> +3) set 'msi_enable' property to false - this forced the bridge to use legacy INTx, > >> + which allows the bridge to notify the OS about hot-plug event without having > >> + BUSMASTER set. > >> + > >> +Implementation > >> +============== > >> +The PCIE-PCI bridge is based on PCI-PCI bridge, > >> +but also accumulates PCI Express features > >> +as a PCI Express device (is_express=1). > >> + > >> > > > > > > -- > Aleksandr Bezzubikov

4 12

[PATCH v3 0/5] Generic PCIE-PCI Bridge
by Aleksandr Bezzubikov 03 Aug '17

03 Aug '17

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. Changes v2->v3: (0). 'do_not_use' capability field flag is still _not_ in here since we haven't come to consesus on it yet. 1. Merge commits 5 (bus_reserve property creation) and 6 (property usage) together - addresses Michael's comment. 2. Add 'bus_reserve' property and QEMU PCI capability only to Generic PCIE Root Port - addresses Michael's and Marcel's comments. 3. Change 'bus_reserve' property's default value to 0 - addresses Michael's comment. 4. Rename QEMU bridge-specific PCI capability creation function - addresses Michael's comment. 5. Init the whole QEMU PCI capability with zeroes - addresses Michael's and Laszlo's comments. 6. Change QEMU PCI capability layout (SeaBIOS side has the same changes) - add 'type' field to distinguish multiple RedHat-specific capabilities - addresses Michael's comment - do not mimiс PCI Config space register layout, but use mutually exclusive differently sized fields for IO and prefetchable memory limits - addresses Laszlo's comment 7. Correct error handling in PCIE-PCI bridge realize function. 8. Replace a '2' constant with PCI_CAP_FLAGS in the capability creation function - addresses Michael's comment. 9. Remove a comment on _OSC which isn't correct anymore - address Marcel's comment. 10. Add documentation for the Generic PCIE-PCI Bridge and QEMU PCI capability - addresses Michael's comment. 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 (5): hw/i386: allow SHPC for Q35 machine hw/pci: introduce pcie-pci-bridge device hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware hw/pci: add QEMU-specific PCI capability to Generic PCI Express Root Port docs: update documentation considering PCIE-PCI bridge docs/pcie.txt | 46 ++++---- docs/pcie_pci_bridge.txt | 121 ++++++++++++++++++++ hw/i386/acpi-build.c | 4 +- hw/pci-bridge/Makefile.objs | 2 +- hw/pci-bridge/gen_pcie_root_port.c | 23 ++++ hw/pci-bridge/pcie_pci_bridge.c | 220 +++++++++++++++++++++++++++++++++++++ hw/pci-bridge/pcie_root_port.c | 2 +- hw/pci/pci_bridge.c | 37 +++++++ include/hw/pci/pci.h | 1 + include/hw/pci/pci_bridge.h | 28 +++++ include/hw/pci/pcie_port.h | 2 + 11 files changed, 462 insertions(+), 24 deletions(-) create mode 100644 docs/pcie_pci_bridge.txt create mode 100644 hw/pci-bridge/pcie_pci_bridge.c -- 2.7.4

5 18

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

SeaBIOS