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

["Michael S. Tsirkin" <mst@redhat.com>]

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