[v5,2/2] acpi: pci: use build_append_foo() API to construct MCFG
diff mbox series

Message ID 20190521033249.1960-3-richardw.yang@linux.intel.com
State New
Headers show
Series
  • Extract build_mcfg Part 2
Related show

Commit Message

Wei Yang May 21, 2019, 3:32 a.m. UTC
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(-)

Comments

Michael S. Tsirkin May 21, 2019, 3:37 a.m. UTC | #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
> ---


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
Michael S. Tsirkin May 21, 2019, 3:44 a.m. UTC | #2
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
Wei Yang May 21, 2019, 6:20 a.m. UTC | #3
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
Igor Mammedov May 21, 2019, 2:29 p.m. UTC | #4
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
Michael S. Tsirkin May 21, 2019, 2:34 p.m. UTC | #5
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
Igor Mammedov May 22, 2019, 8:20 a.m. UTC | #6
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

Patch
diff mbox series

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
  *