| Message ID | 20190521033249.1960-3-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Extract build_mcfg Part 2 | expand |
On Tue, May 21, 2019 at 11:32:49AM +0800, Wei Yang wrote: > build_append_foo() API doesn't need explicit endianness conversions > which eliminates a source of errors and it makes build_mcfg() look like > declarative definition of MCFG table in ACPI spec, which makes it easy > to review. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > Suggested-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > --- > v5: > * miss the reserved[8] of MCFG in last version, add it back > * drop SOBs and make sure bios-tables-test all OK > --- Please do not add two --- separators. It breaks git am. One --- should come after the commit log. Anything after that and until diff is ignored anyway. > hw/acpi/pci.c | 35 +++++++++++++++++++++++------------ > include/hw/acpi/acpi-defs.h | 18 ------------------ > 2 files changed, 23 insertions(+), 30 deletions(-) > > diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c > index fa0fa30bb9..49df7b7d54 100644 > --- a/hw/acpi/pci.c > +++ b/hw/acpi/pci.c > @@ -30,17 +30,28 @@ > > void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > { > - AcpiTableMcfg *mcfg; > - int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); > - > - mcfg = acpi_data_push(table_data, len); > - mcfg->allocation[0].address = cpu_to_le64(info->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->size - 1); > - > - build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL); > + int mcfg_start = table_data->len; > + > + acpi_data_push(table_data, sizeof(AcpiTableHeader)); > + > + /* > + * PCI Firmware Specification, Revision 3.0 > + * 4.1.2 MCFG Table Description. > + */ > + /* Reserved */ > + build_append_int_noprefix(table_data, 0, 8); > + /* Base address, processor-relative */ > + build_append_int_noprefix(table_data, info->base, 8); > + /* PCI segment group number */ > + build_append_int_noprefix(table_data, 0, 2); > + /* Starting PCI Bus number */ > + build_append_int_noprefix(table_data, 0, 1); > + /* Final PCI Bus number */ > + build_append_int_noprefix(table_data, PCIE_MMCFG_BUS(info->size - 1), 1); > + /* Reserved */ > + build_append_int_noprefix(table_data, 0, 4); > + > + build_header(linker, table_data, (void *)(table_data->data + mcfg_start), > + "MCFG", table_data->len - mcfg_start, 1, NULL, NULL); > } > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index f9aa4bd398..57a3f58b0c 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -449,24 +449,6 @@ struct AcpiSratProcessorGiccAffinity { > > typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity; > > -/* PCI fw r3.0 MCFG table. */ > -/* Subtable */ > -struct AcpiMcfgAllocation { > - uint64_t address; /* Base address, processor-relative */ > - uint16_t pci_segment; /* PCI segment group number */ > - uint8_t start_bus_number; /* Starting PCI Bus number */ > - uint8_t end_bus_number; /* Final PCI Bus number */ > - uint32_t reserved; > -} QEMU_PACKED; > -typedef struct AcpiMcfgAllocation AcpiMcfgAllocation; > - > -struct AcpiTableMcfg { > - ACPI_TABLE_HEADER_DEF; > - uint8_t reserved[8]; > - AcpiMcfgAllocation allocation[0]; > -} QEMU_PACKED; > -typedef struct AcpiTableMcfg AcpiTableMcfg; > - > /* > * TCPA Description Table > * > -- > 2.19.1
On Tue, May 21, 2019 at 11:32:49AM +0800, Wei Yang wrote: > build_append_foo() API doesn't need explicit endianness conversions > which eliminates a source of errors and it makes build_mcfg() look like > declarative definition of MCFG table in ACPI spec, which makes it easy > to review. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > Suggested-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > --- > v5: > * miss the reserved[8] of MCFG in last version, add it back > * drop SOBs and make sure bios-tables-test all OK > --- > hw/acpi/pci.c | 35 +++++++++++++++++++++++------------ > include/hw/acpi/acpi-defs.h | 18 ------------------ > 2 files changed, 23 insertions(+), 30 deletions(-) > > diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c > index fa0fa30bb9..49df7b7d54 100644 > --- a/hw/acpi/pci.c > +++ b/hw/acpi/pci.c > @@ -30,17 +30,28 @@ > > void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > { > - AcpiTableMcfg *mcfg; > - int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); > - > - mcfg = acpi_data_push(table_data, len); > - mcfg->allocation[0].address = cpu_to_le64(info->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->size - 1); > - > - build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL); > + int mcfg_start = table_data->len; > + > + acpi_data_push(table_data, sizeof(AcpiTableHeader)); > + > + /* > + * PCI Firmware Specification, Revision 3.0 > + * 4.1.2 MCFG Table Description. > + */ > + /* Reserved */ > + build_append_int_noprefix(table_data, 0, 8); below is in fact Memory Mapped Enhanced Configuration Space Base Address Allocation Structure maybe document this? > + /* Base address, processor-relative */ > + build_append_int_noprefix(table_data, info->base, 8); > + /* PCI segment group number */ > + build_append_int_noprefix(table_data, 0, 2); > + /* Starting PCI Bus number */ > + build_append_int_noprefix(table_data, 0, 1); > + /* Final PCI Bus number */ > + build_append_int_noprefix(table_data, PCIE_MMCFG_BUS(info->size - 1), 1); > + /* Reserved */ > + build_append_int_noprefix(table_data, 0, 4); > + > + build_header(linker, table_data, (void *)(table_data->data + mcfg_start), > + "MCFG", table_data->len - mcfg_start, 1, NULL, NULL); > } > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index f9aa4bd398..57a3f58b0c 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -449,24 +449,6 @@ struct AcpiSratProcessorGiccAffinity { > > typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity; > > -/* PCI fw r3.0 MCFG table. */ > -/* Subtable */ > -struct AcpiMcfgAllocation { > - uint64_t address; /* Base address, processor-relative */ > - uint16_t pci_segment; /* PCI segment group number */ > - uint8_t start_bus_number; /* Starting PCI Bus number */ > - uint8_t end_bus_number; /* Final PCI Bus number */ > - uint32_t reserved; > -} QEMU_PACKED; > -typedef struct AcpiMcfgAllocation AcpiMcfgAllocation; > - > -struct AcpiTableMcfg { > - ACPI_TABLE_HEADER_DEF; > - uint8_t reserved[8]; > - AcpiMcfgAllocation allocation[0]; > -} QEMU_PACKED; > -typedef struct AcpiTableMcfg AcpiTableMcfg; > - > /* > * TCPA Description Table > * > -- > 2.19.1
On Mon, May 20, 2019 at 11:44:28PM -0400, Michael S. Tsirkin wrote: >On Tue, May 21, 2019 at 11:32:49AM +0800, Wei Yang wrote: >> build_append_foo() API doesn't need explicit endianness conversions >> which eliminates a source of errors and it makes build_mcfg() look like >> declarative definition of MCFG table in ACPI spec, which makes it easy >> to review. >> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >> Suggested-by: Igor Mammedov <imammedo@redhat.com> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com> >> >> --- >> v5: >> * miss the reserved[8] of MCFG in last version, add it back >> * drop SOBs and make sure bios-tables-test all OK >> --- >> hw/acpi/pci.c | 35 +++++++++++++++++++++++------------ >> include/hw/acpi/acpi-defs.h | 18 ------------------ >> 2 files changed, 23 insertions(+), 30 deletions(-) >> >> diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c >> index fa0fa30bb9..49df7b7d54 100644 >> --- a/hw/acpi/pci.c >> +++ b/hw/acpi/pci.c >> @@ -30,17 +30,28 @@ >> >> void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) >> { >> - AcpiTableMcfg *mcfg; >> - int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); >> - >> - mcfg = acpi_data_push(table_data, len); >> - mcfg->allocation[0].address = cpu_to_le64(info->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->size - 1); >> - >> - build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL); >> + int mcfg_start = table_data->len; >> + >> + acpi_data_push(table_data, sizeof(AcpiTableHeader)); >> + >> + /* >> + * PCI Firmware Specification, Revision 3.0 >> + * 4.1.2 MCFG Table Description. >> + */ >> + /* Reserved */ >> + build_append_int_noprefix(table_data, 0, 8); > >below is in fact > Memory Mapped Enhanced Configuration Space Base Address Allocation Structure > >maybe document this? > Sure. Let me add this. >> + /* Base address, processor-relative */ >> + build_append_int_noprefix(table_data, info->base, 8); >> + /* PCI segment group number */ >> + build_append_int_noprefix(table_data, 0, 2); >> + /* Starting PCI Bus number */ >> + build_append_int_noprefix(table_data, 0, 1); >> + /* Final PCI Bus number */ >> + build_append_int_noprefix(table_data, PCIE_MMCFG_BUS(info->size - 1), 1); >> + /* Reserved */ >> + build_append_int_noprefix(table_data, 0, 4); >> + >> + build_header(linker, table_data, (void *)(table_data->data + mcfg_start), >> + "MCFG", table_data->len - mcfg_start, 1, NULL, NULL); >> } >> >> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h >> index f9aa4bd398..57a3f58b0c 100644 >> --- a/include/hw/acpi/acpi-defs.h >> +++ b/include/hw/acpi/acpi-defs.h >> @@ -449,24 +449,6 @@ struct AcpiSratProcessorGiccAffinity { >> >> typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity; >> >> -/* PCI fw r3.0 MCFG table. */ >> -/* Subtable */ >> -struct AcpiMcfgAllocation { >> - uint64_t address; /* Base address, processor-relative */ >> - uint16_t pci_segment; /* PCI segment group number */ >> - uint8_t start_bus_number; /* Starting PCI Bus number */ >> - uint8_t end_bus_number; /* Final PCI Bus number */ >> - uint32_t reserved; >> -} QEMU_PACKED; >> -typedef struct AcpiMcfgAllocation AcpiMcfgAllocation; >> - >> -struct AcpiTableMcfg { >> - ACPI_TABLE_HEADER_DEF; >> - uint8_t reserved[8]; >> - AcpiMcfgAllocation allocation[0]; >> -} QEMU_PACKED; >> -typedef struct AcpiTableMcfg AcpiTableMcfg; >> - >> /* >> * TCPA Description Table >> * >> -- >> 2.19.1
On Mon, 20 May 2019 23:37:22 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, May 21, 2019 at 11:32:49AM +0800, Wei Yang wrote: > > build_append_foo() API doesn't need explicit endianness conversions > > which eliminates a source of errors and it makes build_mcfg() look like > > declarative definition of MCFG table in ACPI spec, which makes it easy > > to review. > > > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > v5: > > * miss the reserved[8] of MCFG in last version, add it back > > * drop SOBs and make sure bios-tables-test all OK > > --- > > > Please do not add two --- separators. It breaks git am. that worked just fine for the last xx years and still works for me. > > One --- should come after the commit log. Anything after that and > until diff is ignored anyway. That's fine if we wish to commit change log into history, but Typically we don't want change log to be committed (as it's useless from history pov), hence we put it after separator to get it dropped on applying. > > > hw/acpi/pci.c | 35 +++++++++++++++++++++++------------ > > include/hw/acpi/acpi-defs.h | 18 ------------------ > > 2 files changed, 23 insertions(+), 30 deletions(-) > > > > diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c > > index fa0fa30bb9..49df7b7d54 100644 > > --- a/hw/acpi/pci.c > > +++ b/hw/acpi/pci.c > > @@ -30,17 +30,28 @@ > > > > void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > > { > > - AcpiTableMcfg *mcfg; > > - int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); > > - > > - mcfg = acpi_data_push(table_data, len); > > - mcfg->allocation[0].address = cpu_to_le64(info->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->size - 1); > > - > > - build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL); > > + int mcfg_start = table_data->len; > > + > > + acpi_data_push(table_data, sizeof(AcpiTableHeader)); > > + > > + /* > > + * PCI Firmware Specification, Revision 3.0 > > + * 4.1.2 MCFG Table Description. > > + */ > > + /* Reserved */ > > + build_append_int_noprefix(table_data, 0, 8); > > + /* Base address, processor-relative */ > > + build_append_int_noprefix(table_data, info->base, 8); > > + /* PCI segment group number */ > > + build_append_int_noprefix(table_data, 0, 2); > > + /* Starting PCI Bus number */ > > + build_append_int_noprefix(table_data, 0, 1); > > + /* Final PCI Bus number */ > > + build_append_int_noprefix(table_data, PCIE_MMCFG_BUS(info->size - 1), 1); > > + /* Reserved */ > > + build_append_int_noprefix(table_data, 0, 4); > > + > > + build_header(linker, table_data, (void *)(table_data->data + mcfg_start), > > + "MCFG", table_data->len - mcfg_start, 1, NULL, NULL); > > } > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > > index f9aa4bd398..57a3f58b0c 100644 > > --- a/include/hw/acpi/acpi-defs.h > > +++ b/include/hw/acpi/acpi-defs.h > > @@ -449,24 +449,6 @@ struct AcpiSratProcessorGiccAffinity { > > > > typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity; > > > > -/* PCI fw r3.0 MCFG table. */ > > -/* Subtable */ > > -struct AcpiMcfgAllocation { > > - uint64_t address; /* Base address, processor-relative */ > > - uint16_t pci_segment; /* PCI segment group number */ > > - uint8_t start_bus_number; /* Starting PCI Bus number */ > > - uint8_t end_bus_number; /* Final PCI Bus number */ > > - uint32_t reserved; > > -} QEMU_PACKED; > > -typedef struct AcpiMcfgAllocation AcpiMcfgAllocation; > > - > > -struct AcpiTableMcfg { > > - ACPI_TABLE_HEADER_DEF; > > - uint8_t reserved[8]; > > - AcpiMcfgAllocation allocation[0]; > > -} QEMU_PACKED; > > -typedef struct AcpiTableMcfg AcpiTableMcfg; > > - > > /* > > * TCPA Description Table > > * > > -- > > 2.19.1
On Tue, May 21, 2019 at 04:29:58PM +0200, Igor Mammedov wrote: > On Mon, 20 May 2019 23:37:22 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, May 21, 2019 at 11:32:49AM +0800, Wei Yang wrote: > > > build_append_foo() API doesn't need explicit endianness conversions > > > which eliminates a source of errors and it makes build_mcfg() look like > > > declarative definition of MCFG table in ACPI spec, which makes it easy > > > to review. > > > > > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > > > > --- > > > v5: > > > * miss the reserved[8] of MCFG in last version, add it back > > > * drop SOBs and make sure bios-tables-test all OK > > > --- > > > > > > Please do not add two --- separators. It breaks git am. > that worked just fine for the last xx years and still works for me. Because you don't use git am? It's true I often am forced to edit your patches to apply them. Pls do not do this it makes no sense. > > > > One --- should come after the commit log. Anything after that and > > until diff is ignored anyway. > That's fine if we wish to commit change log into history, but > Typically we don't want change log to be committed (as it's useless from history pov), > hence we put it after separator to get it dropped on applying. Then you should do build_append_foo() API doesn't need explicit endianness conversions which eliminates a source of errors and it makes build_mcfg() look like declarative definition of MCFG table in ACPI spec, which makes it easy to review. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> Suggested-by: Igor Mammedov <imammedo@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> --- v5: * miss the reserved[8] of MCFG in last version, add it back * drop SOBs and make sure bios-tables-test all OK hw/acpi/pci.c | 35 +++++++++++++++++++++++------------ include/hw/acpi/acpi-defs.h | 18 ------------------ 2 files changed, 23 insertions(+), 30 deletions(-) diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c Anything between --- and diff is ignored. Second --- is poointless and harmful. > > > > > hw/acpi/pci.c | 35 +++++++++++++++++++++++------------ > > > include/hw/acpi/acpi-defs.h | 18 ------------------ > > > 2 files changed, 23 insertions(+), 30 deletions(-) > > > > > > diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c > > > index fa0fa30bb9..49df7b7d54 100644 > > > --- a/hw/acpi/pci.c > > > +++ b/hw/acpi/pci.c > > > @@ -30,17 +30,28 @@ > > > > > > void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > > > { > > > - AcpiTableMcfg *mcfg; > > > - int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); > > > - > > > - mcfg = acpi_data_push(table_data, len); > > > - mcfg->allocation[0].address = cpu_to_le64(info->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->size - 1); > > > - > > > - build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL); > > > + int mcfg_start = table_data->len; > > > + > > > + acpi_data_push(table_data, sizeof(AcpiTableHeader)); > > > + > > > + /* > > > + * PCI Firmware Specification, Revision 3.0 > > > + * 4.1.2 MCFG Table Description. > > > + */ > > > + /* Reserved */ > > > + build_append_int_noprefix(table_data, 0, 8); > > > + /* Base address, processor-relative */ > > > + build_append_int_noprefix(table_data, info->base, 8); > > > + /* PCI segment group number */ > > > + build_append_int_noprefix(table_data, 0, 2); > > > + /* Starting PCI Bus number */ > > > + build_append_int_noprefix(table_data, 0, 1); > > > + /* Final PCI Bus number */ > > > + build_append_int_noprefix(table_data, PCIE_MMCFG_BUS(info->size - 1), 1); > > > + /* Reserved */ > > > + build_append_int_noprefix(table_data, 0, 4); > > > + > > > + build_header(linker, table_data, (void *)(table_data->data + mcfg_start), > > > + "MCFG", table_data->len - mcfg_start, 1, NULL, NULL); > > > } > > > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > > > index f9aa4bd398..57a3f58b0c 100644 > > > --- a/include/hw/acpi/acpi-defs.h > > > +++ b/include/hw/acpi/acpi-defs.h > > > @@ -449,24 +449,6 @@ struct AcpiSratProcessorGiccAffinity { > > > > > > typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity; > > > > > > -/* PCI fw r3.0 MCFG table. */ > > > -/* Subtable */ > > > -struct AcpiMcfgAllocation { > > > - uint64_t address; /* Base address, processor-relative */ > > > - uint16_t pci_segment; /* PCI segment group number */ > > > - uint8_t start_bus_number; /* Starting PCI Bus number */ > > > - uint8_t end_bus_number; /* Final PCI Bus number */ > > > - uint32_t reserved; > > > -} QEMU_PACKED; > > > -typedef struct AcpiMcfgAllocation AcpiMcfgAllocation; > > > - > > > -struct AcpiTableMcfg { > > > - ACPI_TABLE_HEADER_DEF; > > > - uint8_t reserved[8]; > > > - AcpiMcfgAllocation allocation[0]; > > > -} QEMU_PACKED; > > > -typedef struct AcpiTableMcfg AcpiTableMcfg; > > > - > > > /* > > > * TCPA Description Table > > > * > > > -- > > > 2.19.1
On Tue, 21 May 2019 10:34:03 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, May 21, 2019 at 04:29:58PM +0200, Igor Mammedov wrote: > > On Mon, 20 May 2019 23:37:22 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Tue, May 21, 2019 at 11:32:49AM +0800, Wei Yang wrote: > > > > build_append_foo() API doesn't need explicit endianness conversions > > > > which eliminates a source of errors and it makes build_mcfg() look like > > > > declarative definition of MCFG table in ACPI spec, which makes it easy > > > > to review. > > > > > > > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > --- > > > > v5: > > > > * miss the reserved[8] of MCFG in last version, add it back > > > > * drop SOBs and make sure bios-tables-test all OK > > > > --- > > > > > > > > > Please do not add two --- separators. It breaks git am. > > that worked just fine for the last xx years and still works for me. > > Because you don't use git am? > It's true I often am forced to edit your patches to apply them. > Pls do not do this it makes no sense. I do not use it as much as you, but I do use git am to test patches from list and it works with double separator as expected (the second separator is ignored). > > > One --- should come after the commit log. Anything after that and > > > until diff is ignored anyway. > > That's fine if we wish to commit change log into history, but > > Typically we don't want change log to be committed (as it's useless from history pov), > > hence we put it after separator to get it dropped on applying. > > Then you should do the problem is that the second separator is added by git, so one would have to edit it out after format-patch (which is error prone in case of multi-patch series). I don't know much about git though, so could you suggest how to handle situation to avoid the second --- (i.e. how do you do it)? PS: There is 'git notes' to add/track per patch annotations, but it a bit tedious to use notes compared to in-lining change log in commit message. > build_append_foo() API doesn't need explicit endianness conversions > which eliminates a source of errors and it makes build_mcfg() look like > declarative definition of MCFG table in ACPI spec, which makes it easy > to review. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > Suggested-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > --- > v5: > * miss the reserved[8] of MCFG in last version, add it back > * drop SOBs and make sure bios-tables-test all OK > > hw/acpi/pci.c | 35 +++++++++++++++++++++++------------ > include/hw/acpi/acpi-defs.h | 18 ------------------ > 2 files changed, 23 insertions(+), 30 deletions(-) > > diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c > > > Anything between --- and diff is ignored. Second --- is poointless and > harmful. > > > > > > > > hw/acpi/pci.c | 35 +++++++++++++++++++++++------------ > > > > include/hw/acpi/acpi-defs.h | 18 ------------------ > > > > 2 files changed, 23 insertions(+), 30 deletions(-) > > > > > > > > diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c > > > > index fa0fa30bb9..49df7b7d54 100644 > > > > --- a/hw/acpi/pci.c > > > > +++ b/hw/acpi/pci.c > > > > @@ -30,17 +30,28 @@ > > > > > > > > void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > > > > { > > > > - AcpiTableMcfg *mcfg; > > > > - int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); > > > > - > > > > - mcfg = acpi_data_push(table_data, len); > > > > - mcfg->allocation[0].address = cpu_to_le64(info->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->size - 1); > > > > - > > > > - build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL); > > > > + int mcfg_start = table_data->len; > > > > + > > > > + acpi_data_push(table_data, sizeof(AcpiTableHeader)); > > > > + > > > > + /* > > > > + * PCI Firmware Specification, Revision 3.0 > > > > + * 4.1.2 MCFG Table Description. > > > > + */ > > > > + /* Reserved */ > > > > + build_append_int_noprefix(table_data, 0, 8); > > > > + /* Base address, processor-relative */ > > > > + build_append_int_noprefix(table_data, info->base, 8); > > > > + /* PCI segment group number */ > > > > + build_append_int_noprefix(table_data, 0, 2); > > > > + /* Starting PCI Bus number */ > > > > + build_append_int_noprefix(table_data, 0, 1); > > > > + /* Final PCI Bus number */ > > > > + build_append_int_noprefix(table_data, PCIE_MMCFG_BUS(info->size - 1), 1); > > > > + /* Reserved */ > > > > + build_append_int_noprefix(table_data, 0, 4); > > > > + > > > > + build_header(linker, table_data, (void *)(table_data->data + mcfg_start), > > > > + "MCFG", table_data->len - mcfg_start, 1, NULL, NULL); > > > > } > > > > > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > > > > index f9aa4bd398..57a3f58b0c 100644 > > > > --- a/include/hw/acpi/acpi-defs.h > > > > +++ b/include/hw/acpi/acpi-defs.h > > > > @@ -449,24 +449,6 @@ struct AcpiSratProcessorGiccAffinity { > > > > > > > > typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity; > > > > > > > > -/* PCI fw r3.0 MCFG table. */ > > > > -/* Subtable */ > > > > -struct AcpiMcfgAllocation { > > > > - uint64_t address; /* Base address, processor-relative */ > > > > - uint16_t pci_segment; /* PCI segment group number */ > > > > - uint8_t start_bus_number; /* Starting PCI Bus number */ > > > > - uint8_t end_bus_number; /* Final PCI Bus number */ > > > > - uint32_t reserved; > > > > -} QEMU_PACKED; > > > > -typedef struct AcpiMcfgAllocation AcpiMcfgAllocation; > > > > - > > > > -struct AcpiTableMcfg { > > > > - ACPI_TABLE_HEADER_DEF; > > > > - uint8_t reserved[8]; > > > > - AcpiMcfgAllocation allocation[0]; > > > > -} QEMU_PACKED; > > > > -typedef struct AcpiTableMcfg AcpiTableMcfg; > > > > - > > > > /* > > > > * TCPA Description Table > > > > * > > > > -- > > > > 2.19.1
diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c index fa0fa30bb9..49df7b7d54 100644 --- a/hw/acpi/pci.c +++ b/hw/acpi/pci.c @@ -30,17 +30,28 @@ void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) { - AcpiTableMcfg *mcfg; - int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); - - mcfg = acpi_data_push(table_data, len); - mcfg->allocation[0].address = cpu_to_le64(info->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->size - 1); - - build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL); + int mcfg_start = table_data->len; + + acpi_data_push(table_data, sizeof(AcpiTableHeader)); + + /* + * PCI Firmware Specification, Revision 3.0 + * 4.1.2 MCFG Table Description. + */ + /* Reserved */ + build_append_int_noprefix(table_data, 0, 8); + /* Base address, processor-relative */ + build_append_int_noprefix(table_data, info->base, 8); + /* PCI segment group number */ + build_append_int_noprefix(table_data, 0, 2); + /* Starting PCI Bus number */ + build_append_int_noprefix(table_data, 0, 1); + /* Final PCI Bus number */ + build_append_int_noprefix(table_data, PCIE_MMCFG_BUS(info->size - 1), 1); + /* Reserved */ + build_append_int_noprefix(table_data, 0, 4); + + build_header(linker, table_data, (void *)(table_data->data + mcfg_start), + "MCFG", table_data->len - mcfg_start, 1, NULL, NULL); } diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h index f9aa4bd398..57a3f58b0c 100644 --- a/include/hw/acpi/acpi-defs.h +++ b/include/hw/acpi/acpi-defs.h @@ -449,24 +449,6 @@ struct AcpiSratProcessorGiccAffinity { typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity; -/* PCI fw r3.0 MCFG table. */ -/* Subtable */ -struct AcpiMcfgAllocation { - uint64_t address; /* Base address, processor-relative */ - uint16_t pci_segment; /* PCI segment group number */ - uint8_t start_bus_number; /* Starting PCI Bus number */ - uint8_t end_bus_number; /* Final PCI Bus number */ - uint32_t reserved; -} QEMU_PACKED; -typedef struct AcpiMcfgAllocation AcpiMcfgAllocation; - -struct AcpiTableMcfg { - ACPI_TABLE_HEADER_DEF; - uint8_t reserved[8]; - AcpiMcfgAllocation allocation[0]; -} QEMU_PACKED; -typedef struct AcpiTableMcfg AcpiTableMcfg; - /* * TCPA Description Table *