Re: [Qemu-devel] [PATCH v4 3/3] pc: acpi-build: migrate RSDP table

[Michael S. Tsirkin]

On Mon, Feb 09, 2015 at 01:59:55PM +0000, Igor Mammedov wrote:
> Makes sure that RSDP stays the same
> /i.e. matches ACPI tables blob in source/
> if guest is migrated during RSDP reading or
> has been already shadowed by firmware.
> 
> 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 <address@hidden>

This worries me: the RAM block is not marked dirty when we modify it
with memcpy, so might it grow stale?
I would prefer that we give it the same handling as table_ram.
Will send a patch on top.

> ---
>  hw/i386/acpi-build.c | 24 +++++++++++++++---------
>  hw/i386/pc_piix.c    |  3 +++
>  hw/i386/pc_q35.c     |  3 +++
>  include/hw/i386/pc.h |  1 +
>  4 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 21ea3db..1583cca 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1810,15 +1810,21 @@ void acpi_setup(PcGuestInfo *guest_info)
>      fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>  
> -    /*
> -     * Though RSDP is small, its contents isn't immutable, so
> -     * update it along with the rest of tables on guest access.
> -     */
> -    fw_cfg_add_file_callback(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE,
> -                             acpi_build_update, build_state,
> -                             tables.rsdp->data, acpi_data_len(tables.rsdp));
> -
> -    build_state->rsdp = tables.rsdp->data;
> +    if (guest_info->has_immutable_rsdp) {
> +        /*
> +         * Keep for compatibility with old machine types.
> +         * Though RSDP is small, its contents isn't immutable, so
> +         * update it along with the rest of tables on guest access.
> +         */
> +        fw_cfg_add_file_callback(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE,
> +                                 acpi_build_update, build_state,
> +                                 tables.rsdp->data, 
> acpi_data_len(tables.rsdp));
> +        build_state->rsdp = tables.rsdp->data;
> +    } else {
> +        build_state->rsdp = qemu_get_ram_ptr(
> +            acpi_add_rom_blob(build_state, tables.rsdp, 
> ACPI_BUILD_RSDP_FILE, 0)
> +        );
> +    }
>  
>      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..e586c7b 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_immutable_rsdp;
>  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_immutable_rsdp = has_immutable_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_immutable_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..6151f2f 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_immutable_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_immutable_rsdp = has_immutable_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_immutable_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..b0a80cf 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_immutable_rsdp;
>  };
>  
>  /* parallel.c */
> -- 
> 1.8.3.1