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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 28/07/2014 21:07, Michael S. Tsirkin ha scritto:
> 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?

Yeah, that would help but I think these patches are too much for 2.1.

Paolo

>>> +
>>>  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