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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Mon, Jul 28, 2014 at 05:52:27PM +0100, Dr. David Alan Gilbert wrote:
> > * Michael S. Tsirkin (mst@redhat.com) 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 <mst@redhat.com>
> > > ---
> > >  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?

Yes, if it was something that was just switched on with new machine types
I wouldn't worry as much.

Dave

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