Re: [Qemu-devel] [RFC v2 2/3] acpi-build: allocate mcfg for pxb-pcie host bridges
On 06/21/2018 07:49 PM, Zihan Yang wrote:
Thanks for your review.
Marcel Apfelbaum 于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
---
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);
+
Re: [Qemu-devel] [RFC v2 2/3] acpi-build: allocate mcfg for pxb-pcie host bridges
Thanks for your review.
Marcel Apfelbaum 于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
> > ---
> > 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
Re: [Qemu-devel] [RFC v2 2/3] acpi-build: allocate mcfg for pxb-pcie host bridges
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
---
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 =
