Hi Zihan,
On 05/20/2018 10:28 AM, Zihan Yang wrote:
Thank you for posting the patches!
For the next version please add a cover letter so we can discuss cross
patchesideas.
To put each pxb into separate pci domain, we need to reserve enough MCFG space
for each pxb host in the main memory.
Right
First try to put them under 4G, before
pci_hole_start, if there are too many hosts, try to put them into main memory
above 4G, before pci_hole64_start. We should check if there is enough memory
to reserve for them
I don't think we should try to place the MMCFGs before 4G even if there
is enough room. Is better to place them always after 4G.
Signed-off-by: Zihan Yang <whois.zihan.y...@gmail.com>
---
hw/i386/pc.c | 5 ++
hw/pci-bridge/pci_expander_bridge.c | 96 ++++++++++++++++++++++++++++-
include/hw/pci-bridge/pci_expander_bridge.h | 7 +++
3 files changed, 107 insertions(+), 1 deletion(-)
create mode 100644 include/hw/pci-bridge/pci_expander_bridge.h
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d768930..98097fd 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"
@@ -1466,6 +1467,7 @@ uint64_t pc_pci_hole64_start(void)
PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
MachineState *ms = MACHINE(pcms);
uint64_t hole64_start = 0;
+ int pxb_hosts;
if (pcmc->has_reserved_memory && ms->device_memory->base) {
hole64_start = ms->device_memory->base;
@@ -1473,6 +1475,9 @@ uint64_t pc_pci_hole64_start(void)
hole64_start += memory_region_size(&ms->device_memory->mr);
}
} else {
+ /* make sure enough space is left for pxb host, otherwise fail */
+ pxb_hosts = pxb_get_expander_hosts();
+ g_assert (pxb_hosts <= 0 || pcms->above_4g_mem_size >= (pxb_hosts <<
28ULL));
"above_4g_mem" PCI hole it is reserved for PCI devices hotplug. We
cannot use if for
MMCFGs. What I think we can do is to "move" the 64 bit PCI hole after
the MMCFGs.
So the layout of over 4G space will be:
[RAM hotplug][MMCFGs][PCI hotplug]...
hole64_start = 0x100000000ULL + pcms->above_4g_mem_size;
}
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index e62de42..8409c87 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -12,10 +12,13 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qapi/visitor.h"
#include "hw/pci/pci.h"
#include "hw/pci/pci_bus.h"
#include "hw/pci/pci_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"
@@ -57,7 +60,13 @@ static PXBDev *convert_to_pxb(PCIDevice *dev)
static GList *pxb_dev_list;
+typedef struct PXBPCIHost {
+ PCIExpressHost parent_obj;
+} PXBPCIHost;
+
#define TYPE_PXB_HOST "pxb-host"
+#define PXB_HOST_DEVICE(obj) \
+ OBJECT_CHECK(PXBPCIHost, (obj), TYPE_PXB_HOST)
static int pxb_bus_num(PCIBus *bus)
{
@@ -111,6 +120,14 @@ static const char *pxb_host_root_bus_path(PCIHostState
*host_bridge,
return bus->bus_path;
}
+static void pxb_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 +159,80 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice
*dev)
return NULL;
}
+static Object *pxb_get_i386_pci_host(void)
+{
+ PCIHostState *host;
+
+ host = OBJECT_CHECK(PCIHostState,
+ object_resolve_path("/machine/i440fx", NULL),
+ TYPE_PCI_HOST_BRIDGE);
+ if (!host) {
+ host = OBJECT_CHECK(PCIHostState,
+ object_resolve_path("/machine/q35", NULL),
+ TYPE_PCI_HOST_BRIDGE);
+ }
+
+ return OBJECT(host);
+}
+
The about function appears also in hw/i386/acpi-build.c, you may wantto
re-use.
+int pxhb_cnt = 0;
+
+/* -1 means to exclude q35 host */
+#define MCFG_IN_PCI_HOLE (((IO_APIC_DEFAULT_ADDRESS -
MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT) >> 28) - 1)
+
I don't understand the above. If you need a single MMCFG size you can
use MCH_HOST_BRIDGE_PCIEXBAR_MAX.
+int pxb_get_expander_hosts(void)
+{
+ return pxhb_cnt - MCFG_IN_PCI_HOLE;
+}
Do you need the number of existing expander hosts? We have a
pxbdev_list, just query it.
+
+/* Dirty workaround */
+static void modify_q35_pci_hole(void)
+{
+ Object *pci_host;
+ Q35PCIHost *s;
+
+ pci_host = pxb_get_i386_pci_host();
+ g_assert(pci_host);
+ s = Q35_HOST_DEVICE(pci_host);
+
+ ++pxhb_cnt;
+ if (pxhb_cnt <= MCFG_IN_PCI_HOLE) {
+ range_set_bounds(&s->mch.pci_hole,
+ MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + ((1 + pxhb_cnt) << 28),
+ IO_APIC_DEFAULT_ADDRESS - 1);
+ }
+
+ // leave pci hole64 to acpi build part
+}
+
+static void pxb_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_host_get_mmcfg_size,
+ NULL, NULL, NULL, NULL);
+
+ /* Leave enough space for the biggest MCFG BAR */
+ /* TODO. Since pxb host is just an expander bridge without an mch,
+ * we modify the range in q35 host. It should be workable as it is
+ * before acpi build, although it is dirty
+ */
+ modify_q35_pci_hole();
The above will need to change. We move the pci hole, not resize it.
I am not sure this is the right place to handle it, maybe we add a new
property
right beside pci_hole ones (extra-mmcfg?) and default it to 0.
+}
+
+/* default value does not matter as guest firmware will overwrite it */
+static Property pxb_host_props[] = {
+ DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, PXBPCIHost, parent_obj.base_addr,
+ MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
You cannot use the MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT as it is in use
by the "main" root complex MMCFG. Actually I don't think we can come up
with a valid default.
+ DEFINE_PROP_END_OF_LIST(),
+};
+
static void pxb_host_class_init(ObjectClass *class, void *data)
{
DeviceClass *dc = DEVICE_CLASS(class);
@@ -149,6 +240,7 @@ static void pxb_host_class_init(ObjectClass *class, void
*data)
PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class);
dc->fw_name = "pci";
+ dc->props = pxb_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;
@@ -157,7 +249,9 @@ static void pxb_host_class_init(ObjectClass *class, void
*data)
static const TypeInfo pxb_host_info = {
.name = TYPE_PXB_HOST,
- .parent = TYPE_PCI_HOST_BRIDGE,
+ .parent = TYPE_PCIE_HOST_BRIDGE,
Be aware this is used by both pxb and pxb-pcie devices, I think you
should split the type
for each one and let the pxb's one as before.
+ .instance_size = sizeof(PXBPCIHost),
+ .instance_init = pxb_host_initfn,
.class_init = pxb_host_class_init,
};
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..d48ddb1
--- /dev/null
+++ b/include/hw/pci-bridge/pci_expander_bridge.h
@@ -0,0 +1,7 @@
+#ifndef HW_PCI_EXPANDER_H
+#define HW_PCI_EXPANDER_H
+
+/* return the number of pxb hosts that resides in the main memory above 4G */
+int pxb_get_expander_hosts(void);
+
+#endif
Thanks,
Marcel
