* [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST @ 2018-06-12 9:13 Zihan Yang 2018-06-12 9:13 ` [Qemu-devel] [RFC v2 3/3] acpi-build: describe new pci domain in AML Zihan Yang ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: Zihan Yang @ 2018-06-12 9:13 UTC (permalink / raw) To: qemu-devel; +Cc: Zihan Yang, Michael S. Tsirkin, Marcel Apfelbaum The inner host bridge created by pxb-pcie is TYPE_PXB_PCI_HOST by default, add a new type TYPE_PXB_PCIE_HOST to better utilize the ECAM of PCIe Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> --- hw/pci-bridge/pci_expander_bridge.c | 118 ++++++++++++++++++++++++++++++++++-- 1 file changed, 114 insertions(+), 4 deletions(-) diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c index e62de42..448b9fb 100644 --- a/hw/pci-bridge/pci_expander_bridge.c +++ b/hw/pci-bridge/pci_expander_bridge.c @@ -15,10 +15,12 @@ #include "hw/pci/pci.h" #include "hw/pci/pci_bus.h" #include "hw/pci/pci_host.h" +#include "hw/pci/pcie_host.h" #include "hw/pci/pci_bridge.h" #include "qemu/range.h" #include "qemu/error-report.h" #include "sysemu/numa.h" +#include "qapi/visitor.h" #define TYPE_PXB_BUS "pxb-bus" #define PXB_BUS(obj) OBJECT_CHECK(PXBBus, (obj), TYPE_PXB_BUS) @@ -45,7 +47,10 @@ typedef struct PXBDev { PCIDevice parent_obj; /*< public >*/ - uint8_t bus_nr; + bool sep_domain; /* whether it resides in separate PCI segment */ + uint32_t domain_nr; /* PCI domain(segment) number, should be 0 if sep_domain is false */ + uint8_t max_bus; /* max number of buses to use */ + uint8_t bus_nr; /* should be 0 when in separate domain */ uint16_t numa_node; } PXBDev; @@ -58,6 +63,18 @@ static PXBDev *convert_to_pxb(PCIDevice *dev) static GList *pxb_dev_list; #define TYPE_PXB_HOST "pxb-host" +#define TYPE_PXB_PCIE_HOST "pxb-pcie-host" +#define PXB_PCIE_HOST_DEVICE(obj) \ + OBJECT_CHECK(PXBPCIEHost, (obj), TYPE_PXB_PCIE_HOST) + +typedef struct PXBPCIEHost { + PCIExpressHost parent_obj; + + /* should only inherit from PXBDev */ + uint32_t domain_nr; + uint8_t bus_nr; + uint8_t max_bus; +} PXBPCIEHost; static int pxb_bus_num(PCIBus *bus) { @@ -111,6 +128,31 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge, return bus->bus_path; } +/* Use a dedicated function for PCIe since pxb-host does + * not have a domain_nr field */ +static const char *pxb_pcie_host_root_bus_path(PCIHostState *host_bridge, + PCIBus *rootbus) +{ + if (!pci_bus_is_express(rootbus)) { + /* pxb-pcie-host cannot reside on a PCI bus */ + return NULL; + } + PXBBus *bus = PXB_PCIE_BUS(rootbus); + + snprintf(bus->bus_path, 8, "%04lx:%02x", + object_property_get_uint((Object *)host_bridge, "domain_nr", NULL), + pxb_bus_num(rootbus)); + return bus->bus_path; +} + +static void pxb_pcie_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + PCIExpressHost *e = PCIE_HOST_BRIDGE(obj); + + visit_type_uint64(v, name, &e->size, errp); +} + static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) { const PCIHostState *pxb_host; @@ -142,6 +184,30 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) return NULL; } +static void pxb_pcie_host_initfn(Object *obj) +{ + PCIHostState *phb = PCI_HOST_BRIDGE(obj); + + memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb, + "pci-conf-idx", 4); + memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb, + "pci-conf-data", 4); + + object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64", + pxb_pcie_host_get_mmcfg_size, + NULL, NULL, NULL, NULL); + +} + +static Property pxb_pcie_host_props[] = { + DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, PXBPCIEHost, parent_obj.base_addr, + PCIE_BASE_ADDR_UNMAPPED), + DEFINE_PROP_UINT32("domain_nr", PXBPCIEHost, domain_nr, 0), + DEFINE_PROP_UINT8("bus_nr", PXBPCIEHost, bus_nr, 0), + DEFINE_PROP_UINT8("max_bus", PXBPCIEHost, max_bus, 255), + DEFINE_PROP_END_OF_LIST(), +}; + static void pxb_host_class_init(ObjectClass *class, void *data) { DeviceClass *dc = DEVICE_CLASS(class); @@ -155,12 +221,34 @@ static void pxb_host_class_init(ObjectClass *class, void *data) hc->root_bus_path = pxb_host_root_bus_path; } +static void pxb_pcie_host_class_init(ObjectClass *class, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(class); + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class); + PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class); + + dc->fw_name = "pci"; + dc->props = pxb_pcie_host_props; + /* Reason: Internal part of the pxb/pxb-pcie device, not usable by itself */ + dc->user_creatable = false; + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address; + hc->root_bus_path = pxb_pcie_host_root_bus_path; +} + static const TypeInfo pxb_host_info = { .name = TYPE_PXB_HOST, .parent = TYPE_PCI_HOST_BRIDGE, .class_init = pxb_host_class_init, }; +static const TypeInfo pxb_pcie_host_info = { + .name = TYPE_PXB_PCIE_HOST, + .parent = TYPE_PCIE_HOST_BRIDGE, + .instance_size = sizeof(PXBPCIEHost), + .instance_init = pxb_pcie_host_initfn, + .class_init = pxb_pcie_host_class_init, +}; + /* * Registers the PXB bus as a child of pci host root bus. */ @@ -205,7 +293,10 @@ static gint pxb_compare(gconstpointer a, gconstpointer b) { const PXBDev *pxb_a = a, *pxb_b = b; - return pxb_a->bus_nr < pxb_b->bus_nr ? -1 : + /* check domain_nr, then bus_nr */ + return pxb_a->domain_nr < pxb_b->domain_nr ? -1 : + pxb_a->domain_nr > pxb_b->domain_nr ? 1 : + pxb_a->bus_nr < pxb_b->bus_nr ? -1 : pxb_a->bus_nr > pxb_b->bus_nr ? 1 : 0; } @@ -228,10 +319,17 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) dev_name = dev->qdev.id; } - ds = qdev_create(NULL, TYPE_PXB_HOST); if (pcie) { + /* either in sep_domain or stay in domain 0 */ + g_assert (pxb->sep_domain || pxb->domain_nr == 0); + g_assert (pxb->max_bus >= pxb->bus_nr); + ds = qdev_create(NULL, TYPE_PXB_PCIE_HOST); + qdev_prop_set_uint8(ds, "bus_nr", pxb->bus_nr); //TODO. + qdev_prop_set_uint8(ds, "max_bus", pxb->max_bus); + qdev_prop_set_uint8(ds, "domain_nr", pxb->domain_nr); bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS); } else { + ds = qdev_create(NULL, TYPE_PXB_HOST); bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS); bds = qdev_create(BUS(bus), "pci-bridge"); bds->id = dev_name; @@ -294,6 +392,17 @@ static Property pxb_dev_properties[] = { DEFINE_PROP_END_OF_LIST(), }; +static Property pxb_pcie_dev_properties[] = { + /* Note: 0 is not a legal PXB bus number. */ + DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), + DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), + DEFINE_PROP_BOOL("sep_domain", PXBDev, sep_domain, false), + DEFINE_PROP_UINT32("domain_nr", PXBDev, domain_nr, 0), + DEFINE_PROP_UINT8("max_bus", PXBDev, max_bus, 255), + + DEFINE_PROP_END_OF_LIST(), +}; + static void pxb_dev_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -344,7 +453,7 @@ static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data) k->class_id = PCI_CLASS_BRIDGE_HOST; dc->desc = "PCI Express Expander Bridge"; - dc->props = pxb_dev_properties; + dc->props = pxb_pcie_dev_properties; dc->hotpluggable = false; set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); } @@ -365,6 +474,7 @@ static void pxb_register_types(void) type_register_static(&pxb_bus_info); type_register_static(&pxb_pcie_bus_info); type_register_static(&pxb_host_info); + type_register_static(&pxb_pcie_host_info); type_register_static(&pxb_dev_info); type_register_static(&pxb_pcie_dev_info); } -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [RFC v2 3/3] acpi-build: describe new pci domain in AML 2018-06-12 9:13 [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST Zihan Yang @ 2018-06-12 9:13 ` Zihan Yang 2018-06-20 7:46 ` Marcel Apfelbaum 2018-06-12 13:43 ` [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST Michael S. Tsirkin ` (2 subsequent siblings) 3 siblings, 1 reply; 15+ messages in thread From: Zihan Yang @ 2018-06-12 9:13 UTC (permalink / raw) To: qemu-devel Cc: Zihan Yang, Michael S. Tsirkin, Igor Mammedov, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost Describe new pci segments of host bridges in AML. The host bridge list is replaced by QTAILQ to let q35 host be processed first in every traverse Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> --- hw/i386/acpi-build.c | 69 ++++++++++++++++++++++++++++++----------------- hw/pci/pci.c | 9 ++++--- include/hw/pci/pci_host.h | 2 +- 3 files changed, 50 insertions(+), 30 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 104e52d..a9f1503 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -2123,36 +2123,55 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, sb_scope = aml_scope("\\_SB"); { Object *pci_host; + QObject *o; PCIBus *bus = NULL; + uint32_t domain_nr; + bool q35host = true; pci_host = acpi_get_i386_pci_host(); - if (pci_host) { + while (pci_host) { + o = object_property_get_qobject(pci_host, "domain_nr", NULL); + assert(o); + domain_nr = qnum_get_uint(qobject_to(QNum, o)); + qobject_unref(o); + + /* skip expander bridges that still reside in domain 0 */ + if (!q35host && domain_nr == 0) { + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); + continue; + } bus = PCI_HOST_BRIDGE(pci_host)->bus; - } - if (bus) { - Aml *scope = aml_scope("PCI0"); - /* Scan all PCI buses. Generate tables to support hotplug. */ - build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en); + if (bus) { + Aml *scope = aml_scope("PCI0"); + aml_append(scope, aml_name_decl("_SEG", aml_int(domain_nr))); + /* For simplicity, base bus number starts from 0 */ + aml_append(scope, aml_name_decl("_BBN", aml_int(0))); + /* Scan all PCI buses. Generate tables to support hotplug. */ + build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en); - if (TPM_IS_TIS(tpm_find())) { - dev = aml_device("ISA.TPM"); - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); - aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); - crs = aml_resource_template(); - aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE, - TPM_TIS_ADDR_SIZE, AML_READ_WRITE)); - /* - FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, - Rewrite to take IRQ from TPM device model and - fix default IRQ value there to use some unused IRQ - */ - /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ - aml_append(dev, aml_name_decl("_CRS", crs)); - aml_append(scope, dev); + if (TPM_IS_TIS(tpm_find())) { + dev = aml_device("ISA.TPM"); + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); + aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); + crs = aml_resource_template(); + aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE, + TPM_TIS_ADDR_SIZE, AML_READ_WRITE)); + /* + FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, + Rewrite to take IRQ from TPM device model and + fix default IRQ value there to use some unused IRQ + */ + /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ + aml_append(dev, aml_name_decl("_CRS", crs)); + aml_append(scope, dev); + } + + aml_append(sb_scope, scope); } - - aml_append(sb_scope, scope); + /* q35 host is the first one in the tail queue */ + q35host = false; + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); } } @@ -2645,7 +2664,7 @@ static AcpiMcfgInfo *acpi_get_mcfg(void) qobject_unref(o); /* skip q35 host and bridges that reside in the same domain with it */ if (domain_nr == 0) { - pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); continue; } @@ -2674,7 +2693,7 @@ static AcpiMcfgInfo *acpi_get_mcfg(void) assert(o); mcfg->domain_nr = qnum_get_uint(qobject_to(QNum, o)); - pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); } return head; diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 80bc459..f63385f 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -196,7 +196,8 @@ static void pci_del_option_rom(PCIDevice *pdev); static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET; static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU; -static QLIST_HEAD(, PCIHostState) pci_host_bridges; +static QTAILQ_HEAD(, PCIHostState) pci_host_bridges = + QTAILQ_HEAD_INITIALIZER(pci_host_bridges); int pci_bar(PCIDevice *d, int reg) { @@ -330,7 +331,7 @@ static void pci_host_bus_register(DeviceState *host) { PCIHostState *host_bridge = PCI_HOST_BRIDGE(host); - QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next); + QTAILQ_INSERT_HEAD(&pci_host_bridges, host_bridge, next); } PCIBus *pci_device_root_bus(const PCIDevice *d) @@ -1798,7 +1799,7 @@ PciInfoList *qmp_query_pci(Error **errp) PciInfoList *info, *head = NULL, *cur_item = NULL; PCIHostState *host_bridge; - QLIST_FOREACH(host_bridge, &pci_host_bridges, next) { + QTAILQ_FOREACH(host_bridge, &pci_host_bridges, next) { info = g_malloc0(sizeof(*info)); info->value = qmp_query_pci_bus(host_bridge->bus, pci_bus_num(host_bridge->bus)); @@ -2493,7 +2494,7 @@ int pci_qdev_find_device(const char *id, PCIDevice **pdev) PCIHostState *host_bridge; int rc = -ENODEV; - QLIST_FOREACH(host_bridge, &pci_host_bridges, next) { + QTAILQ_FOREACH(host_bridge, &pci_host_bridges, next) { int tmp = pci_qdev_find_recursive(host_bridge->bus, id, pdev); if (!tmp) { rc = 0; diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h index ba31595..a5617cf 100644 --- a/include/hw/pci/pci_host.h +++ b/include/hw/pci/pci_host.h @@ -47,7 +47,7 @@ struct PCIHostState { uint32_t config_reg; PCIBus *bus; - QLIST_ENTRY(PCIHostState) next; + QTAILQ_ENTRY(PCIHostState) next; }; typedef struct PCIHostBridgeClass { -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/3] acpi-build: describe new pci domain in AML 2018-06-12 9:13 ` [Qemu-devel] [RFC v2 3/3] acpi-build: describe new pci domain in AML Zihan Yang @ 2018-06-20 7:46 ` Marcel Apfelbaum 2018-06-21 16:52 ` Zihan Yang 0 siblings, 1 reply; 15+ messages in thread From: Marcel Apfelbaum @ 2018-06-20 7:46 UTC (permalink / raw) To: Zihan Yang, qemu-devel Cc: Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost On 06/12/2018 12:13 PM, Zihan Yang wrote: > Describe new pci segments of host bridges in AML. The host bridge list is > replaced by QTAILQ to let q35 host be processed first in every traverse > > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> > --- > hw/i386/acpi-build.c | 69 ++++++++++++++++++++++++++++++----------------- > hw/pci/pci.c | 9 ++++--- > include/hw/pci/pci_host.h | 2 +- > 3 files changed, 50 insertions(+), 30 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 104e52d..a9f1503 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -2123,36 +2123,55 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > sb_scope = aml_scope("\\_SB"); > { > Object *pci_host; > + QObject *o; > PCIBus *bus = NULL; > + uint32_t domain_nr; > + bool q35host = true; > > pci_host = acpi_get_i386_pci_host(); > - if (pci_host) { > + while (pci_host) { > + o = object_property_get_qobject(pci_host, "domain_nr", NULL); > + assert(o); > + domain_nr = qnum_get_uint(qobject_to(QNum, o)); > + qobject_unref(o); > + > + /* skip expander bridges that still reside in domain 0 */ > + if (!q35host && domain_nr == 0) { > + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); Why do you skip pci_hosts without domain? We still want to add them to the DSDT, right ? > + continue; > + } > bus = PCI_HOST_BRIDGE(pci_host)->bus; > - } > > - if (bus) { > - Aml *scope = aml_scope("PCI0"); > - /* Scan all PCI buses. Generate tables to support hotplug. */ > - build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en); > + if (bus) { > + Aml *scope = aml_scope("PCI0"); > + aml_append(scope, aml_name_decl("_SEG", aml_int(domain_nr))); > + /* For simplicity, base bus number starts from 0 */ > + aml_append(scope, aml_name_decl("_BBN", aml_int(0))); Nice. _SEG and _BBN should be enough to let the guest know we have multiple PCI domains. > + /* Scan all PCI buses. Generate tables to support hotplug. */ > + build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en); > > - if (TPM_IS_TIS(tpm_find())) { > - dev = aml_device("ISA.TPM"); > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); > - aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); > - crs = aml_resource_template(); > - aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE, > - TPM_TIS_ADDR_SIZE, AML_READ_WRITE)); > - /* > - FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, > - Rewrite to take IRQ from TPM device model and > - fix default IRQ value there to use some unused IRQ > - */ > - /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ > - aml_append(dev, aml_name_decl("_CRS", crs)); > - aml_append(scope, dev); > + if (TPM_IS_TIS(tpm_find())) { > + dev = aml_device("ISA.TPM"); > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); > + aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); > + crs = aml_resource_template(); > + aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE, > + TPM_TIS_ADDR_SIZE, AML_READ_WRITE)); > + /* > + FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, > + Rewrite to take IRQ from TPM device model and > + fix default IRQ value there to use some unused IRQ > + */ > + /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ > + aml_append(dev, aml_name_decl("_CRS", crs)); > + aml_append(scope, dev); > + } > + > + aml_append(sb_scope, scope); > } > - > - aml_append(sb_scope, scope); > + /* q35 host is the first one in the tail queue */ > + q35host = false; I don't think you should use this hack. Add a domain_nr property to the q35 host and set it always to 0. Then you don't need special cases. > + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > } > } > > @@ -2645,7 +2664,7 @@ static AcpiMcfgInfo *acpi_get_mcfg(void) > qobject_unref(o); > /* skip q35 host and bridges that reside in the same domain with it */ > if (domain_nr == 0) { > - pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > continue; > } > > @@ -2674,7 +2693,7 @@ static AcpiMcfgInfo *acpi_get_mcfg(void) > assert(o); > mcfg->domain_nr = qnum_get_uint(qobject_to(QNum, o)); > > - pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > } > > return head; > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 80bc459..f63385f 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -196,7 +196,8 @@ static void pci_del_option_rom(PCIDevice *pdev); > static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET; > static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU; > > -static QLIST_HEAD(, PCIHostState) pci_host_bridges; > +static QTAILQ_HEAD(, PCIHostState) pci_host_bridges = > + QTAILQ_HEAD_INITIALIZER(pci_host_bridges); > > int pci_bar(PCIDevice *d, int reg) > { > @@ -330,7 +331,7 @@ static void pci_host_bus_register(DeviceState *host) > { > PCIHostState *host_bridge = PCI_HOST_BRIDGE(host); > > - QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next); > + QTAILQ_INSERT_HEAD(&pci_host_bridges, host_bridge, next); > } > > PCIBus *pci_device_root_bus(const PCIDevice *d) > @@ -1798,7 +1799,7 @@ PciInfoList *qmp_query_pci(Error **errp) > PciInfoList *info, *head = NULL, *cur_item = NULL; > PCIHostState *host_bridge; > > - QLIST_FOREACH(host_bridge, &pci_host_bridges, next) { > + QTAILQ_FOREACH(host_bridge, &pci_host_bridges, next) { > info = g_malloc0(sizeof(*info)); > info->value = qmp_query_pci_bus(host_bridge->bus, > pci_bus_num(host_bridge->bus)); > @@ -2493,7 +2494,7 @@ int pci_qdev_find_device(const char *id, PCIDevice **pdev) > PCIHostState *host_bridge; > int rc = -ENODEV; > > - QLIST_FOREACH(host_bridge, &pci_host_bridges, next) { > + QTAILQ_FOREACH(host_bridge, &pci_host_bridges, next) { > int tmp = pci_qdev_find_recursive(host_bridge->bus, id, pdev); > if (!tmp) { > rc = 0; > diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h > index ba31595..a5617cf 100644 > --- a/include/hw/pci/pci_host.h > +++ b/include/hw/pci/pci_host.h > @@ -47,7 +47,7 @@ struct PCIHostState { > uint32_t config_reg; > PCIBus *bus; > > - QLIST_ENTRY(PCIHostState) next; > + QTAILQ_ENTRY(PCIHostState) next; > }; > > typedef struct PCIHostBridgeClass { Looking good, do you have something working? Thanks, Marcel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/3] acpi-build: describe new pci domain in AML 2018-06-20 7:46 ` Marcel Apfelbaum @ 2018-06-21 16:52 ` Zihan Yang 2018-06-22 16:43 ` Marcel Apfelbaum 0 siblings, 1 reply; 15+ messages in thread From: Zihan Yang @ 2018-06-21 16:52 UTC (permalink / raw) To: Marcel Apfelbaum Cc: qemu-devel, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost Marcel Apfelbaum <marcel.apfelbaum@gmail.com> 于2018年6月20日周三 下午3:46写道: > > > > On 06/12/2018 12:13 PM, Zihan Yang wrote: > > Describe new pci segments of host bridges in AML. The host bridge list is > > replaced by QTAILQ to let q35 host be processed first in every traverse > > > > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> > > --- > > hw/i386/acpi-build.c | 69 ++++++++++++++++++++++++++++++----------------- > > hw/pci/pci.c | 9 ++++--- > > include/hw/pci/pci_host.h | 2 +- > > 3 files changed, 50 insertions(+), 30 deletions(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 104e52d..a9f1503 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -2123,36 +2123,55 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > sb_scope = aml_scope("\\_SB"); > > { > > Object *pci_host; > > + QObject *o; > > PCIBus *bus = NULL; > > + uint32_t domain_nr; > > + bool q35host = true; > > > > pci_host = acpi_get_i386_pci_host(); > > - if (pci_host) { > > + while (pci_host) { > > + o = object_property_get_qobject(pci_host, "domain_nr", NULL); > > + assert(o); > > + domain_nr = qnum_get_uint(qobject_to(QNum, o)); > > + qobject_unref(o); > > + > > + /* skip expander bridges that still reside in domain 0 */ > > + if (!q35host && domain_nr == 0) { > > + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > > Why do you skip pci_hosts without domain? We still want to add them to > the DSDT, right ? I think I might misunderstand it. Currently QEMU seems to treat expander host bridge as normal a PCIe device under main system bus. I thought we want to preserve current behavior, so my purpose is to only add expander bridges with a non-zero domain_nr into DSDT, and let the other bridges be the PCIe devices of these bridges. Do you mean we should add every host bridge no matter we want it to reside in a different domain or not? > > + continue; > > + } > > bus = PCI_HOST_BRIDGE(pci_host)->bus; > > - } > > > > - if (bus) { > > - Aml *scope = aml_scope("PCI0"); > > - /* Scan all PCI buses. Generate tables to support hotplug. */ > > - build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en); > > + if (bus) { > > + Aml *scope = aml_scope("PCI0"); > > + aml_append(scope, aml_name_decl("_SEG", aml_int(domain_nr))); > > + /* For simplicity, base bus number starts from 0 */ > > + aml_append(scope, aml_name_decl("_BBN", aml_int(0))); > > Nice. _SEG and _BBN should be enough to let the guest know we have multiple > PCI domains. > > > + /* Scan all PCI buses. Generate tables to support hotplug. */ > > + build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en); > > > > - if (TPM_IS_TIS(tpm_find())) { > > - dev = aml_device("ISA.TPM"); > > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); > > - aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); > > - crs = aml_resource_template(); > > - aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE, > > - TPM_TIS_ADDR_SIZE, AML_READ_WRITE)); > > - /* > > - FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, > > - Rewrite to take IRQ from TPM device model and > > - fix default IRQ value there to use some unused IRQ > > - */ > > - /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ > > - aml_append(dev, aml_name_decl("_CRS", crs)); > > - aml_append(scope, dev); > > + if (TPM_IS_TIS(tpm_find())) { > > + dev = aml_device("ISA.TPM"); > > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); > > + aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); > > + crs = aml_resource_template(); > > + aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE, > > + TPM_TIS_ADDR_SIZE, AML_READ_WRITE)); > > + /* > > + FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, > > + Rewrite to take IRQ from TPM device model and > > + fix default IRQ value there to use some unused IRQ > > + */ > > + /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ > > + aml_append(dev, aml_name_decl("_CRS", crs)); > > + aml_append(scope, dev); > > + } > > + > > + aml_append(sb_scope, scope); > > } > > - > > - aml_append(sb_scope, scope); > > + /* q35 host is the first one in the tail queue */ > > + q35host = false; > > I don't think you should use this hack. Add a domain_nr property to the > q35 host > and set it always to 0. Then you don't need special cases. This looks the same as the problem above, if each host bridge will be added into DSDT regardless of its domain_nr, then no special cases are needed. > > + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > > } > > } > > > > @@ -2645,7 +2664,7 @@ static AcpiMcfgInfo *acpi_get_mcfg(void) > > qobject_unref(o); > > /* skip q35 host and bridges that reside in the same domain with it */ > > if (domain_nr == 0) { > > - pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > > + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > > continue; > > } > > > > @@ -2674,7 +2693,7 @@ static AcpiMcfgInfo *acpi_get_mcfg(void) > > assert(o); > > mcfg->domain_nr = qnum_get_uint(qobject_to(QNum, o)); > > > > - pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > > + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > > } > > > > return head; > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 80bc459..f63385f 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -196,7 +196,8 @@ static void pci_del_option_rom(PCIDevice *pdev); > > static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET; > > static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU; > > > > -static QLIST_HEAD(, PCIHostState) pci_host_bridges; > > +static QTAILQ_HEAD(, PCIHostState) pci_host_bridges = > > + QTAILQ_HEAD_INITIALIZER(pci_host_bridges); > > > > int pci_bar(PCIDevice *d, int reg) > > { > > @@ -330,7 +331,7 @@ static void pci_host_bus_register(DeviceState *host) > > { > > PCIHostState *host_bridge = PCI_HOST_BRIDGE(host); > > > > - QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next); > > + QTAILQ_INSERT_HEAD(&pci_host_bridges, host_bridge, next); > > } > > > > PCIBus *pci_device_root_bus(const PCIDevice *d) > > @@ -1798,7 +1799,7 @@ PciInfoList *qmp_query_pci(Error **errp) > > PciInfoList *info, *head = NULL, *cur_item = NULL; > > PCIHostState *host_bridge; > > > > - QLIST_FOREACH(host_bridge, &pci_host_bridges, next) { > > + QTAILQ_FOREACH(host_bridge, &pci_host_bridges, next) { > > info = g_malloc0(sizeof(*info)); > > info->value = qmp_query_pci_bus(host_bridge->bus, > > pci_bus_num(host_bridge->bus)); > > @@ -2493,7 +2494,7 @@ int pci_qdev_find_device(const char *id, PCIDevice **pdev) > > PCIHostState *host_bridge; > > int rc = -ENODEV; > > > > - QLIST_FOREACH(host_bridge, &pci_host_bridges, next) { > > + QTAILQ_FOREACH(host_bridge, &pci_host_bridges, next) { > > int tmp = pci_qdev_find_recursive(host_bridge->bus, id, pdev); > > if (!tmp) { > > rc = 0; > > diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h > > index ba31595..a5617cf 100644 > > --- a/include/hw/pci/pci_host.h > > +++ b/include/hw/pci/pci_host.h > > @@ -47,7 +47,7 @@ struct PCIHostState { > > uint32_t config_reg; > > PCIBus *bus; > > > > - QLIST_ENTRY(PCIHostState) next; > > + QTAILQ_ENTRY(PCIHostState) next; > > }; > > > > typedef struct PCIHostBridgeClass { > > Looking good, do you have something working? Not yet, but I will finish my last exam next week, then I will try to get seabios part working as soon as possible. Sorry for the delay. > Thanks, > Marcel > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 3/3] acpi-build: describe new pci domain in AML 2018-06-21 16:52 ` Zihan Yang @ 2018-06-22 16:43 ` Marcel Apfelbaum 0 siblings, 0 replies; 15+ messages in thread From: Marcel Apfelbaum @ 2018-06-22 16:43 UTC (permalink / raw) To: Zihan Yang Cc: qemu-devel, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost On 06/21/2018 07:52 PM, Zihan Yang wrote: > Marcel Apfelbaum <marcel.apfelbaum@gmail.com> 于2018年6月20日周三 下午3:46写道: >> >> >> On 06/12/2018 12:13 PM, Zihan Yang wrote: >>> Describe new pci segments of host bridges in AML. The host bridge list is >>> replaced by QTAILQ to let q35 host be processed first in every traverse >>> >>> Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> >>> --- >>> hw/i386/acpi-build.c | 69 ++++++++++++++++++++++++++++++----------------- >>> hw/pci/pci.c | 9 ++++--- >>> include/hw/pci/pci_host.h | 2 +- >>> 3 files changed, 50 insertions(+), 30 deletions(-) >>> >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >>> index 104e52d..a9f1503 100644 >>> --- a/hw/i386/acpi-build.c >>> +++ b/hw/i386/acpi-build.c >>> @@ -2123,36 +2123,55 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, >>> sb_scope = aml_scope("\\_SB"); >>> { >>> Object *pci_host; >>> + QObject *o; >>> PCIBus *bus = NULL; >>> + uint32_t domain_nr; >>> + bool q35host = true; >>> >>> pci_host = acpi_get_i386_pci_host(); >>> - if (pci_host) { >>> + while (pci_host) { >>> + o = object_property_get_qobject(pci_host, "domain_nr", NULL); >>> + assert(o); >>> + domain_nr = qnum_get_uint(qobject_to(QNum, o)); >>> + qobject_unref(o); >>> + >>> + /* skip expander bridges that still reside in domain 0 */ >>> + if (!q35host && domain_nr == 0) { >>> + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); >> Why do you skip pci_hosts without domain? We still want to add them to >> the DSDT, right ? > I think I might misunderstand it. Currently QEMU seems to treat expander > host bridge as normal a PCIe device under main system bus. I thought we > want to preserve current behavior, so my purpose is to only add expander > bridges with a non-zero domain_nr into DSDT, and let the other bridges > be the PCIe devices of these bridges. Do you mean we should add every > host bridge no matter we want it to reside in a different domain or not? Yes, all the pci host expanders should be in DSDT. Be aware they were there before your patch. We want them to be in DSDT anyway because even if they are not on a different domain. We still need their _BBN and IO/MEM resources. >>> + continue; >>> + } >>> bus = PCI_HOST_BRIDGE(pci_host)->bus; >>> - } >>> >>> - if (bus) { >>> - Aml *scope = aml_scope("PCI0"); >>> - /* Scan all PCI buses. Generate tables to support hotplug. */ >>> - build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en); >>> + if (bus) { >>> + Aml *scope = aml_scope("PCI0"); >>> + aml_append(scope, aml_name_decl("_SEG", aml_int(domain_nr))); >>> + /* For simplicity, base bus number starts from 0 */ >>> + aml_append(scope, aml_name_decl("_BBN", aml_int(0))); >> Nice. _SEG and _BBN should be enough to let the guest know we have multiple >> PCI domains. >> >>> + /* Scan all PCI buses. Generate tables to support hotplug. */ >>> + build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en); >>> >>> - if (TPM_IS_TIS(tpm_find())) { >>> - dev = aml_device("ISA.TPM"); >>> - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); >>> - aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); >>> - crs = aml_resource_template(); >>> - aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE, >>> - TPM_TIS_ADDR_SIZE, AML_READ_WRITE)); >>> - /* >>> - FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, >>> - Rewrite to take IRQ from TPM device model and >>> - fix default IRQ value there to use some unused IRQ >>> - */ >>> - /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ >>> - aml_append(dev, aml_name_decl("_CRS", crs)); >>> - aml_append(scope, dev); >>> + if (TPM_IS_TIS(tpm_find())) { >>> + dev = aml_device("ISA.TPM"); >>> + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31"))); >>> + aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); >>> + crs = aml_resource_template(); >>> + aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE, >>> + TPM_TIS_ADDR_SIZE, AML_READ_WRITE)); >>> + /* >>> + FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs, >>> + Rewrite to take IRQ from TPM device model and >>> + fix default IRQ value there to use some unused IRQ >>> + */ >>> + /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ >>> + aml_append(dev, aml_name_decl("_CRS", crs)); >>> + aml_append(scope, dev); >>> + } >>> + >>> + aml_append(sb_scope, scope); >>> } >>> - >>> - aml_append(sb_scope, scope); >>> + /* q35 host is the first one in the tail queue */ >>> + q35host = false; >> I don't think you should use this hack. Add a domain_nr property to the >> q35 host >> and set it always to 0. Then you don't need special cases. > This looks the same as the problem above, if each host bridge will be added > into DSDT regardless of its domain_nr, then no special cases are needed. Exactly > >>> + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); >>> } >>> } >>> >>> @@ -2645,7 +2664,7 @@ static AcpiMcfgInfo *acpi_get_mcfg(void) >>> qobject_unref(o); >>> /* skip q35 host and bridges that reside in the same domain with it */ >>> if (domain_nr == 0) { >>> - pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); >>> + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); >>> continue; >>> } >>> >>> @@ -2674,7 +2693,7 @@ static AcpiMcfgInfo *acpi_get_mcfg(void) >>> assert(o); >>> mcfg->domain_nr = qnum_get_uint(qobject_to(QNum, o)); >>> >>> - pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); >>> + pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next)); >>> } >>> >>> return head; >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>> index 80bc459..f63385f 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -196,7 +196,8 @@ static void pci_del_option_rom(PCIDevice *pdev); >>> static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET; >>> static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU; >>> >>> -static QLIST_HEAD(, PCIHostState) pci_host_bridges; >>> +static QTAILQ_HEAD(, PCIHostState) pci_host_bridges = >>> + QTAILQ_HEAD_INITIALIZER(pci_host_bridges); >>> >>> int pci_bar(PCIDevice *d, int reg) >>> { >>> @@ -330,7 +331,7 @@ static void pci_host_bus_register(DeviceState *host) >>> { >>> PCIHostState *host_bridge = PCI_HOST_BRIDGE(host); >>> >>> - QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next); >>> + QTAILQ_INSERT_HEAD(&pci_host_bridges, host_bridge, next); >>> } >>> >>> PCIBus *pci_device_root_bus(const PCIDevice *d) >>> @@ -1798,7 +1799,7 @@ PciInfoList *qmp_query_pci(Error **errp) >>> PciInfoList *info, *head = NULL, *cur_item = NULL; >>> PCIHostState *host_bridge; >>> >>> - QLIST_FOREACH(host_bridge, &pci_host_bridges, next) { >>> + QTAILQ_FOREACH(host_bridge, &pci_host_bridges, next) { >>> info = g_malloc0(sizeof(*info)); >>> info->value = qmp_query_pci_bus(host_bridge->bus, >>> pci_bus_num(host_bridge->bus)); >>> @@ -2493,7 +2494,7 @@ int pci_qdev_find_device(const char *id, PCIDevice **pdev) >>> PCIHostState *host_bridge; >>> int rc = -ENODEV; >>> >>> - QLIST_FOREACH(host_bridge, &pci_host_bridges, next) { >>> + QTAILQ_FOREACH(host_bridge, &pci_host_bridges, next) { >>> int tmp = pci_qdev_find_recursive(host_bridge->bus, id, pdev); >>> if (!tmp) { >>> rc = 0; >>> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h >>> index ba31595..a5617cf 100644 >>> --- a/include/hw/pci/pci_host.h >>> +++ b/include/hw/pci/pci_host.h >>> @@ -47,7 +47,7 @@ struct PCIHostState { >>> uint32_t config_reg; >>> PCIBus *bus; >>> >>> - QLIST_ENTRY(PCIHostState) next; >>> + QTAILQ_ENTRY(PCIHostState) next; >>> }; >>> >>> typedef struct PCIHostBridgeClass { >> Looking good, do you have something working? > Not yet, but I will finish my last exam next week, then I will try to get > seabios part working as soon as possible. Sorry for the delay. There is no rush, good luck to your exams! Thanks, Marcel > >> Thanks, >> Marcel >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST 2018-06-12 9:13 [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST Zihan Yang 2018-06-12 9:13 ` [Qemu-devel] [RFC v2 3/3] acpi-build: describe new pci domain in AML Zihan Yang @ 2018-06-12 13:43 ` Michael S. Tsirkin 2018-06-13 8:23 ` Zihan Yang 2018-06-20 6:38 ` Marcel Apfelbaum [not found] ` <1528794804-6289-2-git-send-email-whois.zihan.yang@gmail.com> 3 siblings, 1 reply; 15+ messages in thread From: Michael S. Tsirkin @ 2018-06-12 13:43 UTC (permalink / raw) To: Zihan Yang; +Cc: qemu-devel, Marcel Apfelbaum On Tue, Jun 12, 2018 at 05:13:22PM +0800, Zihan Yang wrote: > The inner host bridge created by pxb-pcie is TYPE_PXB_PCI_HOST by default, > add a new type TYPE_PXB_PCIE_HOST to better utilize the ECAM of PCIe > > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> I have a concern that there are lots of new properties added here, I'm not sure how are upper layers supposed to manage them all. E.g. bus_nr supplied in several places, domain_nr for which it's not clear how it is supposed to be allocated, etc. Can the management interface be simplified? Ideally we wouldn't have to teach libvirt new tricks, just generalize pxb support slightly. > --- > hw/pci-bridge/pci_expander_bridge.c | 118 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 114 insertions(+), 4 deletions(-) > > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c > index e62de42..448b9fb 100644 > --- a/hw/pci-bridge/pci_expander_bridge.c > +++ b/hw/pci-bridge/pci_expander_bridge.c > @@ -15,10 +15,12 @@ > #include "hw/pci/pci.h" > #include "hw/pci/pci_bus.h" > #include "hw/pci/pci_host.h" > +#include "hw/pci/pcie_host.h" > #include "hw/pci/pci_bridge.h" > #include "qemu/range.h" > #include "qemu/error-report.h" > #include "sysemu/numa.h" > +#include "qapi/visitor.h" > > #define TYPE_PXB_BUS "pxb-bus" > #define PXB_BUS(obj) OBJECT_CHECK(PXBBus, (obj), TYPE_PXB_BUS) > @@ -45,7 +47,10 @@ typedef struct PXBDev { > PCIDevice parent_obj; > /*< public >*/ > > - uint8_t bus_nr; > + bool sep_domain; /* whether it resides in separate PCI segment */ > + uint32_t domain_nr; /* PCI domain(segment) number, should be 0 if sep_domain is false */ > + uint8_t max_bus; /* max number of buses to use */ > + uint8_t bus_nr; /* should be 0 when in separate domain */ > uint16_t numa_node; > } PXBDev; > > @@ -58,6 +63,18 @@ static PXBDev *convert_to_pxb(PCIDevice *dev) > static GList *pxb_dev_list; > > #define TYPE_PXB_HOST "pxb-host" > +#define TYPE_PXB_PCIE_HOST "pxb-pcie-host" > +#define PXB_PCIE_HOST_DEVICE(obj) \ > + OBJECT_CHECK(PXBPCIEHost, (obj), TYPE_PXB_PCIE_HOST) > + > +typedef struct PXBPCIEHost { > + PCIExpressHost parent_obj; > + > + /* should only inherit from PXBDev */ > + uint32_t domain_nr; > + uint8_t bus_nr; > + uint8_t max_bus; > +} PXBPCIEHost; > > static int pxb_bus_num(PCIBus *bus) > { > @@ -111,6 +128,31 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge, > return bus->bus_path; > } > > +/* Use a dedicated function for PCIe since pxb-host does > + * not have a domain_nr field */ > +static const char *pxb_pcie_host_root_bus_path(PCIHostState *host_bridge, > + PCIBus *rootbus) > +{ > + if (!pci_bus_is_express(rootbus)) { > + /* pxb-pcie-host cannot reside on a PCI bus */ > + return NULL; > + } > + PXBBus *bus = PXB_PCIE_BUS(rootbus); > + > + snprintf(bus->bus_path, 8, "%04lx:%02x", > + object_property_get_uint((Object *)host_bridge, "domain_nr", NULL), > + pxb_bus_num(rootbus)); > + return bus->bus_path; > +} > + > +static void pxb_pcie_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + PCIExpressHost *e = PCIE_HOST_BRIDGE(obj); > + > + visit_type_uint64(v, name, &e->size, errp); > +} > + > static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) > { > const PCIHostState *pxb_host; > @@ -142,6 +184,30 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) > return NULL; > } > > +static void pxb_pcie_host_initfn(Object *obj) > +{ > + PCIHostState *phb = PCI_HOST_BRIDGE(obj); > + > + memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb, > + "pci-conf-idx", 4); > + memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb, > + "pci-conf-data", 4); > + > + object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64", > + pxb_pcie_host_get_mmcfg_size, > + NULL, NULL, NULL, NULL); > + > +} > + > +static Property pxb_pcie_host_props[] = { > + DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, PXBPCIEHost, parent_obj.base_addr, > + PCIE_BASE_ADDR_UNMAPPED), > + DEFINE_PROP_UINT32("domain_nr", PXBPCIEHost, domain_nr, 0), > + DEFINE_PROP_UINT8("bus_nr", PXBPCIEHost, bus_nr, 0), > + DEFINE_PROP_UINT8("max_bus", PXBPCIEHost, max_bus, 255), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void pxb_host_class_init(ObjectClass *class, void *data) > { > DeviceClass *dc = DEVICE_CLASS(class); > @@ -155,12 +221,34 @@ static void pxb_host_class_init(ObjectClass *class, void *data) > hc->root_bus_path = pxb_host_root_bus_path; > } > > +static void pxb_pcie_host_class_init(ObjectClass *class, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(class); > + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class); > + PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class); > + > + dc->fw_name = "pci"; > + dc->props = pxb_pcie_host_props; > + /* Reason: Internal part of the pxb/pxb-pcie device, not usable by itself */ > + dc->user_creatable = false; > + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address; > + hc->root_bus_path = pxb_pcie_host_root_bus_path; > +} > + > static const TypeInfo pxb_host_info = { > .name = TYPE_PXB_HOST, > .parent = TYPE_PCI_HOST_BRIDGE, > .class_init = pxb_host_class_init, > }; > > +static const TypeInfo pxb_pcie_host_info = { > + .name = TYPE_PXB_PCIE_HOST, > + .parent = TYPE_PCIE_HOST_BRIDGE, > + .instance_size = sizeof(PXBPCIEHost), > + .instance_init = pxb_pcie_host_initfn, > + .class_init = pxb_pcie_host_class_init, > +}; > + > /* > * Registers the PXB bus as a child of pci host root bus. > */ > @@ -205,7 +293,10 @@ static gint pxb_compare(gconstpointer a, gconstpointer b) > { > const PXBDev *pxb_a = a, *pxb_b = b; > > - return pxb_a->bus_nr < pxb_b->bus_nr ? -1 : > + /* check domain_nr, then bus_nr */ > + return pxb_a->domain_nr < pxb_b->domain_nr ? -1 : > + pxb_a->domain_nr > pxb_b->domain_nr ? 1 : > + pxb_a->bus_nr < pxb_b->bus_nr ? -1 : > pxb_a->bus_nr > pxb_b->bus_nr ? 1 : > 0; > } > @@ -228,10 +319,17 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) > dev_name = dev->qdev.id; > } > > - ds = qdev_create(NULL, TYPE_PXB_HOST); > if (pcie) { > + /* either in sep_domain or stay in domain 0 */ > + g_assert (pxb->sep_domain || pxb->domain_nr == 0); > + g_assert (pxb->max_bus >= pxb->bus_nr); > + ds = qdev_create(NULL, TYPE_PXB_PCIE_HOST); > + qdev_prop_set_uint8(ds, "bus_nr", pxb->bus_nr); //TODO. > + qdev_prop_set_uint8(ds, "max_bus", pxb->max_bus); > + qdev_prop_set_uint8(ds, "domain_nr", pxb->domain_nr); > bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS); > } else { > + ds = qdev_create(NULL, TYPE_PXB_HOST); > bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS); > bds = qdev_create(BUS(bus), "pci-bridge"); > bds->id = dev_name; > @@ -294,6 +392,17 @@ static Property pxb_dev_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static Property pxb_pcie_dev_properties[] = { > + /* Note: 0 is not a legal PXB bus number. */ > + DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), > + DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), > + DEFINE_PROP_BOOL("sep_domain", PXBDev, sep_domain, false), > + DEFINE_PROP_UINT32("domain_nr", PXBDev, domain_nr, 0), > + DEFINE_PROP_UINT8("max_bus", PXBDev, max_bus, 255), > + > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void pxb_dev_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -344,7 +453,7 @@ static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data) > k->class_id = PCI_CLASS_BRIDGE_HOST; > > dc->desc = "PCI Express Expander Bridge"; > - dc->props = pxb_dev_properties; > + dc->props = pxb_pcie_dev_properties; > dc->hotpluggable = false; > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > } > @@ -365,6 +474,7 @@ static void pxb_register_types(void) > type_register_static(&pxb_bus_info); > type_register_static(&pxb_pcie_bus_info); > type_register_static(&pxb_host_info); > + type_register_static(&pxb_pcie_host_info); > type_register_static(&pxb_dev_info); > type_register_static(&pxb_pcie_dev_info); > } > -- > 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST 2018-06-12 13:43 ` [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST Michael S. Tsirkin @ 2018-06-13 8:23 ` Zihan Yang 2018-06-13 14:46 ` Michael S. Tsirkin 2018-06-20 4:31 ` Marcel Apfelbaum 0 siblings, 2 replies; 15+ messages in thread From: Zihan Yang @ 2018-06-13 8:23 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel, Marcel Apfelbaum Michael S. Tsirkin <mst@redhat.com> 于2018年6月12日周二 下午9:43写道: > > On Tue, Jun 12, 2018 at 05:13:22PM +0800, Zihan Yang wrote: > > The inner host bridge created by pxb-pcie is TYPE_PXB_PCI_HOST by default, > > add a new type TYPE_PXB_PCIE_HOST to better utilize the ECAM of PCIe > > > > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> > > I have a concern that there are lots of new properties > added here, I'm not sure how are upper layers supposed to > manage them all. > > E.g. bus_nr supplied in several places, domain_nr for which > it's not clear how it is supposed to be allocated, etc. Indeed they seem to double the properties, but the pxb host is an internal structure of pxb-pcie device, created in pxb-pcie's realization procedure, and acpi-build queries host bridges instead of pxb-pcie devices. This means that users can not directly specify the property of pxb host bridge, but must 'inherit' from pxb-pcie devices. I had thought about changing the acpi-build process, but that would require more modifications. As for the properties, bus_nr means the start bus number of this host bridge. It is used when pxb-pcie is in pci domain 0 with q35 host to avoid bus number confliction. When it is placed in a separate pci domain, it is not used and should be 0. max_bus means how many buses the user desires, EACH bus in PCIe requires 1MB configuration space, thus specifying it could reduce the reserved memory in MMCFG as suggested by Marcel. Typically, the user can specify -device pxb-pcie,id=br1,bus="pcie.0",sep_domain=on,domain_nr=1,max_bus=130 this will place the buses under this pxb host bridge in pci domain 1, and reserve (130 + 1) = 131 buses for it. The start bus number is always 0 currently for simplicity. > Can the management interface be simplified? > Ideally we wouldn't have to teach libvirt new tricks, > just generalize pxb support slightly. We can delete 'sep_domain' property, I just find 'domain_nr' already indicates domain number. But domain_nr and max_bus seems unremovable, although they look 'redundant' because they appear twice. I'm not familiar with libvirt, but from the perspective of user, only 2 properties are added(domain_nr and max_bus, if we delete sep_domain), though the internal structure actually has changed. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST 2018-06-13 8:23 ` Zihan Yang @ 2018-06-13 14:46 ` Michael S. Tsirkin 2018-06-14 3:29 ` Zihan Yang 2018-06-20 4:31 ` Marcel Apfelbaum 1 sibling, 1 reply; 15+ messages in thread From: Michael S. Tsirkin @ 2018-06-13 14:46 UTC (permalink / raw) To: Zihan Yang; +Cc: qemu-devel, Marcel Apfelbaum On Wed, Jun 13, 2018 at 04:23:40PM +0800, Zihan Yang wrote: > Michael S. Tsirkin <mst@redhat.com> 于2018年6月12日周二 下午9:43写道: > > > > On Tue, Jun 12, 2018 at 05:13:22PM +0800, Zihan Yang wrote: > > > The inner host bridge created by pxb-pcie is TYPE_PXB_PCI_HOST by default, > > > add a new type TYPE_PXB_PCIE_HOST to better utilize the ECAM of PCIe > > > > > > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> > > > > I have a concern that there are lots of new properties > > added here, I'm not sure how are upper layers supposed to > > manage them all. > > > > E.g. bus_nr supplied in several places, domain_nr for which > > it's not clear how it is supposed to be allocated, etc. > > Indeed they seem to double the properties, but the pxb host is > an internal structure of pxb-pcie device, created in pxb-pcie's > realization procedure, and acpi-build queries host bridges instead > of pxb-pcie devices. This means that users can not directly specify > the property of pxb host bridge, but must 'inherit' from pxb-pcie > devices. I had thought about changing the acpi-build process, > but that would require more modifications. > As for the properties, bus_nr means the start bus number > of this host bridge. It is used when pxb-pcie is in pci domain 0 > with q35 host to avoid bus number confliction. When it is placed > in a separate pci domain, it is not used and should be 0. > > max_bus means how many buses the user desires, EACH bus in > PCIe requires 1MB configuration space, thus specifying it could > reduce the reserved memory in MMCFG as suggested by Marcel. > Typically, the user can specify > > -device pxb-pcie,id=br1,bus="pcie.0",sep_domain=on,domain_nr=1,max_bus=130 > > this will place the buses under this pxb host bridge in pci domain > 1, and reserve (130 + 1) = 131 buses for it. The start bus number > is always 0 currently for simplicity. > > > Can the management interface be simplified? > > Ideally we wouldn't have to teach libvirt new tricks, > > just generalize pxb support slightly. > > We can delete 'sep_domain' property, I just find 'domain_nr' > already indicates domain number. But domain_nr and > max_bus seems unremovable, although they look 'redundant' > because they appear twice. > > I'm not familiar with libvirt, but from the perspective of user, > only 2 properties are added(domain_nr and max_bus, if we > delete sep_domain), though the internal structure actually has > changed. If you want a property for an internal purpose, you can have a property starting with "x-" this way we don't commit to maintaining it. -- MST ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST 2018-06-13 14:46 ` Michael S. Tsirkin @ 2018-06-14 3:29 ` Zihan Yang 0 siblings, 0 replies; 15+ messages in thread From: Zihan Yang @ 2018-06-14 3:29 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel, Marcel Apfelbaum Michael S. Tsirkin <mst@redhat.com> 于2018年6月13日周三 下午10:46写道: > > On Wed, Jun 13, 2018 at 04:23:40PM +0800, Zihan Yang wrote: > > Michael S. Tsirkin <mst@redhat.com> 于2018年6月12日周二 下午9:43写道: > > > > > > On Tue, Jun 12, 2018 at 05:13:22PM +0800, Zihan Yang wrote: > > > > The inner host bridge created by pxb-pcie is TYPE_PXB_PCI_HOST by default, > > > > add a new type TYPE_PXB_PCIE_HOST to better utilize the ECAM of PCIe > > > > > > > > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> > > > > > > I have a concern that there are lots of new properties > > > added here, I'm not sure how are upper layers supposed to > > > manage them all. > > > > > > E.g. bus_nr supplied in several places, domain_nr for which > > > it's not clear how it is supposed to be allocated, etc. > > > > Indeed they seem to double the properties, but the pxb host is > > an internal structure of pxb-pcie device, created in pxb-pcie's > > realization procedure, and acpi-build queries host bridges instead > > of pxb-pcie devices. This means that users can not directly specify > > the property of pxb host bridge, but must 'inherit' from pxb-pcie > > devices. I had thought about changing the acpi-build process, > > but that would require more modifications. > > As for the properties, bus_nr means the start bus number > > of this host bridge. It is used when pxb-pcie is in pci domain 0 > > with q35 host to avoid bus number confliction. When it is placed > > in a separate pci domain, it is not used and should be 0. > > > > max_bus means how many buses the user desires, EACH bus in > > PCIe requires 1MB configuration space, thus specifying it could > > reduce the reserved memory in MMCFG as suggested by Marcel. > > Typically, the user can specify > > > > -device pxb-pcie,id=br1,bus="pcie.0",sep_domain=on,domain_nr=1,max_bus=130 > > > > this will place the buses under this pxb host bridge in pci domain > > 1, and reserve (130 + 1) = 131 buses for it. The start bus number > > is always 0 currently for simplicity. > > > > > Can the management interface be simplified? > > > Ideally we wouldn't have to teach libvirt new tricks, > > > just generalize pxb support slightly. > > > > We can delete 'sep_domain' property, I just find 'domain_nr' > > already indicates domain number. But domain_nr and > > max_bus seems unremovable, although they look 'redundant' > > because they appear twice. > > > > I'm not familiar with libvirt, but from the perspective of user, > > only 2 properties are added(domain_nr and max_bus, if we > > delete sep_domain), though the internal structure actually has > > changed. > > If you want a property for an internal purpose, > you can have a property starting with "x-" this way > we don't commit to maintaining it. OK, I'll change that in pxb-pcie-host. > -- > MST ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST 2018-06-13 8:23 ` Zihan Yang 2018-06-13 14:46 ` Michael S. Tsirkin @ 2018-06-20 4:31 ` Marcel Apfelbaum 1 sibling, 0 replies; 15+ messages in thread From: Marcel Apfelbaum @ 2018-06-20 4:31 UTC (permalink / raw) To: Zihan Yang, Michael S. Tsirkin; +Cc: qemu-devel On 06/13/2018 11:23 AM, Zihan Yang wrote: > Michael S. Tsirkin <mst@redhat.com> 于2018年6月12日周二 下午9:43写道: >> On Tue, Jun 12, 2018 at 05:13:22PM +0800, Zihan Yang wrote: >>> The inner host bridge created by pxb-pcie is TYPE_PXB_PCI_HOST by default, >>> add a new type TYPE_PXB_PCIE_HOST to better utilize the ECAM of PCIe >>> >>> Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> >> I have a concern that there are lots of new properties >> added here, I'm not sure how are upper layers supposed to >> manage them all. >> >> E.g. bus_nr supplied in several places, domain_nr for which >> it's not clear how it is supposed to be allocated, etc. > Indeed they seem to double the properties, but the pxb host is > an internal structure of pxb-pcie device, created in pxb-pcie's > realization procedure, and acpi-build queries host bridges instead > of pxb-pcie devices. This means that users can not directly specify > the property of pxb host bridge, but must 'inherit' from pxb-pcie > devices. I had thought about changing the acpi-build process, > but that would require more modifications. > > As for the properties, bus_nr means the start bus number > of this host bridge. It is used when pxb-pcie is in pci domain 0 > with q35 host to avoid bus number confliction. When it is placed > in a separate pci domain, it is not used and should be 0. > > max_bus means how many buses the user desires, EACH bus in > PCIe requires 1MB configuration space, thus specifying it could > reduce the reserved memory in MMCFG as suggested by Marcel. The max_bus property is optional, you set the default to 255. I am wondering if 255 is too much as a default for an extra bus, I would use a smaller value, like 10. > Typically, the user can specify > > -device pxb-pcie,id=br1,bus="pcie.0",sep_domain=on,domain_nr=1,max_bus=130 > > this will place the buses under this pxb host bridge in pci domain > 1, and reserve (130 + 1) = 131 buses for it. The start bus number > is always 0 currently for simplicity. > >> Can the management interface be simplified? >> Ideally we wouldn't have to teach libvirt new tricks, >> just generalize pxb support slightly. > We can delete 'sep_domain' property, I just find 'domain_nr' > already indicates domain number. Agreed, please remove sep_domain property. Thanks, > But domain_nr and > max_bus seems unremovable, although they look 'redundant' > because they appear twice. > > I'm not familiar with libvirt, but from the perspective of user, > only 2 properties are added(domain_nr and max_bus, if we > delete sep_domain), though the internal structure actually has > changed. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST 2018-06-12 9:13 [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST Zihan Yang 2018-06-12 9:13 ` [Qemu-devel] [RFC v2 3/3] acpi-build: describe new pci domain in AML Zihan Yang 2018-06-12 13:43 ` [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST Michael S. Tsirkin @ 2018-06-20 6:38 ` Marcel Apfelbaum 2018-06-21 16:46 ` Zihan Yang [not found] ` <1528794804-6289-2-git-send-email-whois.zihan.yang@gmail.com> 3 siblings, 1 reply; 15+ messages in thread From: Marcel Apfelbaum @ 2018-06-20 6:38 UTC (permalink / raw) To: Zihan Yang, qemu-devel; +Cc: Michael S. Tsirkin On 06/12/2018 12:13 PM, Zihan Yang wrote: > The inner host bridge created by pxb-pcie is TYPE_PXB_PCI_HOST by default, > add a new type TYPE_PXB_PCIE_HOST to better utilize the ECAM of PCIe > > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> > --- > hw/pci-bridge/pci_expander_bridge.c | 118 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 114 insertions(+), 4 deletions(-) > > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c > index e62de42..448b9fb 100644 > --- a/hw/pci-bridge/pci_expander_bridge.c > +++ b/hw/pci-bridge/pci_expander_bridge.c > @@ -15,10 +15,12 @@ > #include "hw/pci/pci.h" > #include "hw/pci/pci_bus.h" > #include "hw/pci/pci_host.h" > +#include "hw/pci/pcie_host.h" > #include "hw/pci/pci_bridge.h" > #include "qemu/range.h" > #include "qemu/error-report.h" > #include "sysemu/numa.h" > +#include "qapi/visitor.h" > > #define TYPE_PXB_BUS "pxb-bus" > #define PXB_BUS(obj) OBJECT_CHECK(PXBBus, (obj), TYPE_PXB_BUS) > @@ -45,7 +47,10 @@ typedef struct PXBDev { > PCIDevice parent_obj; > /*< public >*/ > > - uint8_t bus_nr; > + bool sep_domain; /* whether it resides in separate PCI segment */ > + uint32_t domain_nr; /* PCI domain(segment) number, should be 0 if sep_domain is false */ > + uint8_t max_bus; /* max number of buses to use */ > + uint8_t bus_nr; /* should be 0 when in separate domain */ > uint16_t numa_node; > } PXBDev; > > @@ -58,6 +63,18 @@ static PXBDev *convert_to_pxb(PCIDevice *dev) > static GList *pxb_dev_list; > > #define TYPE_PXB_HOST "pxb-host" > +#define TYPE_PXB_PCIE_HOST "pxb-pcie-host" > +#define PXB_PCIE_HOST_DEVICE(obj) \ > + OBJECT_CHECK(PXBPCIEHost, (obj), TYPE_PXB_PCIE_HOST) > + > +typedef struct PXBPCIEHost { > + PCIExpressHost parent_obj; > + > + /* should only inherit from PXBDev */ > + uint32_t domain_nr; > + uint8_t bus_nr; > + uint8_t max_bus; I don't think you need the above properties to be part of the PXBPCIEHost object. Please see if you can't get a pointer to the PxbDev. > +} PXBPCIEHost; > > static int pxb_bus_num(PCIBus *bus) > { > @@ -111,6 +128,31 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge, > return bus->bus_path; > } > > +/* Use a dedicated function for PCIe since pxb-host does > + * not have a domain_nr field */ > +static const char *pxb_pcie_host_root_bus_path(PCIHostState *host_bridge, > + PCIBus *rootbus) > +{ > + if (!pci_bus_is_express(rootbus)) { > + /* pxb-pcie-host cannot reside on a PCI bus */ > + return NULL; > + } > + PXBBus *bus = PXB_PCIE_BUS(rootbus); > + > + snprintf(bus->bus_path, 8, "%04lx:%02x", > + object_property_get_uint((Object *)host_bridge, "domain_nr", NULL), > + pxb_bus_num(rootbus)); > + return bus->bus_path; > +} > + > +static void pxb_pcie_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + PCIExpressHost *e = PCIE_HOST_BRIDGE(obj); > + > + visit_type_uint64(v, name, &e->size, errp); > +} > + > static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) > { > const PCIHostState *pxb_host; > @@ -142,6 +184,30 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) > return NULL; > } > > +static void pxb_pcie_host_initfn(Object *obj) > +{ > + PCIHostState *phb = PCI_HOST_BRIDGE(obj); > + > + memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb, > + "pci-conf-idx", 4); > + memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb, > + "pci-conf-data", 4); > + > + object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64", > + pxb_pcie_host_get_mmcfg_size, > + NULL, NULL, NULL, NULL); > + > +} > + > +static Property pxb_pcie_host_props[] = { > + DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, PXBPCIEHost, parent_obj.base_addr, > + PCIE_BASE_ADDR_UNMAPPED), > + DEFINE_PROP_UINT32("domain_nr", PXBPCIEHost, domain_nr, 0), > + DEFINE_PROP_UINT8("bus_nr", PXBPCIEHost, bus_nr, 0), > + DEFINE_PROP_UINT8("max_bus", PXBPCIEHost, max_bus, 255), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void pxb_host_class_init(ObjectClass *class, void *data) > { > DeviceClass *dc = DEVICE_CLASS(class); > @@ -155,12 +221,34 @@ static void pxb_host_class_init(ObjectClass *class, void *data) > hc->root_bus_path = pxb_host_root_bus_path; > } > > +static void pxb_pcie_host_class_init(ObjectClass *class, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(class); > + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class); > + PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class); > + > + dc->fw_name = "pci"; > + dc->props = pxb_pcie_host_props; > + /* Reason: Internal part of the pxb/pxb-pcie device, not usable by itself */ > + dc->user_creatable = false; > + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address; > + hc->root_bus_path = pxb_pcie_host_root_bus_path; > +} > + > static const TypeInfo pxb_host_info = { > .name = TYPE_PXB_HOST, > .parent = TYPE_PCI_HOST_BRIDGE, > .class_init = pxb_host_class_init, > }; > > +static const TypeInfo pxb_pcie_host_info = { > + .name = TYPE_PXB_PCIE_HOST, > + .parent = TYPE_PCIE_HOST_BRIDGE, > + .instance_size = sizeof(PXBPCIEHost), > + .instance_init = pxb_pcie_host_initfn, > + .class_init = pxb_pcie_host_class_init, > +}; > + > /* > * Registers the PXB bus as a child of pci host root bus. > */ > @@ -205,7 +293,10 @@ static gint pxb_compare(gconstpointer a, gconstpointer b) > { > const PXBDev *pxb_a = a, *pxb_b = b; > > - return pxb_a->bus_nr < pxb_b->bus_nr ? -1 : > + /* check domain_nr, then bus_nr */ > + return pxb_a->domain_nr < pxb_b->domain_nr ? -1 : > + pxb_a->domain_nr > pxb_b->domain_nr ? 1 : > + pxb_a->bus_nr < pxb_b->bus_nr ? -1 : > pxb_a->bus_nr > pxb_b->bus_nr ? 1 : > 0; > } > @@ -228,10 +319,17 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) > dev_name = dev->qdev.id; > } > > - ds = qdev_create(NULL, TYPE_PXB_HOST); > if (pcie) { > + /* either in sep_domain or stay in domain 0 */ > + g_assert (pxb->sep_domain || pxb->domain_nr == 0); > + g_assert (pxb->max_bus >= pxb->bus_nr); > + ds = qdev_create(NULL, TYPE_PXB_PCIE_HOST); > + qdev_prop_set_uint8(ds, "bus_nr", pxb->bus_nr); //TODO. > + qdev_prop_set_uint8(ds, "max_bus", pxb->max_bus); > + qdev_prop_set_uint8(ds, "domain_nr", pxb->domain_nr); > bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS); > } else { > + ds = qdev_create(NULL, TYPE_PXB_HOST); > bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS); > bds = qdev_create(BUS(bus), "pci-bridge"); > bds->id = dev_name; > @@ -294,6 +392,17 @@ static Property pxb_dev_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static Property pxb_pcie_dev_properties[] = { > + /* Note: 0 is not a legal PXB bus number. */ > + DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), > + DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), > + DEFINE_PROP_BOOL("sep_domain", PXBDev, sep_domain, false), > + DEFINE_PROP_UINT32("domain_nr", PXBDev, domain_nr, 0), > + DEFINE_PROP_UINT8("max_bus", PXBDev, max_bus, 255), > + Duplicated properties are not an optimal solution. I see 2 possible ones: - Use a macro defining common properties - Use a base class and put the properties there. > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void pxb_dev_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -344,7 +453,7 @@ static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data) > k->class_id = PCI_CLASS_BRIDGE_HOST; > > dc->desc = "PCI Express Expander Bridge"; > - dc->props = pxb_dev_properties; > + dc->props = pxb_pcie_dev_properties; > dc->hotpluggable = false; > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > } > @@ -365,6 +474,7 @@ static void pxb_register_types(void) > type_register_static(&pxb_bus_info); > type_register_static(&pxb_pcie_bus_info); > type_register_static(&pxb_host_info); > + type_register_static(&pxb_pcie_host_info); > type_register_static(&pxb_dev_info); > type_register_static(&pxb_pcie_dev_info); > } Looking good! Thanks, Marcel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST 2018-06-20 6:38 ` Marcel Apfelbaum @ 2018-06-21 16:46 ` Zihan Yang 0 siblings, 0 replies; 15+ messages in thread From: Zihan Yang @ 2018-06-21 16:46 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: qemu-devel, Michael S. Tsirkin Marcel Apfelbaum <marcel.apfelbaum@gmail.com> 于2018年6月20日周三 下午2:38写道: > > > > On 06/12/2018 12:13 PM, Zihan Yang wrote: > > The inner host bridge created by pxb-pcie is TYPE_PXB_PCI_HOST by default, > > add a new type TYPE_PXB_PCIE_HOST to better utilize the ECAM of PCIe > > > > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> > > --- > > hw/pci-bridge/pci_expander_bridge.c | 118 ++++++++++++++++++++++++++++++++++-- > > 1 file changed, 114 insertions(+), 4 deletions(-) > > > > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c > > index e62de42..448b9fb 100644 > > --- a/hw/pci-bridge/pci_expander_bridge.c > > +++ b/hw/pci-bridge/pci_expander_bridge.c > > @@ -15,10 +15,12 @@ > > #include "hw/pci/pci.h" > > #include "hw/pci/pci_bus.h" > > #include "hw/pci/pci_host.h" > > +#include "hw/pci/pcie_host.h" > > #include "hw/pci/pci_bridge.h" > > #include "qemu/range.h" > > #include "qemu/error-report.h" > > #include "sysemu/numa.h" > > +#include "qapi/visitor.h" > > > > #define TYPE_PXB_BUS "pxb-bus" > > #define PXB_BUS(obj) OBJECT_CHECK(PXBBus, (obj), TYPE_PXB_BUS) > > @@ -45,7 +47,10 @@ typedef struct PXBDev { > > PCIDevice parent_obj; > > /*< public >*/ > > > > - uint8_t bus_nr; > > + bool sep_domain; /* whether it resides in separate PCI segment */ > > + uint32_t domain_nr; /* PCI domain(segment) number, should be 0 if sep_domain is false */ > > + uint8_t max_bus; /* max number of buses to use */ > > + uint8_t bus_nr; /* should be 0 when in separate domain */ > > uint16_t numa_node; > > } PXBDev; > > > > @@ -58,6 +63,18 @@ static PXBDev *convert_to_pxb(PCIDevice *dev) > > static GList *pxb_dev_list; > > > > #define TYPE_PXB_HOST "pxb-host" > > +#define TYPE_PXB_PCIE_HOST "pxb-pcie-host" > > +#define PXB_PCIE_HOST_DEVICE(obj) \ > > + OBJECT_CHECK(PXBPCIEHost, (obj), TYPE_PXB_PCIE_HOST) > > + > > +typedef struct PXBPCIEHost { > > + PCIExpressHost parent_obj; > > + > > + /* should only inherit from PXBDev */ > > + uint32_t domain_nr; > > + uint8_t bus_nr; > > + uint8_t max_bus; > > I don't think you need the above properties to be part of the > PXBPCIEHost object. > > Please see if you can't get a pointer to the PxbDev. OK, I'll try pointer then. > > +} PXBPCIEHost; > > > > static int pxb_bus_num(PCIBus *bus) > > { > > @@ -111,6 +128,31 @@ static const char *pxb_host_root_bus_path(PCIHostState *host_bridge, > > return bus->bus_path; > > } > > > > +/* Use a dedicated function for PCIe since pxb-host does > > + * not have a domain_nr field */ > > +static const char *pxb_pcie_host_root_bus_path(PCIHostState *host_bridge, > > + PCIBus *rootbus) > > +{ > > + if (!pci_bus_is_express(rootbus)) { > > + /* pxb-pcie-host cannot reside on a PCI bus */ > > + return NULL; > > + } > > + PXBBus *bus = PXB_PCIE_BUS(rootbus); > > + > > + snprintf(bus->bus_path, 8, "%04lx:%02x", > > + object_property_get_uint((Object *)host_bridge, "domain_nr", NULL), > > + pxb_bus_num(rootbus)); > > + return bus->bus_path; > > +} > > + > > +static void pxb_pcie_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + PCIExpressHost *e = PCIE_HOST_BRIDGE(obj); > > + > > + visit_type_uint64(v, name, &e->size, errp); > > +} > > + > > static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) > > { > > const PCIHostState *pxb_host; > > @@ -142,6 +184,30 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) > > return NULL; > > } > > > > +static void pxb_pcie_host_initfn(Object *obj) > > +{ > > + PCIHostState *phb = PCI_HOST_BRIDGE(obj); > > + > > + memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb, > > + "pci-conf-idx", 4); > > + memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb, > > + "pci-conf-data", 4); > > + > > + object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64", > > + pxb_pcie_host_get_mmcfg_size, > > + NULL, NULL, NULL, NULL); > > + > > +} > > + > > +static Property pxb_pcie_host_props[] = { > > + DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, PXBPCIEHost, parent_obj.base_addr, > > + PCIE_BASE_ADDR_UNMAPPED), > > + DEFINE_PROP_UINT32("domain_nr", PXBPCIEHost, domain_nr, 0), > > + DEFINE_PROP_UINT8("bus_nr", PXBPCIEHost, bus_nr, 0), > > + DEFINE_PROP_UINT8("max_bus", PXBPCIEHost, max_bus, 255), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > static void pxb_host_class_init(ObjectClass *class, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(class); > > @@ -155,12 +221,34 @@ static void pxb_host_class_init(ObjectClass *class, void *data) > > hc->root_bus_path = pxb_host_root_bus_path; > > } > > > > +static void pxb_pcie_host_class_init(ObjectClass *class, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(class); > > + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class); > > + PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class); > > + > > + dc->fw_name = "pci"; > > + dc->props = pxb_pcie_host_props; > > + /* Reason: Internal part of the pxb/pxb-pcie device, not usable by itself */ > > + dc->user_creatable = false; > > + sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address; > > + hc->root_bus_path = pxb_pcie_host_root_bus_path; > > +} > > + > > static const TypeInfo pxb_host_info = { > > .name = TYPE_PXB_HOST, > > .parent = TYPE_PCI_HOST_BRIDGE, > > .class_init = pxb_host_class_init, > > }; > > > > +static const TypeInfo pxb_pcie_host_info = { > > + .name = TYPE_PXB_PCIE_HOST, > > + .parent = TYPE_PCIE_HOST_BRIDGE, > > + .instance_size = sizeof(PXBPCIEHost), > > + .instance_init = pxb_pcie_host_initfn, > > + .class_init = pxb_pcie_host_class_init, > > +}; > > + > > /* > > * Registers the PXB bus as a child of pci host root bus. > > */ > > @@ -205,7 +293,10 @@ static gint pxb_compare(gconstpointer a, gconstpointer b) > > { > > const PXBDev *pxb_a = a, *pxb_b = b; > > > > - return pxb_a->bus_nr < pxb_b->bus_nr ? -1 : > > + /* check domain_nr, then bus_nr */ > > + return pxb_a->domain_nr < pxb_b->domain_nr ? -1 : > > + pxb_a->domain_nr > pxb_b->domain_nr ? 1 : > > + pxb_a->bus_nr < pxb_b->bus_nr ? -1 : > > pxb_a->bus_nr > pxb_b->bus_nr ? 1 : > > 0; > > } > > @@ -228,10 +319,17 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) > > dev_name = dev->qdev.id; > > } > > > > - ds = qdev_create(NULL, TYPE_PXB_HOST); > > if (pcie) { > > + /* either in sep_domain or stay in domain 0 */ > > + g_assert (pxb->sep_domain || pxb->domain_nr == 0); > > + g_assert (pxb->max_bus >= pxb->bus_nr); > > + ds = qdev_create(NULL, TYPE_PXB_PCIE_HOST); > > + qdev_prop_set_uint8(ds, "bus_nr", pxb->bus_nr); //TODO. > > + qdev_prop_set_uint8(ds, "max_bus", pxb->max_bus); > > + qdev_prop_set_uint8(ds, "domain_nr", pxb->domain_nr); > > bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS); > > } else { > > + ds = qdev_create(NULL, TYPE_PXB_HOST); > > bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0, TYPE_PXB_BUS); > > bds = qdev_create(BUS(bus), "pci-bridge"); > > bds->id = dev_name; > > @@ -294,6 +392,17 @@ static Property pxb_dev_properties[] = { > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > +static Property pxb_pcie_dev_properties[] = { > > + /* Note: 0 is not a legal PXB bus number. */ > > + DEFINE_PROP_UINT8("bus_nr", PXBDev, bus_nr, 0), > > + DEFINE_PROP_UINT16("numa_node", PXBDev, numa_node, NUMA_NODE_UNASSIGNED), > > + DEFINE_PROP_BOOL("sep_domain", PXBDev, sep_domain, false), > > + DEFINE_PROP_UINT32("domain_nr", PXBDev, domain_nr, 0), > > + DEFINE_PROP_UINT8("max_bus", PXBDev, max_bus, 255), > > + > > Duplicated properties are not an optimal solution. > I see 2 possible ones: > - Use a macro defining common properties > - Use a base class and put the properties there. Macro sounds good, I will replace those properties. > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > static void pxb_dev_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -344,7 +453,7 @@ static void pxb_pcie_dev_class_init(ObjectClass *klass, void *data) > > k->class_id = PCI_CLASS_BRIDGE_HOST; > > > > dc->desc = "PCI Express Expander Bridge"; > > - dc->props = pxb_dev_properties; > > + dc->props = pxb_pcie_dev_properties; > > dc->hotpluggable = false; > > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > > } > > @@ -365,6 +474,7 @@ static void pxb_register_types(void) > > type_register_static(&pxb_bus_info); > > type_register_static(&pxb_pcie_bus_info); > > type_register_static(&pxb_host_info); > > + type_register_static(&pxb_pcie_host_info); > > type_register_static(&pxb_dev_info); > > type_register_static(&pxb_pcie_dev_info); > > } > > Looking good! > > Thanks, > Marcel > ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <1528794804-6289-2-git-send-email-whois.zihan.yang@gmail.com>]
* Re: [Qemu-devel] [RFC v2 2/3] acpi-build: allocate mcfg for pxb-pcie host bridges [not found] ` <1528794804-6289-2-git-send-email-whois.zihan.yang@gmail.com> @ 2018-06-20 7:41 ` Marcel Apfelbaum 2018-06-21 16:49 ` Zihan Yang 0 siblings, 1 reply; 15+ messages in thread From: Marcel Apfelbaum @ 2018-06-20 7:41 UTC (permalink / raw) To: Zihan Yang, qemu-devel Cc: Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost On 06/12/2018 12:13 PM, Zihan Yang wrote: > Allocate new segment for pxb-pcie host bridges in MCFG table, and reserve > corresponding MCFG space for them. This allows user-defined pxb-pcie > host bridges to be placed in different pci domain than q35 host > > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> > --- > hw/i386/acpi-build.c | 97 +++++++++++++++++++++++------ > hw/i386/pc.c | 14 ++++- > hw/pci-bridge/pci_expander_bridge.c | 52 +++++++++++----- > hw/pci-host/q35.c | 2 + > include/hw/i386/pc.h | 1 + > include/hw/pci-bridge/pci_expander_bridge.h | 6 ++ > include/hw/pci-host/q35.h | 1 + > 7 files changed, 137 insertions(+), 36 deletions(-) > create mode 100644 include/hw/pci-bridge/pci_expander_bridge.h > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 9bc6d97..104e52d 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -89,6 +89,8 @@ > typedef struct AcpiMcfgInfo { > uint64_t mcfg_base; > uint32_t mcfg_size; > + uint32_t domain_nr; > + struct AcpiMcfgInfo *next; > } AcpiMcfgInfo; > > typedef struct AcpiPmInfo { > @@ -2427,14 +2429,16 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > { > AcpiTableMcfg *mcfg; > const char *sig; > - int len = sizeof(*mcfg) + 1 * sizeof(mcfg->allocation[0]); > + int len, count = 0; > + AcpiMcfgInfo *cfg = info; > + > + while (cfg) { > + ++count; > + cfg = cfg->next; > + } > + len = sizeof(*mcfg) + count * sizeof(mcfg->allocation[0]); > > mcfg = acpi_data_push(table_data, len); > - mcfg->allocation[0].address = cpu_to_le64(info->mcfg_base); > - /* Only a single allocation so no need to play with segments */ > - mcfg->allocation[0].pci_segment = cpu_to_le16(0); > - mcfg->allocation[0].start_bus_number = 0; > - mcfg->allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - 1); > > /* MCFG is used for ECAM which can be enabled or disabled by guest. > * To avoid table size changes (which create migration issues), > @@ -2448,6 +2452,15 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > } else { > sig = "MCFG"; > } > + > + while (info) { > + mcfg[count].allocation[0].address = cpu_to_le64(info->mcfg_base); > + mcfg[count].allocation[0].pci_segment = cpu_to_le16(info->domain_nr); > + mcfg[count].allocation[0].start_bus_number = 0; > + mcfg[count++].allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - 1); I think you want to use here max_bus property defined in the prev patch. > + info = info->next; > + } > + > build_header(linker, table_data, (void *)mcfg, sig, len, 1, NULL, NULL); > } > > @@ -2602,26 +2615,69 @@ struct AcpiBuildState { > MemoryRegion *linker_mr; > } AcpiBuildState; > > -static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg) > +static inline void cleanup_mcfg(AcpiMcfgInfo *mcfg) > +{ > + AcpiMcfgInfo *tmp; > + while (mcfg) { > + tmp = mcfg->next; > + g_free(mcfg); > + mcfg = tmp; > + } > +} > + > +static AcpiMcfgInfo *acpi_get_mcfg(void) > { > Object *pci_host; > QObject *o; > + uint32_t domain_nr; > + AcpiMcfgInfo *head = NULL, *tail, *mcfg; > > pci_host = acpi_get_i386_pci_host(); > g_assert(pci_host); > > - o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL); > - if (!o) { > - return false; > + while (pci_host) { > + o = object_property_get_qobject(pci_host, "domain_nr", NULL); > + if (!o) { If every pci_host is supposed to have a domain_nr is better to assert here. > + cleanup_mcfg(head); > + return NULL; > + } > + domain_nr = qnum_get_uint(qobject_to(QNum, o)); > + qobject_unref(o); > + /* skip q35 host and bridges that reside in the same domain with it */ > + if (domain_nr == 0) { > + pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > + continue; The default q35 pci host will not have a MMCFG? > + } > + > + mcfg = g_new0(AcpiMcfgInfo, 1); > + mcfg->next = NULL; > + if (!head) { > + tail = head = mcfg; > + } else { > + tail->next = mcfg; > + tail = mcfg; > + } > + mcfg->domain_nr = domain_nr; > + > + o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL); > + assert(o); > + mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o)); > + qobject_unref(o); > + > + /* firmware will overwrite it */ > + o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL); > + assert(o); > + mcfg->mcfg_size = qnum_get_uint(qobject_to(QNum, o)); > + qobject_unref(o); > + > + o = object_property_get_qobject(pci_host, "domain_nr", NULL); > + assert(o); > + mcfg->domain_nr = qnum_get_uint(qobject_to(QNum, o)); You already have the domain_nr. > + > + pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > } > - mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o)); > - qobject_unref(o); > > - o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL); > - assert(o); > - mcfg->mcfg_size = qnum_get_uint(qobject_to(QNum, o)); > - qobject_unref(o); > - return true; > + return head; > } > > static > @@ -2633,7 +2689,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) > unsigned facs, dsdt, rsdt, fadt; > AcpiPmInfo pm; > AcpiMiscInfo misc; > - AcpiMcfgInfo mcfg; > + AcpiMcfgInfo *mcfg; > Range pci_hole, pci_hole64; > uint8_t *u; > size_t aml_len = 0; > @@ -2714,10 +2770,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) > build_slit(tables_blob, tables->linker); > } > } > - if (acpi_get_mcfg(&mcfg)) { > + if ((mcfg = acpi_get_mcfg()) != NULL) { > acpi_add_table(table_offsets, tables_blob); > - build_mcfg_q35(tables_blob, tables->linker, &mcfg); > + build_mcfg_q35(tables_blob, tables->linker, mcfg); > } > + cleanup_mcfg(mcfg); > if (x86_iommu_get_default()) { > IommuType IOMMUType = x86_iommu_get_type(); > if (IOMMUType == TYPE_AMD) { > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index f3befe6..95f6c34 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -34,6 +34,7 @@ > #include "hw/ide.h" > #include "hw/pci/pci.h" > #include "hw/pci/pci_bus.h" > +#include "hw/pci-bridge/pci_expander_bridge.h" > #include "hw/nvram/fw_cfg.h" > #include "hw/timer/hpet.h" > #include "hw/smbios/smbios.h" > @@ -1469,15 +1470,24 @@ uint64_t pc_pci_hole64_start(void) > if (pcmc->has_reserved_memory && ms->device_memory->base) { > hole64_start = ms->device_memory->base; > if (!pcmc->broken_reserved_end) { > - hole64_start += memory_region_size(&ms->device_memory->mr); > + hole64_start += (memory_region_size(&ms->device_memory->mr) + \ > + pxb_pcie_mcfg_hole()); > } > } else { > - hole64_start = 0x100000000ULL + pcms->above_4g_mem_size; > + /* memory layout [RAM Hotplug][MCFG][..ROUND UP..][PCI HOLE] */ > + hole64_start = pc_pci_mcfg_start() + pxb_pcie_mcfg_hole(); > } > > return ROUND_UP(hole64_start, 1ULL << 30); > } > > +uint64_t pc_pci_mcfg_start(void) > +{ > + PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); > + > + return ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 12); > +} > + > qemu_irq pc_allocate_cpu_irq(void) > { > return qemu_allocate_irq(pic_irq_request, NULL, 0); > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c > index 448b9fb..14f0447 100644 > --- a/hw/pci-bridge/pci_expander_bridge.c > +++ b/hw/pci-bridge/pci_expander_bridge.c > @@ -12,11 +12,14 @@ > > #include "qemu/osdep.h" > #include "qapi/error.h" > +#include "hw/i386/pc.h" > #include "hw/pci/pci.h" > #include "hw/pci/pci_bus.h" > #include "hw/pci/pci_host.h" > #include "hw/pci/pcie_host.h" > #include "hw/pci/pci_bridge.h" > +#include "hw/pci-host/q35.h" > +#include "hw/pci-bridge/pci_expander_bridge.h" > #include "qemu/range.h" > #include "qemu/error-report.h" > #include "sysemu/numa.h" > @@ -118,6 +121,25 @@ static const TypeInfo pxb_pcie_bus_info = { > .class_init = pxb_bus_class_init, > }; > > +static uint64_t pxb_mcfg_hole_size = 0; > + No need to initialize a static to 0. > +static void pxb_pcie_foreach(gpointer data, gpointer user_data) > +{ > + PXBDev *pxb = (PXBDev *)data; > + > + if (pxb->sep_domain && pxb->domain_nr > 0) { > + // only reserve what users ask for to reduce memory cost > + pxb_mcfg_hole_size += ((pxb->max_bus + 1ULL) << 20); Why "+ 1" ? Please replace 20 with a MACRO explaining it. > + } > +} > + > +uint64_t pxb_pcie_mcfg_hole(void) > +{ > + /* foreach is necessary as some pxb still reside in domain 0 */ > + g_list_foreach(pxb_dev_list, pxb_pcie_foreach, NULL); > + return pxb_mcfg_hole_size; > +} > + > static const char *pxb_host_root_bus_path(PCIHostState *host_bridge, > PCIBus *rootbus) > { > @@ -145,14 +167,6 @@ static const char *pxb_pcie_host_root_bus_path(PCIHostState *host_bridge, > return bus->bus_path; > } > > -static void pxb_pcie_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name, > - void *opaque, Error **errp) > -{ > - PCIExpressHost *e = PCIE_HOST_BRIDGE(obj); > - > - visit_type_uint64(v, name, &e->size, errp); > -} > - > static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) > { > const PCIHostState *pxb_host; > @@ -192,16 +206,12 @@ static void pxb_pcie_host_initfn(Object *obj) > "pci-conf-idx", 4); > memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb, > "pci-conf-data", 4); > - > - object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64", > - pxb_pcie_host_get_mmcfg_size, > - NULL, NULL, NULL, NULL); > - > } > > static Property pxb_pcie_host_props[] = { > DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, PXBPCIEHost, parent_obj.base_addr, > - PCIE_BASE_ADDR_UNMAPPED), > + PCIE_BASE_ADDR_UNMAPPED), > + DEFINE_PROP_UINT64(PCIE_HOST_MCFG_SIZE, PXBPCIEHost, parent_obj.size, 0), > DEFINE_PROP_UINT32("domain_nr", PXBPCIEHost, domain_nr, 0), > DEFINE_PROP_UINT8("bus_nr", PXBPCIEHost, bus_nr, 0), > DEFINE_PROP_UINT8("max_bus", PXBPCIEHost, max_bus, 255), > @@ -301,6 +311,8 @@ static gint pxb_compare(gconstpointer a, gconstpointer b) > 0; > } > > +static uint64_t pxb_pcie_mcfg_base = 0; > + > static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) > { > PXBDev *pxb = convert_to_pxb(dev); > @@ -327,6 +339,15 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) > qdev_prop_set_uint8(ds, "bus_nr", pxb->bus_nr); //TODO. > qdev_prop_set_uint8(ds, "max_bus", pxb->max_bus); > qdev_prop_set_uint8(ds, "domain_nr", pxb->domain_nr); > + > + /* will be overwritten by firmware, but kept for readability */ > + qdev_prop_set_uint64(ds, PCIE_HOST_MCFG_BASE, > + pxb->domain_nr ? pxb_pcie_mcfg_base : MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT); > + qdev_prop_set_uint64(ds, PCIE_HOST_MCFG_SIZE, > + pxb->domain_nr ? (pxb->max_bus + 1ULL) << 20 : 0); > + if (pxb->domain_nr) > + pxb_pcie_mcfg_base += ((pxb->max_bus + 1ULL) << 20); > + > bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS); > } else { > ds = qdev_create(NULL, TYPE_PXB_HOST); > @@ -438,6 +459,9 @@ static void pxb_pcie_dev_realize(PCIDevice *dev, Error **errp) > return; > } > > + if (0 == pxb_pcie_mcfg_base) > + pxb_pcie_mcfg_base = pc_pci_mcfg_start(); > + > pxb_dev_realize_common(dev, true, errp); > } > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index 02f9576..a76029d 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -178,6 +178,8 @@ static Property q35_host_props[] = { > DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost, > mch.above_4g_mem_size, 0), > DEFINE_PROP_BOOL("x-pci-hole64-fix", Q35PCIHost, pci_hole64_fix, true), > + /* q35 host bridge should always stay in pci domain 0 */ > + DEFINE_PROP_UINT32("domain_nr", Q35PCIHost, domain_nr, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 04d1f8c..81ba010 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -212,6 +212,7 @@ void pc_memory_init(PCMachineState *pcms, > MemoryRegion *rom_memory, > MemoryRegion **ram_memory); > uint64_t pc_pci_hole64_start(void); > +uint64_t pc_pci_mcfg_start(void); > qemu_irq pc_allocate_cpu_irq(void); > DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus); > void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, > diff --git a/include/hw/pci-bridge/pci_expander_bridge.h b/include/hw/pci-bridge/pci_expander_bridge.h > new file mode 100644 > index 0000000..bb1462c > --- /dev/null > +++ b/include/hw/pci-bridge/pci_expander_bridge.h > @@ -0,0 +1,6 @@ > +#ifndef HW_PCI_EXPANDER_H > +#define HW_PCI_EXPANDER_H > + > +uint64_t pxb_pcie_mcfg_hole(void); > + > +#endif > diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h > index 8f4ddde..432e569 100644 > --- a/include/hw/pci-host/q35.h > +++ b/include/hw/pci-host/q35.h > @@ -69,6 +69,7 @@ typedef struct Q35PCIHost { > /*< public >*/ > > bool pci_hole64_fix; > + uint32_t domain_nr; > MCHPCIState mch; > } Q35PCIHost; > Thanks, Marcel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 2/3] acpi-build: allocate mcfg for pxb-pcie host bridges 2018-06-20 7:41 ` [Qemu-devel] [RFC v2 2/3] acpi-build: allocate mcfg for pxb-pcie host bridges Marcel Apfelbaum @ 2018-06-21 16:49 ` Zihan Yang 2018-06-22 16:37 ` Marcel Apfelbaum 0 siblings, 1 reply; 15+ messages in thread From: Zihan Yang @ 2018-06-21 16:49 UTC (permalink / raw) To: Marcel Apfelbaum Cc: qemu-devel, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost Thanks for your review. Marcel Apfelbaum <marcel.apfelbaum@gmail.com> 于2018年6月20日周三 下午3:41写道: > > > > On 06/12/2018 12:13 PM, Zihan Yang wrote: > > Allocate new segment for pxb-pcie host bridges in MCFG table, and reserve > > corresponding MCFG space for them. This allows user-defined pxb-pcie > > host bridges to be placed in different pci domain than q35 host > > > > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> > > --- > > hw/i386/acpi-build.c | 97 +++++++++++++++++++++++------ > > hw/i386/pc.c | 14 ++++- > > hw/pci-bridge/pci_expander_bridge.c | 52 +++++++++++----- > > hw/pci-host/q35.c | 2 + > > include/hw/i386/pc.h | 1 + > > include/hw/pci-bridge/pci_expander_bridge.h | 6 ++ > > include/hw/pci-host/q35.h | 1 + > > 7 files changed, 137 insertions(+), 36 deletions(-) > > create mode 100644 include/hw/pci-bridge/pci_expander_bridge.h > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 9bc6d97..104e52d 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -89,6 +89,8 @@ > > typedef struct AcpiMcfgInfo { > > uint64_t mcfg_base; > > uint32_t mcfg_size; > > + uint32_t domain_nr; > > + struct AcpiMcfgInfo *next; > > } AcpiMcfgInfo; > > > > typedef struct AcpiPmInfo { > > @@ -2427,14 +2429,16 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > > { > > AcpiTableMcfg *mcfg; > > const char *sig; > > - int len = sizeof(*mcfg) + 1 * sizeof(mcfg->allocation[0]); > > + int len, count = 0; > > + AcpiMcfgInfo *cfg = info; > > + > > + while (cfg) { > > + ++count; > > + cfg = cfg->next; > > + } > > + len = sizeof(*mcfg) + count * sizeof(mcfg->allocation[0]); > > > > mcfg = acpi_data_push(table_data, len); > > - mcfg->allocation[0].address = cpu_to_le64(info->mcfg_base); > > - /* Only a single allocation so no need to play with segments */ > > - mcfg->allocation[0].pci_segment = cpu_to_le16(0); > > - mcfg->allocation[0].start_bus_number = 0; > > - mcfg->allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - 1); > > > > /* MCFG is used for ECAM which can be enabled or disabled by guest. > > * To avoid table size changes (which create migration issues), > > @@ -2448,6 +2452,15 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > > } else { > > sig = "MCFG"; > > } > > + > > + while (info) { > > + mcfg[count].allocation[0].address = cpu_to_le64(info->mcfg_base); > > + mcfg[count].allocation[0].pci_segment = cpu_to_le16(info->domain_nr); > > + mcfg[count].allocation[0].start_bus_number = 0; > > + mcfg[count++].allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - 1); > > I think you want to use here max_bus property defined in the prev patch. mcfg_size is calculated by the max_bus, but it does seems more readable to use max_bus here. > > + info = info->next; > > + } > > + > > build_header(linker, table_data, (void *)mcfg, sig, len, 1, NULL, NULL); > > } > > > > @@ -2602,26 +2615,69 @@ struct AcpiBuildState { > > MemoryRegion *linker_mr; > > } AcpiBuildState; > > > > -static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg) > > +static inline void cleanup_mcfg(AcpiMcfgInfo *mcfg) > > +{ > > + AcpiMcfgInfo *tmp; > > + while (mcfg) { > > + tmp = mcfg->next; > > + g_free(mcfg); > > + mcfg = tmp; > > + } > > +} > > + > > +static AcpiMcfgInfo *acpi_get_mcfg(void) > > { > > Object *pci_host; > > QObject *o; > > + uint32_t domain_nr; > > + AcpiMcfgInfo *head = NULL, *tail, *mcfg; > > > > pci_host = acpi_get_i386_pci_host(); > > g_assert(pci_host); > > > > - o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL); > > - if (!o) { > > - return false; > > + while (pci_host) { > > + o = object_property_get_qobject(pci_host, "domain_nr", NULL); > > + if (!o) { > > If every pci_host is supposed to have a domain_nr is better to assert here. OK, I get it. > > + cleanup_mcfg(head); > > + return NULL; > > + } > > + domain_nr = qnum_get_uint(qobject_to(QNum, o)); > > + qobject_unref(o); > > + /* skip q35 host and bridges that reside in the same domain with it */ > > + if (domain_nr == 0) { > > + pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > > + continue; > > The default q35 pci host will not have a MMCFG? I must have got my brain messed up.. The original purpose was to let q35host reside in pci domain 0, and all the expander host bridges that have non-zero domain_nr reside in their corresponding domains. Expander bridges whose domain_nr equals to 0 are skipped because they don't actually specify a domain. I thought I added q35 host when I wrote the code at the beginning, there might be some mistakes when I fotmat the code later on. I will correct them in next version. > > + } > > + > > + mcfg = g_new0(AcpiMcfgInfo, 1); > > + mcfg->next = NULL; > > + if (!head) { > > + tail = head = mcfg; > > + } else { > > + tail->next = mcfg; > > + tail = mcfg; > > + } > > + mcfg->domain_nr = domain_nr; > > + > > + o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL); > > + assert(o); > > + mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o)); > > + qobject_unref(o); > > + > > + /* firmware will overwrite it */ > > + o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL); > > + assert(o); > > + mcfg->mcfg_size = qnum_get_uint(qobject_to(QNum, o)); > > + qobject_unref(o); > > + > > + o = object_property_get_qobject(pci_host, "domain_nr", NULL); > > + assert(o); > > + mcfg->domain_nr = qnum_get_uint(qobject_to(QNum, o)); > > You already have the domain_nr. That seems a mistake, I will recode this piece of code in next version. > > + > > + pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); > > } > > - mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o)); > > - qobject_unref(o); > > > > - o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL); > > - assert(o); > > - mcfg->mcfg_size = qnum_get_uint(qobject_to(QNum, o)); > > - qobject_unref(o); > > - return true; > > + return head; > > } > > > > static > > @@ -2633,7 +2689,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) > > unsigned facs, dsdt, rsdt, fadt; > > AcpiPmInfo pm; > > AcpiMiscInfo misc; > > - AcpiMcfgInfo mcfg; > > + AcpiMcfgInfo *mcfg; > > Range pci_hole, pci_hole64; > > uint8_t *u; > > size_t aml_len = 0; > > @@ -2714,10 +2770,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) > > build_slit(tables_blob, tables->linker); > > } > > } > > - if (acpi_get_mcfg(&mcfg)) { > > + if ((mcfg = acpi_get_mcfg()) != NULL) { > > acpi_add_table(table_offsets, tables_blob); > > - build_mcfg_q35(tables_blob, tables->linker, &mcfg); > > + build_mcfg_q35(tables_blob, tables->linker, mcfg); > > } > > + cleanup_mcfg(mcfg); > > if (x86_iommu_get_default()) { > > IommuType IOMMUType = x86_iommu_get_type(); > > if (IOMMUType == TYPE_AMD) { > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index f3befe6..95f6c34 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -34,6 +34,7 @@ > > #include "hw/ide.h" > > #include "hw/pci/pci.h" > > #include "hw/pci/pci_bus.h" > > +#include "hw/pci-bridge/pci_expander_bridge.h" > > #include "hw/nvram/fw_cfg.h" > > #include "hw/timer/hpet.h" > > #include "hw/smbios/smbios.h" > > @@ -1469,15 +1470,24 @@ uint64_t pc_pci_hole64_start(void) > > if (pcmc->has_reserved_memory && ms->device_memory->base) { > > hole64_start = ms->device_memory->base; > > if (!pcmc->broken_reserved_end) { > > - hole64_start += memory_region_size(&ms->device_memory->mr); > > + hole64_start += (memory_region_size(&ms->device_memory->mr) + \ > > + pxb_pcie_mcfg_hole()); > > } > > } else { > > - hole64_start = 0x100000000ULL + pcms->above_4g_mem_size; > > + /* memory layout [RAM Hotplug][MCFG][..ROUND UP..][PCI HOLE] */ > > + hole64_start = pc_pci_mcfg_start() + pxb_pcie_mcfg_hole(); > > } > > > > return ROUND_UP(hole64_start, 1ULL << 30); > > } > > > > +uint64_t pc_pci_mcfg_start(void) > > +{ > > + PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); > > + > > + return ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 12); > > +} > > + > > qemu_irq pc_allocate_cpu_irq(void) > > { > > return qemu_allocate_irq(pic_irq_request, NULL, 0); > > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c > > index 448b9fb..14f0447 100644 > > --- a/hw/pci-bridge/pci_expander_bridge.c > > +++ b/hw/pci-bridge/pci_expander_bridge.c > > @@ -12,11 +12,14 @@ > > > > #include "qemu/osdep.h" > > #include "qapi/error.h" > > +#include "hw/i386/pc.h" > > #include "hw/pci/pci.h" > > #include "hw/pci/pci_bus.h" > > #include "hw/pci/pci_host.h" > > #include "hw/pci/pcie_host.h" > > #include "hw/pci/pci_bridge.h" > > +#include "hw/pci-host/q35.h" > > +#include "hw/pci-bridge/pci_expander_bridge.h" > > #include "qemu/range.h" > > #include "qemu/error-report.h" > > #include "sysemu/numa.h" > > @@ -118,6 +121,25 @@ static const TypeInfo pxb_pcie_bus_info = { > > .class_init = pxb_bus_class_init, > > }; > > > > +static uint64_t pxb_mcfg_hole_size = 0; > > + > > No need to initialize a static to 0. > > > +static void pxb_pcie_foreach(gpointer data, gpointer user_data) > > +{ > > + PXBDev *pxb = (PXBDev *)data; > > + > > + if (pxb->sep_domain && pxb->domain_nr > 0) { > > + // only reserve what users ask for to reduce memory cost > > + pxb_mcfg_hole_size += ((pxb->max_bus + 1ULL) << 20); > > Why "+ 1" ? > Please replace 20 with a MACRO explaining it. Sorry for the confusion. This is because max_bus specify the maximum bus number that will be used. For example, if max_bus is 255(0xff), then the actual bus numbers are (255 + 1) = 256. But since max_bus is uint8_t, I can't initialize it to 256, therefore it should +1 when calculating the bus numbers in use. left shift 20 bits means 1MB, I will add a macro for it. > > + } > > +} > > + > > +uint64_t pxb_pcie_mcfg_hole(void) > > +{ > > + /* foreach is necessary as some pxb still reside in domain 0 */ > > + g_list_foreach(pxb_dev_list, pxb_pcie_foreach, NULL); > > + return pxb_mcfg_hole_size; > > +} > > + > > static const char *pxb_host_root_bus_path(PCIHostState *host_bridge, > > PCIBus *rootbus) > > { > > @@ -145,14 +167,6 @@ static const char *pxb_pcie_host_root_bus_path(PCIHostState *host_bridge, > > return bus->bus_path; > > } > > > > -static void pxb_pcie_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name, > > - void *opaque, Error **errp) > > -{ > > - PCIExpressHost *e = PCIE_HOST_BRIDGE(obj); > > - > > - visit_type_uint64(v, name, &e->size, errp); > > -} > > - > > static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) > > { > > const PCIHostState *pxb_host; > > @@ -192,16 +206,12 @@ static void pxb_pcie_host_initfn(Object *obj) > > "pci-conf-idx", 4); > > memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb, > > "pci-conf-data", 4); > > - > > - object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64", > > - pxb_pcie_host_get_mmcfg_size, > > - NULL, NULL, NULL, NULL); > > - > > } > > > > static Property pxb_pcie_host_props[] = { > > DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, PXBPCIEHost, parent_obj.base_addr, > > - PCIE_BASE_ADDR_UNMAPPED), > > + PCIE_BASE_ADDR_UNMAPPED), > > + DEFINE_PROP_UINT64(PCIE_HOST_MCFG_SIZE, PXBPCIEHost, parent_obj.size, 0), > > DEFINE_PROP_UINT32("domain_nr", PXBPCIEHost, domain_nr, 0), > > DEFINE_PROP_UINT8("bus_nr", PXBPCIEHost, bus_nr, 0), > > DEFINE_PROP_UINT8("max_bus", PXBPCIEHost, max_bus, 255), > > @@ -301,6 +311,8 @@ static gint pxb_compare(gconstpointer a, gconstpointer b) > > 0; > > } > > > > +static uint64_t pxb_pcie_mcfg_base = 0; > > + > > static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) > > { > > PXBDev *pxb = convert_to_pxb(dev); > > @@ -327,6 +339,15 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) > > qdev_prop_set_uint8(ds, "bus_nr", pxb->bus_nr); //TODO. > > qdev_prop_set_uint8(ds, "max_bus", pxb->max_bus); > > qdev_prop_set_uint8(ds, "domain_nr", pxb->domain_nr); > > + > > + /* will be overwritten by firmware, but kept for readability */ > > + qdev_prop_set_uint64(ds, PCIE_HOST_MCFG_BASE, > > + pxb->domain_nr ? pxb_pcie_mcfg_base : MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT); > > + qdev_prop_set_uint64(ds, PCIE_HOST_MCFG_SIZE, > > + pxb->domain_nr ? (pxb->max_bus + 1ULL) << 20 : 0); > > + if (pxb->domain_nr) > > + pxb_pcie_mcfg_base += ((pxb->max_bus + 1ULL) << 20); > > + > > bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS); > > } else { > > ds = qdev_create(NULL, TYPE_PXB_HOST); > > @@ -438,6 +459,9 @@ static void pxb_pcie_dev_realize(PCIDevice *dev, Error **errp) > > return; > > } > > > > + if (0 == pxb_pcie_mcfg_base) > > + pxb_pcie_mcfg_base = pc_pci_mcfg_start(); > > + > > pxb_dev_realize_common(dev, true, errp); > > } > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > index 02f9576..a76029d 100644 > > --- a/hw/pci-host/q35.c > > +++ b/hw/pci-host/q35.c > > @@ -178,6 +178,8 @@ static Property q35_host_props[] = { > > DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost, > > mch.above_4g_mem_size, 0), > > DEFINE_PROP_BOOL("x-pci-hole64-fix", Q35PCIHost, pci_hole64_fix, true), > > + /* q35 host bridge should always stay in pci domain 0 */ > > + DEFINE_PROP_UINT32("domain_nr", Q35PCIHost, domain_nr, 0), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > > index 04d1f8c..81ba010 100644 > > --- a/include/hw/i386/pc.h > > +++ b/include/hw/i386/pc.h > > @@ -212,6 +212,7 @@ void pc_memory_init(PCMachineState *pcms, > > MemoryRegion *rom_memory, > > MemoryRegion **ram_memory); > > uint64_t pc_pci_hole64_start(void); > > +uint64_t pc_pci_mcfg_start(void); > > qemu_irq pc_allocate_cpu_irq(void); > > DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus); > > void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, > > diff --git a/include/hw/pci-bridge/pci_expander_bridge.h b/include/hw/pci-bridge/pci_expander_bridge.h > > new file mode 100644 > > index 0000000..bb1462c > > --- /dev/null > > +++ b/include/hw/pci-bridge/pci_expander_bridge.h > > @@ -0,0 +1,6 @@ > > +#ifndef HW_PCI_EXPANDER_H > > +#define HW_PCI_EXPANDER_H > > + > > +uint64_t pxb_pcie_mcfg_hole(void); > > + > > +#endif > > diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h > > index 8f4ddde..432e569 100644 > > --- a/include/hw/pci-host/q35.h > > +++ b/include/hw/pci-host/q35.h > > @@ -69,6 +69,7 @@ typedef struct Q35PCIHost { > > /*< public >*/ > > > > bool pci_hole64_fix; > > + uint32_t domain_nr; > > MCHPCIState mch; > > } Q35PCIHost; > > > > Thanks, > Marcel > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC v2 2/3] acpi-build: allocate mcfg for pxb-pcie host bridges 2018-06-21 16:49 ` Zihan Yang @ 2018-06-22 16:37 ` Marcel Apfelbaum 0 siblings, 0 replies; 15+ messages in thread From: Marcel Apfelbaum @ 2018-06-22 16:37 UTC (permalink / raw) To: Zihan Yang Cc: qemu-devel, Michael S. Tsirkin, Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost On 06/21/2018 07:49 PM, Zihan Yang wrote: > Thanks for your review. > > Marcel Apfelbaum <marcel.apfelbaum@gmail.com> 于2018年6月20日周三 下午3:41写道: >> >> >> On 06/12/2018 12:13 PM, Zihan Yang wrote: >>> Allocate new segment for pxb-pcie host bridges in MCFG table, and reserve >>> corresponding MCFG space for them. This allows user-defined pxb-pcie >>> host bridges to be placed in different pci domain than q35 host >>> >>> Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com> >>> --- >>> hw/i386/acpi-build.c | 97 +++++++++++++++++++++++------ >>> hw/i386/pc.c | 14 ++++- >>> hw/pci-bridge/pci_expander_bridge.c | 52 +++++++++++----- >>> hw/pci-host/q35.c | 2 + >>> include/hw/i386/pc.h | 1 + >>> include/hw/pci-bridge/pci_expander_bridge.h | 6 ++ >>> include/hw/pci-host/q35.h | 1 + >>> 7 files changed, 137 insertions(+), 36 deletions(-) >>> create mode 100644 include/hw/pci-bridge/pci_expander_bridge.h >>> >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >>> index 9bc6d97..104e52d 100644 >>> --- a/hw/i386/acpi-build.c >>> +++ b/hw/i386/acpi-build.c >>> @@ -89,6 +89,8 @@ >>> typedef struct AcpiMcfgInfo { >>> uint64_t mcfg_base; >>> uint32_t mcfg_size; >>> + uint32_t domain_nr; >>> + struct AcpiMcfgInfo *next; >>> } AcpiMcfgInfo; >>> >>> typedef struct AcpiPmInfo { >>> @@ -2427,14 +2429,16 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) >>> { >>> AcpiTableMcfg *mcfg; >>> const char *sig; >>> - int len = sizeof(*mcfg) + 1 * sizeof(mcfg->allocation[0]); >>> + int len, count = 0; >>> + AcpiMcfgInfo *cfg = info; >>> + >>> + while (cfg) { >>> + ++count; >>> + cfg = cfg->next; >>> + } >>> + len = sizeof(*mcfg) + count * sizeof(mcfg->allocation[0]); >>> >>> mcfg = acpi_data_push(table_data, len); >>> - mcfg->allocation[0].address = cpu_to_le64(info->mcfg_base); >>> - /* Only a single allocation so no need to play with segments */ >>> - mcfg->allocation[0].pci_segment = cpu_to_le16(0); >>> - mcfg->allocation[0].start_bus_number = 0; >>> - mcfg->allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - 1); >>> >>> /* MCFG is used for ECAM which can be enabled or disabled by guest. >>> * To avoid table size changes (which create migration issues), >>> @@ -2448,6 +2452,15 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) >>> } else { >>> sig = "MCFG"; >>> } >>> + >>> + while (info) { >>> + mcfg[count].allocation[0].address = cpu_to_le64(info->mcfg_base); >>> + mcfg[count].allocation[0].pci_segment = cpu_to_le16(info->domain_nr); >>> + mcfg[count].allocation[0].start_bus_number = 0; >>> + mcfg[count++].allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - 1); >> I think you want to use here max_bus property defined in the prev patch. > mcfg_size is calculated by the max_bus, but it does seems more readable > to use max_bus here. At the end is up to you :) I just thought is more readable. >>> + info = info->next; >>> + } >>> + >>> build_header(linker, table_data, (void *)mcfg, sig, len, 1, NULL, NULL); >>> } >>> >>> @@ -2602,26 +2615,69 @@ struct AcpiBuildState { >>> MemoryRegion *linker_mr; >>> } AcpiBuildState; >>> >>> -static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg) >>> +static inline void cleanup_mcfg(AcpiMcfgInfo *mcfg) >>> +{ >>> + AcpiMcfgInfo *tmp; >>> + while (mcfg) { >>> + tmp = mcfg->next; >>> + g_free(mcfg); >>> + mcfg = tmp; >>> + } >>> +} >>> + >>> +static AcpiMcfgInfo *acpi_get_mcfg(void) >>> { >>> Object *pci_host; >>> QObject *o; >>> + uint32_t domain_nr; >>> + AcpiMcfgInfo *head = NULL, *tail, *mcfg; >>> >>> pci_host = acpi_get_i386_pci_host(); >>> g_assert(pci_host); >>> >>> - o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL); >>> - if (!o) { >>> - return false; >>> + while (pci_host) { >>> + o = object_property_get_qobject(pci_host, "domain_nr", NULL); >>> + if (!o) { >> If every pci_host is supposed to have a domain_nr is better to assert here. > OK, I get it. > >>> + cleanup_mcfg(head); >>> + return NULL; >>> + } >>> + domain_nr = qnum_get_uint(qobject_to(QNum, o)); >>> + qobject_unref(o); >>> + /* skip q35 host and bridges that reside in the same domain with it */ >>> + if (domain_nr == 0) { >>> + pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); >>> + continue; >> The default q35 pci host will not have a MMCFG? > I must have got my brain messed up.. The original purpose was to let q35host > reside in pci domain 0, and all the expander host bridges that have non-zero > domain_nr reside in their corresponding domains. Correct > Expander bridges whose > domain_nr equals to 0 are skipped because they don't actually specify a domain. Correct again > > I thought I added q35 host when I wrote the code at the beginning, there might > be some mistakes when I fotmat the code later on. I will correct them in next > version. Please double check we keep Q35 main host bridge MMCFG :) >>> + } >>> + >>> + mcfg = g_new0(AcpiMcfgInfo, 1); >>> + mcfg->next = NULL; >>> + if (!head) { >>> + tail = head = mcfg; >>> + } else { >>> + tail->next = mcfg; >>> + tail = mcfg; >>> + } >>> + mcfg->domain_nr = domain_nr; >>> + >>> + o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL); >>> + assert(o); >>> + mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o)); >>> + qobject_unref(o); >>> + >>> + /* firmware will overwrite it */ >>> + o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL); >>> + assert(o); >>> + mcfg->mcfg_size = qnum_get_uint(qobject_to(QNum, o)); >>> + qobject_unref(o); >>> + >>> + o = object_property_get_qobject(pci_host, "domain_nr", NULL); >>> + assert(o); >>> + mcfg->domain_nr = qnum_get_uint(qobject_to(QNum, o)); >> You already have the domain_nr. > That seems a mistake, I will recode this piece of code in next version. > >>> + >>> + pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next)); >>> } >>> - mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o)); >>> - qobject_unref(o); >>> >>> - o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL); >>> - assert(o); >>> - mcfg->mcfg_size = qnum_get_uint(qobject_to(QNum, o)); >>> - qobject_unref(o); >>> - return true; >>> + return head; >>> } >>> >>> static >>> @@ -2633,7 +2689,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) >>> unsigned facs, dsdt, rsdt, fadt; >>> AcpiPmInfo pm; >>> AcpiMiscInfo misc; >>> - AcpiMcfgInfo mcfg; >>> + AcpiMcfgInfo *mcfg; >>> Range pci_hole, pci_hole64; >>> uint8_t *u; >>> size_t aml_len = 0; >>> @@ -2714,10 +2770,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) >>> build_slit(tables_blob, tables->linker); >>> } >>> } >>> - if (acpi_get_mcfg(&mcfg)) { >>> + if ((mcfg = acpi_get_mcfg()) != NULL) { >>> acpi_add_table(table_offsets, tables_blob); >>> - build_mcfg_q35(tables_blob, tables->linker, &mcfg); >>> + build_mcfg_q35(tables_blob, tables->linker, mcfg); >>> } >>> + cleanup_mcfg(mcfg); >>> if (x86_iommu_get_default()) { >>> IommuType IOMMUType = x86_iommu_get_type(); >>> if (IOMMUType == TYPE_AMD) { >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>> index f3befe6..95f6c34 100644 >>> --- a/hw/i386/pc.c >>> +++ b/hw/i386/pc.c >>> @@ -34,6 +34,7 @@ >>> #include "hw/ide.h" >>> #include "hw/pci/pci.h" >>> #include "hw/pci/pci_bus.h" >>> +#include "hw/pci-bridge/pci_expander_bridge.h" >>> #include "hw/nvram/fw_cfg.h" >>> #include "hw/timer/hpet.h" >>> #include "hw/smbios/smbios.h" >>> @@ -1469,15 +1470,24 @@ uint64_t pc_pci_hole64_start(void) >>> if (pcmc->has_reserved_memory && ms->device_memory->base) { >>> hole64_start = ms->device_memory->base; >>> if (!pcmc->broken_reserved_end) { >>> - hole64_start += memory_region_size(&ms->device_memory->mr); >>> + hole64_start += (memory_region_size(&ms->device_memory->mr) + \ >>> + pxb_pcie_mcfg_hole()); >>> } >>> } else { >>> - hole64_start = 0x100000000ULL + pcms->above_4g_mem_size; >>> + /* memory layout [RAM Hotplug][MCFG][..ROUND UP..][PCI HOLE] */ >>> + hole64_start = pc_pci_mcfg_start() + pxb_pcie_mcfg_hole(); >>> } >>> >>> return ROUND_UP(hole64_start, 1ULL << 30); >>> } >>> >>> +uint64_t pc_pci_mcfg_start(void) >>> +{ >>> + PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); >>> + >>> + return ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 12); >>> +} >>> + >>> qemu_irq pc_allocate_cpu_irq(void) >>> { >>> return qemu_allocate_irq(pic_irq_request, NULL, 0); >>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c >>> index 448b9fb..14f0447 100644 >>> --- a/hw/pci-bridge/pci_expander_bridge.c >>> +++ b/hw/pci-bridge/pci_expander_bridge.c >>> @@ -12,11 +12,14 @@ >>> >>> #include "qemu/osdep.h" >>> #include "qapi/error.h" >>> +#include "hw/i386/pc.h" >>> #include "hw/pci/pci.h" >>> #include "hw/pci/pci_bus.h" >>> #include "hw/pci/pci_host.h" >>> #include "hw/pci/pcie_host.h" >>> #include "hw/pci/pci_bridge.h" >>> +#include "hw/pci-host/q35.h" >>> +#include "hw/pci-bridge/pci_expander_bridge.h" >>> #include "qemu/range.h" >>> #include "qemu/error-report.h" >>> #include "sysemu/numa.h" >>> @@ -118,6 +121,25 @@ static const TypeInfo pxb_pcie_bus_info = { >>> .class_init = pxb_bus_class_init, >>> }; >>> >>> +static uint64_t pxb_mcfg_hole_size = 0; >>> + >> No need to initialize a static to 0. >> >>> +static void pxb_pcie_foreach(gpointer data, gpointer user_data) >>> +{ >>> + PXBDev *pxb = (PXBDev *)data; >>> + >>> + if (pxb->sep_domain && pxb->domain_nr > 0) { >>> + // only reserve what users ask for to reduce memory cost >>> + pxb_mcfg_hole_size += ((pxb->max_bus + 1ULL) << 20); >> Why "+ 1" ? >> Please replace 20 with a MACRO explaining it. > Sorry for the confusion. This is because max_bus specify the maximum > bus number that will be used. For example, if max_bus is 255(0xff), > then the actual bus numbers are (255 + 1) = 256. But since max_bus > is uint8_t, I can't initialize it to 256, therefore it should +1 when > calculating the bus numbers in use. OK . In order to be more clear maybe you can use (max_bus - start_bus + 1) > > left shift 20 bits means 1MB, I will add a macro for it. > Thanks, Marcel >>> + } >>> +} >>> + >>> +uint64_t pxb_pcie_mcfg_hole(void) >>> +{ >>> + /* foreach is necessary as some pxb still reside in domain 0 */ >>> + g_list_foreach(pxb_dev_list, pxb_pcie_foreach, NULL); >>> + return pxb_mcfg_hole_size; >>> +} >>> + >>> static const char *pxb_host_root_bus_path(PCIHostState *host_bridge, >>> PCIBus *rootbus) >>> { >>> @@ -145,14 +167,6 @@ static const char *pxb_pcie_host_root_bus_path(PCIHostState *host_bridge, >>> return bus->bus_path; >>> } >>> >>> -static void pxb_pcie_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name, >>> - void *opaque, Error **errp) >>> -{ >>> - PCIExpressHost *e = PCIE_HOST_BRIDGE(obj); >>> - >>> - visit_type_uint64(v, name, &e->size, errp); >>> -} >>> - >>> static char *pxb_host_ofw_unit_address(const SysBusDevice *dev) >>> { >>> const PCIHostState *pxb_host; >>> @@ -192,16 +206,12 @@ static void pxb_pcie_host_initfn(Object *obj) >>> "pci-conf-idx", 4); >>> memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb, >>> "pci-conf-data", 4); >>> - >>> - object_property_add(obj, PCIE_HOST_MCFG_SIZE, "uint64", >>> - pxb_pcie_host_get_mmcfg_size, >>> - NULL, NULL, NULL, NULL); >>> - >>> } >>> >>> static Property pxb_pcie_host_props[] = { >>> DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, PXBPCIEHost, parent_obj.base_addr, >>> - PCIE_BASE_ADDR_UNMAPPED), >>> + PCIE_BASE_ADDR_UNMAPPED), >>> + DEFINE_PROP_UINT64(PCIE_HOST_MCFG_SIZE, PXBPCIEHost, parent_obj.size, 0), >>> DEFINE_PROP_UINT32("domain_nr", PXBPCIEHost, domain_nr, 0), >>> DEFINE_PROP_UINT8("bus_nr", PXBPCIEHost, bus_nr, 0), >>> DEFINE_PROP_UINT8("max_bus", PXBPCIEHost, max_bus, 255), >>> @@ -301,6 +311,8 @@ static gint pxb_compare(gconstpointer a, gconstpointer b) >>> 0; >>> } >>> >>> +static uint64_t pxb_pcie_mcfg_base = 0; >>> + >>> static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) >>> { >>> PXBDev *pxb = convert_to_pxb(dev); >>> @@ -327,6 +339,15 @@ static void pxb_dev_realize_common(PCIDevice *dev, bool pcie, Error **errp) >>> qdev_prop_set_uint8(ds, "bus_nr", pxb->bus_nr); //TODO. >>> qdev_prop_set_uint8(ds, "max_bus", pxb->max_bus); >>> qdev_prop_set_uint8(ds, "domain_nr", pxb->domain_nr); >>> + >>> + /* will be overwritten by firmware, but kept for readability */ >>> + qdev_prop_set_uint64(ds, PCIE_HOST_MCFG_BASE, >>> + pxb->domain_nr ? pxb_pcie_mcfg_base : MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT); >>> + qdev_prop_set_uint64(ds, PCIE_HOST_MCFG_SIZE, >>> + pxb->domain_nr ? (pxb->max_bus + 1ULL) << 20 : 0); >>> + if (pxb->domain_nr) >>> + pxb_pcie_mcfg_base += ((pxb->max_bus + 1ULL) << 20); >>> + >>> bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0, TYPE_PXB_PCIE_BUS); >>> } else { >>> ds = qdev_create(NULL, TYPE_PXB_HOST); >>> @@ -438,6 +459,9 @@ static void pxb_pcie_dev_realize(PCIDevice *dev, Error **errp) >>> return; >>> } >>> >>> + if (0 == pxb_pcie_mcfg_base) >>> + pxb_pcie_mcfg_base = pc_pci_mcfg_start(); >>> + >>> pxb_dev_realize_common(dev, true, errp); >>> } >>> >>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c >>> index 02f9576..a76029d 100644 >>> --- a/hw/pci-host/q35.c >>> +++ b/hw/pci-host/q35.c >>> @@ -178,6 +178,8 @@ static Property q35_host_props[] = { >>> DEFINE_PROP_SIZE(PCI_HOST_ABOVE_4G_MEM_SIZE, Q35PCIHost, >>> mch.above_4g_mem_size, 0), >>> DEFINE_PROP_BOOL("x-pci-hole64-fix", Q35PCIHost, pci_hole64_fix, true), >>> + /* q35 host bridge should always stay in pci domain 0 */ >>> + DEFINE_PROP_UINT32("domain_nr", Q35PCIHost, domain_nr, 0), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >>> index 04d1f8c..81ba010 100644 >>> --- a/include/hw/i386/pc.h >>> +++ b/include/hw/i386/pc.h >>> @@ -212,6 +212,7 @@ void pc_memory_init(PCMachineState *pcms, >>> MemoryRegion *rom_memory, >>> MemoryRegion **ram_memory); >>> uint64_t pc_pci_hole64_start(void); >>> +uint64_t pc_pci_mcfg_start(void); >>> qemu_irq pc_allocate_cpu_irq(void); >>> DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus); >>> void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, >>> diff --git a/include/hw/pci-bridge/pci_expander_bridge.h b/include/hw/pci-bridge/pci_expander_bridge.h >>> new file mode 100644 >>> index 0000000..bb1462c >>> --- /dev/null >>> +++ b/include/hw/pci-bridge/pci_expander_bridge.h >>> @@ -0,0 +1,6 @@ >>> +#ifndef HW_PCI_EXPANDER_H >>> +#define HW_PCI_EXPANDER_H >>> + >>> +uint64_t pxb_pcie_mcfg_hole(void); >>> + >>> +#endif >>> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h >>> index 8f4ddde..432e569 100644 >>> --- a/include/hw/pci-host/q35.h >>> +++ b/include/hw/pci-host/q35.h >>> @@ -69,6 +69,7 @@ typedef struct Q35PCIHost { >>> /*< public >*/ >>> >>> bool pci_hole64_fix; >>> + uint32_t domain_nr; >>> MCHPCIState mch; >>> } Q35PCIHost; >>> >> Thanks, >> Marcel >> ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-06-22 16:43 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-12 9:13 [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST Zihan Yang 2018-06-12 9:13 ` [Qemu-devel] [RFC v2 3/3] acpi-build: describe new pci domain in AML Zihan Yang 2018-06-20 7:46 ` Marcel Apfelbaum 2018-06-21 16:52 ` Zihan Yang 2018-06-22 16:43 ` Marcel Apfelbaum 2018-06-12 13:43 ` [Qemu-devel] [RFC v2 1/3] pci_expander_bridge: add type TYPE_PXB_PCIE_HOST Michael S. Tsirkin 2018-06-13 8:23 ` Zihan Yang 2018-06-13 14:46 ` Michael S. Tsirkin 2018-06-14 3:29 ` Zihan Yang 2018-06-20 4:31 ` Marcel Apfelbaum 2018-06-20 6:38 ` Marcel Apfelbaum 2018-06-21 16:46 ` Zihan Yang [not found] ` <1528794804-6289-2-git-send-email-whois.zihan.yang@gmail.com> 2018-06-20 7:41 ` [Qemu-devel] [RFC v2 2/3] acpi-build: allocate mcfg for pxb-pcie host bridges Marcel Apfelbaum 2018-06-21 16:49 ` Zihan Yang 2018-06-22 16:37 ` Marcel Apfelbaum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).