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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> Sent: 31 March 2020 11:46
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> eric.auger@redhat.com; imammedo@redhat.com; peter.maydell@linaro.org;
> xiaoguangrong.eric@gmail.com; david@redhat.com; mst@redhat.com;
> Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>;
> shannon.zhaosl@gmail.com; lersek@redhat.com
> Subject: Re: [PATCH for-5.0 2/3] fw_cfg: Migrate ACPI table mr sizes separately

[...]

> > +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),
> 
> The checker found something I also spotted; which is you can't use a
> VMSTATE_UINT64 against a field that is size_t - it's not portable;
> I suggest the easiest fix is to make your fields in fw_cfg.h uint64's.

Thanks for that. Yes, checker also spotted this and I was clueless. Sure, I will change
that.

Shameer


> Dave
> 
> > +        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
> >
> >
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK