[Qemu-devel] [PATCH v4 0/3] pc: acpi-build: make linker & RSDP tables dynamic

[Qemu-devel] [PATCH v4 0/3] pc: acpi-build: make linker & RSDP tables dynamic

[Igor Mammedov]

changes since v3:
  * split out linker changes
  * fix Michael's patch, by passing build_state
    to callback so it would actually do something
  * split out RSDP migration in separate patch
  * s/imutable/immutable/

Patches 1-2: fix reboot issue after bridge hotplug
Patch 3: addresses RSDP reading race migration issue
         for new machine types

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.

Also make sure that new machine types won't see RSDP
changed in the middle of reading duing migration, by
migrating it along with the rest of the tables.

Igor Mammedov (2):
  pc: acpi-build: update linker on guest access
  pc: acpi-build: migrate RSDP table

Michael S. Tsirkin (1):
  acpi: update RSDP on guest access

 hw/i386/acpi-build.c | 33 ++++++++++++++++++++++++---------
 hw/i386/pc_piix.c    |  3 +++
 hw/i386/pc_q35.c     |  3 +++
 include/hw/i386/pc.h |  1 +
 4 files changed, 31 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v4 1/3] acpi: update RSDP on guest access

[Igor Mammedov]

From: "Michael S. Tsirkin" <mst@redhat.com>

RSDT offset can change across reboots and that makes
immutable RSDP, which is build at startup, point to
incorrect place in ACPI table blob. That results in
BIOS corrupting tables and guest OS failing to find
ACPI tables.
We really should have put it in a ROM region, but
we can't change that for old machine types,
let's just set the callback and update it explicitly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * do not forget to pass build_state to callback
    otherwise it's NOP.
---
 hw/i386/acpi-build.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4944249..5b2b017 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1526,6 +1526,7 @@ struct AcpiBuildState {
     /* Is table patched? */
     uint8_t patched;
     PcGuestInfo *guest_info;
+    void *rsdp;
 } AcpiBuildState;
 
 static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
@@ -1660,8 +1661,6 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
 
     /* We'll expose it all to Guest so we want to reduce
      * chance of size changes.
-     * RSDP is small so it's easy to keep it immutable, no need to
-     * bother with alignment.
      *
      * We used to align the tables to 4k, but of course this would
      * too simple to be enough.  4k turned out to be too small an
@@ -1733,6 +1732,7 @@ 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(build_state->rsdp, tables.rsdp->data, acpi_data_len(tables.rsdp));
 
     cpu_physical_memory_set_dirty_range_nocode(build_state->table_ram,
                                                build_state->table_size);
@@ -1805,11 +1805,14 @@ void acpi_setup(PcGuestInfo *guest_info)
                     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.
+     * 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(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE,
-                    tables.rsdp->data, acpi_data_len(tables.rsdp));
+    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;
 
     qemu_register_reset(acpi_build_reset, build_state);
     acpi_build_reset(build_state);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 1/3] acpi: update RSDP on guest access

[Marcel Apfelbaum]

On 02/09/2015 03:59 PM, Igor Mammedov wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
>
> RSDT offset can change across reboots and that makes
> immutable RSDP, which is build at startup, point to
> incorrect place in ACPI table blob. That results in
> BIOS corrupting tables and guest OS failing to find
> ACPI tables.
> We really should have put it in a ROM region, but
> we can't change that for old machine types,
> let's just set the callback and update it explicitly.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>    * do not forget to pass build_state to callback
>      otherwise it's NOP.
> ---
>   hw/i386/acpi-build.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4944249..5b2b017 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1526,6 +1526,7 @@ struct AcpiBuildState {
>       /* Is table patched? */
>       uint8_t patched;
>       PcGuestInfo *guest_info;
> +    void *rsdp;
>   } AcpiBuildState;
>
>   static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
> @@ -1660,8 +1661,6 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>
>       /* We'll expose it all to Guest so we want to reduce
>        * chance of size changes.
> -     * RSDP is small so it's easy to keep it immutable, no need to
> -     * bother with alignment.
>        *
>        * We used to align the tables to 4k, but of course this would
>        * too simple to be enough.  4k turned out to be too small an
> @@ -1733,6 +1732,7 @@ 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(build_state->rsdp, tables.rsdp->data, acpi_data_len(tables.rsdp));
>
>       cpu_physical_memory_set_dirty_range_nocode(build_state->table_ram,
>                                                  build_state->table_size);
> @@ -1805,11 +1805,14 @@ void acpi_setup(PcGuestInfo *guest_info)
>                       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.
> +     * 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(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE,
> -                    tables.rsdp->data, acpi_data_len(tables.rsdp));
> +    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;
>
>       qemu_register_reset(acpi_build_reset, build_state);
>       acpi_build_reset(build_state);
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

[Qemu-devel] [PATCH v4 2/3] pc: acpi-build: update linker on guest access

[Igor Mammedov]

Linker table is build only once, so if later during
tables rebuild sizes of other ACPI tables change
pointers will be patched incorrectly due to wrong
offsets in linker. Resulting in guest not being able
to find ACPI tables.
Fix it by updating 'linker' table with the rest of
tables when firmware reads it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 5b2b017..21ea3db 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1527,6 +1527,8 @@ struct AcpiBuildState {
     uint8_t patched;
     PcGuestInfo *guest_info;
     void *rsdp;
+    ram_addr_t linker_ram;
+    uint32_t linker_size;
 } AcpiBuildState;
 
 static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
@@ -1733,6 +1735,8 @@ 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(build_state->rsdp, tables.rsdp->data, acpi_data_len(tables.rsdp));
+    memcpy(qemu_get_ram_ptr(build_state->linker_ram), tables.linker->data,
+           build_state->linker_size);
 
     cpu_physical_memory_set_dirty_range_nocode(build_state->table_ram,
                                                build_state->table_size);
@@ -1799,7 +1803,9 @@ 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));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4 2/3] pc: acpi-build: update linker on guest access

[Marcel Apfelbaum]

On 02/09/2015 03:59 PM, Igor Mammedov wrote:
> Linker table is build only once, so if later during
> tables rebuild sizes of other ACPI tables change
> pointers will be patched incorrectly due to wrong
> offsets in linker. Resulting in guest not being able
> to find ACPI tables.
> Fix it by updating 'linker' table with the rest of
> tables when firmware reads it.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/i386/acpi-build.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 5b2b017..21ea3db 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1527,6 +1527,8 @@ struct AcpiBuildState {
>       uint8_t patched;
>       PcGuestInfo *guest_info;
>       void *rsdp;
> +    ram_addr_t linker_ram;
> +    uint32_t linker_size;
>   } AcpiBuildState;
>
>   static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
> @@ -1733,6 +1735,8 @@ 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(build_state->rsdp, tables.rsdp->data, acpi_data_len(tables.rsdp));
> +    memcpy(qemu_get_ram_ptr(build_state->linker_ram), tables.linker->data,
> +           build_state->linker_size);
>
>       cpu_physical_memory_set_dirty_range_nocode(build_state->table_ram,
>                                                  build_state->table_size);
> @@ -1799,7 +1803,9 @@ 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));
>


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Re: [Qemu-devel] [PATCH v4 2/3] pc: acpi-build: update linker on guest access

[Michael S. Tsirkin]

On Mon, Feb 09, 2015 at 01:59:54PM +0000, Igor Mammedov wrote:
> Linker table is build only once, so if later during
> tables rebuild sizes of other ACPI tables change
> pointers will be patched incorrectly due to wrong
> offsets in linker. Resulting in guest not being able
> to find ACPI tables.
> Fix it by updating 'linker' table with the rest of
> tables when firmware reads it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/acpi-build.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 5b2b017..21ea3db 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1527,6 +1527,8 @@ struct AcpiBuildState {
>      uint8_t patched;
>      PcGuestInfo *guest_info;
>      void *rsdp;
> +    ram_addr_t linker_ram;
> +    uint32_t linker_size;
>  } AcpiBuildState;
>  
>  static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
> @@ -1733,6 +1735,8 @@ 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(build_state->rsdp, tables.rsdp->data, acpi_data_len(tables.rsdp));
> +    memcpy(qemu_get_ram_ptr(build_state->linker_ram), tables.linker->data,
> +           build_state->linker_size);
>  
>      cpu_physical_memory_set_dirty_range_nocode(build_state->table_ram,
>                                                 build_state->table_size);

OK, but it looks like linker data needs to be marked dirty
as well. I'll send a patch to do this.

> @@ -1799,7 +1803,9 @@ 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));
> -- 
> 1.8.3.1

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

[Igor Mammedov]

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 <imammedo@redhat.com>
---
 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

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

[Marcel Apfelbaum]

On 02/09/2015 03:59 PM, 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 <imammedo@redhat.com>
> ---
>   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 */
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

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 <imammedo@redhat.com>

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