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

[Laszlo Ersek <lersek@redhat.com>]

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 <peter.maydell@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc Marí <markmb@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  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.


> + * 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 <lersek@redhat.com>