diff mbox series

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

Message ID 20200403101827.30664-3-shameerali.kolothum.thodi@huawei.com (mailing list archive)
State New, archived
Headers show
Series acpi: Fixes for inconsistency in ACPI MR size during migration | expand

Commit Message

Shameerali Kolothum Thodi April 3, 2020, 10:18 a.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé April 7, 2020, 2:17 p.m. UTC | #1 
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;

> +}
> +
> +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 +669,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 +695,7 @@ static const VMStateDescription vmstate_fw_cfg = {
>       },
>       .subsections = (const VMStateDescription*[]) {
>           &vmstate_fw_cfg_dma,
> +        &vmstate_fw_cfg_acpi_mr,
>           NULL,
>       }
>   };
> @@ -815,6 +880,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 +996,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 +1020,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 +1057,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 +1184,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..25d9307018 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;
> +    uint64_t table_mr_size;
> +    uint64_t linker_mr_size;
> +    uint64_t rsdp_mr_size;
>   };
>   
>   struct FWCfgIoState {
>
Michael S. Tsirkin April 7, 2020, 2:34 p.m. UTC | #2 
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?

> > +}
> > +
> > +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 +669,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 +695,7 @@ static const VMStateDescription vmstate_fw_cfg = {
> >       },
> >       .subsections = (const VMStateDescription*[]) {
> >           &vmstate_fw_cfg_dma,
> > +        &vmstate_fw_cfg_acpi_mr,
> >           NULL,
> >       }
> >   };
> > @@ -815,6 +880,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 +996,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 +1020,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 +1057,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 +1184,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..25d9307018 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;
> > +    uint64_t table_mr_size;
> > +    uint64_t linker_mr_size;
> > +    uint64_t rsdp_mr_size;
> >   };
> >   struct FWCfgIoState {
> >
David Hildenbrand April 7, 2020, 2:54 p.m. UTC | #3 
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.
Philippe Mathieu-Daudé April 7, 2020, 5:03 p.m. UTC | #4 
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 :)
diff mbox series

Patch

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;
+}
+
+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 +669,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 +695,7 @@  static const VMStateDescription vmstate_fw_cfg = {
     },
     .subsections = (const VMStateDescription*[]) {
         &vmstate_fw_cfg_dma,
+        &vmstate_fw_cfg_acpi_mr,
         NULL,
     }
 };
@@ -815,6 +880,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 +996,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 +1020,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 +1057,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 +1184,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..25d9307018 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;
+    uint64_t table_mr_size;
+    uint64_t linker_mr_size;
+    uint64_t rsdp_mr_size;
 };
 
 struct FWCfgIoState {