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 <m...@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
