Re: [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update

[Amit Shah]

On (Mon) 17 Nov 2014 [19:04:13], Michael S. Tsirkin wrote:
> acpi build modifies internal FW CFG RAM on first access
> but we forgot to mark it dirty.
> If this RAM has been migrated already, it won't be
> migrated again, returning corrupted tables to guest.
> 
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  include/hw/loader.h  |  2 +-
>  hw/core/loader.c     |  8 +++++---
>  hw/i386/acpi-build.c | 11 ++++++++---
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 054c6a2..6481639 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -59,7 +59,7 @@ extern bool rom_file_has_mr;
>  int rom_add_file(const char *file, const char *fw_dir,
>                   hwaddr addr, int32_t bootindex,
>                   bool option_rom);
> -void *rom_add_blob(const char *name, const void *blob, size_t len,
> +ram_addr_t rom_add_blob(const char *name, const void *blob, size_t len,
>                     hwaddr addr, const char *fw_file_name,
>                     FWCfgReadCallback fw_callback, void *callback_opaque);

Here, and in the next hunks where function signatures are modified,
indent of following lines go off.  Minor nit.

>  int rom_add_elf_program(const char *name, void *data, size_t datasize,
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index bbe6eb3..5cf686d 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -798,12 +798,12 @@ err:
>      return -1;
>  }
>  
> -void *rom_add_blob(const char *name, const void *blob, size_t len,
> +ram_addr_t rom_add_blob(const char *name, const void *blob, size_t len,
>                     hwaddr addr, const char *fw_file_name,
>                     FWCfgReadCallback fw_callback, void *callback_opaque)
>  {
>      Rom *rom;
> -    void *data = NULL;
> +    ram_addr_t ret = RAM_ADDR_MAX;
>  
>      rom           = g_malloc0(sizeof(*rom));
>      rom->name     = g_strdup(name);
> @@ -815,11 +815,13 @@ void *rom_add_blob(const char *name, const void *blob, 
> size_t len,
>      rom_insert(rom);
>      if (fw_file_name && fw_cfg) {
>          char devpath[100];
> +        void *data;
>  
>          snprintf(devpath, sizeof(devpath), "/address@hidden", fw_file_name);
>  
>          if (rom_file_has_mr) {
>              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> +            ret = memory_region_get_ram_addr(rom->mr);
>          } else {
>              data = rom->data;
>          }
> @@ -828,7 +830,7 @@ void *rom_add_blob(const char *name, const void *blob, 
> size_t len,
>                                   fw_callback, callback_opaque,
>                                   data, rom->romsize);
>      }
> -    return data;
> +    return ret;
>  }
>  
>  /* This function is specific for elf program because we don't need to 
> allocate
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4003b6b..92a36e3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -56,6 +56,7 @@
>  
>  #include "qapi/qmp/qint.h"
>  #include "qom/qom-qobject.h"
> +#include "exec/ram_addr.h"
>  
>  /* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
>   * -M pc-i440fx-2.0.  Even if the actual amount of AML generated grows
> @@ -1511,7 +1512,7 @@ static inline void 
> acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
>  typedef
>  struct AcpiBuildState {
>      /* Copy of table in RAM (for patching). */
> -    uint8_t *table_ram;
> +    ram_addr_t table_ram;
>      uint32_t table_size;
>      /* Is table patched? */
>      uint8_t patched;
> @@ -1716,9 +1717,12 @@ 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);
> -    memcpy(build_state->table_ram, tables.table_data->data,
> +    memcpy(qemu_get_ram_ptr(build_state->table_ram), tables.table_data->data,
>             build_state->table_size);

This looks like something not necessary for this patch?  Can be split
off into another one?

> +    cpu_physical_memory_set_dirty_range_nocode(build_state->table_ram,
> +                                               build_state->table_size);
> +
>      acpi_build_tables_cleanup(&tables, true);
>  }
>  
> @@ -1728,7 +1732,7 @@ static void acpi_build_reset(void *build_opaque)
>      build_state->patched = 0;
>  }
>  
> -static void *acpi_add_rom_blob(AcpiBuildState *build_state, GArray *blob,
> +static ram_addr_t acpi_add_rom_blob(AcpiBuildState *build_state, GArray 
> *blob,
>                                 const char *name)
>  {
>      return rom_add_blob(name, blob->data, acpi_data_len(blob), -1, name,
> @@ -1777,6 +1781,7 @@ void acpi_setup(PcGuestInfo *guest_info)
>      /* Now expose it all to Guest */
>      build_state->table_ram = acpi_add_rom_blob(build_state, 
> tables.table_data,
>                                                 ACPI_BUILD_TABLE_FILE);
> +    assert(build_state->table_ram != RAM_ADDR_MAX);
>      build_state->table_size = acpi_data_len(tables.table_data);

Isn't an assert too strong if this happens during hotplug?

I'm trying to follow this code, but looks like this isn't called in
the hotplug path - is that right?


                Amit