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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 4/7/20 4:54 PM, David Hildenbrand wrote:
> 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 <david@redhat.com>
>>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>>> Acked-by: David Hildenbrand <david@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.

OK :)