On Thu, Dec 11, 2014 at 01:31:58PM +0100, Igor Mammedov wrote: > On Thu, 11 Dec 2014 12:37:10 +0200 > "Michael S. Tsirkin" <m...@redhat.com> wrote: > > > On Thu, Dec 11, 2014 at 09:10:53AM +0000, Igor Mammedov wrote: > > > linker and RSDP tables are build only once, so if later > > > > s/build/built/ > > > > > 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. > > > > Actually, I understand the argument about the > > linker, but do you really believe RSDP will ever change? > it changes since RSDT is at the end and its offset changes > every time a below table changes its size.
At the end of what? It's in a separate file, isn't it? > If RSDT were at the start then it might be > possible to keep RSDP immutable. > > I'll check if it's feasible. > > This fix however is more robust and doesn't care about > to table order, the best would be to combine ordering > fix for old machines for stable and this patch for > new machines since 2.3. > > > > > How about we split out RSDP and linker changes? > > > > Also s/imutable/immutable/ in a bunch of places below. > Thanks, I'll fix this one. > > > > > > 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. > > > > > > keep brokenness in 2.1 and older machine types for > > > the sake of migration. 2.2.0 can't be helped but we > > > can fix it with 2.2.1 > > > > > > Why do you say this? > > It can be helped by patch that I sent, skipping > > hotplugged bridges, no? > > > > > > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > > > --- > > > v2: > > > move compat fix to 2.1 machine type, > > > suggsted by: "Michael S. Tsirkin" <m...@redhat.com> > > > --- > > > 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..61170de 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; > > > > s/imutable/immutable/ > > > > > 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); > > > @@ -323,6 +325,7 @@ static void pc_compat_2_1(MachineState *machine) > > > x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, > > > 0); > > > x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, > > > CPUID_EXT3_SVM); > > > pcms->enforce_aligned_dimm = false; > > > + has_imutable_rsdp = true; > > > } > > > > > > static void pc_compat_2_0(MachineState *machine) > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > > > index 121f620..0f071a9 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). > > > @@ -302,6 +304,7 @@ static void pc_compat_2_1(MachineState *machine) > > > 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); > > > x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, > > > CPUID_EXT3_SVM); > > > + has_imutable_rsdp = true; > > > } > > > > > > static void pc_compat_2_0(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