Re: [Qemu-devel] [PATCH 1/3] loader: add support for resizeable blobs

[Michael S. Tsirkin]

On Mon, Jul 28, 2014 at 05:52:27PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (address@hidden) wrote:
> > Support resizeable blobs: we allocate more memory than currently
> > available in the blob, which can later be filled in.
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  include/hw/loader.h       | 14 +++++++--
> >  include/hw/nvram/fw_cfg.h |  2 +-
> >  hw/core/loader.c          | 15 +++++----
> >  hw/nvram/fw_cfg.c         | 79 
> > ++++++++++++++++++++++++++++++++++++++++++++---
> >  4 files changed, 96 insertions(+), 14 deletions(-)
> > 
> 
> 
> > +static bool fw_cfg_len_needed(void *opaque, int version_id)
> > +{
> > +    FWCfgState *s = FW_CFG(opaque);
> > +    int i, j;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(s->entries); ++i) {
> > +        for (j = 0; j < ARRAY_SIZE(s->entries[0]); ++j) {
> > +            if (s->entries[i][j].len != s->entries[i][j].max_len) {
> > +                return true;
> > +            }
> > +        }
> > +    }
> > +
> > +    return false;
> > +}
> 
> This feels a bit delicate; it means that increasing the 'max_len' changes
> the expected migration stream, which seems odd for a parameter whose
> job is to make things easier to migrate.
> 
> Dave

It's an API issue - so you are more comfortable with
an explicit flag?

> > +
> >  static const VMStateDescription vmstate_fw_cfg = {
> >      .name = "fw_cfg",
> >      .version_id = 2,
> >      .minimum_version_id = 1,
> > +    .post_load = fw_cfg_post_load,
> > +    .pre_save = fw_cfg_pre_save,
> >      .fields = (VMStateField[]) {
> >          VMSTATE_UINT16(cur_entry, FWCfgState),
> >          VMSTATE_UINT16_HACK(cur_offset, FWCfgState, is_version_1),
> >          VMSTATE_UINT32_V(cur_offset, FWCfgState, 2),
> > +        VMSTATE_ARRAY_TEST(len, FWCfgState, FW_CFG_LEN_ENTRIES,
> > +                           fw_cfg_len_needed,
> > +                           vmstate_info_uint32, uint32_t),
> >          VMSTATE_END_OF_LIST()
> >      }
> >  };
> > @@ -388,23 +449,28 @@ static const VMStateDescription vmstate_fw_cfg = {
> >  static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
> >                                             FWCfgReadCallback callback,
> >                                             void *callback_opaque,
> > -                                           void *data, size_t len)
> > +                                           void *data, size_t len,
> > +                                           size_t max_len)
> >  {
> >      int arch = !!(key & FW_CFG_ARCH_LOCAL);
> >  
> > +    assert(len <= max_len);
> > +
> >      key &= FW_CFG_ENTRY_MASK;
> >  
> >      assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
> >  
> >      s->entries[arch][key].data = data;
> >      s->entries[arch][key].len = (uint32_t)len;
> > +    s->entries[arch][key].reset_len = (uint32_t)len;
> > +    s->entries[arch][key].max_len = (uint32_t)max_len;
> >      s->entries[arch][key].read_callback = callback;
> >      s->entries[arch][key].callback_opaque = callback_opaque;
> >  }
> >  
> >  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
> >  {
> > -    fw_cfg_add_bytes_read_callback(s, key, NULL, NULL, data, len);
> > +    fw_cfg_add_bytes_read_callback(s, key, NULL, NULL, data, len, len);
> >  }
> >  
> >  void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
> > @@ -454,13 +520,15 @@ void fw_cfg_add_callback(FWCfgState *s, uint16_t key, 
> > FWCfgCallback callback,
> >  
> >      s->entries[arch][key].data = data;
> >      s->entries[arch][key].len = (uint32_t)len;
> > +    s->entries[arch][key].reset_len = (uint32_t)len;
> > +    s->entries[arch][key].max_len = (uint32_t)len;
> >      s->entries[arch][key].callback_opaque = callback_opaque;
> >      s->entries[arch][key].callback = callback;
> >  }
> >  
> >  void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
> >                                FWCfgReadCallback callback, void 
> > *callback_opaque,
> > -                              void *data, size_t len)
> > +                              void *data, size_t len, size_t max_len)
> >  {
> >      int i, index;
> >      size_t dsize;
> > @@ -475,7 +543,8 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const 
> > char *filename,
> >      assert(index < FW_CFG_FILE_SLOTS);
> >  
> >      fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index,
> > -                                   callback, callback_opaque, data, len);
> > +                                   callback, callback_opaque, data, len,
> > +                                   max_len);
> >  
> >      pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
> >              filename);
> > @@ -496,7 +565,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const 
> > char *filename,
> >  void fw_cfg_add_file(FWCfgState *s,  const char *filename,
> >                       void *data, size_t len)
> >  {
> > -    fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len);
> > +    fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len, len);
> >  }
> >  
> >  static void fw_cfg_machine_ready(struct Notifier *n, void *data)
> > -- 
> > MST
> > 
> > 
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK