[Qemu-devel] [PATCH v5 0/2] Extract build_mcfg Part 2

[Qemu-devel] [PATCH v5 0/2] Extract build_mcfg Part 2

[Wei Yang]

This patch set tries to generalize MCFG table build process. Several patches
are already merged and these two are left for the following reasons:

  * conflict with latest upstream
  * ACPI_PCI dependency fix
  * missed reserved[8] in MCFG

v4->v5:
    * ACPI_PCI depends on both ACPI and PCI
    * rebase on latest master, adjust arm Kconfig
    * miss the reserved[8] of MCFG, add it back
    * make sure bios-tables-test all OK

Wei Yang (2):
  hw/acpi: Consolidate build_mcfg to pci.c
  acpi: pci: use build_append_foo() API to construct MCFG

 default-configs/i386-softmmu.mak |  1 +
 hw/acpi/Kconfig                  |  4 +++
 hw/acpi/Makefile.objs            |  1 +
 hw/acpi/pci.c                    | 57 ++++++++++++++++++++++++++++++++
 hw/arm/Kconfig                   |  1 +
 hw/arm/virt-acpi-build.c         | 17 ----------
 hw/i386/acpi-build.c             | 18 +---------
 include/hw/acpi/acpi-defs.h      | 18 ----------
 include/hw/acpi/pci.h            |  1 +
 9 files changed, 66 insertions(+), 52 deletions(-)
 create mode 100644 hw/acpi/pci.c

-- 
2.19.1

[Qemu-devel] [PATCH v5 1/2] hw/acpi: Consolidate build_mcfg to pci.c

[Wei Yang]

Now we have two identical build_mcfg functions.

Consolidate them in acpi/pci.c.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

---
v5:
  * ACPI_PCI depends on both ACPI and PCI
  * rebase on latest master, adjust arm Kconfig
v3:
  * adjust changelog based on Igor's suggestion
---
 default-configs/i386-softmmu.mak |  1 +
 hw/acpi/Kconfig                  |  4 +++
 hw/acpi/Makefile.objs            |  1 +
 hw/acpi/pci.c                    | 46 ++++++++++++++++++++++++++++++++
 hw/arm/Kconfig                   |  1 +
 hw/arm/virt-acpi-build.c         | 17 ------------
 hw/i386/acpi-build.c             | 18 +------------
 include/hw/acpi/pci.h            |  1 +
 8 files changed, 55 insertions(+), 34 deletions(-)
 create mode 100644 hw/acpi/pci.c

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index ba3fb3ff50..cd5ea391e8 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -25,3 +25,4 @@
 CONFIG_ISAPC=y
 CONFIG_I440FX=y
 CONFIG_Q35=y
+CONFIG_ACPI_PCI=y
diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
index eca3beed75..7c59cf900b 100644
--- a/hw/acpi/Kconfig
+++ b/hw/acpi/Kconfig
@@ -23,6 +23,10 @@ config ACPI_NVDIMM
     bool
     depends on ACPI
 
+config ACPI_PCI
+    bool
+    depends on ACPI && PCI
+
 config ACPI_VMGENID
     bool
     default y
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 2d46e3789a..661a9b8c2f 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -11,6 +11,7 @@ common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
 common-obj-y += acpi_interface.o
 common-obj-y += bios-linker-loader.o
 common-obj-y += aml-build.o
+common-obj-$(CONFIG_ACPI_PCI) += pci.o
 common-obj-$(CONFIG_TPM) += tpm.o
 
 common-obj-$(CONFIG_IPMI) += ipmi.o
diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c
new file mode 100644
index 0000000000..fa0fa30bb9
--- /dev/null
+++ b/hw/acpi/pci.c
@@ -0,0 +1,46 @@
+/*
+ * Support for generating PCI related ACPI tables and passing them to Guests
+ *
+ * Copyright (C) 2006 Fabrice Bellard
+ * Copyright (C) 2008-2010  Kevin O'Connor <kevin@koconnor.net>
+ * Copyright (C) 2013-2019 Red Hat Inc
+ * Copyright (C) 2019 Intel Corporation
+ *
+ * Author: Wei Yang <richardw.yang@linux.intel.com>
+ * Author: Michael S. Tsirkin <mst@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/pci.h"
+#include "hw/pci/pcie_host.h"
+
+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);
+}
+
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index af8cffde9c..9aced9d54d 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -19,6 +19,7 @@ config ARM_VIRT
     select PLATFORM_BUS
     select SMBIOS
     select VIRTIO_MMIO
+    select ACPI_PCI
 
 config CHEETAH
     bool
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index e7c96d658e..4a64f9985c 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -546,23 +546,6 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
                  "SRAT", table_data->len - srat_start, 3, NULL, NULL);
 }
 
-static 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);
-}
-
 /* GTDT */
 static void
 build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0d78d73894..85dc1640bc 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2405,22 +2405,6 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
                  table_data->len - srat_start, 1, NULL, NULL);
 }
 
-static void
-build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
-{
-    AcpiTableMcfg *mcfg;
-    int len = sizeof(*mcfg) + 1 * 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);
-}
-
 /*
  * VT-d spec 8.1 DMA Remapping Reporting Structure
  * (version Oct. 2014 or later)
@@ -2690,7 +2674,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     }
     if (acpi_get_mcfg(&mcfg)) {
         acpi_add_table(table_offsets, tables_blob);
-        build_mcfg_q35(tables_blob, tables->linker, &mcfg);
+        build_mcfg(tables_blob, tables->linker, &mcfg);
     }
     if (x86_iommu_get_default()) {
         IommuType IOMMUType = x86_iommu_get_type();
diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h
index 124af7d32a..8bbd32cf45 100644
--- a/include/hw/acpi/pci.h
+++ b/include/hw/acpi/pci.h
@@ -30,4 +30,5 @@ typedef struct AcpiMcfgInfo {
     uint32_t size;
 } AcpiMcfgInfo;
 
+void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info);
 #endif
-- 
2.19.1

Re: [Qemu-devel] [PATCH v5 1/2] hw/acpi: Consolidate build_mcfg to pci.c

[Michael S. Tsirkin]

On Tue, May 21, 2019 at 11:32:48AM +0800, Wei Yang wrote:
> Now we have two identical build_mcfg functions.
> 
> Consolidate them in acpi/pci.c.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> 
> ---
> v5:
>   * ACPI_PCI depends on both ACPI and PCI
>   * rebase on latest master, adjust arm Kconfig
> v3:
>   * adjust changelog based on Igor's suggestion
> ---


same as 2/2 - do not use two --- separators pls.

>  default-configs/i386-softmmu.mak |  1 +
>  hw/acpi/Kconfig                  |  4 +++
>  hw/acpi/Makefile.objs            |  1 +
>  hw/acpi/pci.c                    | 46 ++++++++++++++++++++++++++++++++
>  hw/arm/Kconfig                   |  1 +
>  hw/arm/virt-acpi-build.c         | 17 ------------
>  hw/i386/acpi-build.c             | 18 +------------
>  include/hw/acpi/pci.h            |  1 +
>  8 files changed, 55 insertions(+), 34 deletions(-)
>  create mode 100644 hw/acpi/pci.c
> 
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index ba3fb3ff50..cd5ea391e8 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -25,3 +25,4 @@
>  CONFIG_ISAPC=y
>  CONFIG_I440FX=y
>  CONFIG_Q35=y
> +CONFIG_ACPI_PCI=y
> diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> index eca3beed75..7c59cf900b 100644
> --- a/hw/acpi/Kconfig
> +++ b/hw/acpi/Kconfig
> @@ -23,6 +23,10 @@ config ACPI_NVDIMM
>      bool
>      depends on ACPI
>  
> +config ACPI_PCI
> +    bool
> +    depends on ACPI && PCI
> +
>  config ACPI_VMGENID
>      bool
>      default y
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 2d46e3789a..661a9b8c2f 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -11,6 +11,7 @@ common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>  common-obj-y += acpi_interface.o
>  common-obj-y += bios-linker-loader.o
>  common-obj-y += aml-build.o
> +common-obj-$(CONFIG_ACPI_PCI) += pci.o
>  common-obj-$(CONFIG_TPM) += tpm.o
>  
>  common-obj-$(CONFIG_IPMI) += ipmi.o
> diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c
> new file mode 100644
> index 0000000000..fa0fa30bb9
> --- /dev/null
> +++ b/hw/acpi/pci.c
> @@ -0,0 +1,46 @@
> +/*
> + * Support for generating PCI related ACPI tables and passing them to Guests
> + *
> + * Copyright (C) 2006 Fabrice Bellard
> + * Copyright (C) 2008-2010  Kevin O'Connor <kevin@koconnor.net>
> + * Copyright (C) 2013-2019 Red Hat Inc
> + * Copyright (C) 2019 Intel Corporation
> + *
> + * Author: Wei Yang <richardw.yang@linux.intel.com>
> + * Author: Michael S. Tsirkin <mst@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/pci.h"
> +#include "hw/pci/pcie_host.h"
> +
> +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);
> +}
> +
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index af8cffde9c..9aced9d54d 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -19,6 +19,7 @@ config ARM_VIRT
>      select PLATFORM_BUS
>      select SMBIOS
>      select VIRTIO_MMIO
> +    select ACPI_PCI
>  
>  config CHEETAH
>      bool
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index e7c96d658e..4a64f9985c 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -546,23 +546,6 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>                   "SRAT", table_data->len - srat_start, 3, NULL, NULL);
>  }
>  
> -static 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);
> -}
> -
>  /* GTDT */
>  static void
>  build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 0d78d73894..85dc1640bc 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2405,22 +2405,6 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
>                   table_data->len - srat_start, 1, NULL, NULL);
>  }
>  
> -static void
> -build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
> -{
> -    AcpiTableMcfg *mcfg;
> -    int len = sizeof(*mcfg) + 1 * 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);
> -}
> -
>  /*
>   * VT-d spec 8.1 DMA Remapping Reporting Structure
>   * (version Oct. 2014 or later)
> @@ -2690,7 +2674,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>      }
>      if (acpi_get_mcfg(&mcfg)) {
>          acpi_add_table(table_offsets, tables_blob);
> -        build_mcfg_q35(tables_blob, tables->linker, &mcfg);
> +        build_mcfg(tables_blob, tables->linker, &mcfg);
>      }
>      if (x86_iommu_get_default()) {
>          IommuType IOMMUType = x86_iommu_get_type();
> diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h
> index 124af7d32a..8bbd32cf45 100644
> --- a/include/hw/acpi/pci.h
> +++ b/include/hw/acpi/pci.h
> @@ -30,4 +30,5 @@ typedef struct AcpiMcfgInfo {
>      uint32_t size;
>  } AcpiMcfgInfo;
>  
> +void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info);
>  #endif
> -- 
> 2.19.1

Re: [Qemu-devel] [PATCH v5 1/2] hw/acpi: Consolidate build_mcfg to pci.c

[Igor Mammedov]

On Mon, 20 May 2019 23:37:46 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, May 21, 2019 at 11:32:48AM +0800, Wei Yang wrote:
> > Now we have two identical build_mcfg functions.
> > 
> > Consolidate them in acpi/pci.c.
> > 
> > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> > 
> > ---
> > v5:
> >   * ACPI_PCI depends on both ACPI and PCI
> >   * rebase on latest master, adjust arm Kconfig
> > v3:
> >   * adjust changelog based on Igor's suggestion
> > ---  
> 
> 
> same as 2/2 - do not use two --- separators pls.

I don't really get this requirement, it's common practice on
list(s) and used to work just fine.

The 1st separator is added by hand when editing commit message
to separate change log from main commit message so that 'git am'
would drop change log. While the second separator is added
automatically by git when patch is created.

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

[Wei Yang]

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);
+    /* 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

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

[Michael S. Tsirkin]

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

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

[Igor Mammedov]

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  

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

[Michael S. Tsirkin]

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  

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

[Igor Mammedov]

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    

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

[Michael S. Tsirkin]

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

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

[Wei Yang]

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

-- 
Wei Yang
Help you, Help me