Re: [Qemu-devel] [PATCH v2 1/4] fw_cfg: move internal function call docs to header file

[Laszlo Ersek]

On 11/02/15 21:36, Gabriel L. Somlo wrote:
> On Mon, Nov 02, 2015 at 02:41:58PM +0100, Laszlo Ersek wrote:
>> Three (well, two n' half) comments:
>>
>> On 10/28/15 18:20, Gabriel L. Somlo wrote:
>>> Move documentation for fw_cfg functions internal to qemu from
>>> docs/specs/fw_cfg.txt to the fw_cfg.h header file, next to their
>>> prototype declarations, formatted as doc-comments.
>>>
>>> Suggested-by: Peter Maydell <address@hidden>
>>> Cc: Laszlo Ersek <address@hidden>
>>> Cc: Gerd Hoffmann <address@hidden>
>>> Cc: Marc Marí <address@hidden>
>>> Cc: Jordan Justen <address@hidden>
>>> Cc: Paolo Bonzini <address@hidden>
>>> Cc: Peter Maydell <address@hidden>
>>> Signed-off-by: Gabriel Somlo <address@hidden>
>>> ---
>>>  docs/specs/fw_cfg.txt     |  85 +-----------------------------
>>>  include/hw/nvram/fw_cfg.h | 128 
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 129 insertions(+), 84 deletions(-)
>>>
>>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>>> index b8c794f..2099ad9 100644
>>> --- a/docs/specs/fw_cfg.txt
>>> +++ b/docs/specs/fw_cfg.txt
>>> @@ -192,90 +192,7 @@ To check the result, read the "control" field:
>>>                              today due to implementation not being async,
>>>                              but may in the future).
>>>  
>>> -= Host-side API =
>>> -
>>> -The following functions are available to the QEMU programmer for adding
>>> -data to a fw_cfg device during guest initialization (see fw_cfg.h for
>>> -each function's complete prototype):
>>> -
>>> -== fw_cfg_add_bytes() ==
>>> -
>>> -Given a selector key value, starting pointer, and size, create an item
>>> -as a raw "blob" of the given size, available by selecting the given key.
>>> -The data referenced by the starting pointer is only linked, NOT copied,
>>> -into the data structure of the fw_cfg device.
>>> -
>>> -== fw_cfg_add_string() ==
>>> -
>>> -Instead of a starting pointer and size, this function accepts a pointer
>>> -to a NUL-terminated ascii string, and inserts a newly allocated copy of
>>> -the string (including the NUL terminator) into the fw_cfg device data
>>> -structure.
>>> -
>>> -== fw_cfg_add_iXX() ==
>>> -
>>> -Insert an XX-bit item, where XX may be 16, 32, or 64. These functions
>>> -will convert a 16-, 32-, or 64-bit integer to little-endian, then add
>>> -a dynamically allocated copy of the appropriately sized item to fw_cfg
>>> -under the given selector key value.
>>> -
>>> -== fw_cfg_modify_iXX() ==
>>> -
>>> -Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
>>> -Similarly to the corresponding fw_cfg_add_iXX() function set, convert
>>> -a 16-, 32-, or 64-bit integer to little endian, create a dynamically
>>> -allocated copy of the required size, and replace the existing item at
>>> -the given selector key value with the newly allocated one. The previous
>>> -item, assumed to have been allocated during an earlier call to
>>> -fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
>>> -before the function returns.
>>> -
>>> -== fw_cfg_add_file() ==
>>> -
>>> -Given a filename (i.e., fw_cfg item name), starting pointer, and size,
>>> -create an item as a raw "blob" of the given size. Unlike fw_cfg_add_bytes()
>>> -above, the next available selector key (above 0x0020, FW_CFG_FILE_FIRST)
>>> -will be used, and a new entry will be added to the file directory structure
>>> -(at key 0x0019), containing the item name, blob size, and automatically
>>> -assigned selector key value. The data referenced by the starting pointer
>>> -is only linked, NOT copied, into the fw_cfg data structure.
>>> -
>>> -== fw_cfg_add_file_callback() ==
>>> -
>>> -Like fw_cfg_add_file(), but additionally sets pointers to a callback
>>> -function (and opaque argument), which will be executed host-side by
>>> -QEMU each time a byte is read by the guest from this particular item.
>>> -
>>> -NOTE: The callback function is given the opaque argument set by
>>> -fw_cfg_add_file_callback(), but also the current data offset,
>>> -allowing it the option of only acting upon specific offset values
>>> -(e.g., 0, before the first data byte of the selected item is
>>> -returned to the guest).
>>> -
>>> -== fw_cfg_modify_file() ==
>>> -
>>> -Given a filename (i.e., fw_cfg item name), starting pointer, and size,
>>> -completely replace the configuration item referenced by the given item
>>> -name with the new given blob. If an existing blob is found, its
>>> -callback information is removed, and a pointer to the old data is
>>> -returned to allow the caller to free it, helping avoid memory leaks.
>>> -If a configuration item does not already exist under the given item
>>> -name, a new item will be created as with fw_cfg_add_file(), and NULL
>>> -is returned to the caller. In any case, the data referenced by the
>>> -starting pointer is only linked, NOT copied, into the fw_cfg data
>>> -structure.
>>> -
>>> -== fw_cfg_add_callback() ==
>>> -
>>> -Like fw_cfg_add_bytes(), but additionally sets pointers to a callback
>>> -function (and opaque argument), which will be executed host-side by
>>> -QEMU each time a guest-side write operation to this particular item
>>> -completes fully overwriting the item's data.
>>> -
>>> -NOTE: This function is deprecated, and will be completely removed
>>> -starting with QEMU v2.4.
>>
>> (1) Please mention in the commit message that this paragraph disappears
>> without replacement, because the fw_cfg_add_callback() function is
>> already gone.
>>
>>> -
>>> -== Externally Provided Items ==
>>> += Externally Provided Items =
>>>  
>>>  As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
>>>  FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
>>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>>> index ee0cd8a..422e2e9 100644
>>> --- a/include/hw/nvram/fw_cfg.h
>>> +++ b/include/hw/nvram/fw_cfg.h
>>> @@ -73,19 +73,147 @@ typedef struct FWCfgDmaAccess {
>>>  typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
>>>  typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset);
>>>  
>>> +/**
>>> + * fw_cfg_add_bytes:
>>> + * @s: fw_cfg device being modified
>>> + * @key: selector key value for new fw_cfg item
>>> + * @data: pointer to start of item data
>>> + * @len: size of item data
>>> + *
>>> + * Add a new fw_cfg item, available by selecting the given key, as a raw
>>> + * "blob" of the given size. The data referenced by the starting pointer
>>> + * is only linked, NOT copied, into the data structure of the fw_cfg 
>>> device.
>>> + */
>>>  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len);
>>> +
>>> +/**
>>> + * fw_cfg_add_string:
>>> + * @s: fw_cfg device being modified
>>> + * @key: selector key value for new fw_cfg item
>>> + * @value: NUL-terminated ascii string
>>> + *
>>> + * Add a new fw_cfg item, available by selecting the given key. The item
>>> + * data will consist of a dynamically allocated copy of the provided 
>>> string,
>>> + * including its NUL terminator.
>>> + */
>>>  void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
>>> +
>>> +/**
>>> + * fw_cfg_add_i16:
>>> + * @s: fw_cfg device being modified
>>> + * @key: selector key value for new fw_cfg item
>>> + * @value: 16-bit integer
>>> + *
>>> + * Add a new fw_cfg item, available by selecting the given key. The item
>>> + * data will consist of a dynamically allocated copy of the given 16-bit
>>> + * value, converted to little-endian representation.
>>> + */
>>>  void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
>>> +
>>> +/**
>>> + * fw_cfg_modify_i16:
>>> + * @s: fw_cfg device being modified
>>> + * @key: selector key value for new fw_cfg item
>>> + * @value: 16-bit integer
>>> + *
>>> + * Replace the fw_cfg item available by selecting the given key. The new
>>> + * data will consist of a dynamically allocated copy of the given 16-bit
>>> + * value, converted to little-endian representation. The data being 
>>> replaced,
>>> + * assumed to have been dynamically allocated during an earlier call to
>>> + * either fw_cfg_add_i16() or fw_cfg_modify_i16(), is freed before 
>>> returning.
>>> + */
>>>  void fw_cfg_modify_i16(FWCfgState *s, uint16_t key, uint16_t value);
>>> +
>>> +/**
>>> + * fw_cfg_add_i32:
>>> + * @s: fw_cfg device being modified
>>> + * @key: selector key value for new fw_cfg item
>>> + * @value: 32-bit integer
>>> + *
>>> + * Add a new fw_cfg item, available by selecting the given key. The item
>>> + * data will consist of a dynamically allocated copy of the given 32-bit
>>> + * value, converted to little-endian representation.
>>> + */
>>>  void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
>>> +
>>> +/**
>>> + * fw_cfg_add_i64:
>>> + * @s: fw_cfg device being modified
>>> + * @key: selector key value for new fw_cfg item
>>> + * @value: 64-bit integer
>>> + *
>>> + * Add a new fw_cfg item, available by selecting the given key. The item
>>> + * data will consist of a dynamically allocated copy of the given 64-bit
>>> + * value, converted to little-endian representation.
>>> + */
>>>  void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
>>> +
>>> +/**
>>> + * fw_cfg_add_file:
>>> + * @s: fw_cfg device being modified
>>> + * @filename: name of new fw_cfg file item
>>> + * @data: pointer to start of item data
>>> + * @len: size of item data
>>> + *
>>> + * Add a new NAMED fw_cfg item as a raw "blob" of the given size. The data
>>> + * referenced by the starting pointer is only linked, NOT copied, into the
>>> + * data structure of the fw_cfg device.
>>> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
>>> + * will be used; also, a new entry will be added to the file directory
>>> + * structure residing at key value FW_CFG_FILE_DIR, containing the item 
>>> name,
>>> + * data size, and assigned selector key value.
>>> + */
>>>  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
>>>                       size_t len);
>>> +
>>> +/**
>>> + * fw_cfg_add_file_callback:
>>> + * @s: fw_cfg device being modified
>>> + * @filename: name of new fw_cfg file item
>>> + * @callback: callback function
>>> + * @callback_opaque: argument to be passed into callback function
>>> + * @data: pointer to start of item data
>>> + * @len: size of item data
>>> + *
>>> + * Add a new NAMED fw_cfg item as a raw "blob" of the given size. The data
>>> + * referenced by the starting pointer is only linked, NOT copied, into the
>>> + * data structure of the fw_cfg device.
>>> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
>>> + * will be used; also, a new entry will be added to the file directory
>>> + * structure residing at key value FW_CFG_FILE_DIR, containing the item 
>>> name,
>>> + * data size, and assigned selector key value.
>>> + * Additionally, set a callback function (and argument) to be called each
>>> + * time a byte is read by the guest from this particular item, or once per
>>> + * each DMA guest read operation.
>>
>> (2) -- (This is the "half" comment.) We could make the DMA language a
>> bit more precise, because the callback is not invoked if the start
>> offset of the DMA transfer falls outside the fw_cfg blob in question.
>> However, I don't think it is necessary to update this paragraph, because
>> in the next patch precisely the callback-on-DMA behavior is changed.
> 
> I'll do this, if only so that the historical record wouldn't be
> unnecessarily hard to decipher on anyone who might (unlikely) end
> up doing archaeology on this :)
> 
> How about this:
> 
>  Additionally, set a callback function (and argument) to be called each
>  time a byte is read by the guest from this particular item, or, in the
>  case of DMA, each time a read or skip request overlaps with a defined
>  portion of the item.

s/a defined portion/the valid offset range/? :)

Thanks
Laszlo

> 
> ack on (and thanks for) the rest of the review!
> 
> --Gabriel
> 
>>
>>
>>> + * NOTE: In addition to the opaque argument set here, the callback function
>>> + * takes the current data offset as an additional argument, allowing it the
>>> + * option of only acting upon specific offset values (e.g., 0, before the
>>> + * first data byte of the selected item is returned to the guest).
>>> + */
>>>  void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
>>>                                FWCfgReadCallback callback, void 
>>> *callback_opaque,
>>>                                void *data, size_t len);
>>> +
>>> +/**
>>> + * fw_cfg_modify_file:
>>> + * @s: fw_cfg device being modified
>>> + * @filename: name of new fw_cfg file item
>>> + * @data: pointer to start of item data
>>> + * @len: size of item data
>>> + *
>>> + * Replace a NAMED fw_cfg item. If an existing item is found, its callback
>>> + * information will be cleared, and a pointer to its data will be returned
>>
>> (3) "returned [to] the caller"
>>
>>> + * the caller, so that it may be freed if necessary. If an existing item is
>>> + * not found, this call defaults to fw_cfg_add_file(), and NULL is returned
>>> + * to the caller.
>>> + * In either case, the new item data is only linked, NOT copied, into the
>>> + * data structure of the fw_cfg device.
>>> + *
>>> + * Returns: pointer to old item's data, or NULL if old item does not exist.
>>> + */
>>>  void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
>>>                           size_t len);
>>> +
>>>  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>>>                                  AddressSpace *dma_as);
>>>  FWCfgState *fw_cfg_init_io(uint32_t iobase);
>>>
>>
>> With (1) and (3) addressed, and with our without fixing up (2):
>>
>> Reviewed-by: Laszlo Ersek <address@hidden>