| Message ID | 1418289053-32607-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 11 Dec 2014 11:18:50 +0200 Marcel Apfelbaum <marcel.a@redhat.com> wrote: > On Thu, 2014-12-11 at 09:10 +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. > > > > 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 > > Hi Igor, > I followed the prev conversation and I agree with the patch, > I do have one question: > Why you didn't set has_imutable_rsdp to true for 2.2.0 machines? > What is special about it? I think I missed it. V1 was only from 2.3 leaving 2.2 broken, but Michael asked about fixing 2.2 as well. Since 2.2.0 is out we can't fix it without breaking migration whichever way we choose to fix it, but we can fix 2.2 machine in 2.2.1 at least allowing downstream to pickup and ship fixed version without shipping broken one. > > Thanks, > Marcel > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > v2: > > move compat fix to 2.1 machine type, > > suggsted by: "Michael S. Tsirkin" <mst@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; > > 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 */ > > > >
On Thu, 2014-12-11 at 12:23 +0100, Igor Mammedov wrote: > On Thu, 11 Dec 2014 11:18:50 +0200 > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > On Thu, 2014-12-11 at 09:10 +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. > > > > > > 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 > > > > Hi Igor, > > I followed the prev conversation and I agree with the patch, > > I do have one question: > > Why you didn't set has_imutable_rsdp to true for 2.2.0 machines? > > What is special about it? I think I missed it. > V1 was only from 2.3 leaving 2.2 broken, but Michael asked > about fixing 2.2 as well. > > Since 2.2.0 is out we can't fix it without breaking migration > whichever way we choose to fix it, but we can fix 2.2 machine > in 2.2.1 at least allowing downstream to pickup and ship > fixed version without shipping broken one. This I understood, but it doesn't answer my question (I think...) I will rephrase: Machines < 2.2 have has_imutable_rsdp = true => remain the same Machines >= 2.3 have has_imutable_rsdp = false => new functionality For Machine 2.2 we *also* have has_imutable_rsdp = false => new functionality, not old. Am I right? If yes, why not retaining the same policy for 2.2 as we did for 2.1 and less? This was my question, forgive me if it wasn't clear Thanks, Marcel > > > > > Thanks, > > Marcel > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > v2: > > > move compat fix to 2.1 machine type, > > > suggsted by: "Michael S. Tsirkin" <mst@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; > > > 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 */ > > > > > > > > >
On Thu, 11 Dec 2014 13:41:07 +0200 Marcel Apfelbaum <marcel.a@redhat.com> wrote: > On Thu, 2014-12-11 at 12:23 +0100, Igor Mammedov wrote: > > On Thu, 11 Dec 2014 11:18:50 +0200 > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > On Thu, 2014-12-11 at 09:10 +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. > > > > > > > > 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 > > > > > > Hi Igor, > > > I followed the prev conversation and I agree with the patch, > > > I do have one question: > > > Why you didn't set has_imutable_rsdp to true for 2.2.0 machines? > > > What is special about it? I think I missed it. > > V1 was only from 2.3 leaving 2.2 broken, but Michael asked > > about fixing 2.2 as well. > > > > Since 2.2.0 is out we can't fix it without breaking migration > > whichever way we choose to fix it, but we can fix 2.2 machine > > in 2.2.1 at least allowing downstream to pickup and ship > > fixed version without shipping broken one. > This I understood, but it doesn't answer my question (I think...) > I will rephrase: > Machines < 2.2 have has_imutable_rsdp = true => remain the same > Machines >= 2.3 have has_imutable_rsdp = false => new functionality > > For Machine 2.2 we *also* have has_imutable_rsdp = false => new > functionality, not old. Am I right? If yes, why not retaining the > same policy for 2.2 as we did for 2.1 and less? It would break migration due to missing rsdp section i.e. QEMU-2.1 -M pc-2.1 => QEMU-2.3 -M pc-2.1 > > This was my question, forgive me if it wasn't clear > Thanks, > Marcel > > > > > > > > > Thanks, > > > Marcel > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > v2: > > > > move compat fix to 2.1 machine type, > > > > suggsted by: "Michael S. Tsirkin" <mst@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; > > > > 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 */ > > > > > > > > > > > > > > > > >
On Thu, Dec 11, 2014 at 01:21:13PM +0100, Igor Mammedov wrote: > On Thu, 11 Dec 2014 13:41:07 +0200 > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > On Thu, 2014-12-11 at 12:23 +0100, Igor Mammedov wrote: > > > On Thu, 11 Dec 2014 11:18:50 +0200 > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > On Thu, 2014-12-11 at 09:10 +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. > > > > > > > > > > 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 > > > > > > > > Hi Igor, > > > > I followed the prev conversation and I agree with the patch, > > > > I do have one question: > > > > Why you didn't set has_imutable_rsdp to true for 2.2.0 machines? > > > > What is special about it? I think I missed it. > > > V1 was only from 2.3 leaving 2.2 broken, but Michael asked > > > about fixing 2.2 as well. > > > > > > Since 2.2.0 is out we can't fix it without breaking migration > > > whichever way we choose to fix it, but we can fix 2.2 machine > > > in 2.2.1 at least allowing downstream to pickup and ship > > > fixed version without shipping broken one. > > This I understood, but it doesn't answer my question (I think...) > > I will rephrase: > > Machines < 2.2 have has_imutable_rsdp = true => remain the same > > Machines >= 2.3 have has_imutable_rsdp = false => new functionality > > > > For Machine 2.2 we *also* have has_imutable_rsdp = false => new > > functionality, not old. Am I right? If yes, why not retaining the > > same policy for 2.2 as we did for 2.1 and less? > It would break migration due to missing rsdp section i.e. > QEMU-2.1 -M pc-2.1 => QEMU-2.3 -M pc-2.1 Igor, could you create a minimal bugfix patch appropriate for stable? I think something along the lines of the patch I sent would be the most appropriate. > > > > This was my question, forgive me if it wasn't clear > > Thanks, > > Marcel > > > > > > > > > > > > > Thanks, > > > > Marcel > > > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > --- > > > > > v2: > > > > > move compat fix to 2.1 machine type, > > > > > suggsted by: "Michael S. Tsirkin" <mst@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; > > > > > 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 */ > > > > > > > > > > > > > > > > > > > > > > > > >
On Thu, 11 Dec 2014 12:37:10 +0200 "Michael S. Tsirkin" <mst@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. 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 <imammedo@redhat.com> > > --- > > v2: > > move compat fix to 2.1 machine type, > > suggsted by: "Michael S. Tsirkin" <mst@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
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" <mst@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 <imammedo@redhat.com> > > > --- > > > v2: > > > move compat fix to 2.1 machine type, > > > suggsted by: "Michael S. Tsirkin" <mst@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
On Thu, 11 Dec 2014 14:50:15 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > 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" <mst@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? RSDP is separate file but RSDT is a part of tables blob. and RSDP has a pointer to moving RSDT, hence it changes too when tables blob size changes. > > > 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 <imammedo@redhat.com> > > > > --- > > > > v2: > > > > move compat fix to 2.1 machine type, > > > > suggsted by: "Michael S. Tsirkin" <mst@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
On Thu, Dec 11, 2014 at 02:18:27PM +0100, Igor Mammedov wrote: > On Thu, 11 Dec 2014 14:50:15 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > 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" <mst@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? > RSDP is separate file but RSDT is a part of tables blob. > > and RSDP has a pointer to moving RSDT, hence it changes > too when tables blob size changes. I finally got it. Thanks. Ouch. That's a nasty bug, though it only hurts us if we migrate while tables are being shadowed. To fix it for old machine types, we can find the RSDT and fix up the RSDP. For new machine that's a reason to either put RSDP in ROM too, or move RSDT out to a separate file. > > > > > 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 <imammedo@redhat.com> > > > > > --- > > > > > v2: > > > > > move compat fix to 2.1 machine type, > > > > > suggsted by: "Michael S. Tsirkin" <mst@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
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; 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 */
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. 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 Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v2: move compat fix to 2.1 machine type, suggsted by: "Michael S. Tsirkin" <mst@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(-)