[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v3 02/10] fw_cfg: Migrate ACPI table mr sizes separately
From: |
Shameerali Kolothum Thodi |
Subject: |
RE: [PATCH v3 02/10] fw_cfg: Migrate ACPI table mr sizes separately |
Date: |
Fri, 20 Mar 2020 11:53:08 +0000 |
> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden]
> Sent: 19 March 2020 17:51
> To: Shameerali Kolothum Thodi <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; xuwei (O) <address@hidden>; address@hidden;
> Linuxarm <address@hidden>
> Subject: Re: [PATCH v3 02/10] fw_cfg: Migrate ACPI table mr sizes separately
>
> On Thu, Mar 12, 2020 at 09:27:32AM +0000, Shameerali Kolothum Thodi
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Michael S. Tsirkin [mailto:address@hidden]
> > > Sent: 11 March 2020 21:10
> > > To: Shameerali Kolothum Thodi <address@hidden>
> > > Cc: address@hidden; address@hidden;
> > > address@hidden; address@hidden;
> address@hidden;
> > > address@hidden; address@hidden;
> > > address@hidden; xuwei (O) <address@hidden>;
> address@hidden;
> > > Linuxarm <address@hidden>
> > > Subject: Re: [PATCH v3 02/10] fw_cfg: Migrate ACPI table mr sizes
> separately
> > >
> > > On Wed, Mar 11, 2020 at 05:20:06PM +0000, Shameer Kolothum wrote:
> > > > Any sub-page size update to ACPI table 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 sizes
> > > > between source and destination. In order to avoid this, save and
> > > > restore them separately during migration.
> >
> >
> > > Is there a reason this is part of nvdimm patchset?
> >
> > Not really. But this problem is more visible if we have nvdimm hotplug
> > support added to arm/virt. On x86, both acpi table and linker MRs are
> > already
> > aligned and I don't know a use case where you can change RSDP MR size(See
> below).
> >
> > >
> > > Hmm but for old machine types we still have a problem right?
> > > How about aligning size on source for them?
> > > Then there won't be an inconsistency across migration.
> > > Wastes some boot time/memory but maybe that's better
> > > than a chance of not booting ...
> >
> > Right. That was considered. On x86, except RSDP MR, both the LINKER and
> ACPI
> > TABLE MRs are already aligned/padded. And we cannot make RSDP mr
> aligned
> > as it will break the seabios based boot.
>
> Hmm. So right now if we migrate just before RSDP is read, there's
> a failure?
I am not sure that will be the case. IIUC, on migration path,
ram_load_precopy() -->qemu_ram_resize()
won't be called as both length and block->used_length will be aligned size.
Even if it calls, the current qemu_ram_resize() works on aligned size and wont
invoke
the callback to update the FWCfgEntry. And I believe on destination, the bios
read will trigger
the fw_cfg_select() which will call the acpi_build_update() to rebuild the
tables and update the
FWCfgEntry.
Having said that my knowledge on this is limited, but I can test and confirm
this, if there is
an easy way to trigger this usecase. Please let me know.
Thanks,
Shameer
> > So a generic solution based on alignment
> > is not possible unless we guarantee that RSDP is not going to be modified.
> >
> > What we could do for Arm/virt is just follow the x86 way and add padding for
> > table and linker MRs. But this was discussed before and IIRC, was not well
> > received.
> >
> > Thanks,
> > Shameer
> >
> > > > Suggested-by: David Hildenbrand <address@hidden>
> > > > Signed-off-by: Shameer Kolothum
> <address@hidden>
> > > > ---
> > > > Please find the discussion here,
> > > > https://patchwork.kernel.org/patch/11339591/
> > > > ---
> > > > hw/core/machine.c | 1 +
> > > > hw/nvram/fw_cfg.c | 86
> > > ++++++++++++++++++++++++++++++++++++++-
> > > > include/hw/nvram/fw_cfg.h | 6 +++
> > > > 3 files changed, 92 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index 9e8c06036f..6d960bd47f 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..36d1e32f83 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,50 @@ bool fw_cfg_dma_enabled(void *opaque)
> > > > return s->dma_enabled;
> > > > }
> > > >
> > > > +static bool fw_cfg_acpi_mr_restore(void *opaque)
> > > > +{
> > > > + FWCfgState *s = opaque;
> > > > + return s->acpi_mr_restore;
> > > > +}
> > > > +
> > > > +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 +664,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 +690,7 @@ static const VMStateDescription vmstate_fw_cfg
> = {
> > > > },
> > > > .subsections = (const VMStateDescription*[]) {
> > > > &vmstate_fw_cfg_dma,
> > > > + &vmstate_fw_cfg_acpi_mr,
> > > > NULL,
> > > > }
> > > > };
> > > > @@ -815,6 +875,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 +991,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 +1015,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 +1052,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 +1179,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..457fee7425 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;
> > > > + size_t table_mr_size;
> > > > + size_t linker_mr_size;
> > > > + size_t rsdp_mr_size;
> > > > };
> > > >
> > > > struct FWCfgIoState {
> > > > --
> > > > 2.17.1
> > > >
[PATCH v3 04/10] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length, Shameer Kolothum, 2020/03/11
[PATCH v3 03/10] exec: Fix for qemu_ram_resize() callback, Shameer Kolothum, 2020/03/11
[PATCH v3 05/10] nvdimm: Use configurable ACPI IO base and size, Shameer Kolothum, 2020/03/11
[PATCH v3 06/10] hw/arm/virt: Add nvdimm hot-plug infrastructure, Shameer Kolothum, 2020/03/11