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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Nov 19, 2014 at 12:51:00PM +0530, Amit Shah wrote:
> 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 <mst@redhat.com>
> > ---
> >  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), "/rom@%s", 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?

How can it?
We need to track ram address in order to dirty it.
I could track both pointer and ram address, but this
seems like unnecessary data duplication that will just
lead to bugs.
the benefit of splitting one like off seems to small.


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

This can never happen. If it does, we don't know how to recover.

> if this happens during hotplug?

This does not run on hotplug.

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