On Tue, Apr 07, 2020 at 04:17:46PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/3/20 12:18 PM, Shameer Kolothum wrote:
> > Any sub-page size update to ACPI 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
> > FWCfgEntry sizes between source and destination. In order to avoid
> > this, save and restore them separately during migration.
> >
> > Up until now, this problem may not be that relevant for x86 as both
> > ACPI table and Linker MRs gets padded and aligned. Also at present,
> > qemu_ram_resize() doesn't invoke callback to update FWCfgEntry for
> > unaligned size changes. But since we are going to fix the
> > qemu_ram_resize() in the subsequent patch, the issue may become
> > more serious especially for RSDP MR case.
> >
> > Moreover, the issue will soon become prominent in arm/virt as well
> > where the MRs are not padded or aligned at all and eventually have
> > acpi table changes as part of future additions like NVDIMM hot-add
> > feature.
> >
> > Suggested-by: David Hildenbrand <da...@redhat.com>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.th...@huawei.com>
> > Acked-by: David Hildenbrand <da...@redhat.com>
> > ---
> > v1 --> v2
> > - Changed *_mr_size from size_t to uint64_t to address portability.
> > - post_copy only done if sizes are not aligned.
> >
> > Please find previous discussions here,
> > https://patchwork.kernel.org/patch/11339591/#23140343
> > ---
> > hw/core/machine.c | 1 +
> > hw/nvram/fw_cfg.c | 91 ++++++++++++++++++++++++++++++++++++++-
> > include/hw/nvram/fw_cfg.h | 6 +++
> > 3 files changed, 97 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index de0c425605..c1a444cb75 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..4be6c9d9fd 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,55 @@ bool fw_cfg_dma_enabled(void *opaque)
> > return s->dma_enabled;
> > }
> > +static bool fw_cfg_acpi_mr_restore(void *opaque)
> > +{
> > + FWCfgState *s = opaque;
> > + bool mr_aligned;
> > +
> > + mr_aligned = QEMU_IS_ALIGNED(s->table_mr_size,
> > qemu_real_host_page_size) &&
> > + QEMU_IS_ALIGNED(s->linker_mr_size,
> > qemu_real_host_page_size) &&
> > + QEMU_IS_ALIGNED(s->rsdp_mr_size,
> > qemu_real_host_page_size);
> > + return s->acpi_mr_restore && !mr_aligned;
>
> This code is hard to review.
>
> Is this equivalent?
>
> if (!s->acpi_mr_restore) {
> return false;
> }
> if (!QEMU_IS_ALIGNED(s->table_mr_size, qemu_real_host_page_size)) {
> return false;
> }
> if (!QEMU_IS_ALIGNED(s->linker_mr_size, qemu_real_host_page_size)) {
> return false;
> }
> if (!QEMU_IS_ALIGNED(s->rsdp_mr_size, qemu_real_host_page_size)) {
> return false;
> }
> return true;
I think I prefer the original version though. Matter of taste?
> > +}
> > +
> > +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 +669,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 +695,7 @@ static const VMStateDescription vmstate_fw_cfg = {
> > },
> > .subsections = (const VMStateDescription*[]) {
> > &vmstate_fw_cfg_dma,
> > + &vmstate_fw_cfg_acpi_mr,
> > NULL,
> > }
> > };
> > @@ -815,6 +880,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 +996,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 +1020,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 +1057,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 +1184,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..25d9307018 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;
> > + uint64_t table_mr_size;
> > + uint64_t linker_mr_size;
> > + uint64_t rsdp_mr_size;
> > };
> > struct FWCfgIoState {
> >
