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" <m...@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.y...@linux.intel.com>
> > > Suggested-by: Igor Mammedov <imamm...@redhat.com>
> > > Reviewed-by: Igor Mammedov <imamm...@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.y...@linux.intel.com>
Suggested-by: Igor Mammedov <imamm...@redhat.com>
Reviewed-by: Igor Mammedov <imamm...@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
