Re: [Qemu-devel] [PATCH v3] pc: acpi-build: make linker & RSDP tables dynamic

[Marcel Apfelbaum <marcel@redhat.com>]

On 02/04/2015 11:01 AM, Igor Mammedov wrote:
> Linker and RSDP tables are build only once, so if later
> during rebuild sizes of other ACPI tables change
> pointers will be patched incorrectly due to wrong
> offsets in RSDP and linker.
>
> To fix it rebuild linker and RSDP tables along with
> the rest of ACPI tables so that they would have
> offsets that match just built tables.
>
> Here is a simple reproducer:
>   1: hotplug bridge using command:
>       device_add pci-bridge,chassis_nr=1
>   2: reset system from monitor:
>       system_reset
>
> As result pointers to ACPI tables are not correct
> and guest can't read/parse ACPI tables and on top
> of it linker corrupted them by patching at stale
> offsets.
>
> Windows guests just refuses to boot and
> Linux guests are more resilient and try to boot without
> ACPI, sometimes successfully.
>
> Fix applies only to new machine types starting from 2.3,
> so it won't break migration for old machine types.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/i386/acpi-build.c | 27 ++++++++++++++++++++-------
>   hw/i386/pc_piix.c    |  3 +++
>   hw/i386/pc_q35.c     |  3 +++
>   include/hw/i386/pc.h |  1 +
>   4 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4944249..58cf8b7 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1523,6 +1523,10 @@ struct AcpiBuildState {
>       /* Copy of table in RAM (for patching). */
>       ram_addr_t table_ram;
>       uint32_t table_size;
> +    ram_addr_t linker_ram;
> +    uint32_t linker_size;
> +    ram_addr_t rsdp_ram;
> +    uint32_t rsdp_size;
>       /* Is table patched? */
>       uint8_t patched;
>       PcGuestInfo *guest_info;
> @@ -1733,6 +1737,10 @@ static void acpi_build_update(void *build_opaque, uint32_t offset)
>
>       memcpy(qemu_get_ram_ptr(build_state->table_ram), tables.table_data->data,
>              build_state->table_size);
> +    memcpy(qemu_get_ram_ptr(build_state->linker_ram), tables.linker->data,
> +           build_state->linker_size);
> +    memcpy(qemu_get_ram_ptr(build_state->rsdp_ram), tables.rsdp->data,
> +           build_state->rsdp_size);
>
>       cpu_physical_memory_set_dirty_range_nocode(build_state->table_ram,
>                                                  build_state->table_size);
> @@ -1799,17 +1807,22 @@ void acpi_setup(PcGuestInfo *guest_info)
>       assert(build_state->table_ram != RAM_ADDR_MAX);
>       build_state->table_size = acpi_data_len(tables.table_data);
>
> -    acpi_add_rom_blob(NULL, tables.linker, "etc/table-loader", 0);
> +    build_state->linker_ram = acpi_add_rom_blob(build_state, tables.linker,
> +                                                "etc/table-loader", 0);
> +    build_state->linker_size = acpi_data_len(tables.linker);
>
>       fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>                       tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>
> -    /*
> -     * RSDP is small so it's easy to keep it immutable, no need to
> -     * bother with ROM blobs.
> -     */
> -    fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE,
> -                    tables.rsdp->data, acpi_data_len(tables.rsdp));
> +    if (guest_info->has_imutable_rsdp) {
> +        /* Keep for compatibility with old machine types */
> +        fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE,
> +                        tables.rsdp->data, acpi_data_len(tables.rsdp));
> +    } else {
> +        build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp,
> +                                                  ACPI_BUILD_RSDP_FILE, 0);
> +        build_state->rsdp_size = acpi_data_len(tables.rsdp);
> +    }
>
>       qemu_register_reset(acpi_build_reset, build_state);
>       acpi_build_reset(build_state);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 38b42b0..866b783 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -60,6 +60,7 @@ static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
>   static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
>
>   static bool has_acpi_build = true;
> +static bool has_imutable_rsdp;
imutable -> immutable
Other than that, it looks good and clean to me.

Thanks,
Marcel


>   static int legacy_acpi_table_size;
>   static bool smbios_defaults = true;
>   static bool smbios_legacy_mode;
> @@ -168,6 +169,7 @@ static void pc_init1(MachineState *machine,
>
>       guest_info->isapc_ram_fw = !pci_enabled;
>       guest_info->has_reserved_memory = has_reserved_memory;
> +    guest_info->has_imutable_rsdp = has_imutable_rsdp;
>
>       if (smbios_defaults) {
>           MachineClass *mc = MACHINE_GET_CLASS(machine);
> @@ -310,6 +312,7 @@ static void pc_init_pci(MachineState *machine)
>
>   static void pc_compat_2_2(MachineState *machine)
>   {
> +    has_imutable_rsdp = true;
>       x86_cpu_compat_set_features("kvm64", FEAT_1_EDX, 0, CPUID_VME);
>       x86_cpu_compat_set_features("kvm32", FEAT_1_EDX, 0, CPUID_VME);
>       x86_cpu_compat_set_features("Conroe", FEAT_1_EDX, 0, CPUID_VME);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 63027ee..6f649a1 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -50,6 +50,7 @@
>   #define MAX_SATA_PORTS     6
>
>   static bool has_acpi_build = true;
> +static bool has_imutable_rsdp;
>   static bool smbios_defaults = true;
>   static bool smbios_legacy_mode;
>   static bool smbios_uuid_encoded = true;
> @@ -154,6 +155,7 @@ static void pc_q35_init(MachineState *machine)
>       guest_info->isapc_ram_fw = false;
>       guest_info->has_acpi_build = has_acpi_build;
>       guest_info->has_reserved_memory = has_reserved_memory;
> +    guest_info->has_imutable_rsdp = has_imutable_rsdp;
>
>       /* Migration was not supported in 2.0 for Q35, so do not bother
>        * with this hack (see hw/i386/acpi-build.c).
> @@ -289,6 +291,7 @@ static void pc_q35_init(MachineState *machine)
>
>   static void pc_compat_2_2(MachineState *machine)
>   {
> +    has_imutable_rsdp = true;
>       x86_cpu_compat_set_features("kvm64", FEAT_1_EDX, 0, CPUID_VME);
>       x86_cpu_compat_set_features("kvm32", FEAT_1_EDX, 0, CPUID_VME);
>       x86_cpu_compat_set_features("Conroe", FEAT_1_EDX, 0, CPUID_VME);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 69d9cf8..acc95ea 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -104,6 +104,7 @@ struct PcGuestInfo {
>       int legacy_acpi_table_size;
>       bool has_acpi_build;
>       bool has_reserved_memory;
> +    bool has_imutable_rsdp;
>   };
>
>   /* parallel.c */
>