[2/4] acpi-build: fix ACPI RAM management

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 <mst@redhat.com>
---
 hw/i386/acpi-build.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

On Tue, 17 Feb 2015 11:05:39 +0100
"Michael S. Tsirkin" <mst@redhat.com> 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 <mst@redhat.com>
> ---
>  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 

> +        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

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" <mst@redhat.com> 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 <mst@redhat.com>
> > ---
> >  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