| Message ID | 20200311172014.33052-3-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM virt: Add NVDIMM support | expand |
On 11.03.20 18:20, Shameer Kolothum wrote: > Any sub-page size update to ACPI table MRs will be lost during > migration, as we use aligned size in ram_load_precopy() -> > qemu_ram_resize() path. This will result in inconsistency in sizes > between source and destination. In order to avoid this, save and > restore them separately during migration. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > Please find the discussion here, > https://patchwork.kernel.org/patch/11339591/ > --- > hw/core/machine.c | 1 + > hw/nvram/fw_cfg.c | 86 ++++++++++++++++++++++++++++++++++++++- > include/hw/nvram/fw_cfg.h | 6 +++ > 3 files changed, 92 insertions(+), 1 deletion(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 9e8c06036f..6d960bd47f 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -39,6 +39,7 @@ GlobalProperty hw_compat_4_2[] = { > { "usb-redir", "suppress-remote-wake", "off" }, > { "qxl", "revision", "4" }, > { "qxl-vga", "revision", "4" }, > + { "fw_cfg", "acpi-mr-restore", "false" }, > }; > const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 179b302f01..36d1e32f83 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -39,6 +39,7 @@ > #include "qemu/config-file.h" > #include "qemu/cutils.h" > #include "qapi/error.h" > +#include "hw/acpi/aml-build.h" > > #define FW_CFG_FILE_SLOTS_DFLT 0x20 > > @@ -610,6 +611,50 @@ bool fw_cfg_dma_enabled(void *opaque) > return s->dma_enabled; > } > > +static bool fw_cfg_acpi_mr_restore(void *opaque) > +{ > + FWCfgState *s = opaque; > + return s->acpi_mr_restore; > +} > + > +static void fw_cfg_update_mr(FWCfgState *s, uint16_t key, size_t size) > +{ > + MemoryRegion *mr; > + ram_addr_t offset; > + int arch = !!(key & FW_CFG_ARCH_LOCAL); > + void *ptr; > + > + key &= FW_CFG_ENTRY_MASK; > + assert(key < fw_cfg_max_entry(s)); > + > + ptr = s->entries[arch][key].data; > + mr = memory_region_from_host(ptr, &offset); > + > + memory_region_ram_resize(mr, size, &error_abort); > +} > + > +static int fw_cfg_acpi_mr_restore_post_load(void *opaque, int version_id) > +{ > + FWCfgState *s = opaque; > + int i, index; > + > + assert(s->files); > + > + index = be32_to_cpu(s->files->count); > + > + for (i = 0; i < index; i++) { > + if (!strcmp(s->files->f[i].name, ACPI_BUILD_TABLE_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->table_mr_size); > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_LOADER_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->linker_mr_size); > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_RSDP_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->rsdp_mr_size); > + } > + } > + > + return 0; > +} > + > static const VMStateDescription vmstate_fw_cfg_dma = { > .name = "fw_cfg/dma", > .needed = fw_cfg_dma_enabled, > @@ -619,6 +664,20 @@ static const VMStateDescription vmstate_fw_cfg_dma = { > }, > }; > > +static const VMStateDescription vmstate_fw_cfg_acpi_mr = { > + .name = "fw_cfg/acpi_mr", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = fw_cfg_acpi_mr_restore, > + .post_load = fw_cfg_acpi_mr_restore_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(table_mr_size, FWCfgState), > + VMSTATE_UINT64(linker_mr_size, FWCfgState), > + VMSTATE_UINT64(rsdp_mr_size, FWCfgState), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > static const VMStateDescription vmstate_fw_cfg = { > .name = "fw_cfg", > .version_id = 2, > @@ -631,6 +690,7 @@ static const VMStateDescription vmstate_fw_cfg = { > }, > .subsections = (const VMStateDescription*[]) { > &vmstate_fw_cfg_dma, > + &vmstate_fw_cfg_acpi_mr, > NULL, > } > }; > @@ -815,6 +875,23 @@ static struct { > #define FW_CFG_ORDER_OVERRIDE_LAST 200 > }; > > +/* > + * Any sub-page size update to these table MRs will be lost during migration, > + * as we use aligned size in ram_load_precopy() -> qemu_ram_resize() path. > + * In order to avoid the inconsistency in sizes save them seperately and > + * migrate over in vmstate post_load(). > + */ > +static void fw_cfg_acpi_mr_save(FWCfgState *s, const char *filename, size_t len) > +{ > + if (!strcmp(filename, ACPI_BUILD_TABLE_FILE)) { > + s->table_mr_size = len; > + } else if (!strcmp(filename, ACPI_BUILD_LOADER_FILE)) { > + s->linker_mr_size = len; > + } else if (!strcmp(filename, ACPI_BUILD_RSDP_FILE)) { > + s->rsdp_mr_size = len; > + } > +} > + > static int get_fw_cfg_order(FWCfgState *s, const char *name) > { > int i; > @@ -914,6 +991,7 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > > s->files->count = cpu_to_be32(count+1); > + fw_cfg_acpi_mr_save(s, filename, len); > } > > void fw_cfg_add_file(FWCfgState *s, const char *filename, > @@ -937,6 +1015,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, > ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i, > data, len); > s->files->f[i].size = cpu_to_be32(len); > + fw_cfg_acpi_mr_save(s, filename, len); > return ptr; > } > } > @@ -973,7 +1052,10 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data) > qemu_register_reset(fw_cfg_machine_reset, s); > } > > - > +static Property fw_cfg_properties[] = { > + DEFINE_PROP_BOOL("acpi-mr-restore", FWCfgState, acpi_mr_restore, true), > + DEFINE_PROP_END_OF_LIST(), > +}; > > static void fw_cfg_common_realize(DeviceState *dev, Error **errp) > { > @@ -1097,6 +1179,8 @@ static void fw_cfg_class_init(ObjectClass *klass, void *data) > > dc->reset = fw_cfg_reset; > dc->vmsd = &vmstate_fw_cfg; > + > + device_class_set_props(dc, fw_cfg_properties); > } > > static const TypeInfo fw_cfg_info = { > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index b5291eefad..457fee7425 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -53,6 +53,12 @@ struct FWCfgState { > dma_addr_t dma_addr; > AddressSpace *dma_as; > MemoryRegion dma_iomem; > + > + /* restore during migration */ > + bool acpi_mr_restore; > + size_t table_mr_size; > + size_t linker_mr_size; > + size_t rsdp_mr_size; > }; > > struct FWCfgIoState { > Makes sense to me, the RAM block migration code will migrate full pages. Only the sub-page size has to be migrated and reset. Not an expert of the FW code etc, but the general idea sounds correct to me Acked-by: David Hildenbrand <david@redhat.com>
On Wed, Mar 11, 2020 at 05:20:06PM +0000, Shameer Kolothum wrote: > Any sub-page size update to ACPI table MRs will be lost during > migration, as we use aligned size in ram_load_precopy() -> > qemu_ram_resize() path. This will result in inconsistency in sizes > between source and destination. In order to avoid this, save and > restore them separately during migration. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> Is there a reason this is part of nvdimm patchset? > --- > Please find the discussion here, > https://patchwork.kernel.org/patch/11339591/ > --- > hw/core/machine.c | 1 + > hw/nvram/fw_cfg.c | 86 ++++++++++++++++++++++++++++++++++++++- > include/hw/nvram/fw_cfg.h | 6 +++ > 3 files changed, 92 insertions(+), 1 deletion(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 9e8c06036f..6d960bd47f 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -39,6 +39,7 @@ GlobalProperty hw_compat_4_2[] = { > { "usb-redir", "suppress-remote-wake", "off" }, > { "qxl", "revision", "4" }, > { "qxl-vga", "revision", "4" }, > + { "fw_cfg", "acpi-mr-restore", "false" }, > }; > const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 179b302f01..36d1e32f83 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -39,6 +39,7 @@ > #include "qemu/config-file.h" > #include "qemu/cutils.h" > #include "qapi/error.h" > +#include "hw/acpi/aml-build.h" > > #define FW_CFG_FILE_SLOTS_DFLT 0x20 > > @@ -610,6 +611,50 @@ bool fw_cfg_dma_enabled(void *opaque) > return s->dma_enabled; > } > > +static bool fw_cfg_acpi_mr_restore(void *opaque) > +{ > + FWCfgState *s = opaque; > + return s->acpi_mr_restore; > +} > + > +static void fw_cfg_update_mr(FWCfgState *s, uint16_t key, size_t size) > +{ > + MemoryRegion *mr; > + ram_addr_t offset; > + int arch = !!(key & FW_CFG_ARCH_LOCAL); > + void *ptr; > + > + key &= FW_CFG_ENTRY_MASK; > + assert(key < fw_cfg_max_entry(s)); > + > + ptr = s->entries[arch][key].data; > + mr = memory_region_from_host(ptr, &offset); > + > + memory_region_ram_resize(mr, size, &error_abort); > +} > + > +static int fw_cfg_acpi_mr_restore_post_load(void *opaque, int version_id) > +{ > + FWCfgState *s = opaque; > + int i, index; > + > + assert(s->files); > + > + index = be32_to_cpu(s->files->count); > + > + for (i = 0; i < index; i++) { > + if (!strcmp(s->files->f[i].name, ACPI_BUILD_TABLE_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->table_mr_size); > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_LOADER_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->linker_mr_size); > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_RSDP_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->rsdp_mr_size); > + } > + } > + > + return 0; > +} > + > static const VMStateDescription vmstate_fw_cfg_dma = { > .name = "fw_cfg/dma", > .needed = fw_cfg_dma_enabled, > @@ -619,6 +664,20 @@ static const VMStateDescription vmstate_fw_cfg_dma = { > }, > }; > > +static const VMStateDescription vmstate_fw_cfg_acpi_mr = { > + .name = "fw_cfg/acpi_mr", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = fw_cfg_acpi_mr_restore, > + .post_load = fw_cfg_acpi_mr_restore_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(table_mr_size, FWCfgState), > + VMSTATE_UINT64(linker_mr_size, FWCfgState), > + VMSTATE_UINT64(rsdp_mr_size, FWCfgState), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > static const VMStateDescription vmstate_fw_cfg = { > .name = "fw_cfg", > .version_id = 2, > @@ -631,6 +690,7 @@ static const VMStateDescription vmstate_fw_cfg = { > }, > .subsections = (const VMStateDescription*[]) { > &vmstate_fw_cfg_dma, > + &vmstate_fw_cfg_acpi_mr, > NULL, > } > }; > @@ -815,6 +875,23 @@ static struct { > #define FW_CFG_ORDER_OVERRIDE_LAST 200 > }; > > +/* > + * Any sub-page size update to these table MRs will be lost during migration, > + * as we use aligned size in ram_load_precopy() -> qemu_ram_resize() path. > + * In order to avoid the inconsistency in sizes save them seperately and > + * migrate over in vmstate post_load(). > + */ > +static void fw_cfg_acpi_mr_save(FWCfgState *s, const char *filename, size_t len) > +{ > + if (!strcmp(filename, ACPI_BUILD_TABLE_FILE)) { > + s->table_mr_size = len; > + } else if (!strcmp(filename, ACPI_BUILD_LOADER_FILE)) { > + s->linker_mr_size = len; > + } else if (!strcmp(filename, ACPI_BUILD_RSDP_FILE)) { > + s->rsdp_mr_size = len; > + } > +} > + > static int get_fw_cfg_order(FWCfgState *s, const char *name) > { > int i; > @@ -914,6 +991,7 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > > s->files->count = cpu_to_be32(count+1); > + fw_cfg_acpi_mr_save(s, filename, len); > } > > void fw_cfg_add_file(FWCfgState *s, const char *filename, > @@ -937,6 +1015,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, > ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i, > data, len); > s->files->f[i].size = cpu_to_be32(len); > + fw_cfg_acpi_mr_save(s, filename, len); > return ptr; > } > } > @@ -973,7 +1052,10 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data) > qemu_register_reset(fw_cfg_machine_reset, s); > } > > - > +static Property fw_cfg_properties[] = { > + DEFINE_PROP_BOOL("acpi-mr-restore", FWCfgState, acpi_mr_restore, true), > + DEFINE_PROP_END_OF_LIST(), > +}; > > static void fw_cfg_common_realize(DeviceState *dev, Error **errp) > { > @@ -1097,6 +1179,8 @@ static void fw_cfg_class_init(ObjectClass *klass, void *data) > > dc->reset = fw_cfg_reset; > dc->vmsd = &vmstate_fw_cfg; > + > + device_class_set_props(dc, fw_cfg_properties); > } > > static const TypeInfo fw_cfg_info = { > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index b5291eefad..457fee7425 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -53,6 +53,12 @@ struct FWCfgState { > dma_addr_t dma_addr; > AddressSpace *dma_as; > MemoryRegion dma_iomem; > + > + /* restore during migration */ > + bool acpi_mr_restore; > + size_t table_mr_size; > + size_t linker_mr_size; > + size_t rsdp_mr_size; > }; > > struct FWCfgIoState { > -- > 2.17.1 >
On Wed, Mar 11, 2020 at 05:20:06PM +0000, Shameer Kolothum wrote: > Any sub-page size update to ACPI table MRs will be lost during > migration, as we use aligned size in ram_load_precopy() -> > qemu_ram_resize() path. This will result in inconsistency in sizes > between source and destination. In order to avoid this, save and > restore them separately during migration. Hmm but for old machine types we still have a problem right? How about aligning size on source for them? Then there won't be an inconsistency across migration. Wastes some boot time/memory but maybe that's better than a chance of not booting ... > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > Please find the discussion here, > https://patchwork.kernel.org/patch/11339591/ > --- > hw/core/machine.c | 1 + > hw/nvram/fw_cfg.c | 86 ++++++++++++++++++++++++++++++++++++++- > include/hw/nvram/fw_cfg.h | 6 +++ > 3 files changed, 92 insertions(+), 1 deletion(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 9e8c06036f..6d960bd47f 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -39,6 +39,7 @@ GlobalProperty hw_compat_4_2[] = { > { "usb-redir", "suppress-remote-wake", "off" }, > { "qxl", "revision", "4" }, > { "qxl-vga", "revision", "4" }, > + { "fw_cfg", "acpi-mr-restore", "false" }, > }; > const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 179b302f01..36d1e32f83 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -39,6 +39,7 @@ > #include "qemu/config-file.h" > #include "qemu/cutils.h" > #include "qapi/error.h" > +#include "hw/acpi/aml-build.h" > > #define FW_CFG_FILE_SLOTS_DFLT 0x20 > > @@ -610,6 +611,50 @@ bool fw_cfg_dma_enabled(void *opaque) > return s->dma_enabled; > } > > +static bool fw_cfg_acpi_mr_restore(void *opaque) > +{ > + FWCfgState *s = opaque; > + return s->acpi_mr_restore; > +} > + > +static void fw_cfg_update_mr(FWCfgState *s, uint16_t key, size_t size) > +{ > + MemoryRegion *mr; > + ram_addr_t offset; > + int arch = !!(key & FW_CFG_ARCH_LOCAL); > + void *ptr; > + > + key &= FW_CFG_ENTRY_MASK; > + assert(key < fw_cfg_max_entry(s)); > + > + ptr = s->entries[arch][key].data; > + mr = memory_region_from_host(ptr, &offset); > + > + memory_region_ram_resize(mr, size, &error_abort); > +} > + > +static int fw_cfg_acpi_mr_restore_post_load(void *opaque, int version_id) > +{ > + FWCfgState *s = opaque; > + int i, index; > + > + assert(s->files); > + > + index = be32_to_cpu(s->files->count); > + > + for (i = 0; i < index; i++) { > + if (!strcmp(s->files->f[i].name, ACPI_BUILD_TABLE_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->table_mr_size); > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_LOADER_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->linker_mr_size); > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_RSDP_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->rsdp_mr_size); > + } > + } > + > + return 0; > +} > + > static const VMStateDescription vmstate_fw_cfg_dma = { > .name = "fw_cfg/dma", > .needed = fw_cfg_dma_enabled, > @@ -619,6 +664,20 @@ static const VMStateDescription vmstate_fw_cfg_dma = { > }, > }; > > +static const VMStateDescription vmstate_fw_cfg_acpi_mr = { > + .name = "fw_cfg/acpi_mr", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = fw_cfg_acpi_mr_restore, > + .post_load = fw_cfg_acpi_mr_restore_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(table_mr_size, FWCfgState), > + VMSTATE_UINT64(linker_mr_size, FWCfgState), > + VMSTATE_UINT64(rsdp_mr_size, FWCfgState), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > static const VMStateDescription vmstate_fw_cfg = { > .name = "fw_cfg", > .version_id = 2, > @@ -631,6 +690,7 @@ static const VMStateDescription vmstate_fw_cfg = { > }, > .subsections = (const VMStateDescription*[]) { > &vmstate_fw_cfg_dma, > + &vmstate_fw_cfg_acpi_mr, > NULL, > } > }; > @@ -815,6 +875,23 @@ static struct { > #define FW_CFG_ORDER_OVERRIDE_LAST 200 > }; > > +/* > + * Any sub-page size update to these table MRs will be lost during migration, > + * as we use aligned size in ram_load_precopy() -> qemu_ram_resize() path. > + * In order to avoid the inconsistency in sizes save them seperately and > + * migrate over in vmstate post_load(). > + */ > +static void fw_cfg_acpi_mr_save(FWCfgState *s, const char *filename, size_t len) > +{ > + if (!strcmp(filename, ACPI_BUILD_TABLE_FILE)) { > + s->table_mr_size = len; > + } else if (!strcmp(filename, ACPI_BUILD_LOADER_FILE)) { > + s->linker_mr_size = len; > + } else if (!strcmp(filename, ACPI_BUILD_RSDP_FILE)) { > + s->rsdp_mr_size = len; > + } > +} > + > static int get_fw_cfg_order(FWCfgState *s, const char *name) > { > int i; > @@ -914,6 +991,7 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > > s->files->count = cpu_to_be32(count+1); > + fw_cfg_acpi_mr_save(s, filename, len); > } > > void fw_cfg_add_file(FWCfgState *s, const char *filename, > @@ -937,6 +1015,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, > ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i, > data, len); > s->files->f[i].size = cpu_to_be32(len); > + fw_cfg_acpi_mr_save(s, filename, len); > return ptr; > } > } > @@ -973,7 +1052,10 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data) > qemu_register_reset(fw_cfg_machine_reset, s); > } > > - > +static Property fw_cfg_properties[] = { > + DEFINE_PROP_BOOL("acpi-mr-restore", FWCfgState, acpi_mr_restore, true), > + DEFINE_PROP_END_OF_LIST(), > +}; > > static void fw_cfg_common_realize(DeviceState *dev, Error **errp) > { > @@ -1097,6 +1179,8 @@ static void fw_cfg_class_init(ObjectClass *klass, void *data) > > dc->reset = fw_cfg_reset; > dc->vmsd = &vmstate_fw_cfg; > + > + device_class_set_props(dc, fw_cfg_properties); > } > > static const TypeInfo fw_cfg_info = { > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index b5291eefad..457fee7425 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -53,6 +53,12 @@ struct FWCfgState { > dma_addr_t dma_addr; > AddressSpace *dma_as; > MemoryRegion dma_iomem; > + > + /* restore during migration */ > + bool acpi_mr_restore; > + size_t table_mr_size; > + size_t linker_mr_size; > + size_t rsdp_mr_size; > }; > > struct FWCfgIoState { > -- > 2.17.1 >
> -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: 11 March 2020 21:10 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > eric.auger@redhat.com; imammedo@redhat.com; peter.maydell@linaro.org; > shannon.zhaosl@gmail.com; xiaoguangrong.eric@gmail.com; > david@redhat.com; xuwei (O) <xuwei5@huawei.com>; lersek@redhat.com; > Linuxarm <linuxarm@huawei.com> > Subject: Re: [PATCH v3 02/10] fw_cfg: Migrate ACPI table mr sizes separately > > On Wed, Mar 11, 2020 at 05:20:06PM +0000, Shameer Kolothum wrote: > > Any sub-page size update to ACPI table MRs will be lost during > > migration, as we use aligned size in ram_load_precopy() -> > > qemu_ram_resize() path. This will result in inconsistency in sizes > > between source and destination. In order to avoid this, save and > > restore them separately during migration. > Is there a reason this is part of nvdimm patchset? Not really. But this problem is more visible if we have nvdimm hotplug support added to arm/virt. On x86, both acpi table and linker MRs are already aligned and I don't know a use case where you can change RSDP MR size(See below). > > Hmm but for old machine types we still have a problem right? > How about aligning size on source for them? > Then there won't be an inconsistency across migration. > Wastes some boot time/memory but maybe that's better > than a chance of not booting ... Right. That was considered. On x86, except RSDP MR, both the LINKER and ACPI TABLE MRs are already aligned/padded. And we cannot make RSDP mr aligned as it will break the seabios based boot. So a generic solution based on alignment is not possible unless we guarantee that RSDP is not going to be modified. What we could do for Arm/virt is just follow the x86 way and add padding for table and linker MRs. But this was discussed before and IIRC, was not well received. Thanks, Shameer > > Suggested-by: David Hildenbrand <david@redhat.com> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > --- > > Please find the discussion here, > > https://patchwork.kernel.org/patch/11339591/ > > --- > > hw/core/machine.c | 1 + > > hw/nvram/fw_cfg.c | 86 > ++++++++++++++++++++++++++++++++++++++- > > include/hw/nvram/fw_cfg.h | 6 +++ > > 3 files changed, 92 insertions(+), 1 deletion(-) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index 9e8c06036f..6d960bd47f 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -39,6 +39,7 @@ GlobalProperty hw_compat_4_2[] = { > > { "usb-redir", "suppress-remote-wake", "off" }, > > { "qxl", "revision", "4" }, > > { "qxl-vga", "revision", "4" }, > > + { "fw_cfg", "acpi-mr-restore", "false" }, > > }; > > const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); > > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > > index 179b302f01..36d1e32f83 100644 > > --- a/hw/nvram/fw_cfg.c > > +++ b/hw/nvram/fw_cfg.c > > @@ -39,6 +39,7 @@ > > #include "qemu/config-file.h" > > #include "qemu/cutils.h" > > #include "qapi/error.h" > > +#include "hw/acpi/aml-build.h" > > > > #define FW_CFG_FILE_SLOTS_DFLT 0x20 > > > > @@ -610,6 +611,50 @@ bool fw_cfg_dma_enabled(void *opaque) > > return s->dma_enabled; > > } > > > > +static bool fw_cfg_acpi_mr_restore(void *opaque) > > +{ > > + FWCfgState *s = opaque; > > + return s->acpi_mr_restore; > > +} > > + > > +static void fw_cfg_update_mr(FWCfgState *s, uint16_t key, size_t size) > > +{ > > + MemoryRegion *mr; > > + ram_addr_t offset; > > + int arch = !!(key & FW_CFG_ARCH_LOCAL); > > + void *ptr; > > + > > + key &= FW_CFG_ENTRY_MASK; > > + assert(key < fw_cfg_max_entry(s)); > > + > > + ptr = s->entries[arch][key].data; > > + mr = memory_region_from_host(ptr, &offset); > > + > > + memory_region_ram_resize(mr, size, &error_abort); > > +} > > + > > +static int fw_cfg_acpi_mr_restore_post_load(void *opaque, int version_id) > > +{ > > + FWCfgState *s = opaque; > > + int i, index; > > + > > + assert(s->files); > > + > > + index = be32_to_cpu(s->files->count); > > + > > + for (i = 0; i < index; i++) { > > + if (!strcmp(s->files->f[i].name, ACPI_BUILD_TABLE_FILE)) { > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > s->table_mr_size); > > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_LOADER_FILE)) { > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > s->linker_mr_size); > > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_RSDP_FILE)) { > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > s->rsdp_mr_size); > > + } > > + } > > + > > + return 0; > > +} > > + > > static const VMStateDescription vmstate_fw_cfg_dma = { > > .name = "fw_cfg/dma", > > .needed = fw_cfg_dma_enabled, > > @@ -619,6 +664,20 @@ static const VMStateDescription > vmstate_fw_cfg_dma = { > > }, > > }; > > > > +static const VMStateDescription vmstate_fw_cfg_acpi_mr = { > > + .name = "fw_cfg/acpi_mr", > > + .version_id = 1, > > + .minimum_version_id = 1, > > + .needed = fw_cfg_acpi_mr_restore, > > + .post_load = fw_cfg_acpi_mr_restore_post_load, > > + .fields = (VMStateField[]) { > > + VMSTATE_UINT64(table_mr_size, FWCfgState), > > + VMSTATE_UINT64(linker_mr_size, FWCfgState), > > + VMSTATE_UINT64(rsdp_mr_size, FWCfgState), > > + VMSTATE_END_OF_LIST() > > + }, > > +}; > > + > > static const VMStateDescription vmstate_fw_cfg = { > > .name = "fw_cfg", > > .version_id = 2, > > @@ -631,6 +690,7 @@ static const VMStateDescription vmstate_fw_cfg = { > > }, > > .subsections = (const VMStateDescription*[]) { > > &vmstate_fw_cfg_dma, > > + &vmstate_fw_cfg_acpi_mr, > > NULL, > > } > > }; > > @@ -815,6 +875,23 @@ static struct { > > #define FW_CFG_ORDER_OVERRIDE_LAST 200 > > }; > > > > +/* > > + * Any sub-page size update to these table MRs will be lost during migration, > > + * as we use aligned size in ram_load_precopy() -> qemu_ram_resize() path. > > + * In order to avoid the inconsistency in sizes save them seperately and > > + * migrate over in vmstate post_load(). > > + */ > > +static void fw_cfg_acpi_mr_save(FWCfgState *s, const char *filename, > size_t len) > > +{ > > + if (!strcmp(filename, ACPI_BUILD_TABLE_FILE)) { > > + s->table_mr_size = len; > > + } else if (!strcmp(filename, ACPI_BUILD_LOADER_FILE)) { > > + s->linker_mr_size = len; > > + } else if (!strcmp(filename, ACPI_BUILD_RSDP_FILE)) { > > + s->rsdp_mr_size = len; > > + } > > +} > > + > > static int get_fw_cfg_order(FWCfgState *s, const char *name) > > { > > int i; > > @@ -914,6 +991,7 @@ void fw_cfg_add_file_callback(FWCfgState *s, > const char *filename, > > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > > > > s->files->count = cpu_to_be32(count+1); > > + fw_cfg_acpi_mr_save(s, filename, len); > > } > > > > void fw_cfg_add_file(FWCfgState *s, const char *filename, > > @@ -937,6 +1015,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char > *filename, > > ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i, > > data, len); > > s->files->f[i].size = cpu_to_be32(len); > > + fw_cfg_acpi_mr_save(s, filename, len); > > return ptr; > > } > > } > > @@ -973,7 +1052,10 @@ static void fw_cfg_machine_ready(struct Notifier > *n, void *data) > > qemu_register_reset(fw_cfg_machine_reset, s); > > } > > > > - > > +static Property fw_cfg_properties[] = { > > + DEFINE_PROP_BOOL("acpi-mr-restore", FWCfgState, acpi_mr_restore, > true), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > > > static void fw_cfg_common_realize(DeviceState *dev, Error **errp) > > { > > @@ -1097,6 +1179,8 @@ static void fw_cfg_class_init(ObjectClass *klass, > void *data) > > > > dc->reset = fw_cfg_reset; > > dc->vmsd = &vmstate_fw_cfg; > > + > > + device_class_set_props(dc, fw_cfg_properties); > > } > > > > static const TypeInfo fw_cfg_info = { > > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > > index b5291eefad..457fee7425 100644 > > --- a/include/hw/nvram/fw_cfg.h > > +++ b/include/hw/nvram/fw_cfg.h > > @@ -53,6 +53,12 @@ struct FWCfgState { > > dma_addr_t dma_addr; > > AddressSpace *dma_as; > > MemoryRegion dma_iomem; > > + > > + /* restore during migration */ > > + bool acpi_mr_restore; > > + size_t table_mr_size; > > + size_t linker_mr_size; > > + size_t rsdp_mr_size; > > }; > > > > struct FWCfgIoState { > > -- > > 2.17.1 > >
On Thu, Mar 12, 2020 at 09:27:32AM +0000, Shameerali Kolothum Thodi wrote: > > > > -----Original Message----- > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > Sent: 11 March 2020 21:10 > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > > eric.auger@redhat.com; imammedo@redhat.com; peter.maydell@linaro.org; > > shannon.zhaosl@gmail.com; xiaoguangrong.eric@gmail.com; > > david@redhat.com; xuwei (O) <xuwei5@huawei.com>; lersek@redhat.com; > > Linuxarm <linuxarm@huawei.com> > > Subject: Re: [PATCH v3 02/10] fw_cfg: Migrate ACPI table mr sizes separately > > > > On Wed, Mar 11, 2020 at 05:20:06PM +0000, Shameer Kolothum wrote: > > > Any sub-page size update to ACPI table MRs will be lost during > > > migration, as we use aligned size in ram_load_precopy() -> > > > qemu_ram_resize() path. This will result in inconsistency in sizes > > > between source and destination. In order to avoid this, save and > > > restore them separately during migration. > > > > Is there a reason this is part of nvdimm patchset? > > Not really. But this problem is more visible if we have nvdimm hotplug > support added to arm/virt. On x86, both acpi table and linker MRs are already > aligned and I don't know a use case where you can change RSDP MR size(See below). > > > > > Hmm but for old machine types we still have a problem right? > > How about aligning size on source for them? > > Then there won't be an inconsistency across migration. > > Wastes some boot time/memory but maybe that's better > > than a chance of not booting ... > > Right. That was considered. On x86, except RSDP MR, both the LINKER and ACPI > TABLE MRs are already aligned/padded. And we cannot make RSDP mr aligned > as it will break the seabios based boot. Hmm. So right now if we migrate just before RSDP is read, there's a failure? > So a generic solution based on alignment > is not possible unless we guarantee that RSDP is not going to be modified. > > What we could do for Arm/virt is just follow the x86 way and add padding for > table and linker MRs. But this was discussed before and IIRC, was not well > received. > > Thanks, > Shameer > > > > Suggested-by: David Hildenbrand <david@redhat.com> > > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > > --- > > > Please find the discussion here, > > > https://patchwork.kernel.org/patch/11339591/ > > > --- > > > hw/core/machine.c | 1 + > > > hw/nvram/fw_cfg.c | 86 > > ++++++++++++++++++++++++++++++++++++++- > > > include/hw/nvram/fw_cfg.h | 6 +++ > > > 3 files changed, 92 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > > index 9e8c06036f..6d960bd47f 100644 > > > --- a/hw/core/machine.c > > > +++ b/hw/core/machine.c > > > @@ -39,6 +39,7 @@ GlobalProperty hw_compat_4_2[] = { > > > { "usb-redir", "suppress-remote-wake", "off" }, > > > { "qxl", "revision", "4" }, > > > { "qxl-vga", "revision", "4" }, > > > + { "fw_cfg", "acpi-mr-restore", "false" }, > > > }; > > > const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); > > > > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > > > index 179b302f01..36d1e32f83 100644 > > > --- a/hw/nvram/fw_cfg.c > > > +++ b/hw/nvram/fw_cfg.c > > > @@ -39,6 +39,7 @@ > > > #include "qemu/config-file.h" > > > #include "qemu/cutils.h" > > > #include "qapi/error.h" > > > +#include "hw/acpi/aml-build.h" > > > > > > #define FW_CFG_FILE_SLOTS_DFLT 0x20 > > > > > > @@ -610,6 +611,50 @@ bool fw_cfg_dma_enabled(void *opaque) > > > return s->dma_enabled; > > > } > > > > > > +static bool fw_cfg_acpi_mr_restore(void *opaque) > > > +{ > > > + FWCfgState *s = opaque; > > > + return s->acpi_mr_restore; > > > +} > > > + > > > +static void fw_cfg_update_mr(FWCfgState *s, uint16_t key, size_t size) > > > +{ > > > + MemoryRegion *mr; > > > + ram_addr_t offset; > > > + int arch = !!(key & FW_CFG_ARCH_LOCAL); > > > + void *ptr; > > > + > > > + key &= FW_CFG_ENTRY_MASK; > > > + assert(key < fw_cfg_max_entry(s)); > > > + > > > + ptr = s->entries[arch][key].data; > > > + mr = memory_region_from_host(ptr, &offset); > > > + > > > + memory_region_ram_resize(mr, size, &error_abort); > > > +} > > > + > > > +static int fw_cfg_acpi_mr_restore_post_load(void *opaque, int version_id) > > > +{ > > > + FWCfgState *s = opaque; > > > + int i, index; > > > + > > > + assert(s->files); > > > + > > > + index = be32_to_cpu(s->files->count); > > > + > > > + for (i = 0; i < index; i++) { > > > + if (!strcmp(s->files->f[i].name, ACPI_BUILD_TABLE_FILE)) { > > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > > s->table_mr_size); > > > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_LOADER_FILE)) { > > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > > s->linker_mr_size); > > > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_RSDP_FILE)) { > > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > > s->rsdp_mr_size); > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > + > > > static const VMStateDescription vmstate_fw_cfg_dma = { > > > .name = "fw_cfg/dma", > > > .needed = fw_cfg_dma_enabled, > > > @@ -619,6 +664,20 @@ static const VMStateDescription > > vmstate_fw_cfg_dma = { > > > }, > > > }; > > > > > > +static const VMStateDescription vmstate_fw_cfg_acpi_mr = { > > > + .name = "fw_cfg/acpi_mr", > > > + .version_id = 1, > > > + .minimum_version_id = 1, > > > + .needed = fw_cfg_acpi_mr_restore, > > > + .post_load = fw_cfg_acpi_mr_restore_post_load, > > > + .fields = (VMStateField[]) { > > > + VMSTATE_UINT64(table_mr_size, FWCfgState), > > > + VMSTATE_UINT64(linker_mr_size, FWCfgState), > > > + VMSTATE_UINT64(rsdp_mr_size, FWCfgState), > > > + VMSTATE_END_OF_LIST() > > > + }, > > > +}; > > > + > > > static const VMStateDescription vmstate_fw_cfg = { > > > .name = "fw_cfg", > > > .version_id = 2, > > > @@ -631,6 +690,7 @@ static const VMStateDescription vmstate_fw_cfg = { > > > }, > > > .subsections = (const VMStateDescription*[]) { > > > &vmstate_fw_cfg_dma, > > > + &vmstate_fw_cfg_acpi_mr, > > > NULL, > > > } > > > }; > > > @@ -815,6 +875,23 @@ static struct { > > > #define FW_CFG_ORDER_OVERRIDE_LAST 200 > > > }; > > > > > > +/* > > > + * Any sub-page size update to these table MRs will be lost during migration, > > > + * as we use aligned size in ram_load_precopy() -> qemu_ram_resize() path. > > > + * In order to avoid the inconsistency in sizes save them seperately and > > > + * migrate over in vmstate post_load(). > > > + */ > > > +static void fw_cfg_acpi_mr_save(FWCfgState *s, const char *filename, > > size_t len) > > > +{ > > > + if (!strcmp(filename, ACPI_BUILD_TABLE_FILE)) { > > > + s->table_mr_size = len; > > > + } else if (!strcmp(filename, ACPI_BUILD_LOADER_FILE)) { > > > + s->linker_mr_size = len; > > > + } else if (!strcmp(filename, ACPI_BUILD_RSDP_FILE)) { > > > + s->rsdp_mr_size = len; > > > + } > > > +} > > > + > > > static int get_fw_cfg_order(FWCfgState *s, const char *name) > > > { > > > int i; > > > @@ -914,6 +991,7 @@ void fw_cfg_add_file_callback(FWCfgState *s, > > const char *filename, > > > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > > > > > > s->files->count = cpu_to_be32(count+1); > > > + fw_cfg_acpi_mr_save(s, filename, len); > > > } > > > > > > void fw_cfg_add_file(FWCfgState *s, const char *filename, > > > @@ -937,6 +1015,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char > > *filename, > > > ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i, > > > data, len); > > > s->files->f[i].size = cpu_to_be32(len); > > > + fw_cfg_acpi_mr_save(s, filename, len); > > > return ptr; > > > } > > > } > > > @@ -973,7 +1052,10 @@ static void fw_cfg_machine_ready(struct Notifier > > *n, void *data) > > > qemu_register_reset(fw_cfg_machine_reset, s); > > > } > > > > > > - > > > +static Property fw_cfg_properties[] = { > > > + DEFINE_PROP_BOOL("acpi-mr-restore", FWCfgState, acpi_mr_restore, > > true), > > > + DEFINE_PROP_END_OF_LIST(), > > > +}; > > > > > > static void fw_cfg_common_realize(DeviceState *dev, Error **errp) > > > { > > > @@ -1097,6 +1179,8 @@ static void fw_cfg_class_init(ObjectClass *klass, > > void *data) > > > > > > dc->reset = fw_cfg_reset; > > > dc->vmsd = &vmstate_fw_cfg; > > > + > > > + device_class_set_props(dc, fw_cfg_properties); > > > } > > > > > > static const TypeInfo fw_cfg_info = { > > > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > > > index b5291eefad..457fee7425 100644 > > > --- a/include/hw/nvram/fw_cfg.h > > > +++ b/include/hw/nvram/fw_cfg.h > > > @@ -53,6 +53,12 @@ struct FWCfgState { > > > dma_addr_t dma_addr; > > > AddressSpace *dma_as; > > > MemoryRegion dma_iomem; > > > + > > > + /* restore during migration */ > > > + bool acpi_mr_restore; > > > + size_t table_mr_size; > > > + size_t linker_mr_size; > > > + size_t rsdp_mr_size; > > > }; > > > > > > struct FWCfgIoState { > > > -- > > > 2.17.1 > > >
> -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: 19 March 2020 17:51 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > eric.auger@redhat.com; imammedo@redhat.com; peter.maydell@linaro.org; > shannon.zhaosl@gmail.com; xiaoguangrong.eric@gmail.com; > david@redhat.com; xuwei (O) <xuwei5@huawei.com>; lersek@redhat.com; > Linuxarm <linuxarm@huawei.com> > Subject: Re: [PATCH v3 02/10] fw_cfg: Migrate ACPI table mr sizes separately > > On Thu, Mar 12, 2020 at 09:27:32AM +0000, Shameerali Kolothum Thodi > wrote: > > > > > > > -----Original Message----- > > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > > Sent: 11 March 2020 21:10 > > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > > > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > > > eric.auger@redhat.com; imammedo@redhat.com; > peter.maydell@linaro.org; > > > shannon.zhaosl@gmail.com; xiaoguangrong.eric@gmail.com; > > > david@redhat.com; xuwei (O) <xuwei5@huawei.com>; > lersek@redhat.com; > > > Linuxarm <linuxarm@huawei.com> > > > Subject: Re: [PATCH v3 02/10] fw_cfg: Migrate ACPI table mr sizes > separately > > > > > > On Wed, Mar 11, 2020 at 05:20:06PM +0000, Shameer Kolothum wrote: > > > > Any sub-page size update to ACPI table MRs will be lost during > > > > migration, as we use aligned size in ram_load_precopy() -> > > > > qemu_ram_resize() path. This will result in inconsistency in sizes > > > > between source and destination. In order to avoid this, save and > > > > restore them separately during migration. > > > > > > > Is there a reason this is part of nvdimm patchset? > > > > Not really. But this problem is more visible if we have nvdimm hotplug > > support added to arm/virt. On x86, both acpi table and linker MRs are already > > aligned and I don't know a use case where you can change RSDP MR size(See > below). > > > > > > > > Hmm but for old machine types we still have a problem right? > > > How about aligning size on source for them? > > > Then there won't be an inconsistency across migration. > > > Wastes some boot time/memory but maybe that's better > > > than a chance of not booting ... > > > > Right. That was considered. On x86, except RSDP MR, both the LINKER and > ACPI > > TABLE MRs are already aligned/padded. And we cannot make RSDP mr > aligned > > as it will break the seabios based boot. > > Hmm. So right now if we migrate just before RSDP is read, there's > a failure? I am not sure that will be the case. IIUC, on migration path, ram_load_precopy() -->qemu_ram_resize() won't be called as both length and block->used_length will be aligned size. Even if it calls, the current qemu_ram_resize() works on aligned size and wont invoke the callback to update the FWCfgEntry. And I believe on destination, the bios read will trigger the fw_cfg_select() which will call the acpi_build_update() to rebuild the tables and update the FWCfgEntry. Having said that my knowledge on this is limited, but I can test and confirm this, if there is an easy way to trigger this usecase. Please let me know. Thanks, Shameer > > So a generic solution based on alignment > > is not possible unless we guarantee that RSDP is not going to be modified. > > > > What we could do for Arm/virt is just follow the x86 way and add padding for > > table and linker MRs. But this was discussed before and IIRC, was not well > > received. > > > > Thanks, > > Shameer > > > > > > Suggested-by: David Hildenbrand <david@redhat.com> > > > > Signed-off-by: Shameer Kolothum > <shameerali.kolothum.thodi@huawei.com> > > > > --- > > > > Please find the discussion here, > > > > https://patchwork.kernel.org/patch/11339591/ > > > > --- > > > > hw/core/machine.c | 1 + > > > > hw/nvram/fw_cfg.c | 86 > > > ++++++++++++++++++++++++++++++++++++++- > > > > include/hw/nvram/fw_cfg.h | 6 +++ > > > > 3 files changed, 92 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > > > index 9e8c06036f..6d960bd47f 100644 > > > > --- a/hw/core/machine.c > > > > +++ b/hw/core/machine.c > > > > @@ -39,6 +39,7 @@ GlobalProperty hw_compat_4_2[] = { > > > > { "usb-redir", "suppress-remote-wake", "off" }, > > > > { "qxl", "revision", "4" }, > > > > { "qxl-vga", "revision", "4" }, > > > > + { "fw_cfg", "acpi-mr-restore", "false" }, > > > > }; > > > > const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); > > > > > > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > > > > index 179b302f01..36d1e32f83 100644 > > > > --- a/hw/nvram/fw_cfg.c > > > > +++ b/hw/nvram/fw_cfg.c > > > > @@ -39,6 +39,7 @@ > > > > #include "qemu/config-file.h" > > > > #include "qemu/cutils.h" > > > > #include "qapi/error.h" > > > > +#include "hw/acpi/aml-build.h" > > > > > > > > #define FW_CFG_FILE_SLOTS_DFLT 0x20 > > > > > > > > @@ -610,6 +611,50 @@ bool fw_cfg_dma_enabled(void *opaque) > > > > return s->dma_enabled; > > > > } > > > > > > > > +static bool fw_cfg_acpi_mr_restore(void *opaque) > > > > +{ > > > > + FWCfgState *s = opaque; > > > > + return s->acpi_mr_restore; > > > > +} > > > > + > > > > +static void fw_cfg_update_mr(FWCfgState *s, uint16_t key, size_t size) > > > > +{ > > > > + MemoryRegion *mr; > > > > + ram_addr_t offset; > > > > + int arch = !!(key & FW_CFG_ARCH_LOCAL); > > > > + void *ptr; > > > > + > > > > + key &= FW_CFG_ENTRY_MASK; > > > > + assert(key < fw_cfg_max_entry(s)); > > > > + > > > > + ptr = s->entries[arch][key].data; > > > > + mr = memory_region_from_host(ptr, &offset); > > > > + > > > > + memory_region_ram_resize(mr, size, &error_abort); > > > > +} > > > > + > > > > +static int fw_cfg_acpi_mr_restore_post_load(void *opaque, int > version_id) > > > > +{ > > > > + FWCfgState *s = opaque; > > > > + int i, index; > > > > + > > > > + assert(s->files); > > > > + > > > > + index = be32_to_cpu(s->files->count); > > > > + > > > > + for (i = 0; i < index; i++) { > > > > + if (!strcmp(s->files->f[i].name, ACPI_BUILD_TABLE_FILE)) { > > > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > > > s->table_mr_size); > > > > + } else if (!strcmp(s->files->f[i].name, > ACPI_BUILD_LOADER_FILE)) { > > > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > > > s->linker_mr_size); > > > > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_RSDP_FILE)) > { > > > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > > > s->rsdp_mr_size); > > > > + } > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static const VMStateDescription vmstate_fw_cfg_dma = { > > > > .name = "fw_cfg/dma", > > > > .needed = fw_cfg_dma_enabled, > > > > @@ -619,6 +664,20 @@ static const VMStateDescription > > > vmstate_fw_cfg_dma = { > > > > }, > > > > }; > > > > > > > > +static const VMStateDescription vmstate_fw_cfg_acpi_mr = { > > > > + .name = "fw_cfg/acpi_mr", > > > > + .version_id = 1, > > > > + .minimum_version_id = 1, > > > > + .needed = fw_cfg_acpi_mr_restore, > > > > + .post_load = fw_cfg_acpi_mr_restore_post_load, > > > > + .fields = (VMStateField[]) { > > > > + VMSTATE_UINT64(table_mr_size, FWCfgState), > > > > + VMSTATE_UINT64(linker_mr_size, FWCfgState), > > > > + VMSTATE_UINT64(rsdp_mr_size, FWCfgState), > > > > + VMSTATE_END_OF_LIST() > > > > + }, > > > > +}; > > > > + > > > > static const VMStateDescription vmstate_fw_cfg = { > > > > .name = "fw_cfg", > > > > .version_id = 2, > > > > @@ -631,6 +690,7 @@ static const VMStateDescription vmstate_fw_cfg > = { > > > > }, > > > > .subsections = (const VMStateDescription*[]) { > > > > &vmstate_fw_cfg_dma, > > > > + &vmstate_fw_cfg_acpi_mr, > > > > NULL, > > > > } > > > > }; > > > > @@ -815,6 +875,23 @@ static struct { > > > > #define FW_CFG_ORDER_OVERRIDE_LAST 200 > > > > }; > > > > > > > > +/* > > > > + * Any sub-page size update to these table MRs will be lost during > migration, > > > > + * as we use aligned size in ram_load_precopy() -> qemu_ram_resize() > path. > > > > + * In order to avoid the inconsistency in sizes save them seperately and > > > > + * migrate over in vmstate post_load(). > > > > + */ > > > > +static void fw_cfg_acpi_mr_save(FWCfgState *s, const char *filename, > > > size_t len) > > > > +{ > > > > + if (!strcmp(filename, ACPI_BUILD_TABLE_FILE)) { > > > > + s->table_mr_size = len; > > > > + } else if (!strcmp(filename, ACPI_BUILD_LOADER_FILE)) { > > > > + s->linker_mr_size = len; > > > > + } else if (!strcmp(filename, ACPI_BUILD_RSDP_FILE)) { > > > > + s->rsdp_mr_size = len; > > > > + } > > > > +} > > > > + > > > > static int get_fw_cfg_order(FWCfgState *s, const char *name) > > > > { > > > > int i; > > > > @@ -914,6 +991,7 @@ void fw_cfg_add_file_callback(FWCfgState *s, > > > const char *filename, > > > > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > > > > > > > > s->files->count = cpu_to_be32(count+1); > > > > + fw_cfg_acpi_mr_save(s, filename, len); > > > > } > > > > > > > > void fw_cfg_add_file(FWCfgState *s, const char *filename, > > > > @@ -937,6 +1015,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const > char > > > *filename, > > > > ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST > + i, > > > > data, len); > > > > s->files->f[i].size = cpu_to_be32(len); > > > > + fw_cfg_acpi_mr_save(s, filename, len); > > > > return ptr; > > > > } > > > > } > > > > @@ -973,7 +1052,10 @@ static void fw_cfg_machine_ready(struct > Notifier > > > *n, void *data) > > > > qemu_register_reset(fw_cfg_machine_reset, s); > > > > } > > > > > > > > - > > > > +static Property fw_cfg_properties[] = { > > > > + DEFINE_PROP_BOOL("acpi-mr-restore", FWCfgState, > acpi_mr_restore, > > > true), > > > > + DEFINE_PROP_END_OF_LIST(), > > > > +}; > > > > > > > > static void fw_cfg_common_realize(DeviceState *dev, Error **errp) > > > > { > > > > @@ -1097,6 +1179,8 @@ static void fw_cfg_class_init(ObjectClass > *klass, > > > void *data) > > > > > > > > dc->reset = fw_cfg_reset; > > > > dc->vmsd = &vmstate_fw_cfg; > > > > + > > > > + device_class_set_props(dc, fw_cfg_properties); > > > > } > > > > > > > > static const TypeInfo fw_cfg_info = { > > > > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > > > > index b5291eefad..457fee7425 100644 > > > > --- a/include/hw/nvram/fw_cfg.h > > > > +++ b/include/hw/nvram/fw_cfg.h > > > > @@ -53,6 +53,12 @@ struct FWCfgState { > > > > dma_addr_t dma_addr; > > > > AddressSpace *dma_as; > > > > MemoryRegion dma_iomem; > > > > + > > > > + /* restore during migration */ > > > > + bool acpi_mr_restore; > > > > + size_t table_mr_size; > > > > + size_t linker_mr_size; > > > > + size_t rsdp_mr_size; > > > > }; > > > > > > > > struct FWCfgIoState { > > > > -- > > > > 2.17.1 > > > >
On Wed, 11 Mar 2020 17:20:06 +0000 Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote: > Any sub-page size update to ACPI table MRs will be lost during > migration, as we use aligned size in ram_load_precopy() -> > qemu_ram_resize() path. This will result in inconsistency in sizes > between source and destination. I'm not sure what problem is and if it matters in case of migration, an example here with numbers from affected acpi blob would be useful here. PS: could you point to mail thread where problem was discussed > In order to avoid this, save and > restore them separately during migration. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > Please find the discussion here, > https://patchwork.kernel.org/patch/11339591/ > --- > hw/core/machine.c | 1 + > hw/nvram/fw_cfg.c | 86 ++++++++++++++++++++++++++++++++++++++- > include/hw/nvram/fw_cfg.h | 6 +++ > 3 files changed, 92 insertions(+), 1 deletion(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 9e8c06036f..6d960bd47f 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -39,6 +39,7 @@ GlobalProperty hw_compat_4_2[] = { > { "usb-redir", "suppress-remote-wake", "off" }, > { "qxl", "revision", "4" }, > { "qxl-vga", "revision", "4" }, > + { "fw_cfg", "acpi-mr-restore", "false" }, > }; > const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 179b302f01..36d1e32f83 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -39,6 +39,7 @@ > #include "qemu/config-file.h" > #include "qemu/cutils.h" > #include "qapi/error.h" > +#include "hw/acpi/aml-build.h" > > #define FW_CFG_FILE_SLOTS_DFLT 0x20 > > @@ -610,6 +611,50 @@ bool fw_cfg_dma_enabled(void *opaque) > return s->dma_enabled; > } > > +static bool fw_cfg_acpi_mr_restore(void *opaque) > +{ > + FWCfgState *s = opaque; > + return s->acpi_mr_restore; > +} > + > +static void fw_cfg_update_mr(FWCfgState *s, uint16_t key, size_t size) > +{ > + MemoryRegion *mr; > + ram_addr_t offset; > + int arch = !!(key & FW_CFG_ARCH_LOCAL); > + void *ptr; > + > + key &= FW_CFG_ENTRY_MASK; > + assert(key < fw_cfg_max_entry(s)); > + > + ptr = s->entries[arch][key].data; > + mr = memory_region_from_host(ptr, &offset); > + > + memory_region_ram_resize(mr, size, &error_abort); > +} > + > +static int fw_cfg_acpi_mr_restore_post_load(void *opaque, int version_id) > +{ > + FWCfgState *s = opaque; > + int i, index; > + > + assert(s->files); > + > + index = be32_to_cpu(s->files->count); > + > + for (i = 0; i < index; i++) { > + if (!strcmp(s->files->f[i].name, ACPI_BUILD_TABLE_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->table_mr_size); > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_LOADER_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->linker_mr_size); > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_RSDP_FILE)) { > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->rsdp_mr_size); > + } > + } > + > + return 0; > +} > + > static const VMStateDescription vmstate_fw_cfg_dma = { > .name = "fw_cfg/dma", > .needed = fw_cfg_dma_enabled, > @@ -619,6 +664,20 @@ static const VMStateDescription vmstate_fw_cfg_dma = { > }, > }; > > +static const VMStateDescription vmstate_fw_cfg_acpi_mr = { > + .name = "fw_cfg/acpi_mr", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = fw_cfg_acpi_mr_restore, > + .post_load = fw_cfg_acpi_mr_restore_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(table_mr_size, FWCfgState), > + VMSTATE_UINT64(linker_mr_size, FWCfgState), > + VMSTATE_UINT64(rsdp_mr_size, FWCfgState), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > static const VMStateDescription vmstate_fw_cfg = { > .name = "fw_cfg", > .version_id = 2, > @@ -631,6 +690,7 @@ static const VMStateDescription vmstate_fw_cfg = { > }, > .subsections = (const VMStateDescription*[]) { > &vmstate_fw_cfg_dma, > + &vmstate_fw_cfg_acpi_mr, > NULL, > } > }; > @@ -815,6 +875,23 @@ static struct { > #define FW_CFG_ORDER_OVERRIDE_LAST 200 > }; > > +/* > + * Any sub-page size update to these table MRs will be lost during migration, > + * as we use aligned size in ram_load_precopy() -> qemu_ram_resize() path. > + * In order to avoid the inconsistency in sizes save them seperately and > + * migrate over in vmstate post_load(). > + */ > +static void fw_cfg_acpi_mr_save(FWCfgState *s, const char *filename, size_t len) > +{ > + if (!strcmp(filename, ACPI_BUILD_TABLE_FILE)) { > + s->table_mr_size = len; > + } else if (!strcmp(filename, ACPI_BUILD_LOADER_FILE)) { > + s->linker_mr_size = len; > + } else if (!strcmp(filename, ACPI_BUILD_RSDP_FILE)) { > + s->rsdp_mr_size = len; > + } > +} > + > static int get_fw_cfg_order(FWCfgState *s, const char *name) > { > int i; > @@ -914,6 +991,7 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > > s->files->count = cpu_to_be32(count+1); > + fw_cfg_acpi_mr_save(s, filename, len); > } > > void fw_cfg_add_file(FWCfgState *s, const char *filename, > @@ -937,6 +1015,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, > ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i, > data, len); > s->files->f[i].size = cpu_to_be32(len); > + fw_cfg_acpi_mr_save(s, filename, len); > return ptr; > } > } > @@ -973,7 +1052,10 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data) > qemu_register_reset(fw_cfg_machine_reset, s); > } > > - > +static Property fw_cfg_properties[] = { > + DEFINE_PROP_BOOL("acpi-mr-restore", FWCfgState, acpi_mr_restore, true), > + DEFINE_PROP_END_OF_LIST(), > +}; > > static void fw_cfg_common_realize(DeviceState *dev, Error **errp) > { > @@ -1097,6 +1179,8 @@ static void fw_cfg_class_init(ObjectClass *klass, void *data) > > dc->reset = fw_cfg_reset; > dc->vmsd = &vmstate_fw_cfg; > + > + device_class_set_props(dc, fw_cfg_properties); > } > > static const TypeInfo fw_cfg_info = { > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index b5291eefad..457fee7425 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -53,6 +53,12 @@ struct FWCfgState { > dma_addr_t dma_addr; > AddressSpace *dma_as; > MemoryRegion dma_iomem; > + > + /* restore during migration */ > + bool acpi_mr_restore; > + size_t table_mr_size; > + size_t linker_mr_size; > + size_t rsdp_mr_size; > }; > > struct FWCfgIoState {
> -----Original Message----- > From: Igor Mammedov [mailto:imammedo@redhat.com] > Sent: 23 March 2020 12:35 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; > eric.auger@redhat.com; peter.maydell@linaro.org; > xiaoguangrong.eric@gmail.com; david@redhat.com; mst@redhat.com; > Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>; > shannon.zhaosl@gmail.com; lersek@redhat.com > Subject: Re: [PATCH v3 02/10] fw_cfg: Migrate ACPI table mr sizes separately > > On Wed, 11 Mar 2020 17:20:06 +0000 > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote: > > > Any sub-page size update to ACPI table MRs will be lost during > > migration, as we use aligned size in ram_load_precopy() -> > > qemu_ram_resize() path. This will result in inconsistency in sizes > > between source and destination. > I'm not sure what problem is and if it matters in case of migration, > an example here with numbers from affected acpi blob would be useful here. This happens when we try to fix the qemu_ram_resize() callback for sub-age changes(patch # 03/10 in this series). In the previous discussion David Hildenbrand pointed out that the fix will create an inconsistency between source and target. I can add more details and some numbers in the commit log here. > PS: > could you point to mail thread where problem was discussed It is here , https://patchwork.kernel.org/patch/11339591/#23138505 Thanks, Shameer > > In order to avoid this, save and > > restore them separately during migration. > > > > Suggested-by: David Hildenbrand <david@redhat.com> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > --- > > Please find the discussion here, > > https://patchwork.kernel.org/patch/11339591/ > > --- > > hw/core/machine.c | 1 + > > hw/nvram/fw_cfg.c | 86 > ++++++++++++++++++++++++++++++++++++++- > > include/hw/nvram/fw_cfg.h | 6 +++ > > 3 files changed, 92 insertions(+), 1 deletion(-) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index 9e8c06036f..6d960bd47f 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -39,6 +39,7 @@ GlobalProperty hw_compat_4_2[] = { > > { "usb-redir", "suppress-remote-wake", "off" }, > > { "qxl", "revision", "4" }, > > { "qxl-vga", "revision", "4" }, > > + { "fw_cfg", "acpi-mr-restore", "false" }, > > }; > > const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); > > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > > index 179b302f01..36d1e32f83 100644 > > --- a/hw/nvram/fw_cfg.c > > +++ b/hw/nvram/fw_cfg.c > > @@ -39,6 +39,7 @@ > > #include "qemu/config-file.h" > > #include "qemu/cutils.h" > > #include "qapi/error.h" > > +#include "hw/acpi/aml-build.h" > > > > #define FW_CFG_FILE_SLOTS_DFLT 0x20 > > > > @@ -610,6 +611,50 @@ bool fw_cfg_dma_enabled(void *opaque) > > return s->dma_enabled; > > } > > > > +static bool fw_cfg_acpi_mr_restore(void *opaque) > > +{ > > + FWCfgState *s = opaque; > > + return s->acpi_mr_restore; > > +} > > + > > +static void fw_cfg_update_mr(FWCfgState *s, uint16_t key, size_t size) > > +{ > > + MemoryRegion *mr; > > + ram_addr_t offset; > > + int arch = !!(key & FW_CFG_ARCH_LOCAL); > > + void *ptr; > > + > > + key &= FW_CFG_ENTRY_MASK; > > + assert(key < fw_cfg_max_entry(s)); > > + > > + ptr = s->entries[arch][key].data; > > + mr = memory_region_from_host(ptr, &offset); > > + > > + memory_region_ram_resize(mr, size, &error_abort); > > +} > > + > > +static int fw_cfg_acpi_mr_restore_post_load(void *opaque, int version_id) > > +{ > > + FWCfgState *s = opaque; > > + int i, index; > > + > > + assert(s->files); > > + > > + index = be32_to_cpu(s->files->count); > > + > > + for (i = 0; i < index; i++) { > > + if (!strcmp(s->files->f[i].name, ACPI_BUILD_TABLE_FILE)) { > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > s->table_mr_size); > > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_LOADER_FILE)) { > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > s->linker_mr_size); > > + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_RSDP_FILE)) { > > + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, > s->rsdp_mr_size); > > + } > > + } > > + > > + return 0; > > +} > > + > > static const VMStateDescription vmstate_fw_cfg_dma = { > > .name = "fw_cfg/dma", > > .needed = fw_cfg_dma_enabled, > > @@ -619,6 +664,20 @@ static const VMStateDescription > vmstate_fw_cfg_dma = { > > }, > > }; > > > > +static const VMStateDescription vmstate_fw_cfg_acpi_mr = { > > + .name = "fw_cfg/acpi_mr", > > + .version_id = 1, > > + .minimum_version_id = 1, > > + .needed = fw_cfg_acpi_mr_restore, > > + .post_load = fw_cfg_acpi_mr_restore_post_load, > > + .fields = (VMStateField[]) { > > + VMSTATE_UINT64(table_mr_size, FWCfgState), > > + VMSTATE_UINT64(linker_mr_size, FWCfgState), > > + VMSTATE_UINT64(rsdp_mr_size, FWCfgState), > > + VMSTATE_END_OF_LIST() > > + }, > > +}; > > + > > static const VMStateDescription vmstate_fw_cfg = { > > .name = "fw_cfg", > > .version_id = 2, > > @@ -631,6 +690,7 @@ static const VMStateDescription vmstate_fw_cfg = { > > }, > > .subsections = (const VMStateDescription*[]) { > > &vmstate_fw_cfg_dma, > > + &vmstate_fw_cfg_acpi_mr, > > NULL, > > } > > }; > > @@ -815,6 +875,23 @@ static struct { > > #define FW_CFG_ORDER_OVERRIDE_LAST 200 > > }; > > > > +/* > > + * Any sub-page size update to these table MRs will be lost during migration, > > + * as we use aligned size in ram_load_precopy() -> qemu_ram_resize() path. > > + * In order to avoid the inconsistency in sizes save them seperately and > > + * migrate over in vmstate post_load(). > > + */ > > +static void fw_cfg_acpi_mr_save(FWCfgState *s, const char *filename, > size_t len) > > +{ > > + if (!strcmp(filename, ACPI_BUILD_TABLE_FILE)) { > > + s->table_mr_size = len; > > + } else if (!strcmp(filename, ACPI_BUILD_LOADER_FILE)) { > > + s->linker_mr_size = len; > > + } else if (!strcmp(filename, ACPI_BUILD_RSDP_FILE)) { > > + s->rsdp_mr_size = len; > > + } > > +} > > + > > static int get_fw_cfg_order(FWCfgState *s, const char *name) > > { > > int i; > > @@ -914,6 +991,7 @@ void fw_cfg_add_file_callback(FWCfgState *s, > const char *filename, > > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > > > > s->files->count = cpu_to_be32(count+1); > > + fw_cfg_acpi_mr_save(s, filename, len); > > } > > > > void fw_cfg_add_file(FWCfgState *s, const char *filename, > > @@ -937,6 +1015,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char > *filename, > > ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i, > > data, len); > > s->files->f[i].size = cpu_to_be32(len); > > + fw_cfg_acpi_mr_save(s, filename, len); > > return ptr; > > } > > } > > @@ -973,7 +1052,10 @@ static void fw_cfg_machine_ready(struct Notifier > *n, void *data) > > qemu_register_reset(fw_cfg_machine_reset, s); > > } > > > > - > > +static Property fw_cfg_properties[] = { > > + DEFINE_PROP_BOOL("acpi-mr-restore", FWCfgState, acpi_mr_restore, > true), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > > > static void fw_cfg_common_realize(DeviceState *dev, Error **errp) > > { > > @@ -1097,6 +1179,8 @@ static void fw_cfg_class_init(ObjectClass *klass, > void *data) > > > > dc->reset = fw_cfg_reset; > > dc->vmsd = &vmstate_fw_cfg; > > + > > + device_class_set_props(dc, fw_cfg_properties); > > } > > > > static const TypeInfo fw_cfg_info = { > > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > > index b5291eefad..457fee7425 100644 > > --- a/include/hw/nvram/fw_cfg.h > > +++ b/include/hw/nvram/fw_cfg.h > > @@ -53,6 +53,12 @@ struct FWCfgState { > > dma_addr_t dma_addr; > > AddressSpace *dma_as; > > MemoryRegion dma_iomem; > > + > > + /* restore during migration */ > > + bool acpi_mr_restore; > > + size_t table_mr_size; > > + size_t linker_mr_size; > > + size_t rsdp_mr_size; > > }; > > > > struct FWCfgIoState {
diff --git a/hw/core/machine.c b/hw/core/machine.c index 9e8c06036f..6d960bd47f 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -39,6 +39,7 @@ GlobalProperty hw_compat_4_2[] = { { "usb-redir", "suppress-remote-wake", "off" }, { "qxl", "revision", "4" }, { "qxl-vga", "revision", "4" }, + { "fw_cfg", "acpi-mr-restore", "false" }, }; const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 179b302f01..36d1e32f83 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -39,6 +39,7 @@ #include "qemu/config-file.h" #include "qemu/cutils.h" #include "qapi/error.h" +#include "hw/acpi/aml-build.h" #define FW_CFG_FILE_SLOTS_DFLT 0x20 @@ -610,6 +611,50 @@ bool fw_cfg_dma_enabled(void *opaque) return s->dma_enabled; } +static bool fw_cfg_acpi_mr_restore(void *opaque) +{ + FWCfgState *s = opaque; + return s->acpi_mr_restore; +} + +static void fw_cfg_update_mr(FWCfgState *s, uint16_t key, size_t size) +{ + MemoryRegion *mr; + ram_addr_t offset; + int arch = !!(key & FW_CFG_ARCH_LOCAL); + void *ptr; + + key &= FW_CFG_ENTRY_MASK; + assert(key < fw_cfg_max_entry(s)); + + ptr = s->entries[arch][key].data; + mr = memory_region_from_host(ptr, &offset); + + memory_region_ram_resize(mr, size, &error_abort); +} + +static int fw_cfg_acpi_mr_restore_post_load(void *opaque, int version_id) +{ + FWCfgState *s = opaque; + int i, index; + + assert(s->files); + + index = be32_to_cpu(s->files->count); + + for (i = 0; i < index; i++) { + if (!strcmp(s->files->f[i].name, ACPI_BUILD_TABLE_FILE)) { + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->table_mr_size); + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_LOADER_FILE)) { + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->linker_mr_size); + } else if (!strcmp(s->files->f[i].name, ACPI_BUILD_RSDP_FILE)) { + fw_cfg_update_mr(s, FW_CFG_FILE_FIRST + i, s->rsdp_mr_size); + } + } + + return 0; +} + static const VMStateDescription vmstate_fw_cfg_dma = { .name = "fw_cfg/dma", .needed = fw_cfg_dma_enabled, @@ -619,6 +664,20 @@ static const VMStateDescription vmstate_fw_cfg_dma = { }, }; +static const VMStateDescription vmstate_fw_cfg_acpi_mr = { + .name = "fw_cfg/acpi_mr", + .version_id = 1, + .minimum_version_id = 1, + .needed = fw_cfg_acpi_mr_restore, + .post_load = fw_cfg_acpi_mr_restore_post_load, + .fields = (VMStateField[]) { + VMSTATE_UINT64(table_mr_size, FWCfgState), + VMSTATE_UINT64(linker_mr_size, FWCfgState), + VMSTATE_UINT64(rsdp_mr_size, FWCfgState), + VMSTATE_END_OF_LIST() + }, +}; + static const VMStateDescription vmstate_fw_cfg = { .name = "fw_cfg", .version_id = 2, @@ -631,6 +690,7 @@ static const VMStateDescription vmstate_fw_cfg = { }, .subsections = (const VMStateDescription*[]) { &vmstate_fw_cfg_dma, + &vmstate_fw_cfg_acpi_mr, NULL, } }; @@ -815,6 +875,23 @@ static struct { #define FW_CFG_ORDER_OVERRIDE_LAST 200 }; +/* + * Any sub-page size update to these table MRs will be lost during migration, + * as we use aligned size in ram_load_precopy() -> qemu_ram_resize() path. + * In order to avoid the inconsistency in sizes save them seperately and + * migrate over in vmstate post_load(). + */ +static void fw_cfg_acpi_mr_save(FWCfgState *s, const char *filename, size_t len) +{ + if (!strcmp(filename, ACPI_BUILD_TABLE_FILE)) { + s->table_mr_size = len; + } else if (!strcmp(filename, ACPI_BUILD_LOADER_FILE)) { + s->linker_mr_size = len; + } else if (!strcmp(filename, ACPI_BUILD_RSDP_FILE)) { + s->rsdp_mr_size = len; + } +} + static int get_fw_cfg_order(FWCfgState *s, const char *name) { int i; @@ -914,6 +991,7 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); s->files->count = cpu_to_be32(count+1); + fw_cfg_acpi_mr_save(s, filename, len); } void fw_cfg_add_file(FWCfgState *s, const char *filename, @@ -937,6 +1015,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i, data, len); s->files->f[i].size = cpu_to_be32(len); + fw_cfg_acpi_mr_save(s, filename, len); return ptr; } } @@ -973,7 +1052,10 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data) qemu_register_reset(fw_cfg_machine_reset, s); } - +static Property fw_cfg_properties[] = { + DEFINE_PROP_BOOL("acpi-mr-restore", FWCfgState, acpi_mr_restore, true), + DEFINE_PROP_END_OF_LIST(), +}; static void fw_cfg_common_realize(DeviceState *dev, Error **errp) { @@ -1097,6 +1179,8 @@ static void fw_cfg_class_init(ObjectClass *klass, void *data) dc->reset = fw_cfg_reset; dc->vmsd = &vmstate_fw_cfg; + + device_class_set_props(dc, fw_cfg_properties); } static const TypeInfo fw_cfg_info = { diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h index b5291eefad..457fee7425 100644 --- a/include/hw/nvram/fw_cfg.h +++ b/include/hw/nvram/fw_cfg.h @@ -53,6 +53,12 @@ struct FWCfgState { dma_addr_t dma_addr; AddressSpace *dma_as; MemoryRegion dma_iomem; + + /* restore during migration */ + bool acpi_mr_restore; + size_t table_mr_size; + size_t linker_mr_size; + size_t rsdp_mr_size; }; struct FWCfgIoState {
Any sub-page size update to ACPI table MRs will be lost during migration, as we use aligned size in ram_load_precopy() -> qemu_ram_resize() path. This will result in inconsistency in sizes between source and destination. In order to avoid this, save and restore them separately during migration. Suggested-by: David Hildenbrand <david@redhat.com> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- Please find the discussion here, https://patchwork.kernel.org/patch/11339591/ --- hw/core/machine.c | 1 + hw/nvram/fw_cfg.c | 86 ++++++++++++++++++++++++++++++++++++++- include/hw/nvram/fw_cfg.h | 6 +++ 3 files changed, 92 insertions(+), 1 deletion(-)