Re: [Qemu-devel] [PATCH 2/4] acpi-build: fix ACPI RAM management

[Michael S. Tsirkin]

On Tue, Feb 17, 2015 at 02:52:08PM +0100, Igor Mammedov wrote:
> On Tue, 17 Feb 2015 11:05:39 +0100
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > This fixes multiple issues around ACPI RAM management:
> > 
> > RSDP and linker RAM aren't currently marked dirty
> > on update, so they won't be migrated correctly.
> > 
> > Let's handle all tables in the same way: set correct size (assert if
> > too big), update, mark RAM dirty.
> > 
> > This also drops assert checking that table size didn't change: table
> > size is fundamentally dynamic and depends on hw configuration,
> > just set the correct size and use that (memory core asserts if size is
> > too large).
> > 
> > This also means we can drop tracking table size, memory core does this
> > for us now.
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  hw/i386/acpi-build.c | 43 +++++++++++++++++++++++--------------------
> >  1 file changed, 23 insertions(+), 20 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 1dfdf35..e78d6cb 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1266,13 +1266,12 @@ typedef
> >  struct AcpiBuildState {
> >      /* Copy of table in RAM (for patching). */
> >      ram_addr_t table_ram;
> > -    uint32_t table_size;
> >      /* Is table patched? */
> >      uint8_t patched;
> >      PcGuestInfo *guest_info;
> >      void *rsdp;
> > +    ram_addr_t rsdp_ram;
> >      ram_addr_t linker_ram;
> > -    uint32_t linker_size;
> >  } AcpiBuildState;
> >  
> >  static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
> > @@ -1455,6 +1454,17 @@ void acpi_build(PcGuestInfo *guest_info, 
> > AcpiBuildTables *tables)
> >      g_array_free(table_offsets, true);
> >  }
> >  
> > +static void acpi_ram_update(ram_addr_t ram, GArray *data)
> > +{
> > +    uint32_t size = acpi_data_len(data);
> > +
> > +    /* Make sure RAM size is correct - in case it got changed e.g. by 
> > migration */
> > +    qemu_ram_resize(ram, size, &error_abort);
> > +
> > +    memcpy(qemu_get_ram_ptr(ram), data->data, size);
> > +    cpu_physical_memory_set_dirty_range_nocode(ram, size);
> > +}
> > +
> >  static void acpi_build_update(void *build_opaque, uint32_t offset)
> >  {
> >      AcpiBuildState *build_state = build_opaque;
> > @@ -1470,21 +1480,15 @@ static void acpi_build_update(void *build_opaque, 
> > uint32_t offset)
> >  
> >      acpi_build(build_state->guest_info, &tables);
> >  
> > -    assert(acpi_data_len(tables.table_data) == build_state->table_size);
> > +    acpi_ram_update(build_state->table_ram, tables.table_data);
> >  
> > -    /* Make sure RAM size is correct - in case it got changed by migration 
> > */
> > -    qemu_ram_resize(build_state->table_ram, build_state->table_size,
> > -                    &error_abort);
> > -
> > -    memcpy(qemu_get_ram_ptr(build_state->table_ram), 
> > tables.table_data->data,
> > -           build_state->table_size);
> > -    memcpy(build_state->rsdp, tables.rsdp->data, 
> > acpi_data_len(tables.rsdp));
> > -    memcpy(qemu_get_ram_ptr(build_state->linker_ram), tables.linker->data,
> > -           build_state->linker_size);
> > -
> > -    cpu_physical_memory_set_dirty_range_nocode(build_state->table_ram,
> > -                                               build_state->table_size);
> > +    if (build_state->rsdp) {
> > +        memcpy(build_state->rsdp, tables.rsdp->data, 
> > acpi_data_len(tables.rsdp));
> > +    } else {
> > +        acpi_ram_update(build_state->rsdp_ram, tables.rsdp);
> > +    }
> >  
> > +    acpi_ram_update(build_state->linker_ram, tables.linker);
> >      acpi_build_tables_cleanup(&tables, true);
> >  }
> >  
> > @@ -1545,11 +1549,9 @@ void acpi_setup(PcGuestInfo *guest_info)
> >                                                 ACPI_BUILD_TABLE_FILE,
> >                                                 ACPI_BUILD_TABLE_MAX_SIZE);
> >      assert(build_state->table_ram != RAM_ADDR_MAX);
> > -    build_state->table_size = acpi_data_len(tables.table_data);
> >  
> >      build_state->linker_ram =
> >          acpi_add_rom_blob(build_state, tables.linker, "etc/table-loader", 
> > 0);
> > -    build_state->linker_size = acpi_data_len(tables.linker);
> >  
> >      fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> >                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
> > @@ -1564,10 +1566,11 @@ void acpi_setup(PcGuestInfo *guest_info)
> >                                   acpi_build_update, build_state,
> >                                   tables.rsdp->data, 
> > acpi_data_len(tables.rsdp));
> >          build_state->rsdp = tables.rsdp->data;
> > +        build_state->rsdp_ram = (ram_addr_t)-1;
> I'd drop this and 
> 
> >      } else {
> > -        build_state->rsdp = qemu_get_ram_ptr(
> > -            acpi_add_rom_blob(build_state, tables.rsdp, 
> > ACPI_BUILD_RSDP_FILE, 0)
> > -        );
> > +        build_state->rsdp = NULL;
> this as unnecessary 

We've been here, I prefer explict initialization.

> > +        build_state->rsdp_ram = acpi_add_rom_blob(build_state, tables.rsdp,
> > +                                                  ACPI_BUILD_RSDP_FILE, 0);
> >      }
> >  
> >      qemu_register_reset(acpi_build_reset, build_state);
> 
> Otherwise looks as a very nice improvement of current mess