From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59543) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtFN9-0002dB-2n for qemu-devel@nongnu.org; Mon, 02 Nov 2015 08:42:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtFN4-0007Qg-R0 for qemu-devel@nongnu.org; Mon, 02 Nov 2015 08:42:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53708) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtFN4-0007Qb-Gb for qemu-devel@nongnu.org; Mon, 02 Nov 2015 08:42:02 -0500 References: <1446052836-31737-1-git-send-email-somlo@cmu.edu> <1446052836-31737-2-git-send-email-somlo@cmu.edu> From: Laszlo Ersek Message-ID: <56376826.1010702@redhat.com> Date: Mon, 2 Nov 2015 14:41:58 +0100 MIME-Version: 1.0 In-Reply-To: <1446052836-31737-2-git-send-email-somlo@cmu.edu> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 1/4] fw_cfg: move internal function call docs to header file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gabriel L. Somlo" , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, markmb@redhat.com, pbonzini@redhat.com, kraxel@redhat.com, jordan.l.justen@intel.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. >=20 > Suggested-by: Peter Maydell > Cc: Laszlo Ersek > Cc: Gerd Hoffmann > Cc: Marc Mar=C3=AD > Cc: Jordan Justen > Cc: Paolo Bonzini > Cc: Peter Maydell > Signed-off-by: Gabriel Somlo > --- > docs/specs/fw_cfg.txt | 85 +----------------------------- > include/hw/nvram/fw_cfg.h | 128 ++++++++++++++++++++++++++++++++++++++= ++++++++ > 2 files changed, 129 insertions(+), 84 deletions(-) >=20 > 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 asyn= c, > but may in the future). > =20 > -=3D Host-side API =3D > - > -The following functions are available to the QEMU programmer for addin= g > -data to a fw_cfg device during guest initialization (see fw_cfg.h for > -each function's complete prototype): > - > -=3D=3D fw_cfg_add_bytes() =3D=3D > - > -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 ke= y. > -The data referenced by the starting pointer is only linked, NOT copied= , > -into the data structure of the fw_cfg device. > - > -=3D=3D fw_cfg_add_string() =3D=3D > - > -Instead of a starting pointer and size, this function accepts a pointe= r > -to a NUL-terminated ascii string, and inserts a newly allocated copy o= f > -the string (including the NUL terminator) into the fw_cfg device data > -structure. > - > -=3D=3D fw_cfg_add_iXX() =3D=3D > - > -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. > - > -=3D=3D fw_cfg_modify_iXX() =3D=3D > - > -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 previou= s > -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 fre= ed > -before the function returns. > - > -=3D=3D fw_cfg_add_file() =3D=3D > - > -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_by= tes() > -above, the next available selector key (above 0x0020, FW_CFG_FILE_FIRS= T) > -will be used, and a new entry will be added to the file directory stru= cture > -(at key 0x0019), containing the item name, blob size, and automaticall= y > -assigned selector key value. The data referenced by the starting point= er > -is only linked, NOT copied, into the fw_cfg data structure. > - > -=3D=3D fw_cfg_add_file_callback() =3D=3D > - > -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). > - > -=3D=3D fw_cfg_modify_file() =3D=3D > - > -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. > - > -=3D=3D fw_cfg_add_callback() =3D=3D > - > -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. > - > -=3D=3D Externally Provided Items =3D=3D > +=3D Externally Provided Items =3D > =20 > 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); > =20 > +/** > + * 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 r= aw > + * "blob" of the given size. The data referenced by the starting point= er > + * is only linked, NOT copied, into the data structure of the fw_cfg d= evice. > + */ > 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 it= em > + * data will consist of a dynamically allocated copy of the provided s= tring, > + * 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 it= em > + * data will consist of a dynamically allocated copy of the given 16-b= it > + * 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 n= ew > + * data will consist of a dynamically allocated copy of the given 16-b= it > + * value, converted to little-endian representation. The data being re= placed, > + * assumed to have been dynamically allocated during an earlier call t= o > + * either fw_cfg_add_i16() or fw_cfg_modify_i16(), is freed before ret= urning. > + */ > 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 it= em > + * data will consist of a dynamically allocated copy of the given 32-b= it > + * 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 it= em > + * data will consist of a dynamically allocated copy of the given 64-b= it > + * 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_FI= RST > + * 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 ite= m 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_FI= RST > + * 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 ite= m name, > + * data size, and assigned selector key value. > + * Additionally, set a callback function (and argument) to be called e= ach > + * 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 fun= ction > + * takes the current data offset as an additional argument, allowing i= t 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 *callba= ck_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 call= back > + * information will be cleared, and a pointer to its data will be retu= rned (3) "returned [to] the caller" > + * the caller, so that it may be freed if necessary. If an existing it= em is > + * not found, this call defaults to fw_cfg_add_file(), and NULL is ret= urned > + * 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 e= xist. > + */ > void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *da= ta, > 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); >=20 With (1) and (3) addressed, and with our without fixing up (2): Reviewed-by: Laszlo Ersek