[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-5.0 v2 2/3] fw_cfg: Migrate ACPI table mr sizes separatel
From: |
David Hildenbrand |
Subject: |
Re: [PATCH for-5.0 v2 2/3] fw_cfg: Migrate ACPI table mr sizes separately |
Date: |
Tue, 7 Apr 2020 16:54:01 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 |
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 <address@hidden>
>>> Signed-off-by: Shameer Kolothum <address@hidden>
>>> Acked-by: David Hildenbrand <address@hidden>
>>> ---
>>> 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
[PATCH for-5.0 v2 3/3] exec: Fix for qemu_ram_resize() callback, Shameer Kolothum, 2020/04/03
Re: [PATCH for-5.0 v2 0/3] acpi: Fixes for inconsistency in ACPI MR size during migration, Michael S. Tsirkin, 2020/04/03