Re: [Qemu-devel] [PATCH v5 2/2] acpi: pci: use build_append_foo() API to construct MCFG

[Igor Mammedov <imammedo@redhat.com>]

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