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

[Michael S. Tsirkin]

On Thu, Dec 11, 2014 at 01:21:13PM +0100, Igor Mammedov wrote:
> On Thu, 11 Dec 2014 13:41:07 +0200
> Marcel Apfelbaum <address@hidden> wrote:
> 
> > On Thu, 2014-12-11 at 12:23 +0100, Igor Mammedov wrote:
> > > On Thu, 11 Dec 2014 11:18:50 +0200
> > > Marcel Apfelbaum <address@hidden> 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 <address@hidden>
> > > > > ---
> > > > > v2:
> > > > >   move compat fix to 2.1 machine type,
> > > > >    suggsted by: "Michael S. Tsirkin" <address@hidden>
> > > > > ---
> > > > >  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 */
> > > > 
> > > > 
> > > > 
> > > > 
> > > 
> > 
> > 
> >