On Mon, 17 Dec 2018 11:48:37 +0100
Samuel Ortiz <sa...@linux.intel.com> wrote:
> Now that build_rsdp() supports building both legacy and current RSDP
> tables, we can move it to a generic folder (hw/acpi) and have the i386
> ACPI code reuse it in order to reduce code duplication.
>
> Signed-off-by: Samuel Ortiz <sa...@linux.intel.com>
> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com>
> Reviewed-by: Andrew Jones <drjo...@redhat.com>
> Reviewed-by: Igor Mammedov <imamm...@redhat.com>
for future reference, if one changes patch in a significant way,
one is supposed to drop Tested/Reviewed-by tags so that reviewers
would look at it again and we by mistake won't merge not actually
reviewed changes.
[...]
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fb877648ac..846cb6d755 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2547,35 +2547,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
> "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
> }
>
> -static void
> -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
> -{
> - /* AcpiRsdpDescriptor describes revision 2 RSDP table and as result we
> - * allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep extra 16
> bytes
> - * wasted to make sure we won't breake migration for machine types older
> - * than 2.3 due to size mismatch.
> - */
> - 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, 20 /* ACPI rev 1.0 RSDP size */,
> - (char *)&rsdp->checksum - rsdp_table->data);
> -}
> -
> typedef
> struct AcpiBuildState {
> /* Copy of table in RAM (for patching). */
> @@ -2732,7 +2703,25 @@ void acpi_build(AcpiBuildTables *tables, MachineState
> *machine)
> slic_oem.id, slic_oem.table_id);
>
> /* RSDP is in FSEG memory, so allocate it separately */
> - build_rsdp(tables->rsdp, tables->linker, rsdt);
> + {
> + AcpiRsdpData rsdp_data = {
> + .revision = 0,
> + .oem_id = ACPI_BUILD_APPNAME6,
> + .xsdt_tbl_offset = NULL,
> + .rsdt_tbl_offset = &rsdt,
> + };
> + build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> + if (!pcmc->rsdp_in_ram) {
> + /*
> + * Legacy machine types (2.2 and older) expect to get a complete
> + * revision 2 RSDP table, even though they only look at the
not true, rev is set to 0 for pc machines, the point of the original comment was
that we allocate extra 16 bytes but not actually using them and why it's bad
to drop it suddenly.
> + * revision 0 fields (xsdt pointer is not set). So in order to
> + * not break migration to those machine types we waste 16 bytes
> + * that we amend to the RSDP revision 0 structure.
^^^ added
> + */
Perhaps amended original comment would be clearer:
/* We used to allocate extra space for RSDP rev 2 but used only space for
* legacy RSDP and extra bytes were zeroed out. Keep wasting extra 16 bytes
* to make sure we won't breake migration for machine types 2.2 and older
* due to RSDP blob size mismatch.
*/
> + build_append_int_noprefix(tables->rsdp, 0, 16);
> + }
> + }
>
> /* We'll expose it all to Guest so we want to reduce
> * chance of size changes.
With comment fixed:
Reviewed-by: Igor Mammedov <imamm...@redhat.com>
since it's minor fixup and doesn't affect applying remaining patches you can
post
v5 7/8 as reply to v4 7/8
