* [PATCH for-5.0 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration
@ 2020-03-30 16:49 Shameer Kolothum
2020-03-30 16:49 ` [PATCH for-5.0 1/3] acpi: Use macro for table-loader file name Shameer Kolothum
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Shameer Kolothum @ 2020-03-30 16:49 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, david, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
This is to fix few issues discovered while adding NVDIMM hot-add
support to arm/virt. These were previously part of [1] and since
the fixes are generic in nature and might be an issue in x86 as
well, they are being treated separately now.
1. https://patchwork.kernel.org/cover/11432383/
Updates:
-Added R-by and A-by tags.
-Edited commit log for patch#2
-Updated patch#3 as per David's comment
David Hildenbrand (1):
exec: Fix for qemu_ram_resize() callback
Shameer Kolothum (2):
acpi: Use macro for table-loader file name
fw_cfg: Migrate ACPI table mr sizes separately
exec.c | 16 ++++++-
hw/arm/virt-acpi-build.c | 2 +-
hw/core/machine.c | 1 +
hw/i386/acpi-build.c | 2 +-
hw/nvram/fw_cfg.c | 86 ++++++++++++++++++++++++++++++++++++-
include/hw/acpi/aml-build.h | 1 +
include/hw/nvram/fw_cfg.h | 6 +++
7 files changed, 109 insertions(+), 5 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH for-5.0 1/3] acpi: Use macro for table-loader file name
2020-03-30 16:49 [PATCH for-5.0 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration Shameer Kolothum
@ 2020-03-30 16:49 ` Shameer Kolothum
2020-03-30 16:49 ` [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately Shameer Kolothum
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Shameer Kolothum @ 2020-03-30 16:49 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, david, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
Use macro for "etc/table-loader" and move it to the header
file similar to ACPI_BUILD_TABLE_FILE/ACPI_BUILD_RSDP_FILE etc.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
hw/arm/virt-acpi-build.c | 2 +-
hw/i386/acpi-build.c | 2 +-
include/hw/acpi/aml-build.h | 1 +
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index fb4b166f82..c13710b727 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -929,7 +929,7 @@ void virt_acpi_setup(VirtMachineState *vms)
build_state->linker_mr =
acpi_add_rom_blob(virt_acpi_build_update, build_state,
- tables.linker->cmd_blob, "etc/table-loader", 0);
+ tables.linker->cmd_blob, ACPI_BUILD_LOADER_FILE, 0);
fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, tables.tcpalog->data,
acpi_data_len(tables.tcpalog));
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9a19c14e66..80f05d728d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -3043,7 +3043,7 @@ void acpi_setup(void)
build_state->linker_mr =
acpi_add_rom_blob(acpi_build_update, build_state,
- tables.linker->cmd_blob, "etc/table-loader", 0);
+ tables.linker->cmd_blob, ACPI_BUILD_LOADER_FILE, 0);
fw_cfg_add_file(x86ms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
tables.tcpalog->data, acpi_data_len(tables.tcpalog));
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index de4a406568..0f4ed53d7f 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -13,6 +13,7 @@
#define ACPI_BUILD_TABLE_FILE "etc/acpi/tables"
#define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"
#define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log"
+#define ACPI_BUILD_LOADER_FILE "etc/table-loader"
#define AML_NOTIFY_METHOD "NTFY"
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately
2020-03-30 16:49 [PATCH for-5.0 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration Shameer Kolothum
2020-03-30 16:49 ` [PATCH for-5.0 1/3] acpi: Use macro for table-loader file name Shameer Kolothum
@ 2020-03-30 16:49 ` Shameer Kolothum
2020-03-31 10:45 ` Dr. David Alan Gilbert
` (2 more replies)
2020-03-30 16:49 ` [PATCH for-5.0 3/3] exec: Fix for qemu_ram_resize() callback Shameer Kolothum
2020-03-30 19:59 ` [PATCH for-5.0 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration no-reply
3 siblings, 3 replies; 11+ messages in thread
From: Shameer Kolothum @ 2020-03-30 16:49 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, david, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
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 <david@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Acked-by: David Hildenbrand <david@redhat.com>
---
Please find previous discussions here,
https://patchwork.kernel.org/patch/11339591/#23140343
---
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 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..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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH for-5.0 3/3] exec: Fix for qemu_ram_resize() callback
2020-03-30 16:49 [PATCH for-5.0 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration Shameer Kolothum
2020-03-30 16:49 ` [PATCH for-5.0 1/3] acpi: Use macro for table-loader file name Shameer Kolothum
2020-03-30 16:49 ` [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately Shameer Kolothum
@ 2020-03-30 16:49 ` Shameer Kolothum
2020-03-30 19:59 ` [PATCH for-5.0 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration no-reply
3 siblings, 0 replies; 11+ messages in thread
From: Shameer Kolothum @ 2020-03-30 16:49 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, david, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
From: David Hildenbrand <david@redhat.com>
Summarizing the issue:
1. Memory regions contain ram blocks with a different size, if the
size is not properly aligned. While memory regions can have an
unaligned size, ram blocks can't. This is true when creating
resizable memory region with an unaligned size.
2. When resizing a ram block/memory region, the size of the memory
region is set to the aligned size. The callback is called with
the aligned size. The unaligned piece is lost.
Because of the above, if ACPI blob length modifications happens
after the initial virt_acpi_build() call, and the changed blob
length is within the PAGE size boundary, then the revised size
is not seen by the firmware on Guest reboot.
Hence make sure callback is called if memory region size is changed,
irrespective of aligned or not.
Signed-off-by: David Hildenbrand <david@redhat.com>
[Shameer: added commit log]
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
Please find previous discussion here,
https://patchwork.kernel.org/patch/11432375/#23216751
---
exec.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/exec.c b/exec.c
index de9d949902..2874bb5088 100644
--- a/exec.c
+++ b/exec.c
@@ -2074,11 +2074,23 @@ static int memory_try_enable_merging(void *addr, size_t len)
*/
int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
{
+ const ram_addr_t unaligned_size = newsize;
+
assert(block);
newsize = HOST_PAGE_ALIGN(newsize);
if (block->used_length == newsize) {
+ /*
+ * We don't have to resize the ram block (which only knows aligned
+ * sizes), however, we have to notify if the unaligned size changed.
+ */
+ if (unaligned_size != memory_region_size(block->mr)) {
+ memory_region_set_size(block->mr, unaligned_size);
+ if (block->resized) {
+ block->resized(block->idstr, unaligned_size, block->host);
+ }
+ }
return 0;
}
@@ -2102,9 +2114,9 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
block->used_length = newsize;
cpu_physical_memory_set_dirty_range(block->offset, block->used_length,
DIRTY_CLIENTS_ALL);
- memory_region_set_size(block->mr, newsize);
+ memory_region_set_size(block->mr, unaligned_size);
if (block->resized) {
- block->resized(block->idstr, newsize, block->host);
+ block->resized(block->idstr, unaligned_size, block->host);
}
return 0;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH for-5.0 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration
2020-03-30 16:49 [PATCH for-5.0 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration Shameer Kolothum
` (2 preceding siblings ...)
2020-03-30 16:49 ` [PATCH for-5.0 3/3] exec: Fix for qemu_ram_resize() callback Shameer Kolothum
@ 2020-03-30 19:59 ` no-reply
3 siblings, 0 replies; 11+ messages in thread
From: no-reply @ 2020-03-30 19:59 UTC (permalink / raw)
To: shameerali.kolothum.thodi
Cc: peter.maydell, xiaoguangrong.eric, david, shannon.zhaosl, mst,
qemu-devel, xuwei5, linuxarm, eric.auger, qemu-arm, imammedo,
lersek
Patchew URL: https://patchew.org/QEMU/20200330164909.28324-1-shameerali.kolothum.thodi@huawei.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
CC hw/smbios/smbios.o
In file included from /tmp/qemu-test/src/include/qemu/osdep.h:51,
from /tmp/qemu-test/src/hw/nvram/fw_cfg.c:25:
/tmp/qemu-test/src/include/qemu/compiler.h:81:35: error: invalid operands to binary - (have 'uint64_t *' {aka 'long long unsigned int *'} and 'size_t *' {aka 'unsigned int *'})
#define type_check(t1,t2) ((t1*)0 - (t2*)0)
^
/tmp/qemu-test/src/include/migration/vmstate.h:254:6: note: in expansion of macro 'type_check'
---
/tmp/qemu-test/src/hw/nvram/fw_cfg.c:674:9: note: in expansion of macro 'VMSTATE_UINT64'
VMSTATE_UINT64(table_mr_size, FWCfgState),
^~~~~~~~~~~~~~
/tmp/qemu-test/src/include/qemu/compiler.h:81:35: error: invalid operands to binary - (have 'uint64_t *' {aka 'long long unsigned int *'} and 'size_t *' {aka 'unsigned int *'})
#define type_check(t1,t2) ((t1*)0 - (t2*)0)
^
/tmp/qemu-test/src/include/migration/vmstate.h:254:6: note: in expansion of macro 'type_check'
---
/tmp/qemu-test/src/hw/nvram/fw_cfg.c:675:9: note: in expansion of macro 'VMSTATE_UINT64'
VMSTATE_UINT64(linker_mr_size, FWCfgState),
^~~~~~~~~~~~~~
/tmp/qemu-test/src/include/qemu/compiler.h:81:35: error: invalid operands to binary - (have 'uint64_t *' {aka 'long long unsigned int *'} and 'size_t *' {aka 'unsigned int *'})
#define type_check(t1,t2) ((t1*)0 - (t2*)0)
^
/tmp/qemu-test/src/include/migration/vmstate.h:254:6: note: in expansion of macro 'type_check'
---
/tmp/qemu-test/src/hw/nvram/fw_cfg.c:676:9: note: in expansion of macro 'VMSTATE_UINT64'
VMSTATE_UINT64(rsdp_mr_size, FWCfgState),
^~~~~~~~~~~~~~
make: *** [/tmp/qemu-test/src/rules.mak:69: hw/nvram/fw_cfg.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=e975dafce18c4d8d8bdc1aeccfe2c3fe', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-d8s6cyrl/src/docker-src.2020-03-30-15.54.26.3049:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=e975dafce18c4d8d8bdc1aeccfe2c3fe
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-d8s6cyrl/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 5m22.202s
user 0m8.599s
The full log is available at
http://patchew.org/logs/20200330164909.28324-1-shameerali.kolothum.thodi@huawei.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately
2020-03-30 16:49 ` [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately Shameer Kolothum
@ 2020-03-31 10:45 ` Dr. David Alan Gilbert
2020-04-01 7:45 ` Shameerali Kolothum Thodi
2020-03-31 14:50 ` Igor Mammedov
2020-03-31 15:03 ` Michael S. Tsirkin
2 siblings, 1 reply; 11+ messages in thread
From: Dr. David Alan Gilbert @ 2020-03-31 10:45 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, xiaoguangrong.eric, david, shannon.zhaosl, mst,
qemu-devel, xuwei5, linuxarm, eric.auger, qemu-arm, imammedo,
lersek
* Shameer Kolothum (shameerali.kolothum.thodi@huawei.com) 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 <david@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> ---
> Please find previous discussions here,
> https://patchwork.kernel.org/patch/11339591/#23140343
> ---
>
> 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 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..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),
The checker found something I also spotted; which is you can't use a
VMSTATE_UINT64 against a field that is size_t - it's not portable;
I suggest the easiest fix is to make your fields in fw_cfg.h uint64's.
Dave
> + 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
>
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately
2020-03-30 16:49 ` [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately Shameer Kolothum
2020-03-31 10:45 ` Dr. David Alan Gilbert
@ 2020-03-31 14:50 ` Igor Mammedov
2020-03-31 15:02 ` Michael S. Tsirkin
2020-03-31 15:03 ` Michael S. Tsirkin
2 siblings, 1 reply; 11+ messages in thread
From: Igor Mammedov @ 2020-03-31 14:50 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, xiaoguangrong.eric, david, shannon.zhaosl, mst,
qemu-devel, xuwei5, linuxarm, eric.auger, qemu-arm, lersek
On Mon, 30 Mar 2020 17:49:08 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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 <david@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Acked-by: David Hildenbrand <david@redhat.com>
without 3/3, current master is fine (x86/arm acpi)
2-3/s looks to me a bit risky for hard freeze,
so I'd postpone this series till the next merge window.
> ---
> Please find previous discussions here,
> https://patchwork.kernel.org/patch/11339591/#23140343
> ---
>
> 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 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..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 {
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately
2020-03-31 14:50 ` Igor Mammedov
@ 2020-03-31 15:02 ` Michael S. Tsirkin
0 siblings, 0 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2020-03-31 15:02 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, david, shannon.zhaosl,
qemu-devel, Shameer Kolothum, linuxarm, eric.auger, qemu-arm,
xuwei5, lersek
On Tue, Mar 31, 2020 at 04:50:38PM +0200, Igor Mammedov wrote:
> On Mon, 30 Mar 2020 17:49:08 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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 <david@redhat.com>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Acked-by: David Hildenbrand <david@redhat.com>
>
> without 3/3, current master is fine (x86/arm acpi)
> 2-3/s looks to me a bit risky for hard freeze,
> so I'd postpone this series till the next merge window.
What worries me is if we do so, how are we going to
handle ACPI changes on arm going forward?
There are advantages in merging migration changes early
as this reduces need for compat hackery ...
>
> > ---
> > Please find previous discussions here,
> > https://patchwork.kernel.org/patch/11339591/#23140343
> > ---
> >
> > 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 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..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 {
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately
2020-03-30 16:49 ` [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately Shameer Kolothum
2020-03-31 10:45 ` Dr. David Alan Gilbert
2020-03-31 14:50 ` Igor Mammedov
@ 2020-03-31 15:03 ` Michael S. Tsirkin
2020-04-01 7:48 ` Shameerali Kolothum Thodi
2 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2020-03-31 15:03 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, xiaoguangrong.eric, shannon.zhaosl, david,
qemu-devel, xuwei5, linuxarm, eric.auger, qemu-arm, imammedo,
lersek
On Mon, Mar 30, 2020 at 05:49:08PM +0100, 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 <david@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> ---
> Please find previous discussions here,
> https://patchwork.kernel.org/patch/11339591/#23140343
> ---
>
> 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 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..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;
How about we limit this to the case where the address is
unaligned?
> +}
> +
> +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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately
2020-03-31 10:45 ` Dr. David Alan Gilbert
@ 2020-04-01 7:45 ` Shameerali Kolothum Thodi
0 siblings, 0 replies; 11+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-04-01 7:45 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: peter.maydell, xiaoguangrong.eric, david, shannon.zhaosl, mst,
qemu-devel, Linuxarm, eric.auger, qemu-arm, xuwei (O),
imammedo, lersek
> -----Original Message-----
> From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> Sent: 31 March 2020 11:46
> 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;
> 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 for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately
[...]
> > +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),
>
> The checker found something I also spotted; which is you can't use a
> VMSTATE_UINT64 against a field that is size_t - it's not portable;
> I suggest the easiest fix is to make your fields in fw_cfg.h uint64's.
Thanks for that. Yes, checker also spotted this and I was clueless. Sure, I will change
that.
Shameer
> Dave
>
> > + 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
> >
> >
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately
2020-03-31 15:03 ` Michael S. Tsirkin
@ 2020-04-01 7:48 ` Shameerali Kolothum Thodi
0 siblings, 0 replies; 11+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-04-01 7:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: peter.maydell, xiaoguangrong.eric, shannon.zhaosl, david,
qemu-devel, Linuxarm, eric.auger, qemu-arm, xuwei (O),
imammedo, lersek
> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: 31 March 2020 16:03
> 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 for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately
>
> On Mon, Mar 30, 2020 at 05:49:08PM +0100, 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 <david@redhat.com>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Acked-by: David Hildenbrand <david@redhat.com>
> > ---
> > Please find previous discussions here,
> > https://patchwork.kernel.org/patch/11339591/#23140343
> > ---
> >
> > 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 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..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;
>
> How about we limit this to the case where the address is
> unaligned?
Ok. I will add that check as well.
Thanks,
Shameer
> > +}
> > +
> > +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
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-01 7:49 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-30 16:49 [PATCH for-5.0 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration Shameer Kolothum
2020-03-30 16:49 ` [PATCH for-5.0 1/3] acpi: Use macro for table-loader file name Shameer Kolothum
2020-03-30 16:49 ` [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately Shameer Kolothum
2020-03-31 10:45 ` Dr. David Alan Gilbert
2020-04-01 7:45 ` Shameerali Kolothum Thodi
2020-03-31 14:50 ` Igor Mammedov
2020-03-31 15:02 ` Michael S. Tsirkin
2020-03-31 15:03 ` Michael S. Tsirkin
2020-04-01 7:48 ` Shameerali Kolothum Thodi
2020-03-30 16:49 ` [PATCH for-5.0 3/3] exec: Fix for qemu_ram_resize() callback Shameer Kolothum
2020-03-30 19:59 ` [PATCH for-5.0 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.