From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48941) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtLy6-0001wC-RG for qemu-devel@nongnu.org; Mon, 02 Nov 2015 15:44:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtLy3-0006Bc-FA for qemu-devel@nongnu.org; Mon, 02 Nov 2015 15:44:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49572) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtLy3-0006BS-4h for qemu-devel@nongnu.org; Mon, 02 Nov 2015 15:44:39 -0500 References: <1446052836-31737-1-git-send-email-somlo@cmu.edu> <1446052836-31737-2-git-send-email-somlo@cmu.edu> <56376826.1010702@redhat.com> <20151102203657.GD10717@HEDWIG.INI.CMU.EDU> From: Laszlo Ersek Message-ID: <5637CB33.4060704@redhat.com> Date: Mon, 2 Nov 2015 21:44:35 +0100 MIME-Version: 1.0 In-Reply-To: <20151102203657.GD10717@HEDWIG.INI.CMU.EDU> Content-Type: text/plain; charset=windows-1252 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" Cc: peter.maydell@linaro.org, jordan.l.justen@intel.com, qemu-devel@nongnu.org, kraxel@redhat.com, pbonzini@redhat.com, markmb@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 >>> Cc: Laszlo Ersek >>> Cc: Gerd Hoffmann >>> Cc: Marc Mar=ED >>> 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(-) >>> >>> 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 as= ync, >>> but may in the future). >>> =20 >>> -=3D Host-side API =3D >>> - >>> -The following functions are available to the QEMU programmer for add= ing >>> -data to a fw_cfg device during guest initialization (see fw_cfg.h fo= r >>> -each function's complete prototype): >>> - >>> -=3D=3D fw_cfg_add_bytes() =3D=3D >>> - >>> -Given a selector key value, starting pointer, and size, create an it= em >>> -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 copi= ed, >>> -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 poin= ter >>> -to a NUL-terminated ascii string, and inserts a newly allocated copy= of >>> -the string (including the NUL terminator) into the fw_cfg device dat= a >>> -structure. >>> - >>> -=3D=3D fw_cfg_add_iXX() =3D=3D >>> - >>> -Insert an XX-bit item, where XX may be 16, 32, or 64. These function= s >>> -will convert a 16-, 32-, or 64-bit integer to little-endian, then ad= d >>> -a dynamically allocated copy of the appropriately sized item to fw_c= fg >>> -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, conver= t >>> -a 16-, 32-, or 64-bit integer to little endian, create a dynamically >>> -allocated copy of the required size, and replace the existing item a= t >>> -the given selector key value with the newly allocated one. The previ= ous >>> -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 f= reed >>> -before the function returns. >>> - >>> -=3D=3D fw_cfg_add_file() =3D=3D >>> - >>> -Given a filename (i.e., fw_cfg item name), starting pointer, and siz= e, >>> -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_FI= RST) >>> -will be used, and a new entry will be added to the file directory st= ructure >>> -(at key 0x0019), containing the item name, blob size, and automatica= lly >>> -assigned selector key value. The data referenced by the starting poi= nter >>> -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 siz= e, >>> -completely replace the configuration item referenced by the given it= em >>> -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 callbac= k >>> -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 disappear= s >> 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 abov= e >>> 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= raw >>> + * "blob" of the given size. The data referenced by the starting poi= nter >>> + * 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 *valu= e); >>> + >>> +/** >>> + * 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 r= eturning. >>> + */ >>> 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. Th= e data >>> + * referenced by the starting pointer is only linked, NOT copied, in= to 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 directo= ry >>> + * structure residing at key value FW_CFG_FILE_DIR, containing the i= tem 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. Th= e data >>> + * referenced by the starting pointer is only linked, NOT copied, in= to 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 directo= ry >>> + * structure residing at key value FW_CFG_FILE_DIR, containing the i= tem 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 on= ce 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, becau= se >> in the next patch precisely the callback-on-DMA behavior is changed. >=20 > 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 :) >=20 > How about this: >=20 > 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 >=20 > ack on (and thanks for) the rest of the review! >=20 > --Gabriel >=20 >> >> >>> + * NOTE: In addition to the opaque argument set here, the callback f= unction >>> + * takes the current data offset as an additional argument, allowing= it the >>> + * option of only acting upon specific offset values (e.g., 0, befor= e 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 *call= back_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 ca= llback >>> + * information will be cleared, and a pointer to its data will be re= turned >> >> (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 r= eturned >>> + * to the caller. >>> + * In either case, the new item data is only linked, NOT copied, int= o 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