[Qemu-devel] [PATCH 0/2] pc: fix ACPI tables corruption after bridge hotplug

[Qemu-devel] [PATCH 0/2] pc: fix ACPI tables corruption after bridge hotplug

[Igor Mammedov]

Reproducer:
  1: hotplug bridge using command:
         device_add pci-bridge,chassis_nr=1

  2: reset system from monitor:
         system_reset

  As result:
    Windows doesn't boot and linux boots without ACPI
    due to not readble/corrupted ACPI tables

See patch 2/2 for more details.


Igor Mammedov (2):
  pc: Add pc-[i440fx|q35]-2.3 machine type and pc_compat_2_2() function
  pc: acpi-build: make linker & RSDP tables dynamic

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

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] pc: Add pc-[i440fx|q35]-2.3 machine type and pc_compat_2_2() function

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc_piix.c | 30 ++++++++++++++++++++++++++----
 hw/i386/pc_q35.c  | 27 ++++++++++++++++++++++++---
 2 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 85ed3c8..685fa54 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -308,9 +308,16 @@ static void pc_init_pci(MachineState *machine)
     pc_init1(machine, 1, 1);
 }
 
+static void pc_compat_2_2(MachineState *machine)
+{
+}
+
 static void pc_compat_2_1(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
+
+    pc_compat_2_2(machine);
+
     smbios_uuid_encoded = false;
     x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
     x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
@@ -385,6 +392,12 @@ static void pc_compat_1_2(MachineState *machine)
     x86_cpu_compat_kvm_no_autoenable(FEAT_KVM, KVM_FEATURE_PV_EOI);
 }
 
+static void pc_init_pci_2_2(MachineState *machine)
+{
+    pc_compat_2_2(machine);
+    pc_init_pci(machine);
+}
+
 static void pc_init_pci_2_1(MachineState *machine)
 {
     pc_compat_2_1(machine);
@@ -478,19 +491,27 @@ static void pc_xen_hvm_init(MachineState *machine)
     .desc = "Standard PC (i440FX + PIIX, 1996)", \
     .hot_add_cpu = pc_hot_add_cpu
 
-#define PC_I440FX_2_2_MACHINE_OPTIONS                           \
+#define PC_I440FX_2_3_MACHINE_OPTIONS                           \
     PC_I440FX_MACHINE_OPTIONS,                                  \
     .default_machine_opts = "firmware=bios-256k.bin",           \
     .default_display = "std"
 
-static QEMUMachine pc_i440fx_machine_v2_2 = {
-    PC_I440FX_2_2_MACHINE_OPTIONS,
-    .name = "pc-i440fx-2.2",
+static QEMUMachine pc_i440fx_machine_v2_3 = {
+    PC_I440FX_2_3_MACHINE_OPTIONS,
+    .name = "pc-i440fx-2.3",
     .alias = "pc",
     .init = pc_init_pci,
     .is_default = 1,
 };
 
+#define PC_I440FX_2_2_MACHINE_OPTIONS PC_I440FX_2_3_MACHINE_OPTIONS
+
+static QEMUMachine pc_i440fx_machine_v2_2 = {
+    PC_I440FX_2_2_MACHINE_OPTIONS,
+    .name = "pc-i440fx-2.2",
+    .init = pc_init_pci_2_2,
+};
+
 #define PC_I440FX_2_1_MACHINE_OPTIONS                           \
     PC_I440FX_MACHINE_OPTIONS,                                  \
     .default_machine_opts = "firmware=bios-256k.bin"
@@ -928,6 +949,7 @@ static QEMUMachine xenfv_machine = {
 
 static void pc_machine_init(void)
 {
+    qemu_register_pc_machine(&pc_i440fx_machine_v2_3);
     qemu_register_pc_machine(&pc_i440fx_machine_v2_2);
     qemu_register_pc_machine(&pc_i440fx_machine_v2_1);
     qemu_register_pc_machine(&pc_i440fx_machine_v2_0);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0262b5e..121f620 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -287,10 +287,16 @@ static void pc_q35_init(MachineState *machine)
     }
 }
 
+static void pc_compat_2_2(MachineState *machine)
+{
+}
+
 static void pc_compat_2_1(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
 
+    pc_compat_2_2(machine);
+
     pcms->enforce_aligned_dimm = false;
     smbios_uuid_encoded = false;
     x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
@@ -334,6 +340,12 @@ static void pc_compat_1_4(MachineState *machine)
     x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ);
 }
 
+static void pc_q35_init_2_2(MachineState *machine)
+{
+    pc_compat_2_2(machine);
+    pc_q35_init(machine);
+}
+
 static void pc_q35_init_2_1(MachineState *machine)
 {
     pc_compat_2_1(machine);
@@ -377,16 +389,24 @@ static void pc_q35_init_1_4(MachineState *machine)
     .hot_add_cpu = pc_hot_add_cpu, \
     .units_per_default_bus = 1
 
-#define PC_Q35_2_2_MACHINE_OPTIONS                      \
+#define PC_Q35_2_3_MACHINE_OPTIONS                      \
     PC_Q35_MACHINE_OPTIONS,                             \
     .default_machine_opts = "firmware=bios-256k.bin",   \
     .default_display = "std"
 
+static QEMUMachine pc_q35_machine_v2_3 = {
+    PC_Q35_2_3_MACHINE_OPTIONS,
+    .name = "pc-q35-2.3",
+    .alias = "q35",
+    .init = pc_q35_init,
+};
+
+#define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
+
 static QEMUMachine pc_q35_machine_v2_2 = {
     PC_Q35_2_2_MACHINE_OPTIONS,
     .name = "pc-q35-2.2",
-    .alias = "q35",
-    .init = pc_q35_init,
+    .init = pc_q35_init_2_2,
 };
 
 #define PC_Q35_2_1_MACHINE_OPTIONS                      \
@@ -465,6 +485,7 @@ static QEMUMachine pc_q35_machine_v1_4 = {
 
 static void pc_q35_machine_init(void)
 {
+    qemu_register_pc_machine(&pc_q35_machine_v2_3);
     qemu_register_pc_machine(&pc_q35_machine_v2_2);
     qemu_register_pc_machine(&pc_q35_machine_v2_1);
     qemu_register_pc_machine(&pc_q35_machine_v2_0);
-- 
1.8.3.1

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

[Igor Mammedov]

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.

To fix it rebuild linker and RSDP tables along with
the rest of ACPI tables so that they would have correct
offsets.

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.

Windows guests just refuse to boot and
Linux guests are more resilient and try to boot without
ACPI, sometimes successfully.

PS:
 keep brokenness in 2.2 and older machine types for the sake
 of migration

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Tested backwards migration from QEMU 2.3 to 2.2 with
pc-i440fx-2.2 machine type

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b37a397..4d2452d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1509,6 +1509,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;
@@ -1714,6 +1718,10 @@ static void acpi_build_update(void *build_opaque, uint32_t offset)
     assert(acpi_data_len(tables.table_data) == build_state->table_size);
     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);
@@ -1779,17 +1787,25 @@ 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");
+    build_state->linker_ram = acpi_add_rom_blob(build_state, tables.linker,
+                                                "etc/table-loader");
+    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) {
+        /*
+         * 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));
+    } else {
+        build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp,
+                                                  ACPI_BUILD_RSDP_FILE);
+        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 685fa54..69d4de2 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;
 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;
 }
 
 static void pc_compat_2_1(MachineState *machine)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 121f620..6bbc100 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;
 }
 
 static void pc_compat_2_1(MachineState *machine)
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 */
-- 
1.8.3.1

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

[Michael S. Tsirkin]

On Wed, Dec 10, 2014 at 04:06:56PM +0000, 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.
> 
> To fix it rebuild linker and RSDP tables along with
> the rest of ACPI tables so that they would have correct
> offsets.
> 
> 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.
> Windows guests just refuse to boot and
> Linux guests are more resilient and try to boot without
> ACPI, sometimes successfully.
> 
> PS:
>  keep brokenness in 2.2 and older machine types for the sake
>  of migration
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I'd like to fix 2.2 as well.

Why does adding bridge in this way cause tables to
be resized?
I think this is a root-cause that should be fixed.

> ---
> Tested backwards migration from QEMU 2.3 to 2.2 with
> pc-i440fx-2.2 machine type
> 
>  hw/i386/acpi-build.c | 30 +++++++++++++++++++++++-------
>  hw/i386/pc_piix.c    |  3 +++
>  hw/i386/pc_q35.c     |  3 +++
>  include/hw/i386/pc.h |  1 +
>  4 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b37a397..4d2452d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1509,6 +1509,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;
> @@ -1714,6 +1718,10 @@ static void acpi_build_update(void *build_opaque, uint32_t offset)
>      assert(acpi_data_len(tables.table_data) == build_state->table_size);
>      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);
> @@ -1779,17 +1787,25 @@ 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");
> +    build_state->linker_ram = acpi_add_rom_blob(build_state, tables.linker,
> +                                                "etc/table-loader");
> +    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) {
> +        /*
> +         * 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));
> +    } else {
> +        build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp,
> +                                                  ACPI_BUILD_RSDP_FILE);
> +        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 685fa54..69d4de2 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;
>  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;
>  }
>  
>  static void pc_compat_2_1(MachineState *machine)
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 121f620..6bbc100 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;
>  }
>  
>  static void pc_compat_2_1(MachineState *machine)
> 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 */
> -- 
> 1.8.3.1

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

[Igor Mammedov]

On Wed, 10 Dec 2014 18:24:24 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Dec 10, 2014 at 04:06:56PM +0000, 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.
> > 
> > To fix it rebuild linker and RSDP tables along with
> > the rest of ACPI tables so that they would have correct
> > offsets.
> > 
> > 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.
> > Windows guests just refuse to boot and
> > Linux guests are more resilient and try to boot without
> > ACPI, sometimes successfully.
> > 
> > PS:
> >  keep brokenness in 2.2 and older machine types for the sake
> >  of migration
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> I'd like to fix 2.2 as well.
that's easy,
 has_imutable_rsdp = true
should be moved into
 pc_compat_2_1()

> 
> Why does adding bridge in this way cause tables to
> be resized?
> I think this is a root-cause that should be fixed.
That's because there wasn't any bridge described in tables
on startup, but after hotplug and reset tables are rebuild
which pickups hotplugged bridge and its secondary PCI bus.

This behavior seems sane, I'd expect bare-metal behave
the same way.

> 
> > ---
> > Tested backwards migration from QEMU 2.3 to 2.2 with
> > pc-i440fx-2.2 machine type
> > 
> >  hw/i386/acpi-build.c | 30 +++++++++++++++++++++++-------
> >  hw/i386/pc_piix.c    |  3 +++
> >  hw/i386/pc_q35.c     |  3 +++
> >  include/hw/i386/pc.h |  1 +
> >  4 files changed, 30 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index b37a397..4d2452d 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1509,6 +1509,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;
> > @@ -1714,6 +1718,10 @@ static void acpi_build_update(void *build_opaque, uint32_t offset)
> >      assert(acpi_data_len(tables.table_data) == build_state->table_size);
> >      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);
> > @@ -1779,17 +1787,25 @@ 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");
> > +    build_state->linker_ram = acpi_add_rom_blob(build_state, tables.linker,
> > +                                                "etc/table-loader");
> > +    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) {
> > +        /*
> > +         * 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));
> > +    } else {
> > +        build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp,
> > +                                                  ACPI_BUILD_RSDP_FILE);
> > +        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 685fa54..69d4de2 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;
> >  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;
> >  }
> >  
> >  static void pc_compat_2_1(MachineState *machine)
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 121f620..6bbc100 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;
> >  }
> >  
> >  static void pc_compat_2_1(MachineState *machine)
> > 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 */
> > -- 
> > 1.8.3.1

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

[Michael S. Tsirkin]

On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote:
> On Wed, 10 Dec 2014 18:24:24 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Dec 10, 2014 at 04:06:56PM +0000, 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.
> > > 
> > > To fix it rebuild linker and RSDP tables along with
> > > the rest of ACPI tables so that they would have correct
> > > offsets.
> > > 
> > > 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.
> > > Windows guests just refuse to boot and
> > > Linux guests are more resilient and try to boot without
> > > ACPI, sometimes successfully.
> > > 
> > > PS:
> > >  keep brokenness in 2.2 and older machine types for the sake
> > >  of migration
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > I'd like to fix 2.2 as well.
> that's easy,
>  has_imutable_rsdp = true
> should be moved into
>  pc_compat_2_1()
> 
> > 
> > Why does adding bridge in this way cause tables to
> > be resized?
> > I think this is a root-cause that should be fixed.
> That's because there wasn't any bridge described in tables
> on startup, but after hotplug and reset tables are rebuild
> which pickups hotplugged bridge and its secondary PCI bus.
> 
> This behavior seems sane, I'd expect bare-metal behave
> the same way.

For a simple fix, we can skip bridges added by hotplug -
treat them as regular devices.

It defintely seems nicer - at least for old machine types -
than "keep brokenness".

I think the following should do the trcik, but it's completely
untested.
Could you pls try?


diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b37a397..039776d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
      * Skip bridge subtree creation if bridge hotplug is disabled
      * to make acpi tables compatible with legacy machine types.
      */
-    if (!child->pcihp_bridge_en && bus->parent_dev) {
+    if (bus->parent_dev && (!child->pcihp_bridge_en ||
+                            !DEVICE(bus->parent_dev)->hotplugged)) {
         return;
     }
 

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

[Igor Mammedov]

On Wed, 10 Dec 2014 19:13:59 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote:
> > On Wed, 10 Dec 2014 18:24:24 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Dec 10, 2014 at 04:06:56PM +0000, 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.
> > > > 
> > > > To fix it rebuild linker and RSDP tables along with
> > > > the rest of ACPI tables so that they would have correct
> > > > offsets.
> > > > 
> > > > 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.
> > > > Windows guests just refuse to boot and
> > > > Linux guests are more resilient and try to boot without
> > > > ACPI, sometimes successfully.
> > > > 
> > > > PS:
> > > >  keep brokenness in 2.2 and older machine types for the sake
> > > >  of migration
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > I'd like to fix 2.2 as well.
> > that's easy,
> >  has_imutable_rsdp = true
> > should be moved into
> >  pc_compat_2_1()
> > 
> > > 
> > > Why does adding bridge in this way cause tables to
> > > be resized?
> > > I think this is a root-cause that should be fixed.
> > That's because there wasn't any bridge described in tables
> > on startup, but after hotplug and reset tables are rebuild
> > which pickups hotplugged bridge and its secondary PCI bus.
> > 
> > This behavior seems sane, I'd expect bare-metal behave
> > the same way.
> 
> For a simple fix, we can skip bridges added by hotplug -
> treat them as regular devices.
> 
> It defintely seems nicer - at least for old machine types -
> than "keep brokenness".
> 
> I think the following should do the trcik, but it's completely
> untested.
> Could you pls try?
> 
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b37a397..039776d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>       * Skip bridge subtree creation if bridge hotplug is disabled
>       * to make acpi tables compatible with legacy machine types.
>       */
> -    if (!child->pcihp_bridge_en && bus->parent_dev) {
> +    if (bus->parent_dev && (!child->pcihp_bridge_en ||
> +                            !DEVICE(bus->parent_dev)->hotplugged)) {
>          return;
>      }
What will happen if
hotplug may bridges in
 QEMU-fixed^^ -M 2.1
migrate to target:
 QEMU-2.1 -M 2.1 + hotplugged on sourcebridges on CLI

My guess it may fail due to ACPI blob size difference.

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

[Michael S. Tsirkin]

On Wed, Dec 10, 2014 at 06:39:49PM +0100, Igor Mammedov wrote:
> On Wed, 10 Dec 2014 19:13:59 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote:
> > > On Wed, 10 Dec 2014 18:24:24 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Dec 10, 2014 at 04:06:56PM +0000, 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.
> > > > > 
> > > > > To fix it rebuild linker and RSDP tables along with
> > > > > the rest of ACPI tables so that they would have correct
> > > > > offsets.
> > > > > 
> > > > > 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.
> > > > > Windows guests just refuse to boot and
> > > > > Linux guests are more resilient and try to boot without
> > > > > ACPI, sometimes successfully.
> > > > > 
> > > > > PS:
> > > > >  keep brokenness in 2.2 and older machine types for the sake
> > > > >  of migration
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > 
> > > > I'd like to fix 2.2 as well.
> > > that's easy,
> > >  has_imutable_rsdp = true
> > > should be moved into
> > >  pc_compat_2_1()
> > > 
> > > > 
> > > > Why does adding bridge in this way cause tables to
> > > > be resized?
> > > > I think this is a root-cause that should be fixed.
> > > That's because there wasn't any bridge described in tables
> > > on startup, but after hotplug and reset tables are rebuild
> > > which pickups hotplugged bridge and its secondary PCI bus.
> > > 
> > > This behavior seems sane, I'd expect bare-metal behave
> > > the same way.
> > 
> > For a simple fix, we can skip bridges added by hotplug -
> > treat them as regular devices.
> > 
> > It defintely seems nicer - at least for old machine types -
> > than "keep brokenness".
> > 
> > I think the following should do the trcik, but it's completely
> > untested.
> > Could you pls try?
> > 
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index b37a397..039776d 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >       * Skip bridge subtree creation if bridge hotplug is disabled
> >       * to make acpi tables compatible with legacy machine types.
> >       */
> > -    if (!child->pcihp_bridge_en && bus->parent_dev) {
> > +    if (bus->parent_dev && (!child->pcihp_bridge_en ||
> > +                            !DEVICE(bus->parent_dev)->hotplugged)) {
> >          return;
> >      }
> What will happen if
> hotplug may bridges in
>  QEMU-fixed^^ -M 2.1
> migrate to target:
>  QEMU-2.1 -M 2.1 + hotplugged on sourcebridges on CLI
> 
> My guess it may fail due to ACPI blob size difference.

Yes it might if we are unlucky and cross a 64 k boundary.
The robust fix is still to resize ROMs on receive.
We'll also put this patch on a stable branch.

-- 
MST

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

[Igor Mammedov]

On Wed, 10 Dec 2014 20:28:01 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Dec 10, 2014 at 06:39:49PM +0100, Igor Mammedov wrote:
> > On Wed, 10 Dec 2014 19:13:59 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote:
> > > > On Wed, 10 Dec 2014 18:24:24 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Wed, Dec 10, 2014 at 04:06:56PM +0000, 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.
> > > > > > 
> > > > > > To fix it rebuild linker and RSDP tables along with
> > > > > > the rest of ACPI tables so that they would have correct
> > > > > > offsets.
> > > > > > 
> > > > > > 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.
> > > > > > Windows guests just refuse to boot and
> > > > > > Linux guests are more resilient and try to boot without
> > > > > > ACPI, sometimes successfully.
> > > > > > 
> > > > > > PS:
> > > > > >  keep brokenness in 2.2 and older machine types for the sake
> > > > > >  of migration
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > 
> > > > > I'd like to fix 2.2 as well.
> > > > that's easy,
> > > >  has_imutable_rsdp = true
> > > > should be moved into
> > > >  pc_compat_2_1()
> > > > 
> > > > > 
> > > > > Why does adding bridge in this way cause tables to
> > > > > be resized?
> > > > > I think this is a root-cause that should be fixed.
> > > > That's because there wasn't any bridge described in tables
> > > > on startup, but after hotplug and reset tables are rebuild
> > > > which pickups hotplugged bridge and its secondary PCI bus.
> > > > 
> > > > This behavior seems sane, I'd expect bare-metal behave
> > > > the same way.
> > > 
> > > For a simple fix, we can skip bridges added by hotplug -
> > > treat them as regular devices.
> > > 
> > > It defintely seems nicer - at least for old machine types -
> > > than "keep brokenness".
> > > 
> > > I think the following should do the trcik, but it's completely
> > > untested.
> > > Could you pls try?
> > > 
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index b37a397..039776d 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus,
> > > void *bus_state)
> > >       * Skip bridge subtree creation if bridge hotplug is disabled
> > >       * to make acpi tables compatible with legacy machine types.
> > >       */
> > > -    if (!child->pcihp_bridge_en && bus->parent_dev) {
> > > +    if (bus->parent_dev && (!child->pcihp_bridge_en ||
> > > +                            !DEVICE(bus->parent_dev)->hotplugged))
> > > { return;
> > >      }
> > What will happen if
> > hotplug may bridges in
> >  QEMU-fixed^^ -M 2.1
> > migrate to target:
> >  QEMU-2.1 -M 2.1 + hotplugged on sourcebridges on CLI
> > 
> > My guess it may fail due to ACPI blob size difference.
> 
> Yes it might if we are unlucky and cross a 64 k boundary.
> The robust fix is still to resize ROMs on receive.
> We'll also put this patch on a stable branch.
> 

Well, in my opinion though above snippet is sipmlier but it's papering
over the bug in table linker and RSDP, instead of fixing it.

In this case SSDT size change is quite legetimate and in future there
could be other cases when it changes. So it would be better fix the root
problem which is corrupting tables due to changed SSDT size on reboot
instead of pretending that issue doesn't exist.

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

[Michael S. Tsirkin]

On Wed, Dec 10, 2014 at 08:43:55PM +0100, Igor Mammedov wrote:
> On Wed, 10 Dec 2014 20:28:01 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Dec 10, 2014 at 06:39:49PM +0100, Igor Mammedov wrote:
> > > On Wed, 10 Dec 2014 19:13:59 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote:
> > > > > On Wed, 10 Dec 2014 18:24:24 +0200
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Wed, Dec 10, 2014 at 04:06:56PM +0000, 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.
> > > > > > > 
> > > > > > > To fix it rebuild linker and RSDP tables along with
> > > > > > > the rest of ACPI tables so that they would have correct
> > > > > > > offsets.
> > > > > > > 
> > > > > > > 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.
> > > > > > > Windows guests just refuse to boot and
> > > > > > > Linux guests are more resilient and try to boot without
> > > > > > > ACPI, sometimes successfully.
> > > > > > > 
> > > > > > > PS:
> > > > > > >  keep brokenness in 2.2 and older machine types for the sake
> > > > > > >  of migration
> > > > > > > 
> > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > 
> > > > > > I'd like to fix 2.2 as well.
> > > > > that's easy,
> > > > >  has_imutable_rsdp = true
> > > > > should be moved into
> > > > >  pc_compat_2_1()
> > > > > 
> > > > > > 
> > > > > > Why does adding bridge in this way cause tables to
> > > > > > be resized?
> > > > > > I think this is a root-cause that should be fixed.
> > > > > That's because there wasn't any bridge described in tables
> > > > > on startup, but after hotplug and reset tables are rebuild
> > > > > which pickups hotplugged bridge and its secondary PCI bus.
> > > > > 
> > > > > This behavior seems sane, I'd expect bare-metal behave
> > > > > the same way.
> > > > 
> > > > For a simple fix, we can skip bridges added by hotplug -
> > > > treat them as regular devices.
> > > > 
> > > > It defintely seems nicer - at least for old machine types -
> > > > than "keep brokenness".
> > > > 
> > > > I think the following should do the trcik, but it's completely
> > > > untested.
> > > > Could you pls try?
> > > > 
> > > > 
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index b37a397..039776d 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus,
> > > > void *bus_state)
> > > >       * Skip bridge subtree creation if bridge hotplug is disabled
> > > >       * to make acpi tables compatible with legacy machine types.
> > > >       */
> > > > -    if (!child->pcihp_bridge_en && bus->parent_dev) {
> > > > +    if (bus->parent_dev && (!child->pcihp_bridge_en ||
> > > > +                            !DEVICE(bus->parent_dev)->hotplugged))
> > > > { return;
> > > >      }
> > > What will happen if
> > > hotplug may bridges in
> > >  QEMU-fixed^^ -M 2.1
> > > migrate to target:
> > >  QEMU-2.1 -M 2.1 + hotplugged on sourcebridges on CLI
> > > 
> > > My guess it may fail due to ACPI blob size difference.
> > 
> > Yes it might if we are unlucky and cross a 64 k boundary.
> > The robust fix is still to resize ROMs on receive.
> > We'll also put this patch on a stable branch.
> > 
> 
> Well, in my opinion though above snippet is sipmlier but it's papering
> over the bug in table linker and RSDP, instead of fixing it.

I'm not against making the change for new machine types,
but I'm also looking for a fix for 2.2.1


> In this case SSDT size change is quite legetimate and in future there
> could be other cases when it changes. So it would be better fix the root
> problem which is corrupting tables due to changed SSDT size on reboot
> instead of pretending that issue doesn't exist.

For -M 2.3 and up, I'm fine with this approach generally.
Of course it all only works on the assumption we make
ROM regions resizeable.
If not your patch creates migration problems:
-hot-add bridge on source
-hot-add on destination

Size now depends on whether source rebooted, destination
has no way to know that.

-- 
MST

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

[Igor Mammedov]

On Wed, 10 Dec 2014 22:18:53 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Dec 10, 2014 at 08:43:55PM +0100, Igor Mammedov wrote:
> > On Wed, 10 Dec 2014 20:28:01 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Dec 10, 2014 at 06:39:49PM +0100, Igor Mammedov wrote:
> > > > On Wed, 10 Dec 2014 19:13:59 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote:
> > > > > > On Wed, 10 Dec 2014 18:24:24 +0200
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > 
> > > > > > > On Wed, Dec 10, 2014 at 04:06:56PM +0000, 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.
> > > > > > > > 
> > > > > > > > To fix it rebuild linker and RSDP tables along with
> > > > > > > > the rest of ACPI tables so that they would have correct
> > > > > > > > offsets.
> > > > > > > > 
> > > > > > > > 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.
> > > > > > > > Windows guests just refuse to boot and
> > > > > > > > Linux guests are more resilient and try to boot without
> > > > > > > > ACPI, sometimes successfully.
> > > > > > > > 
> > > > > > > > PS:
> > > > > > > >  keep brokenness in 2.2 and older machine types for the sake
> > > > > > > >  of migration
> > > > > > > > 
> > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > > 
> > > > > > > I'd like to fix 2.2 as well.
> > > > > > that's easy,
> > > > > >  has_imutable_rsdp = true
> > > > > > should be moved into
> > > > > >  pc_compat_2_1()
> > > > > > 
> > > > > > > 
> > > > > > > Why does adding bridge in this way cause tables to
> > > > > > > be resized?
> > > > > > > I think this is a root-cause that should be fixed.
> > > > > > That's because there wasn't any bridge described in tables
> > > > > > on startup, but after hotplug and reset tables are rebuild
> > > > > > which pickups hotplugged bridge and its secondary PCI bus.
> > > > > > 
> > > > > > This behavior seems sane, I'd expect bare-metal behave
> > > > > > the same way.
> > > > > 
> > > > > For a simple fix, we can skip bridges added by hotplug -
> > > > > treat them as regular devices.
> > > > > 
> > > > > It defintely seems nicer - at least for old machine types -
> > > > > than "keep brokenness".
> > > > > 
> > > > > I think the following should do the trcik, but it's completely
> > > > > untested.
> > > > > Could you pls try?
> > > > > 
> > > > > 
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index b37a397..039776d 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus,
> > > > > void *bus_state)
> > > > >       * Skip bridge subtree creation if bridge hotplug is disabled
> > > > >       * to make acpi tables compatible with legacy machine types.
> > > > >       */
> > > > > -    if (!child->pcihp_bridge_en && bus->parent_dev) {
> > > > > +    if (bus->parent_dev && (!child->pcihp_bridge_en ||
> > > > > +                            !DEVICE(bus->parent_dev)->hotplugged))
> > > > > { return;
> > > > >      }
> > > > What will happen if
> > > > hotplug may bridges in
> > > >  QEMU-fixed^^ -M 2.1
> > > > migrate to target:
> > > >  QEMU-2.1 -M 2.1 + hotplugged on sourcebridges on CLI
> > > > 
> > > > My guess it may fail due to ACPI blob size difference.
> > > 
> > > Yes it might if we are unlucky and cross a 64 k boundary.
> > > The robust fix is still to resize ROMs on receive.
> > > We'll also put this patch on a stable branch.
> > > 
> > 
> > Well, in my opinion though above snippet is sipmlier but it's papering
> > over the bug in table linker and RSDP, instead of fixing it.
> 
> I'm not against making the change for new machine types,
> but I'm also looking for a fix for 2.2.1
we can ignore 2.2.0, like Paolo did with yesterday's migration bugs
and fix 2.2.1 with this patch.

> 
> 
> > In this case SSDT size change is quite legetimate and in future there
> > could be other cases when it changes. So it would be better fix the root
> > problem which is corrupting tables due to changed SSDT size on reboot
> > instead of pretending that issue doesn't exist.
> 
> For -M 2.3 and up, I'm fine with this approach generally.
> Of course it all only works on the assumption we make
> ROM regions resizeable.
> If not your patch creates migration problems:
> -hot-add bridge on source
> -hot-add on destination
> 
> Size now depends on whether source rebooted, destination
> has no way to know that.
Patch of cause doesn't fix migration issue (and hasn't been meant to),
it just highlights already existing migration issue more prominently.
And it's one more argument in favor of resize-able ROMs on migration or
if it's not acceptable then moving blob into DeviceState where we can
freely adjust size on target.