* [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 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 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 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 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
* 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 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 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
* 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
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).