| Message ID | 20190419003053.8260-7-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Extract build_mcfg | expand |
On Fri, Apr 19, 2019 at 08:30:53AM +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> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Causes a regression with an invalid MCFG produced. Dropped. > --- > hw/acpi/pci.c | 33 +++++++++++++++++++++------------ > include/hw/acpi/acpi-defs.h | 18 ------------------ > 2 files changed, 21 insertions(+), 30 deletions(-) > > diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c > index fa0fa30bb9..341805e786 100644 > --- a/hw/acpi/pci.c > +++ b/hw/acpi/pci.c > @@ -30,17 +30,26 @@ > > 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. > + */ > + /* 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 5/15/19 3:10 AM, Michael S. Tsirkin wrote: > On Fri, Apr 19, 2019 at 08:30:53AM +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> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Causes a regression with an invalid MCFG produced. > Dropped. > >> --- >> hw/acpi/pci.c | 33 +++++++++++++++++++++------------ >> include/hw/acpi/acpi-defs.h | 18 ------------------ >> 2 files changed, 21 insertions(+), 30 deletions(-) >> >> diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c >> index fa0fa30bb9..341805e786 100644 >> --- a/hw/acpi/pci.c >> +++ b/hw/acpi/pci.c >> @@ -30,17 +30,26 @@ >> >> 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)); Ah, it seems we missed AcpiTableMcfg.reserved[8]...: build_append_int_noprefix(table_data, 0, 8); >> + >> + /* >> + * PCI Firmware Specification, Revision 3.0 >> + * 4.1.2 MCFG Table Description. >> + */ >> + /* 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]; --------^ Thanks Michael for testing... Wei, can you add a MCFG test in tests/bios-tables-test.c? Regards, Phil. >> - AcpiMcfgAllocation allocation[0]; >> -} QEMU_PACKED; >> -typedef struct AcpiTableMcfg AcpiTableMcfg; >> - >> /* >> * TCPA Description Table >> * >> -- >> 2.19.1
On Tue, May 14, 2019 at 09:10:34PM -0400, Michael S. Tsirkin wrote: >On Fri, Apr 19, 2019 at 08:30:53AM +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> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >Causes a regression with an invalid MCFG produced. >Dropped. > Any more information about this regression? How to reproduce it?
On Wed, May 15, 2019 at 07:29:17AM +0200, Philippe Mathieu-Daudé wrote: >On 5/15/19 3:10 AM, Michael S. Tsirkin wrote: >> On Fri, Apr 19, 2019 at 08:30:53AM +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> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> >> Causes a regression with an invalid MCFG produced. >> Dropped. >> >>> --- >>> hw/acpi/pci.c | 33 +++++++++++++++++++++------------ >>> include/hw/acpi/acpi-defs.h | 18 ------------------ >>> 2 files changed, 21 insertions(+), 30 deletions(-) >>> >>> diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c >>> index fa0fa30bb9..341805e786 100644 >>> --- a/hw/acpi/pci.c >>> +++ b/hw/acpi/pci.c >>> @@ -30,17 +30,26 @@ >>> >>> 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)); > >Ah, it seems we missed AcpiTableMcfg.reserved[8]...: > > build_append_int_noprefix(table_data, 0, 8); > Hmm... you are right. Shame on me. >>> + >>> + /* >>> + * PCI Firmware Specification, Revision 3.0 >>> + * 4.1.2 MCFG Table Description. >>> + */ >>> + /* 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]; > > --------^ > >Thanks Michael for testing... > >Wei, can you add a MCFG test in tests/bios-tables-test.c? > Sure, let me prepare the test. >Regards, > >Phil. > >>> - AcpiMcfgAllocation allocation[0]; >>> -} QEMU_PACKED; >>> -typedef struct AcpiTableMcfg AcpiTableMcfg; >>> - >>> /* >>> * TCPA Description Table >>> * >>> -- >>> 2.19.1
On Wed, May 15, 2019 at 07:29:17AM +0200, Philippe Mathieu-Daudé wrote: > >Thanks Michael for testing... > >Wei, can you add a MCFG test in tests/bios-tables-test.c? > I took a look into the test, current q35 has already has a reference MCFG in tests/data/acpi/q35/MCFG. And there would be a warning message when reserved[8] is missed. /x86_64/acpi/q35/bridge: acpi-test: Warning! MCFG mismatch. Is this enough? Or what more information prefer to add? >Regards, > >Phil. > >>> - AcpiMcfgAllocation allocation[0]; >>> -} QEMU_PACKED; >>> -typedef struct AcpiTableMcfg AcpiTableMcfg; >>> - >>> /* >>> * TCPA Description Table >>> * >>> -- >>> 2.19.1
On Thu, May 16, 2019 at 9:41 AM Wei Yang <richardw.yang@linux.intel.com> wrote: > > On Wed, May 15, 2019 at 07:29:17AM +0200, Philippe Mathieu-Daudé wrote: > > > >Thanks Michael for testing... > > > >Wei, can you add a MCFG test in tests/bios-tables-test.c? > > > > I took a look into the test, current q35 has already has a reference MCFG in > tests/data/acpi/q35/MCFG. > > And there would be a warning message when reserved[8] is missed. > > /x86_64/acpi/q35/bridge: acpi-test: Warning! MCFG mismatch. > > Is this enough? Or what more information prefer to add? Well, the test has to fail for any mismatch (not a simple warning). A mismatch failure seems to be enough IMHO. > >>> - AcpiMcfgAllocation allocation[0]; > >>> -} QEMU_PACKED; > >>> -typedef struct AcpiTableMcfg AcpiTableMcfg; > >>> - > >>> /* > >>> * TCPA Description Table > >>> * > >>> -- > >>> 2.19.1 > > -- > Wei Yang > Help you, Help me
On Thu, 16 May 2019 13:01:31 +0200 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On Thu, May 16, 2019 at 9:41 AM Wei Yang <richardw.yang@linux.intel.com> wrote: > > > > On Wed, May 15, 2019 at 07:29:17AM +0200, Philippe Mathieu-Daudé wrote: > > > > > >Thanks Michael for testing... > > > > > >Wei, can you add a MCFG test in tests/bios-tables-test.c? > > > > > > > I took a look into the test, current q35 has already has a reference MCFG in > > tests/data/acpi/q35/MCFG. > > > > And there would be a warning message when reserved[8] is missed. > > > > /x86_64/acpi/q35/bridge: acpi-test: Warning! MCFG mismatch. > > > > Is this enough? Or what more information prefer to add? > > Well, the test has to fail for any mismatch (not a simple warning). > > A mismatch failure seems to be enough IMHO. Warning is sufficient, we do not fail ACPI tests on mismatch. It was a policy decision for APCI tests as far as I remember. We might reconsider it in the future but it shouldn't affect this patch. > > > >>> - AcpiMcfgAllocation allocation[0]; > > >>> -} QEMU_PACKED; > > >>> -typedef struct AcpiTableMcfg AcpiTableMcfg; > > >>> - > > >>> /* > > >>> * TCPA Description Table > > >>> * > > >>> -- > > >>> 2.19.1 > > > > -- > > Wei Yang > > Help you, Help me >
On Thu, May 16, 2019 at 07:00:33PM +0200, Igor Mammedov wrote: > On Thu, 16 May 2019 13:01:31 +0200 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > On Thu, May 16, 2019 at 9:41 AM Wei Yang <richardw.yang@linux.intel.com> wrote: > > > > > > On Wed, May 15, 2019 at 07:29:17AM +0200, Philippe Mathieu-Daudé wrote: > > > > > > > >Thanks Michael for testing... > > > > > > > >Wei, can you add a MCFG test in tests/bios-tables-test.c? > > > > > > > > > > I took a look into the test, current q35 has already has a reference MCFG in > > > tests/data/acpi/q35/MCFG. > > > > > > And there would be a warning message when reserved[8] is missed. > > > > > > /x86_64/acpi/q35/bridge: acpi-test: Warning! MCFG mismatch. > > > > > > Is this enough? Or what more information prefer to add? > > > > Well, the test has to fail for any mismatch (not a simple warning). > > > > A mismatch failure seems to be enough IMHO. > Warning is sufficient, we do not fail ACPI tests on mismatch. > It was a policy decision for APCI tests as far as I remember. > We might reconsider it in the future but it shouldn't affect this patch. Yes. And the reason is that conflicts in binary expected files are impossible to resolve. So it's important that we can fix expected files after a patch that changes them. I actually have an idea for a better way to fix this: a special list of "warn on mismatch" files. A patch changing tables will add the changed tables to the list. Then maintainer knows to inspec the diff manually and re-generate expected files, and remove the changed tables from the list. Another thing we should do is drop dependency on IASL: if IASL is present we should use it to show diff to simplify debugging but at this point a verbatim difference is good enough if IASL is not installed. And I agree 100%: all this is a subject for a separate patch(set). > > > > > > >>> - AcpiMcfgAllocation allocation[0]; > > > >>> -} QEMU_PACKED; > > > >>> -typedef struct AcpiTableMcfg AcpiTableMcfg; > > > >>> - > > > >>> /* > > > >>> * TCPA Description Table > > > >>> * > > > >>> -- > > > >>> 2.19.1 > > > > > > -- > > > Wei Yang > > > Help you, Help me > >
diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c index fa0fa30bb9..341805e786 100644 --- a/hw/acpi/pci.c +++ b/hw/acpi/pci.c @@ -30,17 +30,26 @@ 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. + */ + /* 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 *