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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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 <sameo@linux.intel.com>
> ---
>   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
>