qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [Qemu-devel] [PATCH v4 03/23] hw: acpi: Export the RSDP b


From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-arm] [Qemu-devel] [PATCH v4 03/23] hw: acpi: Export the RSDP build API
Date: Thu, 1 Nov 2018 18:50:41 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

Hey Samuel,

On 1/11/18 11:22, Samuel Ortiz wrote:
The hardware-reduced API will need to build RSDP as well, so we should
export this routine. While doing so, we also slightly change the
function prototype. Since no caller needs it, and to make it more
consistent with the rest of the AML build API, the function no longer
returns its RSDP table.

Signed-off-by: Samuel Ortiz <address@hidden>
---
  hw/acpi/aml-build.c         | 24 ++++++++++++++++++++++++
  hw/arm/virt-acpi-build.c    |  2 +-

When you modify files from architectures you are not custom to use, you should verify your changes don't break them ;)

  hw/i386/acpi-build.c        | 26 --------------------------
  include/hw/acpi/aml-build.h |  3 +++
  4 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 51b608432f..2d6f538f9d 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1651,6 +1651,30 @@ build_xsdt(GArray *table_data, BIOSLinker *linker, 
GArray *table_offsets,
                   (void *)xsdt, "XSDT", xsdt_len, 1, oem_id, oem_table_id);
  }
+void
+build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
+{
+    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
+    unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
+    unsigned rsdt_pa_offset =
+        (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
+
+    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
+                             true /* fseg memory */);
+
+    memcpy(&rsdp->signature, "RSD PTR ", 8);
+    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
+    /* Address to be filled by Guest linker */
+    bios_linker_loader_add_pointer(linker,
+        ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
+        ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
+
+    /* Checksum to be filled by Guest linker */
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
+        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
+        (char *)&rsdp->checksum - rsdp_table->data);
+}
+
  void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
                         uint64_t len, int node, MemoryAffinityFlags flags)
  {
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f28a2faa53..0ed132b79b 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -367,7 +367,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
  }
/* RSDP */
-static GArray *
+static void
  build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
  {

Since this function doesn't return anything now, you can simply remove the last line to avoid:

hw/arm/virt-acpi-build.c: In function ‘build_rsdp’:
hw/arm/virt-acpi-build.c:396:12: error: ‘return’ with a value, in function returning void [-Werror]
     return rsdp_table;
            ^~~~~~~~~~

(I used 'make subdir-aarch64-softmmu' to build).

Also since you declare the prototype in "hw/acpi/aml-build.h", you have to remove the 'static' keyword:

hw/arm/virt-acpi-build.c:371:1: error: static declaration of ‘build_rsdp’ follows non-static declaration build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
 ^~~~~~~~~~
hw/arm/virt-acpi-build.c:42:
include/hw/acpi/aml-build.h:393:1: note: previous declaration of ‘build_rsdp’ was here
 build_rsdp(GArray *table_data,
 ^~~~~~~~~~

Maybe I was not clear enough in my previous review, I suggest doing this in 3 steps:

1: both functions return void (still static)
2: expose the prototype declaration in "hw/acpi/aml-build.h" (remove static keyword)
3: unify codebase moving build_rsdp() to hw/acpi/aml-build.c

steps 1+2 could be merged, but IMHO 3 steps are cleaner.

      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 81d98fa34f..d7b47e05b5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2513,32 +2513,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
                   "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
  }
-static GArray *
-build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
-{
-    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
-    unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
-    unsigned rsdt_pa_offset =
-        (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
-
-    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
-                             true /* fseg memory */);
-
-    memcpy(&rsdp->signature, "RSD PTR ", 8);
-    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
-    /* Address to be filled by Guest linker */
-    bios_linker_loader_add_pointer(linker,
-        ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
-        ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
-
-    /* Checksum to be filled by Guest linker */
-    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
-        (char *)&rsdp->checksum - rsdp_table->data);
-
-    return rsdp_table;
-}
-
  static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
  {
      Object *pci_host;
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 73fc6659f2..c9bcb32d81 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -390,6 +390,9 @@ void acpi_add_table(GArray *table_offsets, GArray 
*table_data);
  void acpi_build_tables_init(AcpiBuildTables *tables);
  void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
  void
+build_rsdp(GArray *table_data,
+           BIOSLinker *linker, unsigned rsdt_tbl_offset);
+void
  build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
             const char *oem_id, const char *oem_table_id);
  void




reply via email to

[Prev in Thread] Current Thread [Next in Thread]