On 07.04.20 16:34, Michael S. Tsirkin wrote: > 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?
At least I find the original code fairly easy to read - just as the proposed alternative. So, yes, matter of taste I'd say. -- Thanks, David / dhildenb
