Re: [PATCH for-5.0 v2 2/3] fw_cfg: Migrate ACPI table mr sizes separately

[David Hildenbrand]

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