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