* [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update
@ 2014-11-17 17:04 Michael S. Tsirkin
2014-11-17 17:07 ` Paolo Bonzini
2014-11-19 7:21 ` Amit Shah
0 siblings, 2 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-11-17 17:04 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Anthony Liguori, Richard Henderson
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);
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);
+ 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);
acpi_add_rom_blob(NULL, tables.linker, "etc/table-loader");
--
MST
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update
2014-11-17 17:04 [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update Michael S. Tsirkin
@ 2014-11-17 17:07 ` Paolo Bonzini
2014-11-19 7:21 ` Amit Shah
1 sibling, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2014-11-17 17:07 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel; +Cc: Anthony Liguori, Richard Henderson
On 17/11/2014 18:04, 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);
> 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);
>
> + 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);
>
> acpi_add_rom_blob(NULL, tables.linker, "etc/table-loader");
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update
2014-11-17 17:04 [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update Michael S. Tsirkin
2014-11-17 17:07 ` Paolo Bonzini
@ 2014-11-19 7:21 ` Amit Shah
2014-11-19 10:08 ` Igor Mammedov
2014-11-19 10:12 ` Michael S. Tsirkin
1 sibling, 2 replies; 8+ messages in thread
From: Amit Shah @ 2014-11-19 7:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Paolo Bonzini, qemu-devel, Anthony Liguori, Richard Henderson
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?
> + 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update
2014-11-19 7:21 ` Amit Shah
@ 2014-11-19 10:08 ` Igor Mammedov
2014-11-20 4:19 ` Amit Shah
2014-11-19 10:12 ` Michael S. Tsirkin
1 sibling, 1 reply; 8+ messages in thread
From: Igor Mammedov @ 2014-11-19 10:08 UTC (permalink / raw)
To: Amit Shah
Cc: Paolo Bonzini, Richard Henderson, qemu-devel, Anthony Liguori,
Michael S. Tsirkin
On Wed, 19 Nov 2014 12:51:00 +0530
Amit Shah <amit.shah@redhat.com> 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?
>
> > +
> > 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?
yep, it's called only at startup
>
>
> Amit
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update
2014-11-19 10:08 ` Igor Mammedov
@ 2014-11-20 4:19 ` Amit Shah
2014-11-20 8:16 ` Igor Mammedov
0 siblings, 1 reply; 8+ messages in thread
From: Amit Shah @ 2014-11-20 4:19 UTC (permalink / raw)
To: Igor Mammedov
Cc: Paolo Bonzini, Richard Henderson, qemu-devel, Anthony Liguori,
Michael S. Tsirkin
On (Wed) 19 Nov 2014 [11:08:46], Igor Mammedov wrote:
> On Wed, 19 Nov 2014 12:51:00 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> > > -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?
> yep, it's called only at startup
Thanks; what's the path taken for hotplug, then? (Ensuring we have
that case covered too).
Amit
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update
2014-11-20 4:19 ` Amit Shah
@ 2014-11-20 8:16 ` Igor Mammedov
2014-11-21 4:36 ` Amit Shah
0 siblings, 1 reply; 8+ messages in thread
From: Igor Mammedov @ 2014-11-20 8:16 UTC (permalink / raw)
To: Amit Shah
Cc: Paolo Bonzini, Richard Henderson, qemu-devel, Anthony Liguori,
Michael S. Tsirkin
On Thu, 20 Nov 2014 09:49:20 +0530
Amit Shah <amit.shah@redhat.com> wrote:
> On (Wed) 19 Nov 2014 [11:08:46], Igor Mammedov wrote:
> > On Wed, 19 Nov 2014 12:51:00 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
>
> > > > -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?
> > yep, it's called only at startup
>
> Thanks; what's the path taken for hotplug, then? (Ensuring we have
> that case covered too).
In case of hotplug ACPI blob doesn't change at all (it has all
possible device objects in it). It changes on the first time BIOS
fetches ROM blob and on reset.
Later during hotplug, guest gets SCI interrupt and runs related to
hotplug event AML method, which fishes out bits necessary for hotplug
via corresponding mmio interface.
> Amit
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update
2014-11-20 8:16 ` Igor Mammedov
@ 2014-11-21 4:36 ` Amit Shah
0 siblings, 0 replies; 8+ messages in thread
From: Amit Shah @ 2014-11-21 4:36 UTC (permalink / raw)
To: Igor Mammedov
Cc: Paolo Bonzini, Richard Henderson, qemu-devel, Anthony Liguori,
Michael S. Tsirkin
On (Thu) 20 Nov 2014 [09:16:31], Igor Mammedov wrote:
> On Thu, 20 Nov 2014 09:49:20 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
>
> > On (Wed) 19 Nov 2014 [11:08:46], Igor Mammedov wrote:
> > > On Wed, 19 Nov 2014 12:51:00 +0530
> > > Amit Shah <amit.shah@redhat.com> wrote:
> >
> > > > > -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?
> > > yep, it's called only at startup
> >
> > Thanks; what's the path taken for hotplug, then? (Ensuring we have
> > that case covered too).
> In case of hotplug ACPI blob doesn't change at all (it has all
> possible device objects in it). It changes on the first time BIOS
> fetches ROM blob and on reset.
> Later during hotplug, guest gets SCI interrupt and runs related to
> hotplug event AML method, which fishes out bits necessary for hotplug
> via corresponding mmio interface.
Excellent, thanks!
Amit
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update
2014-11-19 7:21 ` Amit Shah
2014-11-19 10:08 ` Igor Mammedov
@ 2014-11-19 10:12 ` Michael S. Tsirkin
1 sibling, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-11-19 10:12 UTC (permalink / raw)
To: Amit Shah; +Cc: Paolo Bonzini, qemu-devel, Anthony Liguori, Richard Henderson
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-11-21 4:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-17 17:04 [Qemu-devel] [PATCH for-2.2] acpi-build: mark RAM dirty on table update Michael S. Tsirkin
2014-11-17 17:07 ` Paolo Bonzini
2014-11-19 7:21 ` Amit Shah
2014-11-19 10:08 ` Igor Mammedov
2014-11-20 4:19 ` Amit Shah
2014-11-20 8:16 ` Igor Mammedov
2014-11-21 4:36 ` Amit Shah
2014-11-19 10:12 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).