Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: add documentation file (docs/specs/fw_cfg.txt)

[Laszlo Ersek]

On 03/19/15 01:18, Gabriel L. Somlo wrote:

> +=== Revision (Key 0x0001, FW_CFG_ID) ===
> +
> +A 32-bit little-endian unsigned int, this item is used as an interface
> +revision number, and is currently set to 1 by all QEMU architectures
> +which expose a fw_cfg device.

arm/virt doesn't :)

It could be argued that that's an error in "hw/arm/virt.c".

On the other hand, all of the other fw_cfg providing boards set the
interface version to 1 manually, despite the device coming from the
same, shared implementation. Therefore one could argue that instead of
adding

    fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);

to arm/virt, all such existing calls should be consolidated in the
fw_cfg initialization code instead.

I guess I must have missed doing it the last time because I had bigger
fish to fry, and because value 1 for FW_CFG_ID didn't, and doesn't, seem
to mean anything specific.

So, I'm just making this note here because I want it on the record that
I read the above paragraph and didn't miss that it was factually
incorrect. How it should be fixed, namely:
- modify the docs,
- modify the arm/virt board code,
- move the FW_CFG_ID setting to fw_cfg_init1() -- after all that's
  where FW_CFG_SIGNATURE is set too --,

I'll leave up to other reviewers.

> +== 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.

Please point out here that the data pointed-to by the starting pointer
is not copied, only linked.

> +
> +== fw_cfg_add_string() ==
> +
> +Instead of a starting pointer and size, this function accepts a
> +pointer to a NUL-terminated ascii string, and creates an item which
> +exactly fits the length of the string, including its NUL terminator.

Please point out that the string, unlike the blob with
fw_cfg_add_bytes(), *is* copied.

> +
> +== 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 format
> +before creating an item of the appropriate size available via the given
> +selector key value.

Please point out that the blob underlying the new key (with the little
endian-encoded integer as contents) will be dynamically allocated
internally.

> +== fw_cfg_add_file() ==
> +
> +Given a file name,

I suggest to add, in parens: (ie. fw_cfg item name) -- that's what you
called it earlier, near the struct.

> starting pointer, and size, create an item as a raw
> +"blob" of the given size.

Please point out that the data will only be linked, not copied.

> 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 file name, blob size, and automatically assigned
> +selector key value.
> +
> +== fw_cfg_add_file_callback() ==
> +
> +Like fw_cfg_add_file(), but additionally sets pointers to a callback
> +function (and argument), which will be executed host-side by QEMU each
> +time a byte is read by the guest from this particular item.

Hm, this surprised me, but you are right; the read callback is indeed
invoked for each byte read. The trick is that the callback function gets
the data offset too, so it can restrict whatever action it does to the
zero offset. I think this would be useful to point out *very briefly*,
but I don't insist.

> +
> +== fw_cfg_modify_file() ==
> +
> +Given a file name, starting pointer, and size, completely replace the
> +configuration item referenced by the given file name

(fw_cfg 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.

Good! This is consistent with the above suggestions. (Ie. always be
explicit about copying, linking, ownership.)

> If a configuration item does
> +not already exist under the given file name, a new item will be created
> +as with fw_cfg_add_file(), and NULL is returned to the caller.
> +
> +== fw_cfg_add_callback() ==
> +
> +Like fw_cfg_add_bytes(), but additionally sets pointers to a callback
> +function (and 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.
> 

I think this version is a significant improvement. I apologize that you
can't immediately write a v3 of this patch; you'll have to wait for
others' input wrt. FW_CFG_ID on arm/virt, or FW_CFG_ID's unification.
(This is why documentation is great, by the way. It pulls skeletons from
closets. :))

Thanks!
Laszlo