| Message ID | 1446052836-31737-2-git-send-email-somlo@cmu.edu |
|---|---|
| State | New |
| Headers | show |
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>
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 <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. 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. 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 <lersek@redhat.com>
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 <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. > > 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 <lersek@redhat.com>
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. - -== 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. + * 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 + * 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);
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(-)