[Qemu-devel] [PATCH 0/6] fw-cfg: documentation, cleanup, and proposed feature

[Qemu-devel] [PATCH 0/6] fw-cfg: documentation, cleanup, and proposed feature

[Gabriel L. Somlo]

Document and clean up fw_cfg; additionally, allow user-provided blobs to
be inserted into fw_cfg via the qemu command line.

Please consider applying 1-5; Patch 6/6 should be considered as just an RFC
at this time, please let me know what you all think about it.

Summary of patches in the series:

  1/6: resubmit Jordan's documentation patch from back in 2011

  2/6: remove support for guest-side writes to fw_cfg data port
       (this also updates the documentation applied earlier in 1/6)

  3/6: add assertion to prevent us from inadvertently introducing memory leaks
       when adding data blobs to fw_cfg

  4/6: adding fw_cfg blobs with the same file name multiple times is a
       memory-leaking erroneous thing to do: make qemu properly quit with
       an error instead of just generating a trace event.

  5/6: allow users to explicitly insert an arbitrary file as a fw_cfg blob
       via the qemu command line

  6/6: guest-side retrieval of named blob from the fw_cfg device
       (please do NOT apply 6/6, as I'm just soliciting feedback at this time)

Thanks much,
  Gabriel

Gabriel L. Somlo (6):
  fw_cfg: add documentation file (docs/specs/fw_cfg.txt)
  fw_cfg: remove support for guest-side data writes
  fw_cfg: assertion to detect memory leak when adding new data blob
  fw_cfg: exit with error when dupe fw_cfg file name inserted
  fw_cfg: insert fw_cfg file blobs via qemu cmdline
  qga: RFC: guest-side retrieval of fw_cfg file

 docs/specs/fw_cfg.txt     | 154 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/nvram/fw_cfg.c         | 123 ++++++++++++++++++------------------
 include/hw/nvram/fw_cfg.h |   8 ++-
 qemu-options.hx           |  10 +++
 qga/Makefile.objs         |   1 +
 qga/get-fwcfg.c           |  92 +++++++++++++++++++++++++++
 qga/guest-agent-core.h    |   2 +
 qga/main.c                |   6 +-
 vl.c                      |  27 ++++++++
 9 files changed, 355 insertions(+), 68 deletions(-)
 create mode 100644 docs/specs/fw_cfg.txt
 create mode 100644 qga/get-fwcfg.c

-- 
2.1.0

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

[Gabriel L. Somlo]

This document covers the generic portions of fw_cfg as well as
the x86/x86-64 architecture specific components.

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

This is a resubmission of Jordan's patch from back when:
  http://lists.gnu.org/archive/html/qemu-devel/2011-04/msg00238.html
My own signed-off-by may or may not belong in the commit log, not
quite 100% sure what the etiquette is. Please apply with or without it :)

Thanks,
  Gabriel

 docs/specs/fw_cfg.txt | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 167 insertions(+)
 create mode 100644 docs/specs/fw_cfg.txt

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
new file mode 100644
index 0000000..7d156b7
--- /dev/null
+++ b/docs/specs/fw_cfg.txt
@@ -0,0 +1,167 @@
+
+= Hardware Interface =
+
+== Index Port ==
+* Two bytes in width (guest native endianness)
+* Write only
+* Can be an I/O port and/or Memory-Mapped I/O
+* Location is platform dependent
+
+A write to this port sets the index of a firmware configuration item
+which can subsequently be accessed at the data port.
+
+Setting the index port will cause the data offset to be set to zero.
+The data offset impacts which data is accessed via the data port, and
+is explained below.
+
+Bit15 of the index value indicates if the configuration setting is
+architecture specific.  If bit15 of the index is 0, then the item is
+a generic configuration item.  If bit15 of the index is 1, then the
+item is specific to a particular architecture.  (In other words,
+generic configuration items are accessed when the index is between
+0x0000-0x7fff, and architecture specific configuration items are
+accessed when the index is between 0x8000-0xffff.)
+
+Bit14 of the index value indicates if the configuration setting is
+being written.  If bit14 of the index is 0, then the item is only
+being read, and all write access to the data port will be completely
+ignored.  If bit14 of the index is 1, then the item's data can be
+written to by writing to the data port.  (In other words,
+configuration write mode is enabled when the index is between
+0x4000-0x7fff or 0xc000-0xffff.)
+
+== Data Port ==
+* One byte in width
+* Read + Write
+* Can be an I/O port and/or Memory-Mapped I/O
+* Location is platform dependent
+
+The data port allows for access to an array of bytes for each firmware
+configuration data item.  This item is selected by a write to the
+index port.
+
+Initially following a write to the index port, the data offset will
+be set to zero.  Each successful read or write to the data port will
+cause the data offset to increment by one byte.  There is only one
+data offset value, and it will be incremented by accesses to any of
+the I/O or MMIO data ports.  A write access will not increment the
+data offset if the selected index did not have bit14 set.
+
+Each firmware configuration item has a maximum length of data
+associated with the item.  After the data offset has passed the
+end of this maximum data length, then any reads will return a data
+value of 0x00, and all writes will be ignored.
+
+A read of the data port will return the current byte of the firmware
+configuration item.
+
+A write of the data port will set the current byte of the firmware
+configuration item.  A write access will not impact the firmware
+configuration data if the selected index did not have bit14 set.
+
+== x86, x86-64 Ports ==
+
+I/O  Index Port: 0x510
+I/O  Data  Port: 0x511
+MMIO Index Port: N/A
+MMIO Data  Port: N/A
+
+= Firmware Configuration Items =
+
+== Ranges ==
+
+There are up to 0x4000 generic firmware configuration items, and up to
+0x4000 architecturally specific firmware configuration items.
+
+Index Port Range  Range Usage
+----------------  -----------
+0x0000 - 0x3fff   Generic Items (0x0000 - 0x3fff) (Read-only)
+0x4000 - 0x7fff   Generic Items (0x0000 - 0x3fff) (Read+Write)
+0x8000 - 0xbfff   Architecture Specific Items (0x0000 - 0x3fff) (Read-only)
+0xc000 - 0xffff   Architecture Specific Items (0x0000 - 0x3fff) (Read+Write)
+
+== Data Items Format ==
+
+uint8_t : 8-bit unsigned integer
+uint16_t: 16-bit unsigned integer
+uint32_t: 32-bit unsigned integer
+uint64_t: 64-bit unsigned integer
+n bytes : byte array of length n
+array   : byte array of a format specific size
+string  : null byte terminated ascii string
+
+All integer data is accessed with the least significant byte first and
+then proceeding to more significant bytes on subsequent accesses.
+
+== Generic Items ==
+
+Index  Data Type  Data Meaning
+-----  ---------  ------------
+0x00   4 bytes    Signature - 'Q', 'E', 'M', 'U'
+0x01   uint32_t   ID
+0x02   16 bytes   System UUID
+0x03   uint64_t   RAM Size
+0x04   uint16_t   0 indicates graphics mode, otherwise non-graphics mode
+0x05   uint16_t   The number of SMP CPUs
+0x06   uint16_t   Machine ID
+0x07   uint32_t   Kernel Address
+0x08   uint32_t   Kernel Size
+0x09   string     Kernel command line
+0x0a   uint32_t   Initrd Address
+0x0b   uint32_t   Initrd Size
+0x0c   uint16_t   Boot Device
+0x0d   array      NUMA Data
+0x0e   uint16_t   Boot Menu
+0x0f   uint16_t   The maximum number of CPUs (hotpluggable)
+0x10   uint32_t   Kernel Entry
+0x11   array      Kernel Data
+0x12   array      Initrd Data
+0x13   uint32_t   Command Line Address
+0x14   uint32_t   Command Line Size
+0x15   string     Command Line Data
+0x16   uint32_t   Setup Address
+0x17   uint32_t   Setup Size
+0x18   array      Setup Data
+0x19   array      File Directory
+
+=== File Directory Structure ===
+
+Note: Integers in the file directory structure (index 0x19) are stored
+in big-endian format regardless of the host or guest endianness.  This
+means that the first byte read of the integer is its most significant
+byte.
+
+The structure begins with a uint32_t in big-endian format.
+This number indicates the number of files that are available.
+
+Offset  Data Type     Data Meaning
+------  ------------  ------------
+0x00    uint32_t(be)  File count
+0x04    array         Array of file instance structures.  The total length
+                      of this array is 64-bytes * file_count.
+
+As shown above, following the initial file count uint32_t,
+there is an array of structures.  Each structure is 64-bytes
+in size.  The number of instances of this structure in the
+array is given by the initial uint32_t data value read.
+Each structure within the array has this format:
+
+Offset  Data Type     Data Meaning
+------  ------------  ------------
+0x00    uint32_t(be)  File size
+0x04    uint16_t(be)  Firmware configuration entry index for file data
+0x06    2 bytes       2 reserved bytes
+0x08    56 bytes      File name as a null terminated ascii string
+
+== x86, x86-64 Architecture Items ==
+
+As architecture specific items, these items should be accessed
+starting at 0x8000 for reading or 0xc000 for reading and writing.
+
+Index  Data Type  Data Meaning
+-----  ---------  ------------
+0x00   array      ACPI Tables Data
+0x01   array      SMBIOS Data
+0x02   uint8_t    IRQ0 Override
+0x03   array      E820 Table
+0x04   array      HPET Data
-- 
2.1.0

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

[Laszlo Ersek]

On 03/16/15 15:15, Gabriel L. Somlo wrote:
> This document covers the generic portions of fw_cfg as well as
> the x86/x86-64 architecture specific components.
> 
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> This is a resubmission of Jordan's patch from back when:
>   http://lists.gnu.org/archive/html/qemu-devel/2011-04/msg00238.html
> My own signed-off-by may or may not belong in the commit log, not
> quite 100% sure what the etiquette is. Please apply with or without it :)
> 
> Thanks,
>   Gabriel
> 
>  docs/specs/fw_cfg.txt | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 167 insertions(+)
>  create mode 100644 docs/specs/fw_cfg.txt
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> new file mode 100644
> index 0000000..7d156b7
> --- /dev/null
> +++ b/docs/specs/fw_cfg.txt
> @@ -0,0 +1,167 @@
> +
> += Hardware Interface =
> +
> +== Index Port ==
> +* Two bytes in width (guest native endianness)

Hmmm. I'm not sure if Jordan's patch should be taken separately, then
updated in a separate patch, or if an updated version should be posted
as one squashed patch.

In any case, I'd like to see the term "selector register" mentioned here
(as a synonym, or even replacing "index port"). We have
"Documentation/devicetree/bindings/arm/fw-cfg.txt" in the kernel tree
now, and wording should be consistent.

Second, the selector register's endianness is not guest native *in
general*. The commit message says "generic portions of fw_cfg as well as
the x86/x86-64 architecture specific components", so it's not easy to
decide which this line belongs to.

Namely, on x86 the selector register is mapped as a 16-bit wide, little
endian ioport (see fw_cfg_comb_mem_ops and fw_cfg_io_realize()), and in
that sense "guest native endianness" is correct.

However, on arm and ppc (and whatever else uses fw_cfg_init_mem*()), the
selector register is memory mapped, and it takes the keys in big endian
representation (see fw_cfg_ctl_mem_ops and fw_cfg_mem_realize()). For
the arm target, I think big endian cannot be called "guest native
endianness".

> +* Write only
> +* Can be an I/O port and/or Memory-Mapped I/O

Okay, if you mention MMIO, then the endianness claim is definitely wrong.

> +* Location is platform dependent
> +
> +A write to this port sets the index of a firmware configuration item
> +which can subsequently be accessed at the data port.
> +
> +Setting the index port will cause the data offset to be set to zero.
> +The data offset impacts which data is accessed via the data port, and
> +is explained below.
> +
> +Bit15 of the index value indicates if the configuration setting is
> +architecture specific.  If bit15 of the index is 0, then the item is
> +a generic configuration item.  If bit15 of the index is 1, then the
> +item is specific to a particular architecture.  (In other words,
> +generic configuration items are accessed when the index is between
> +0x0000-0x7fff, and architecture specific configuration items are
> +accessed when the index is between 0x8000-0xffff.)
> +
> +Bit14 of the index value indicates if the configuration setting is
> +being written.  If bit14 of the index is 0, then the item is only
> +being read, and all write access to the data port will be completely
> +ignored.  If bit14 of the index is 1, then the item's data can be
> +written to by writing to the data port.  (In other words,
> +configuration write mode is enabled when the index is between
> +0x4000-0x7fff or 0xc000-0xffff.)
> +
> +== Data Port ==
> +* One byte in width

Not necessarily so, any longer. The memory mapped register can be 1, 2,
4 or 8 bytes wide (see fw_cfg_init_mem_wide(), data_width), with the
choice depending on board code. Then the guest can access the data
register with all of the above widths that do not exceed the maximum
width, but always only at offset #0.

And, somewhat non-intuitively, the data register has no endianness per
se -- the DEVICE_BIG_ENDIAN you see in fw_cfg_data_mem_ops is a pure
technicality -- because it does not transfer numerical values; it is
*string*-preserving.

Once the guest has read an fw_cfg blob, with a sequence of data register
accesses of various allowed widths, *then* it can interpret the blob as
necessary. (For example, (a) the "initrd blob" is an opaque blob, (b)
the "initrd size blob" is a number in little endian encoding, (c) the
"fw_cfg file directory blob" is an array of structures prefixed with the
number of entries, and both that entry count and the numerical fields in
the entry struct have big endian encoding.) Blob transfer and blob
interpretation are separate.

> +* Read + Write
> +* Can be an I/O port and/or Memory-Mapped I/O
> +* Location is platform dependent

I think we should spell out that in the ioport mapping, the two ports
overlap. (They cannot be mapped separately.)

> +
> +The data port allows for access to an array of bytes for each firmware
> +configuration data item.  This item is selected by a write to the
> +index port.
> +
> +Initially following a write to the index port, the data offset will
> +be set to zero.  Each successful read or write to the data port will
> +cause the data offset to increment by one byte.

(In the mmio case, the offset is incremented by the access width, 1 / 2
/ 4 / 8.)

> There is only one
> +data offset value, and it will be incremented by accesses to any of
> +the I/O or MMIO data ports.

I don't understand "any of". There is one data register only.

Hm, yes, I do understand the text; it was originally written when the
API that board code could use had not yet strictly separated the ioport
mapping from the mmio mapping.

>  A write access will not increment the
> +data offset if the selected index did not have bit14 set.
> +
> +Each firmware configuration item has a maximum length of data
> +associated with the item.  After the data offset has passed the
> +end of this maximum data length, then any reads will return a data
> +value of 0x00, and all writes will be ignored.
> +
> +A read of the data port will return the current byte of the firmware
> +configuration item.

In the mmio case, an N-byte wide read of the data register will return
the next N bytes (as a substring of the blob being read) in increasing
address order, like with memcpy().

> +
> +A write of the data port will set the current byte of the firmware
> +configuration item.  A write access will not impact the firmware
> +configuration data if the selected index did not have bit14 set.
> +
> +== x86, x86-64 Ports ==
> +
> +I/O  Index Port: 0x510
> +I/O  Data  Port: 0x511
> +MMIO Index Port: N/A
> +MMIO Data  Port: N/A

Again, these are now mutually exclusive, as provided by the
qemu-internal API. (Of course insane board code could circumvent that,
but it won't.)

> +
> += Firmware Configuration Items =
> +
> +== Ranges ==
> +
> +There are up to 0x4000 generic firmware configuration items, and up to
> +0x4000 architecturally specific firmware configuration items.
> +
> +Index Port Range  Range Usage
> +----------------  -----------
> +0x0000 - 0x3fff   Generic Items (0x0000 - 0x3fff) (Read-only)
> +0x4000 - 0x7fff   Generic Items (0x0000 - 0x3fff) (Read+Write)
> +0x8000 - 0xbfff   Architecture Specific Items (0x0000 - 0x3fff) (Read-only)
> +0xc000 - 0xffff   Architecture Specific Items (0x0000 - 0x3fff) (Read+Write)
> +
> +== Data Items Format ==
> +
> +uint8_t : 8-bit unsigned integer
> +uint16_t: 16-bit unsigned integer
> +uint32_t: 32-bit unsigned integer
> +uint64_t: 64-bit unsigned integer
> +n bytes : byte array of length n
> +array   : byte array of a format specific size
> +string  : null byte terminated ascii string
> +
> +All integer data is accessed with the least significant byte first and
> +then proceeding to more significant bytes on subsequent accesses.
> +

This section needs to be reworked. As I indicated above, blob
interpretation is specific to the individual selector key.

It *is* true that the following internal helper functions:
- fw_cfg_add_i16
- fw_cfg_add_i32
- fw_cfg_add_i64

imbue the blob in question with little endian encoding (they call
cpu_to_leXX internally, before calling fw_cfg_add_bytes()). However, the
term "All integer data" is simply too generic. Pedantically, it should say

  All fw_cfg blobs added with the fw_cfg_add_iXX() functions carry
  integer values of the indicated size, with little-endian internal
  encoding.

A bunch of other "integer data" exists, as part of different kinds of
blobs, and their encodings are arbitrary.

For example, the FWCfgFiles structure (the fw_cfg file directory blob)
that I mentioned above is exposed to the guest directly, and the
following fields are integers, and have big endian encoding (see
fw_cfg_add_file_callback()):

- FWCfgFiles.count
- FWCfgFiles.f[*].size
- FWCfgFiles.f[*].select

> +== Generic Items ==
> +
> +Index  Data Type  Data Meaning
> +-----  ---------  ------------
> +0x00   4 bytes    Signature - 'Q', 'E', 'M', 'U'
> +0x01   uint32_t   ID
> +0x02   16 bytes   System UUID
> +0x03   uint64_t   RAM Size
> +0x04   uint16_t   0 indicates graphics mode, otherwise non-graphics mode
> +0x05   uint16_t   The number of SMP CPUs
> +0x06   uint16_t   Machine ID
> +0x07   uint32_t   Kernel Address
> +0x08   uint32_t   Kernel Size
> +0x09   string     Kernel command line
> +0x0a   uint32_t   Initrd Address
> +0x0b   uint32_t   Initrd Size
> +0x0c   uint16_t   Boot Device
> +0x0d   array      NUMA Data
> +0x0e   uint16_t   Boot Menu
> +0x0f   uint16_t   The maximum number of CPUs (hotpluggable)
> +0x10   uint32_t   Kernel Entry
> +0x11   array      Kernel Data
> +0x12   array      Initrd Data
> +0x13   uint32_t   Command Line Address
> +0x14   uint32_t   Command Line Size
> +0x15   string     Command Line Data
> +0x16   uint32_t   Setup Address
> +0x17   uint32_t   Setup Size
> +0x18   array      Setup Data
> +0x19   array      File Directory

For all of the uintXX_t blobs, their encoding should be pointed out.
(Assuming they are all added with the fw_cfg_add_iXX() functions, they
can be advertised as little endian.)

"string" should be explained as \0-terminated array.

Also, selector key 0x01 ("ID") is now considered an "interface revision
/ feature bitmap". (With value 0 ATM.)

> +
> +=== File Directory Structure ===
> +
> +Note: Integers in the file directory structure (index 0x19) are stored
> +in big-endian format regardless of the host or guest endianness.  This
> +means that the first byte read of the integer is its most significant
> +byte.

I like to separate the transfer of a blob from the interpretation of a
blob more than the above. It's okay with the 1-byte wide ioport, but
with the wide memory-mapped data register, it very easily confuses
people (including me, obviously). So, when talking about a blob's
interpretation, please avoid words like "read" and "write". Transfer is
complete at that point.

> +
> +The structure begins with a uint32_t in big-endian format.
> +This number indicates the number of files that are available.
> +
> +Offset  Data Type     Data Meaning
> +------  ------------  ------------
> +0x00    uint32_t(be)  File count
> +0x04    array         Array of file instance structures.  The total length
> +                      of this array is 64-bytes * file_count.
> +
> +As shown above, following the initial file count uint32_t,
> +there is an array of structures.  Each structure is 64-bytes
> +in size.  The number of instances of this structure in the
> +array is given by the initial uint32_t data value read.
> +Each structure within the array has this format:
> +
> +Offset  Data Type     Data Meaning
> +------  ------------  ------------
> +0x00    uint32_t(be)  File size
> +0x04    uint16_t(be)  Firmware configuration entry index for file data

(selector value or selector key would be the preferred term -- they are
not inherently superior, it's just what I used in the kernel documentation.)

> +0x06    2 bytes       2 reserved bytes
> +0x08    56 bytes      File name as a null terminated ascii string

"NUL-terminated".

> +
> +== x86, x86-64 Architecture Items ==
> +
> +As architecture specific items, these items should be accessed
> +starting at 0x8000 for reading or 0xc000 for reading and writing.
> +
> +Index  Data Type  Data Meaning
> +-----  ---------  ------------
> +0x00   array      ACPI Tables Data
> +0x01   array      SMBIOS Data
> +0x02   uint8_t    IRQ0 Override
> +0x03   array      E820 Table
> +0x04   array      HPET Data

Urgh. No clue about the last three, but I certainly wouldn't advertise
the first two. I think we have much better interfaces now for ACPI and
SMBIOS than the legacy ones above. (And they are exposed as fw_cfg
files, not with fixed selector keys.)

Thanks
Laszlo

[Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes

[Gabriel L. Somlo]

The fw_cfg device allowed guest-side data writes to overwrite the
selected entry in place, without allowing modification to the size
of the entry, and with the ability to invoke a callback each time
the entry was overwritten completely.

To date, we are not aware of any use case which relies on the guest's
ability to (over)write any given fw_cfg entry, and QEMU does not
register any fw_cfg write callbacks.

This patch removes the unused code path for registering fw_cfg write
callbacks, and also removes support for guest-side data writes to the
fw_cfg device.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 docs/specs/fw_cfg.txt     | 21 ++++------------
 hw/nvram/fw_cfg.c         | 61 +++--------------------------------------------
 include/hw/nvram/fw_cfg.h |  4 +---
 3 files changed, 8 insertions(+), 78 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 7d156b7..01955dd 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -22,17 +22,9 @@ generic configuration items are accessed when the index is between
 0x0000-0x7fff, and architecture specific configuration items are
 accessed when the index is between 0x8000-0xffff.)
 
-Bit14 of the index value indicates if the configuration setting is
-being written.  If bit14 of the index is 0, then the item is only
-being read, and all write access to the data port will be completely
-ignored.  If bit14 of the index is 1, then the item's data can be
-written to by writing to the data port.  (In other words,
-configuration write mode is enabled when the index is between
-0x4000-0x7fff or 0xc000-0xffff.)
-
 == Data Port ==
 * One byte in width
-* Read + Write
+* Read only
 * Can be an I/O port and/or Memory-Mapped I/O
 * Location is platform dependent
 
@@ -41,24 +33,19 @@ configuration data item.  This item is selected by a write to the
 index port.
 
 Initially following a write to the index port, the data offset will
-be set to zero.  Each successful read or write to the data port will
+be set to zero.  Each successful read to the data port will
 cause the data offset to increment by one byte.  There is only one
 data offset value, and it will be incremented by accesses to any of
-the I/O or MMIO data ports.  A write access will not increment the
-data offset if the selected index did not have bit14 set.
+the I/O or MMIO data ports.
 
 Each firmware configuration item has a maximum length of data
 associated with the item.  After the data offset has passed the
 end of this maximum data length, then any reads will return a data
-value of 0x00, and all writes will be ignored.
+value of 0x00.
 
 A read of the data port will return the current byte of the firmware
 configuration item.
 
-A write of the data port will set the current byte of the firmware
-configuration item.  A write access will not impact the firmware
-configuration data if the selected index did not have bit14 set.
-
 == x86, x86-64 Ports ==
 
 I/O  Index Port: 0x510
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 78a37be..86090f3 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -46,7 +46,6 @@ typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
     void *callback_opaque;
-    FWCfgCallback callback;
     FWCfgReadCallback read_callback;
 } FWCfgEntry;
 
@@ -230,23 +229,6 @@ static void fw_cfg_reboot(FWCfgState *s)
     fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4);
 }
 
-static void fw_cfg_write(FWCfgState *s, uint8_t value)
-{
-    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
-
-    trace_fw_cfg_write(s, value);
-
-    if (s->cur_entry & FW_CFG_WRITE_CHANNEL && e->callback &&
-        s->cur_offset < e->len) {
-        e->data[s->cur_offset++] = value;
-        if (s->cur_offset == e->len) {
-            e->callback(e->callback_opaque, e->data);
-            s->cur_offset = 0;
-        }
-    }
-}
-
 static int fw_cfg_select(FWCfgState *s, uint16_t key)
 {
     int ret;
@@ -296,21 +278,10 @@ static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr,
     return value;
 }
 
-static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
-                                  uint64_t value, unsigned size)
-{
-    FWCfgState *s = opaque;
-    unsigned i = size;
-
-    do {
-        fw_cfg_write(s, value >> (8 * --i));
-    } while (i);
-}
-
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
-    return addr == 0;
+    return addr == 0 && !is_write;
 }
 
 static void fw_cfg_ctl_mem_write(void *opaque, hwaddr addr,
@@ -334,20 +305,13 @@ static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
 static void fw_cfg_comb_write(void *opaque, hwaddr addr,
                               uint64_t value, unsigned size)
 {
-    switch (size) {
-    case 1:
-        fw_cfg_write(opaque, (uint8_t)value);
-        break;
-    case 2:
-        fw_cfg_select(opaque, (uint16_t)value);
-        break;
-    }
+    fw_cfg_select(opaque, (uint16_t)value);
 }
 
 static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
-    return (size == 1) || (is_write && size == 2);
+    return (!is_write && size == 1) || (is_write && size == 2);
 }
 
 static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
@@ -358,7 +322,6 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
 
 static const MemoryRegionOps fw_cfg_data_mem_ops = {
     .read = fw_cfg_data_mem_read,
-    .write = fw_cfg_data_mem_write,
     .endianness = DEVICE_BIG_ENDIAN,
     .valid = {
         .min_access_size = 1,
@@ -458,7 +421,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = len;
     s->entries[arch][key].callback_opaque = NULL;
-    s->entries[arch][key].callback = NULL;
 
     return ptr;
 }
@@ -502,23 +464,6 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
-void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                         void *callback_opaque, void *data, size_t len)
-{
-    int arch = !!(key & FW_CFG_ARCH_LOCAL);
-
-    assert(key & FW_CFG_WRITE_CHANNEL);
-
-    key &= FW_CFG_ENTRY_MASK;
-
-    assert(key < FW_CFG_MAX_ENTRY && len <= UINT32_MAX);
-
-    s->entries[arch][key].data = data;
-    s->entries[arch][key].len = (uint32_t)len;
-    s->entries[arch][key].callback_opaque = callback_opaque;
-    s->entries[arch][key].callback = callback;
-}
-
 void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
                               void *data, size_t len)
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 6d8a8ac..f11d7d5 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -40,7 +40,7 @@
 #define FW_CFG_FILE_SLOTS       0x10
 #define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
 
-#define FW_CFG_WRITE_CHANNEL    0x4000
+#define FW_CFG_WRITE_CHANNEL    0x4000	/* reserved (not implemented) */
 #define FW_CFG_ARCH_LOCAL       0x8000
 #define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
 
@@ -69,8 +69,6 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
 void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
 void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
 void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
-void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                         void *callback_opaque, void *data, size_t len);
 void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
                      size_t len);
 void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
-- 
2.1.0

Re: [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes

[Laszlo Ersek]

On 03/16/15 15:15, Gabriel L. Somlo wrote:
> The fw_cfg device allowed guest-side data writes to overwrite the
> selected entry in place, without allowing modification to the size
> of the entry, and with the ability to invoke a callback each time
> the entry was overwritten completely.
> 
> To date, we are not aware of any use case which relies on the guest's
> ability to (over)write any given fw_cfg entry, and QEMU does not
> register any fw_cfg write callbacks.
> 
> This patch removes the unused code path for registering fw_cfg write
> callbacks, and also removes support for guest-side data writes to the
> fw_cfg device.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  docs/specs/fw_cfg.txt     | 21 ++++------------
>  hw/nvram/fw_cfg.c         | 61 +++--------------------------------------------
>  include/hw/nvram/fw_cfg.h |  4 +---
>  3 files changed, 8 insertions(+), 78 deletions(-)
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 7d156b7..01955dd 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -22,17 +22,9 @@ generic configuration items are accessed when the index is between
>  0x0000-0x7fff, and architecture specific configuration items are
>  accessed when the index is between 0x8000-0xffff.)
>  
> -Bit14 of the index value indicates if the configuration setting is
> -being written.  If bit14 of the index is 0, then the item is only
> -being read, and all write access to the data port will be completely
> -ignored.  If bit14 of the index is 1, then the item's data can be
> -written to by writing to the data port.  (In other words,
> -configuration write mode is enabled when the index is between
> -0x4000-0x7fff or 0xc000-0xffff.)
> -
>  == Data Port ==
>  * One byte in width
> -* Read + Write
> +* Read only
>  * Can be an I/O port and/or Memory-Mapped I/O
>  * Location is platform dependent
>  
> @@ -41,24 +33,19 @@ configuration data item.  This item is selected by a write to the
>  index port.
>  
>  Initially following a write to the index port, the data offset will
> -be set to zero.  Each successful read or write to the data port will
> +be set to zero.  Each successful read to the data port will
>  cause the data offset to increment by one byte.  There is only one
>  data offset value, and it will be incremented by accesses to any of
> -the I/O or MMIO data ports.  A write access will not increment the
> -data offset if the selected index did not have bit14 set.
> +the I/O or MMIO data ports.
>  
>  Each firmware configuration item has a maximum length of data
>  associated with the item.  After the data offset has passed the
>  end of this maximum data length, then any reads will return a data
> -value of 0x00, and all writes will be ignored.
> +value of 0x00.
>  
>  A read of the data port will return the current byte of the firmware
>  configuration item.
>  
> -A write of the data port will set the current byte of the firmware
> -configuration item.  A write access will not impact the firmware
> -configuration data if the selected index did not have bit14 set.
> -
>  == x86, x86-64 Ports ==
>  
>  I/O  Index Port: 0x510
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 78a37be..86090f3 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -46,7 +46,6 @@ typedef struct FWCfgEntry {
>      uint32_t len;
>      uint8_t *data;
>      void *callback_opaque;
> -    FWCfgCallback callback;
>      FWCfgReadCallback read_callback;
>  } FWCfgEntry;
>  
> @@ -230,23 +229,6 @@ static void fw_cfg_reboot(FWCfgState *s)
>      fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4);
>  }
>  
> -static void fw_cfg_write(FWCfgState *s, uint8_t value)
> -{
> -    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> -    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
> -
> -    trace_fw_cfg_write(s, value);
> -
> -    if (s->cur_entry & FW_CFG_WRITE_CHANNEL && e->callback &&
> -        s->cur_offset < e->len) {
> -        e->data[s->cur_offset++] = value;
> -        if (s->cur_offset == e->len) {
> -            e->callback(e->callback_opaque, e->data);
> -            s->cur_offset = 0;
> -        }
> -    }
> -}
> -
>  static int fw_cfg_select(FWCfgState *s, uint16_t key)
>  {
>      int ret;
> @@ -296,21 +278,10 @@ static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr,
>      return value;
>  }
>  
> -static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
> -                                  uint64_t value, unsigned size)
> -{
> -    FWCfgState *s = opaque;
> -    unsigned i = size;
> -
> -    do {
> -        fw_cfg_write(s, value >> (8 * --i));
> -    } while (i);
> -}
> -
>  static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> -    return addr == 0;
> +    return addr == 0 && !is_write;
>  }
>  
>  static void fw_cfg_ctl_mem_write(void *opaque, hwaddr addr,
> @@ -334,20 +305,13 @@ static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
>  static void fw_cfg_comb_write(void *opaque, hwaddr addr,
>                                uint64_t value, unsigned size)
>  {
> -    switch (size) {
> -    case 1:
> -        fw_cfg_write(opaque, (uint8_t)value);
> -        break;
> -    case 2:
> -        fw_cfg_select(opaque, (uint16_t)value);
> -        break;
> -    }
> +    fw_cfg_select(opaque, (uint16_t)value);
>  }
>  
>  static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> -    return (size == 1) || (is_write && size == 2);
> +    return (!is_write && size == 1) || (is_write && size == 2);
>  }
>  
>  static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
> @@ -358,7 +322,6 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
>  
>  static const MemoryRegionOps fw_cfg_data_mem_ops = {
>      .read = fw_cfg_data_mem_read,
> -    .write = fw_cfg_data_mem_write,
>      .endianness = DEVICE_BIG_ENDIAN,
>      .valid = {
>          .min_access_size = 1,
> @@ -458,7 +421,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>      s->entries[arch][key].data = data;
>      s->entries[arch][key].len = len;
>      s->entries[arch][key].callback_opaque = NULL;
> -    s->entries[arch][key].callback = NULL;
>  
>      return ptr;
>  }
> @@ -502,23 +464,6 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> -void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
> -                         void *callback_opaque, void *data, size_t len)
> -{
> -    int arch = !!(key & FW_CFG_ARCH_LOCAL);
> -
> -    assert(key & FW_CFG_WRITE_CHANNEL);
> -
> -    key &= FW_CFG_ENTRY_MASK;
> -
> -    assert(key < FW_CFG_MAX_ENTRY && len <= UINT32_MAX);
> -
> -    s->entries[arch][key].data = data;
> -    s->entries[arch][key].len = (uint32_t)len;
> -    s->entries[arch][key].callback_opaque = callback_opaque;
> -    s->entries[arch][key].callback = callback;
> -}
> -
>  void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>                                FWCfgReadCallback callback, void *callback_opaque,
>                                void *data, size_t len)
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 6d8a8ac..f11d7d5 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -40,7 +40,7 @@
>  #define FW_CFG_FILE_SLOTS       0x10
>  #define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
>  
> -#define FW_CFG_WRITE_CHANNEL    0x4000
> +#define FW_CFG_WRITE_CHANNEL    0x4000	/* reserved (not implemented) */
>  #define FW_CFG_ARCH_LOCAL       0x8000
>  #define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
>  
> @@ -69,8 +69,6 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
>  void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
>  void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
>  void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
> -void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
> -                         void *callback_opaque, void *data, size_t len);
>  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
>                       size_t len);
>  void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
> 

- I mostly ignored the documentation hunks, due to my comments for patch
1/6 (ie. it should be reworked first).

- The code hunks seem okay to me. But, you also remove a trace call
(trace_fw_cfg_write), so the corresponding trace point should be dropped
from the file "trace-events".

- I can't tell of the top of my head what happens if the guest attempts
an fw_cfg data write nonetheless. I vaguely recall some unassigned-io
stuff from MemoryRegion land, which ultimately renders the guest access
effect-less. Can anyone please test / confirm / explain that?

Hm, the new validity checks should catch those:

memory_region_dispatch_write()
  memory_region_access_valid()
    mr->ops->valid.accepts()
  unassigned_mem_write()
    cpu_unassigned_access()
      cc->do_unassigned_access()

Seems to land in the CPUClass.do_unassigned_access() member, if there is
one.

Hm. The intersection between "has non-NULL do_unassigned_access()" and
"uses fw_cfg" seems to SPARC. See:
- sparc_cpu_unassigned_access() in "target-sparc/ldst_helper.c",
- fw_cfg_init_mem() in "hw/sparc/sun4m.c",
- fw_cfg_init_io() in "hw/sparc64/sun4u.c".

I don't have the slightest clue if we should care, but *theoretically*,
this change could turn guest code (ie. fw_cfg data writes) that used to
do "nothing" into traps. Someone else will have to chime in on that.

Oh why is nothing simple. :(

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes

[Gabriel L. Somlo]

On Mon, Mar 16, 2015 at 06:02:45PM +0100, Laszlo Ersek wrote:
> On 03/16/15 15:15, Gabriel L. Somlo wrote:
> > The fw_cfg device allowed guest-side data writes to overwrite the
> > selected entry in place, without allowing modification to the size
> > of the entry, and with the ability to invoke a callback each time
> > the entry was overwritten completely.
> > 
> > To date, we are not aware of any use case which relies on the guest's
> > ability to (over)write any given fw_cfg entry, and QEMU does not
> > register any fw_cfg write callbacks.
> > 
> > This patch removes the unused code path for registering fw_cfg write
> > callbacks, and also removes support for guest-side data writes to the
> > fw_cfg device.
> > 
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
>
> - The code hunks seem okay to me. But, you also remove a trace call
> (trace_fw_cfg_write), so the corresponding trace point should be dropped
> from the file "trace-events".
> 
> - I can't tell of the top of my head what happens if the guest attempts
> an fw_cfg data write nonetheless. I vaguely recall some unassigned-io
> stuff from MemoryRegion land, which ultimately renders the guest access
> effect-less. Can anyone please test / confirm / explain that?
> 
> Hm, the new validity checks should catch those:
> 
> memory_region_dispatch_write()
>   memory_region_access_valid()
>     mr->ops->valid.accepts()
>   unassigned_mem_write()
>     cpu_unassigned_access()
>       cc->do_unassigned_access()
> 
> Seems to land in the CPUClass.do_unassigned_access() member, if there is
> one.
> 
> Hm. The intersection between "has non-NULL do_unassigned_access()" and
> "uses fw_cfg" seems to SPARC. See:
> - sparc_cpu_unassigned_access() in "target-sparc/ldst_helper.c",
> - fw_cfg_init_mem() in "hw/sparc/sun4m.c",
> - fw_cfg_init_io() in "hw/sparc64/sun4u.c".
> 
> I don't have the slightest clue if we should care, but *theoretically*,
> this change could turn guest code (ie. fw_cfg data writes) that used to
> do "nothing" into traps. Someone else will have to chime in on that.
> 
> Oh why is nothing simple. :(

Another possibility is to leave the write_ops in there, but turn them
into no-ops. Or just keep support for writing data to fw_cfg -- I don't
have strong feelings in this regard, it only came up during the Q&A
about fw_cfg internals I started a bit earlier... :)

If turning fw_cfg writes into traps is unacceptable for some reason,
and we still collectively think it's a good idea to remove write
support, I also have nothing against trying to wrap my head around
do_unassigned_access(), of which I know nothing right now.

So I echo Laszlo's sentiment that we need more feedback on this. While
I totally welcome opportunities to learn about QEMU, I'd also rather not
go off on too many tangents unless there's a good technical motivation
to do so w.r.t. my original goal (in this case, that's to allow fw_cfg
blobs to be added from the command line).

Thanks much,
--Gabriel

Re: [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes

[Gerd Hoffmann]

  Hi,

> Another possibility is to leave the write_ops in there, but turn them
> into no-ops.

That is the safe choice.  Just have a write function with /* nothing,
write support removed in qemu 2.4+ */.  That is basically the same we
have today (writes are ignored), but a bit more efficient because we
kill the unused infrastructure to register writable fw_cfg entries and
thus don't have to do the lookup just to figure there is no match ;)

cheers,
  Gerd

[Qemu-devel] [PATCH 3/6] fw_cfg: assertion to detect memory leak when adding new data blob

[Gabriel L. Somlo]

Currently, fw_cfg_add_bytes_read_callback() does not deal with
the possibility that the data pointer at the requested key position
has previously been set, and assumes it will be called exactly once
for each key value.

This patch introduces an assertion to codify this assumption, and
insure the data pointer about to be set is NULL at the time the
function is called, which will prevent the inadvertent leaking of
data blobs by erroneous multiple calls using the same key value.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/nvram/fw_cfg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 86090f3..5501a97 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -399,6 +399,7 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
     key &= FW_CFG_ENTRY_MASK;
 
     assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
+    assert(s->entries[arch][key].data == NULL); /* prevent memory leak */
 
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = (uint32_t)len;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 3/6] fw_cfg: assertion to detect memory leak when adding new data blob

[Laszlo Ersek]

On 03/16/15 15:15, Gabriel L. Somlo wrote:
> Currently, fw_cfg_add_bytes_read_callback() does not deal with
> the possibility that the data pointer at the requested key position
> has previously been set, and assumes it will be called exactly once
> for each key value.
> 
> This patch introduces an assertion to codify this assumption, and
> insure the data pointer about to be set is NULL at the time the
> function is called, which will prevent the inadvertent leaking of
> data blobs by erroneous multiple calls using the same key value.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/nvram/fw_cfg.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 86090f3..5501a97 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -399,6 +399,7 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
>      key &= FW_CFG_ENTRY_MASK;
>  
>      assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
> +    assert(s->entries[arch][key].data == NULL); /* prevent memory leak */
>  
>      s->entries[arch][key].data = data;
>      s->entries[arch][key].len = (uint32_t)len;
> 

I think I agree with the patch (the assert itself), but the comment
could be more precise. I'd simply say "avoid selector key conflict" or
some such. The predicate we want to assert here primarily is "single
assignment of selector key", right? Calling
fw_cfg_add_bytes_read_callback() with the same key is an error
regardless of any leaks (you could call it with a pointer to a static
storage duration object, and it would remain an error just the same).

Thanks
Laszlo

[Qemu-devel] [PATCH 4/6] fw_cfg: exit with error when dupe fw_cfg file name inserted

[Gabriel L. Somlo]

Currently, when fw_cfg_add_file_callback() is invoked with a
duplicate file name, it gets to insert the data blob at the
next available selector, but exit (signalling the error via
a call to the trace_fw_cfg_add_file_dupe() function) before
incrementing the next available selector as soon as it finds
the requested file name to be a dupe.

As a consequence, the immediately following invocation of
fw_cfg_add_file_callback() will cause the data blob pointer
at the current selector to be overwritten, and therefore
leak the old data blob inserted during the previous failed
call (or, instead, trigger the newly added assertion which
guards against leaking data blobs by overwriting their
pointers).

This patch modifies fw_cfg_add_file_callback() to exit qemu
with an error whenever a duplicate fw_cfg file name insertion
is requested.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/nvram/fw_cfg.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 5501a97..86f120e 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -481,17 +481,18 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
     index = be32_to_cpu(s->files->count);
     assert(index < FW_CFG_FILE_SLOTS);
 
+    for (i = 0; i < index; i++) {
+        if (strcmp(filename, s->files->f[i].name) == 0) {
+            error_report("duplicate fw_cfg file name: %s", filename);
+            exit(1);
+        }
+    }
+
     fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index,
                                    callback, callback_opaque, data, len);
 
     pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
             filename);
-    for (i = 0; i < index; i++) {
-        if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
-            trace_fw_cfg_add_file_dupe(s, s->files->f[index].name);
-            return;
-        }
-    }
 
     s->files->f[index].size   = cpu_to_be32(len);
     s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
-- 
2.1.0

Re: [Qemu-devel] [PATCH 4/6] fw_cfg: exit with error when dupe fw_cfg file name inserted

[Laszlo Ersek]

On 03/16/15 15:15, Gabriel L. Somlo wrote:
> Currently, when fw_cfg_add_file_callback() is invoked with a
> duplicate file name, it gets to insert the data blob at the
> next available selector, but exit (signalling the error via
> a call to the trace_fw_cfg_add_file_dupe() function) before
> incrementing the next available selector as soon as it finds
> the requested file name to be a dupe.
> 
> As a consequence, the immediately following invocation of
> fw_cfg_add_file_callback() will cause the data blob pointer
> at the current selector to be overwritten, and therefore
> leak the old data blob inserted during the previous failed
> call (or, instead, trigger the newly added assertion which
> guards against leaking data blobs by overwriting their
> pointers).
> 
> This patch modifies fw_cfg_add_file_callback() to exit qemu
> with an error whenever a duplicate fw_cfg file name insertion
> is requested.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/nvram/fw_cfg.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 5501a97..86f120e 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -481,17 +481,18 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>      index = be32_to_cpu(s->files->count);
>      assert(index < FW_CFG_FILE_SLOTS);
>  
> +    for (i = 0; i < index; i++) {
> +        if (strcmp(filename, s->files->f[i].name) == 0) {
> +            error_report("duplicate fw_cfg file name: %s", filename);
> +            exit(1);
> +        }
> +    }
> +
>      fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index,
>                                     callback, callback_opaque, data, len);
>  
>      pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
>              filename);
> -    for (i = 0; i < index; i++) {
> -        if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
> -            trace_fw_cfg_add_file_dupe(s, s->files->f[index].name);
> -            return;
> -        }
> -    }
>  
>      s->files->f[index].size   = cpu_to_be32(len);
>      s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
> 

I agree with the general direction of this patch. I also accept that
exiting here on error is probably "best", considering how changing the
prototype to propagate an error would turn upside down the many callers
for little benefit.

The details can be improved however:
- your hoisted comparison compares the full input filename against
filenames already in the directory. That will miss matches if only a
distant suffix of the filename differs. The pre-patch code truncates
first, then compares. This becomes an issue because you're going to
expose the filename on the command line (ie. it becomes UI from API).

In fact, what speaks against simply replacing the trace + the return
statement with error_report() + exit() in-place?

Alternatively, you could assert that the input filename is not overlong,
and check that predicate in the caller (patch 5/6).

- Another trace call removed; if there are no other such calls, the
tracepoint definition should be removed too.

... I'm tired, I'll have to stop reviewing here.

Thanks
Laszlo

[Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gabriel L. Somlo]

Allow user supplied files to be inserted into the fw_cfg
device before starting the guest. Since fw_cfg_add_file()
already disallows duplicate fw_cfg file names, qemu will
exit with an error message if the user supplies multiple
blobs with the same fw_cfg file name, or if a blob name
collides with a fw_cfg name used internally by qemu.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/nvram/fw_cfg.c         | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/nvram/fw_cfg.h |  4 ++++
 qemu-options.hx           | 10 ++++++++++
 vl.c                      | 27 ++++++++++++++++++++++++++
 4 files changed, 89 insertions(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 86f120e..70e9805 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -29,6 +29,7 @@
 #include "trace.h"
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
+#include "hw/loader.h"
 
 #define FW_CFG_SIZE 2
 #define FW_CFG_NAME "fw_cfg"
@@ -549,6 +550,51 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
 }
 
 
+static struct FWCfgOption {
+    const char *name;
+    const char *file;
+} *fw_cfg_options;
+static int fw_cfg_num_options;
+
+void fw_cfg_option_add(QemuOpts *opts)
+{
+    const char *name = qemu_opt_get(opts, "name");
+    const char *file = qemu_opt_get(opts, "file");
+
+    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
+        error_report("invalid argument value");
+        exit(1);
+    }
+
+    fw_cfg_options = g_realloc(fw_cfg_options, sizeof(struct FWCfgOption) *
+                                               (fw_cfg_num_options + 1));
+    fw_cfg_options[fw_cfg_num_options].name = name;
+    fw_cfg_options[fw_cfg_num_options].file = file;
+    fw_cfg_num_options++;
+}
+
+static void fw_cfg_options_insert(FWCfgState *s)
+{
+    int i, filesize;
+    const char *filename;
+    void *filebuf;
+
+    for (i = 0; i < fw_cfg_num_options; i++) {
+        filename = fw_cfg_options[i].file;
+        filesize = get_image_size(filename);
+        if (filesize == -1) {
+            error_report("Cannot read fw_cfg file %s", filename);
+            exit(1);
+        }
+        filebuf = g_malloc(filesize);
+        if (load_image(filename, filebuf) != filesize) {
+            error_report("Failed to load fw_cfg file %s", filename);
+            exit(1);
+        }
+        fw_cfg_add_file(s, fw_cfg_options[i].name, filebuf, filesize);
+    }
+}
+
 
 static void fw_cfg_init1(DeviceState *dev)
 {
@@ -560,6 +606,8 @@ static void fw_cfg_init1(DeviceState *dev)
 
     qdev_init_nofail(dev);
 
+    fw_cfg_options_insert(s);
+
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index f11d7d5..20a62d4 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -7,6 +7,7 @@
 
 #include "exec/hwaddr.h"
 #include "qemu/typedefs.h"
+#include "qemu/option.h"
 #endif
 
 #define FW_CFG_SIGNATURE        0x00
@@ -76,6 +77,9 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
                               void *data, size_t len);
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
                          size_t len);
+
+void fw_cfg_option_add(QemuOpts *opts);
+
 FWCfgState *fw_cfg_init_io(uint32_t iobase);
 FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
 FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
diff --git a/qemu-options.hx b/qemu-options.hx
index 3c852f1..94ce91b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2668,6 +2668,16 @@ STEXI
 @table @option
 ETEXI
 
+DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
+    "-fw_cfg name=<name>,file=<file>\n"
+    "                add named fw_cfg blob from file\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -fw_cfg name=@var{name},file=@var{file}
+@findex -fw_cfg
+Add named fw_cfg blob from file
+ETEXI
+
 DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
     "-serial dev     redirect the serial port to char device 'dev'\n",
     QEMU_ARCH_ALL)
diff --git a/vl.c b/vl.c
index 694deb4..6a30e61 100644
--- a/vl.c
+++ b/vl.c
@@ -490,6 +490,25 @@ static QemuOptsList qemu_semihosting_config_opts = {
     },
 };
 
+static QemuOptsList qemu_fw_cfg_opts = {
+    .name = "fw_cfg",
+    .implied_opt_name = "name",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_fw_cfg_opts.head),
+    .desc = {
+        {
+            .name = "name",
+            .type = QEMU_OPT_STRING,
+            .help = "Sets the fw_cfg name of the blob to be inserted",
+        }, {
+            .name = "file",
+            .type = QEMU_OPT_STRING,
+            .help = "Sets the name of the file from which\n"
+                    "the fw_cfg blob will be loaded",
+        },
+        { /* end of list */ }
+    },
+};
+
 /**
  * Get machine options
  *
@@ -2804,6 +2823,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_numa_opts);
     qemu_add_opts(&qemu_icount_opts);
     qemu_add_opts(&qemu_semihosting_config_opts);
+    qemu_add_opts(&qemu_fw_cfg_opts);
 
     runstate_init();
 
@@ -3420,6 +3440,13 @@ int main(int argc, char **argv, char **envp)
                 }
                 do_smbios_option(opts);
                 break;
+            case QEMU_OPTION_fwcfg:
+                opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 0);
+                if (opts == NULL) {
+                    exit(1);
+                }
+                fw_cfg_option_add(opts);
+                break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse(olist, "accel=kvm", 0);
-- 
2.1.0

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gerd Hoffmann]

  Hi,

> +static struct FWCfgOption {
> +    const char *name;
> +    const char *file;
> +} *fw_cfg_options;
> +static int fw_cfg_num_options;

> +void fw_cfg_option_add(QemuOpts *opts)

> +static void fw_cfg_options_insert(FWCfgState *s)

Hmm, when looking at this (and the existing fw_cfg init order issues we
have) I get the feeling that we should simply separate the fw_cfg
storage and the fw_cfg device, with the storage being initialized early
as global service ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Matt Fleming]

On Mon, 2015-03-16 at 10:15 -0400, Gabriel L. Somlo wrote:
> Allow user supplied files to be inserted into the fw_cfg
> device before starting the guest. Since fw_cfg_add_file()
> already disallows duplicate fw_cfg file names, qemu will
> exit with an error message if the user supplies multiple
> blobs with the same fw_cfg file name, or if a blob name
> collides with a fw_cfg name used internally by qemu.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/nvram/fw_cfg.c         | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h |  4 ++++
>  qemu-options.hx           | 10 ++++++++++
>  vl.c                      | 27 ++++++++++++++++++++++++++
>  4 files changed, 89 insertions(+)

[...]

> +void fw_cfg_option_add(QemuOpts *opts)
> +{
> +    const char *name = qemu_opt_get(opts, "name");
> +    const char *file = qemu_opt_get(opts, "file");
> +
> +    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
> +        error_report("invalid argument value");
> +        exit(1);
> +    }

Just because I know I'm going to get this wrong when I start using it,
can this error message mention that the fw_cfg argument is invalid,
rather than being ambiguous?

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gabriel L. Somlo]

Matt,

On Tue, Mar 17, 2015 at 10:55:30AM +0000, Matt Fleming wrote:
> > +void fw_cfg_option_add(QemuOpts *opts)
> > +{
> > +    const char *name = qemu_opt_get(opts, "name");
> > +    const char *file = qemu_opt_get(opts, "file");
> > +
> > +    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
> > +        error_report("invalid argument value");
> > +        exit(1);
> > +    }
> 
> Just because I know I'm going to get this wrong when I start using it,
> can this error message mention that the fw_cfg argument is invalid,
> rather than being ambiguous?

With a command line containing e.g. "-fw_cfg file=,name=" to trigger
that error, the full error message ends up reading like this:


qemu-system-x86_64: -fw_cfg file=,name=: invalid argument value


So I figured anything more specific would end up repeating "fw_cfg"
too many times on the same line of output... :)

What do you think ?

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Laszlo Ersek]

comments below

On 03/16/15 15:15, Gabriel L. Somlo wrote:
> Allow user supplied files to be inserted into the fw_cfg
> device before starting the guest. Since fw_cfg_add_file()
> already disallows duplicate fw_cfg file names, qemu will
> exit with an error message if the user supplies multiple
> blobs with the same fw_cfg file name, or if a blob name
> collides with a fw_cfg name used internally by qemu.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/nvram/fw_cfg.c         | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h |  4 ++++
>  qemu-options.hx           | 10 ++++++++++
>  vl.c                      | 27 ++++++++++++++++++++++++++
>  4 files changed, 89 insertions(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 86f120e..70e9805 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -29,6 +29,7 @@
>  #include "trace.h"
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
> +#include "hw/loader.h"
>  
>  #define FW_CFG_SIZE 2
>  #define FW_CFG_NAME "fw_cfg"
> @@ -549,6 +550,51 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
>  }
>  
>  
> +static struct FWCfgOption {
> +    const char *name;
> +    const char *file;
> +} *fw_cfg_options;
> +static int fw_cfg_num_options;

"Number of anything" should always have type "unsigned", or (even
better) size_t.

> +
> +void fw_cfg_option_add(QemuOpts *opts)
> +{
> +    const char *name = qemu_opt_get(opts, "name");
> +    const char *file = qemu_opt_get(opts, "file");
> +
> +    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
> +        error_report("invalid argument value");
> +        exit(1);
> +    }

Okay, I don't recall the details of what I'm going to recommend. :)

Please use the location API to tie the error message to the offending
QemuOpts. I've done that only once before, but it didn't turn out to be
a catastrophe, so now I'm recommending it to you as well. See commit
637a5acb; specifically the code around the "Location" object.

(CC'ing Markus.)

> +
> +    fw_cfg_options = g_realloc(fw_cfg_options, sizeof(struct FWCfgOption) *
> +                                               (fw_cfg_num_options + 1));
> +    fw_cfg_options[fw_cfg_num_options].name = name;
> +    fw_cfg_options[fw_cfg_num_options].file = file;
> +    fw_cfg_num_options++;
> +}

I'm not a big fan of reallocs with linearly increasing size (plus glib
should provide a list type anyway), but it's probably not too bad in
this case.

> +
> +static void fw_cfg_options_insert(FWCfgState *s)
> +{
> +    int i, filesize;

Urgh. :)

The loop variable should match the type of fw_cfg_num_options. It does
now, but after you change that to unsigned or size_t, this should be
updated too.

Second, filesize as "int"? :) Hm, okay, get_image_size() returns an int.
No comment on that. :)

> +    const char *filename;
> +    void *filebuf;
> +
> +    for (i = 0; i < fw_cfg_num_options; i++) {
> +        filename = fw_cfg_options[i].file;
> +        filesize = get_image_size(filename);
> +        if (filesize == -1) {
> +            error_report("Cannot read fw_cfg file %s", filename);
> +            exit(1);
> +        }
> +        filebuf = g_malloc(filesize);
> +        if (load_image(filename, filebuf) != filesize) {
> +            error_report("Failed to load fw_cfg file %s", filename);
> +            exit(1);
> +        }
> +        fw_cfg_add_file(s, fw_cfg_options[i].name, filebuf, filesize);
> +    }
> +}

How about calling g_file_get_contents() instead? That's what I used in
load_image_to_fw_cfg(), in "hw/arm/boot.c" (commit 07abe45c). It
certainly beats the get_image_size() + load_image() pair.

(Function read_splashfile() in this same source file uses
g_file_get_contents() already.)

In addition, I see no reason why loading the file contents couldn't be
moved into fw_cfg_option_add() at once. That way you'll get any
g_file_get_contents()-related errors too while the associated QemuOpts
object is still available, and then you can report those errors too with
a reference to the offending option (see Location again).

This idea would require replacing the "file" member of "FWCfgOption"
with two new fields, "size" and "contents".

> +
>  
>  static void fw_cfg_init1(DeviceState *dev)
>  {
> @@ -560,6 +606,8 @@ static void fw_cfg_init1(DeviceState *dev)
>  
>      qdev_init_nofail(dev);
>  
> +    fw_cfg_options_insert(s);
> +

So this is why you need to separate stashing the filenames from actually
linking the blobs into fw_cfg. Makes sense.

And, according to the suggestion above, fw_cfg_options_insert() would
only consist of a loop that calls fw_cfg_add_file(). That looks very
pleasing to me. :) (Well, I'm suggesting it, duh.) You can't deny it
would closely match the calls just below:

>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
>      fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
>      fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f11d7d5..20a62d4 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -7,6 +7,7 @@
>  
>  #include "exec/hwaddr.h"
>  #include "qemu/typedefs.h"
> +#include "qemu/option.h"
>  #endif
>  
>  #define FW_CFG_SIGNATURE        0x00
> @@ -76,6 +77,9 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
>                                void *data, size_t len);
>  void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
>                           size_t len);
> +
> +void fw_cfg_option_add(QemuOpts *opts);
> +
>  FWCfgState *fw_cfg_init_io(uint32_t iobase);
>  FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
>  FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3c852f1..94ce91b 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2668,6 +2668,16 @@ STEXI
>  @table @option
>  ETEXI
>  
> +DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
> +    "-fw_cfg name=<name>,file=<file>\n"
> +    "                add named fw_cfg blob from file\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> +@item -fw_cfg name=@var{name},file=@var{file}
> +@findex -fw_cfg
> +Add named fw_cfg blob from file

A few words on the "name" property would be appreciated, like
"@var{name} determines the name of the corresponding entry in the fw_cfg
file directory".

> +ETEXI
> +
>  DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
>      "-serial dev     redirect the serial port to char device 'dev'\n",
>      QEMU_ARCH_ALL)
> diff --git a/vl.c b/vl.c
> index 694deb4..6a30e61 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -490,6 +490,25 @@ static QemuOptsList qemu_semihosting_config_opts = {
>      },
>  };
>  
> +static QemuOptsList qemu_fw_cfg_opts = {
> +    .name = "fw_cfg",
> +    .implied_opt_name = "name",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_fw_cfg_opts.head),
> +    .desc = {
> +        {
> +            .name = "name",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Sets the fw_cfg name of the blob to be inserted",
> +        }, {
> +            .name = "file",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Sets the name of the file from which\n"
> +                    "the fw_cfg blob will be loaded",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  /**
>   * Get machine options
>   *
> @@ -2804,6 +2823,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_numa_opts);
>      qemu_add_opts(&qemu_icount_opts);
>      qemu_add_opts(&qemu_semihosting_config_opts);
> +    qemu_add_opts(&qemu_fw_cfg_opts);
>  
>      runstate_init();
>  
> @@ -3420,6 +3440,13 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  do_smbios_option(opts);
>                  break;
> +            case QEMU_OPTION_fwcfg:
> +                opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 0);

The last argument seems wrong: if you set permit_abbrev to zero, then
"implied_opt_name" will have no effect (because the user will be forced
to spell out "name=..."). So, for consistency, you should either drop
implied_opt_name, and keep this last argument 0, or keep
implied_opt_name, and pass 1 as the last argument. (And in the latter
case, "-fw_cfg etc/foo,file=bar" should work.)

> +                if (opts == NULL) {
> +                    exit(1);
> +                }
> +                fw_cfg_option_add(opts);
> +                break;
>              case QEMU_OPTION_enable_kvm:
>                  olist = qemu_find_opts("machine");
>                  qemu_opts_parse(olist, "accel=kvm", 0);
> 

Structurally this looks sane to me.

Perhaps the suggestion to move the file loading from fw_cfg_init1() --
ie. device initialization -- to the earlier option parsing phase will
appease Gerd too :) But, admittedly, I don't know what the "existing
fw_cfg init order issues" that he referenced are.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gerd Hoffmann]

  Hi,

> Perhaps the suggestion to move the file loading from fw_cfg_init1() --
> ie. device initialization -- to the earlier option parsing phase will
> appease Gerd too :) But, admittedly, I don't know what the "existing
> fw_cfg init order issues" that he referenced are.

Basically fw_cfg init picks up information from a bunch of places,
instead of the having the places where the information is generated
store the information in fw_cfg.  Which would be nice as it would make
the fw_cfg dependency more visible in the code.

Due to parsing and using command line switches being separated (via
QemuOpts) this doesn't create that many init order issues though.

Which reminds me:  Going for QemuOpts would be very useful (gives
-readconfig support), and it would solve the init order issue too.
Instead of having your custom storage you just let QemuOpts parse and
store the command line switch, then process them after fw_cfg device
initialization.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gabriel L. Somlo]

On Tue, Mar 17, 2015 at 12:49:50PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > Perhaps the suggestion to move the file loading from fw_cfg_init1() --
> > ie. device initialization -- to the earlier option parsing phase will
> > appease Gerd too :) But, admittedly, I don't know what the "existing
> > fw_cfg init order issues" that he referenced are.
> 
> Basically fw_cfg init picks up information from a bunch of places,
> instead of the having the places where the information is generated
> store the information in fw_cfg.  Which would be nice as it would make
> the fw_cfg dependency more visible in the code.
> 
> Due to parsing and using command line switches being separated (via
> QemuOpts) this doesn't create that many init order issues though.
> 
> Which reminds me:  Going for QemuOpts would be very useful (gives
> -readconfig support), and it would solve the init order issue too.
> Instead of having your custom storage you just let QemuOpts parse and
> store the command line switch, then process them after fw_cfg device
> initialization.

+static struct FWCfgOption {
+    const char *name;
+    const char *file;
+} *fw_cfg_options;
+static size_t fw_cfg_num_options;
+
+void fw_cfg_option_add(QemuOpts *opts)
+{
+    const char *name = qemu_opt_get(opts, "name");
+    const char *file = qemu_opt_get(opts, "file");
+
+    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
+        error_report("invalid argument value");
+        exit(1);
+    }
+
+    fw_cfg_options = g_realloc(fw_cfg_options, sizeof(struct FWCfgOption) *
+                                               (fw_cfg_num_options + 1));
+    fw_cfg_options[fw_cfg_num_options].name = name;
+    fw_cfg_options[fw_cfg_num_options].file = file;
+    fw_cfg_num_options++;
+}

So basically, you're saying "don't process 'opts' here with qemu_opt_get(),
just stash them as is, then process them later" ?  (e.g. during
fw_cfg_options_insert() below) ?

Is there an existing example where that's currently done that I use
for inspiration?

Laszlo's suggestion was almost the opposite of this, i.e. allocate
memory for all the blobs during option processing, them just call
fw_cfg_add_file() during fw_cfg_options_insert().


+static void fw_cfg_options_insert(FWCfgState *s)
+{
+    size_t i;
+    int filesize;
+    const char *filename;
+    void *filebuf;
+
+    for (i = 0; i < fw_cfg_num_options; i++) {
+        filename = fw_cfg_options[i].file;
+        filesize = get_image_size(filename);
+        if (filesize == -1) {
+            error_report("Cannot read fw_cfg file %s", filename);
+            exit(1);
+        }
+        filebuf = g_malloc(filesize);
+        if (load_image(filename, filebuf) != filesize) {
+            error_report("Failed to load fw_cfg file %s", filename);
+            exit(1);
+        }
+        fw_cfg_add_file(s, fw_cfg_options[i].name, filebuf, filesize);
+    }
+}
+
 
 static void fw_cfg_init1(DeviceState *dev)
 {
@@ -584,6 +631,8 @@ static void fw_cfg_init1(DeviceState *dev)
 
     qdev_init_nofail(dev);
 
+    fw_cfg_options_insert(s);
+
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));


Personally, I don't think there are *any* init order issues at all.
It's an error to insert a dupe file name either way, whether the
user's command line blob goes in first and we error out at the
programmatically inserted dupe, or the other way around.


BTW, regarding that suggestion (to allocate blobs during option_add
and just call add_file() from options_insert(): I did think of that,
but decided against it because users could pathologically provide the
same fw_cfg file name multiple times on the command line, and I didn't
like the idea of either allocating RAM for each one blindly, nor did I
like the idea of adding checks for dupes within user-supplied blobs,
so delaying everything until the moment fw_cfg_add_file would enforce
uniqueness on my behalf sounded like the least amount of trouble.

Of course, that's an unlikely scenario, and so what if we allocate
lots of RAM only to exit with an error shortly after :)

Anyway, I'm going to try and figure out the bits about stashing
QemuOpts from fw_cfg_option_add(), then probably fall back to
preallocating blobs during the same function.

If I (very likely) misunderstood something, please let me know :)

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Laszlo Ersek]

On 03/18/15 21:06, Gabriel L. Somlo wrote:
> On Tue, Mar 17, 2015 at 12:49:50PM +0100, Gerd Hoffmann wrote:

[snip]

>> Which reminds me:  Going for QemuOpts would be very useful (gives
>> -readconfig support), and it would solve the init order issue too.
>> Instead of having your custom storage you just let QemuOpts parse and
>> store the command line switch, then process them after fw_cfg device
>> initialization.

[snip]

> So basically, you're saying "don't process 'opts' here with qemu_opt_get(),
> just stash them as is, then process them later" ?  (e.g. during
> fw_cfg_options_insert() below) ?
> 
> Is there an existing example where that's currently done that I use
> for inspiration?
> 
> Laszlo's suggestion was almost the opposite of this, i.e. allocate
> memory for all the blobs during option processing, them just call
> fw_cfg_add_file() during fw_cfg_options_insert().

I was wrong, simply. :) I forgot about -readconfig completely.

(I do recall that I had been annoyed earlier by some option not working
as expected with -readconfig; perhaps -smp? Not sure.)

Meta:

You should always take my qemu reviews with a grain of salt. They're
mostly good for pulling in reviewers who know what they're doing. ;)

I reviewed your v1 quickly for two reasons:
- I didn't want you to regress fw_cfg accidentally, especially code I
  wrote, or code UEFI depends on.
- I'm a nice person (lol) and wanted to get the review gears rolling
  for you (see above).

Cheers,
Laszlo

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gabriel L. Somlo]

On Tue, Mar 17, 2015 at 12:28:20PM +0100, Laszlo Ersek wrote:
> > +
> > +void fw_cfg_option_add(QemuOpts *opts)
> > +{
> > +    const char *name = qemu_opt_get(opts, "name");
> > +    const char *file = qemu_opt_get(opts, "file");
> > +
> > +    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
> > +        error_report("invalid argument value");
> > +        exit(1);
> > +    }
> 
> Okay, I don't recall the details of what I'm going to recommend. :)
> 
> Please use the location API to tie the error message to the offending
> QemuOpts. I've done that only once before, but it didn't turn out to be
> a catastrophe, so now I'm recommending it to you as well. See commit
> 637a5acb; specifically the code around the "Location" object.

I don't think that's applicable here. fw_cfg_option_add() is called
from v.c:

@@ -3420,6 +3440,13 @@ int main(int argc, char **argv, char **envp)
                 }
                 do_smbios_option(opts);
                 break;
+            case QEMU_OPTION_fwcfg:
+                opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 0);
+                if (opts == NULL) {
+                    exit(1);
+                }
+                fw_cfg_option_add(opts);
+                break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse(olist, "accel=kvm", 0);

...and, as such, I'm exactly in the appropriate location for
error_report() to work as expected. In fact, in an earlier reply to
Matt, I quoted an example of what the error message looks like:

qemu-system-x86_64: -fw_cfg file=,name=: invalid argument value

...which shows it works the way we'd want it to.

> (CC'ing Markus.)
> 
> > +static void fw_cfg_options_insert(FWCfgState *s)
> > +{
> > +    int i, filesize;
> > +    const char *filename;
> > +    void *filebuf;
> > +
> > +    for (i = 0; i < fw_cfg_num_options; i++) {
> > +        filename = fw_cfg_options[i].file;
> > +        filesize = get_image_size(filename);
> > +        if (filesize == -1) {
> > +            error_report("Cannot read fw_cfg file %s", filename);
> > +            exit(1);
> > +        }
> > +        filebuf = g_malloc(filesize);
> > +        if (load_image(filename, filebuf) != filesize) {
> > +            error_report("Failed to load fw_cfg file %s", filename);

Now, *this* is where I could use the stashed copy of 'QemuOpts *opts'
from fw_cfg_add_option() to tie the error report back to the "bad"
command line component :) That would work if we decided it's OK to
delay everything, including parsing '*opts' with qemu_opt_get(), until
this point.

> > +            exit(1);
> > +        }
> > +        fw_cfg_add_file(s, fw_cfg_options[i].name, filebuf, filesize);
> > +    }
> > +}

Guess I'm going to wait on some feedback on whether to stash or not to
stash QemuOpts first...

Thanks much,
--Gabriel

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Gerd Hoffmann]

  Hi,

> +            case QEMU_OPTION_fwcfg:
> +                opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 0);
> +                if (opts == NULL) {
> +                    exit(1);
> +                }

That is fine here.

> +                fw_cfg_option_add(opts);

That should be called later.  There is qemu_opts_foreach() to loop over
all QemuOpts added, you'll find other calls of this in main().  And when
called late enough (after fw_cfg init) you don't need the separate
insert function any more.

Testcase:

  (1) qemu -fw_cfg $options -writeconfig fw_cfg_test.cfg
  (2) qemu -readconfig fw_cfg_test.cfg

Both (1) and (2) should give the same result.

cheers,
  Gerd

[Qemu-devel] How to emit errors with nice location information (was: [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline)

[Markus Armbruster]

"Gabriel L. Somlo" <gsomlo@gmail.com> writes:

> On Tue, Mar 17, 2015 at 12:28:20PM +0100, Laszlo Ersek wrote:
>> > +
>> > +void fw_cfg_option_add(QemuOpts *opts)
>> > +{
>> > +    const char *name = qemu_opt_get(opts, "name");
>> > +    const char *file = qemu_opt_get(opts, "file");
>> > +
>> > +    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
>> > +        error_report("invalid argument value");
>> > +        exit(1);
>> > +    }
>> 
>> Okay, I don't recall the details of what I'm going to recommend. :)
>> 
>> Please use the location API to tie the error message to the offending
>> QemuOpts. I've done that only once before, but it didn't turn out to be
>> a catastrophe, so now I'm recommending it to you as well. See commit
>> 637a5acb; specifically the code around the "Location" object.
>
> I don't think that's applicable here. fw_cfg_option_add() is called
> from v.c:
>
> @@ -3420,6 +3440,13 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  do_smbios_option(opts);
>                  break;
> +            case QEMU_OPTION_fwcfg:
> +                opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 0);
> +                if (opts == NULL) {
> +                    exit(1);
> +                }
> +                fw_cfg_option_add(opts);
> +                break;
>              case QEMU_OPTION_enable_kvm:
>                  olist = qemu_find_opts("machine");
>                  qemu_opts_parse(olist, "accel=kvm", 0);
>
> ...and, as such, I'm exactly in the appropriate location for
> error_report() to work as expected. In fact, in an earlier reply to
> Matt, I quoted an example of what the error message looks like:
>
> qemu-system-x86_64: -fw_cfg file=,name=: invalid argument value
>
> ...which shows it works the way we'd want it to.

Yup.

>> (CC'ing Markus.)
>> 
>> > +static void fw_cfg_options_insert(FWCfgState *s)
>> > +{
>> > +    int i, filesize;
>> > +    const char *filename;
>> > +    void *filebuf;
>> > +
>> > +    for (i = 0; i < fw_cfg_num_options; i++) {
>> > +        filename = fw_cfg_options[i].file;
>> > +        filesize = get_image_size(filename);
>> > +        if (filesize == -1) {
>> > +            error_report("Cannot read fw_cfg file %s", filename);
>> > +            exit(1);
>> > +        }
>> > +        filebuf = g_malloc(filesize);
>> > +        if (load_image(filename, filebuf) != filesize) {
>> > +            error_report("Failed to load fw_cfg file %s", filename);
>
> Now, *this* is where I could use the stashed copy of 'QemuOpts *opts'
> from fw_cfg_add_option() to tie the error report back to the "bad"
> command line component :) That would work if we decided it's OK to
> delay everything, including parsing '*opts' with qemu_opt_get(), until
> this point.

error_report() prints the "current location".  This is actually the top
of a location stack.  The stack initially contains just one LOC_NONE
entry, which makes error_report() print no location.  main()'s option
processing puts the current option's location on the stack.  So do
qemu_config_parse() and qemu_opt_foreach().

If you need to report an error later on, you have to do it yourself
(unless you're using qemu_opt_foreach()).  Sadly, we too often can't be
bothered.

The general pattern is

    push a location in one of the several ways
    do stuff, maybe call error_report()
    pop the location with loc_pop()

To find concrete examples, search for loc_pop().

I can see two involving QemuOpts: scsi_bus_legacy_handle_cmdline() and
pc_system_flash_init().  The former is easier to understand, because it
does much less.

A helper error_report_for_opts() could save you the monkeying around
with the location stack.  Maybe when we have more than two potential
users.

For a non-QemuOpts example, see foreach_device_config().

[...]

[Qemu-devel] [PATCH 6/6] qga: RFC: guest-side retrieval of fw_cfg file

[Gabriel L. Somlo]

Add -g (--get-fwcfg) client-mode option to qemu-ga, causing
the named fw_cfg file to be retrieved and written to stdout.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

First off, I have NOT forgotten the suggestion to make this a
standalone binary, and will do so when I submit it "for real".
It's just more comfortable this way for quick-n-dirty testing :)

Two main issues I need help with before this would be ready to
go upstream:

  1. I can't for the life of me figure out how to stop gcc -O2
     from assuming the if() test below is ALWAYS FALSE, and thus
     optimizing it out completely. For now I've forced -O0 on
     the entire function, but for some reason fw_cfg_read(&fcfile, ...)
     does not appear to count as potentially modifying fcfile...

  2. I'm toying with the idea of writing a kernel driver for fw_cfg
     and thus having all this functionality reduced to
     "cat /sys/firmware/fw_cfg/<filename> | grep <something>" :)

     Of course, I have no idea how that would work on Windows, so maybe
     a binary spitting out a file is still the more portable way to go.
     (not to mention that most of the code for that is already written
     below).

Thanks much for any additional clue...
  Gabriel

 qga/Makefile.objs      |  1 +
 qga/get-fwcfg.c        | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h |  2 ++
 qga/main.c             |  6 +++-
 4 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 qga/get-fwcfg.c

diff --git a/qga/Makefile.objs b/qga/Makefile.objs
index 1c5986c..ef53841 100644
--- a/qga/Makefile.objs
+++ b/qga/Makefile.objs
@@ -4,5 +4,6 @@ qga-obj-$(CONFIG_WIN32) += commands-win32.o channel-win32.o service-win32.o
 qga-obj-$(CONFIG_WIN32) += vss-win32.o
 qga-obj-y += qapi-generated/qga-qapi-types.o qapi-generated/qga-qapi-visit.o
 qga-obj-y += qapi-generated/qga-qmp-marshal.o
+qga-obj-y += get-fwcfg.o
 
 qga-vss-dll-obj-$(CONFIG_QGA_VSS) += vss-win32/
diff --git a/qga/get-fwcfg.c b/qga/get-fwcfg.c
new file mode 100644
index 0000000..1928698
--- /dev/null
+++ b/qga/get-fwcfg.c
@@ -0,0 +1,92 @@
+/*
+ * QEMU Guest Agent: retrieve blob from fw_cfg device by name
+ *
+ * Copyright Carnegie Mellon University 2015
+ *
+ * Author:
+ *  Gabriel L. Somlo  <somlo@cmu.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+#include <sys/io.h>
+#include "qemu/bswap.h"
+#include "hw/nvram/fw_cfg.h"
+#include "qga/guest-agent-core.h"
+
+#define PORT_FW_CFG_CTL       0x0510
+#define PORT_FW_CFG_DATA      0x0511
+
+static void
+fw_cfg_select(uint16_t f)
+{
+    outw(f, PORT_FW_CFG_CTL);
+}
+
+static void
+fw_cfg_read(void *buf, int len)
+{
+    insb(PORT_FW_CFG_DATA, buf, len);
+}
+
+static void
+fw_cfg_read_entry(void *buf, int e, int len)
+{
+    fw_cfg_select(e);
+    fw_cfg_read(buf, len);
+}
+
+int
+__attribute__((optimize("O0"))) //FIXME: "gcc -O2" wrongfully optimizes "if"!!!
+ga_get_fwcfg(const char *filename)
+{
+    int i;
+    uint32_t count, len = 0;
+    uint16_t sel;
+    uint8_t sig[] = "QEMU";
+    FWCfgFile fcfile;
+    void *buf;
+
+    /* ensure access to the fw_cfg device */
+    if (ioperm(PORT_FW_CFG_CTL, 2, 1) != 0) {
+        perror("ioperm failed");
+        return EXIT_FAILURE;
+    }
+
+    /* verify presence of fw_cfg device */
+    fw_cfg_select(FW_CFG_SIGNATURE);
+    for (i = 0; i < sizeof(sig) - 1; i++) {
+        sig[i] = inb(PORT_FW_CFG_DATA);
+    }
+    if (memcmp(sig, "QEMU", sizeof(sig)) != 0) {
+        fprintf(stderr, "fw_cfg signature not found!\n");
+        return EXIT_FAILURE;
+    }
+
+    /* read number of fw_cfg entries, then scan for requested entry by name */
+    fw_cfg_read_entry(&count, FW_CFG_FILE_DIR, sizeof(count));
+    count = be32_to_cpu(count);
+    for (i = 0; i < count; i++) {
+        fw_cfg_read(&fcfile, sizeof(fcfile));
+        //FIXME: why does gcc -O2 optimize away the whole if {} block below?!?
+        if (!strcmp(fcfile.name, filename)) {
+            len = be32_to_cpu(fcfile.size);
+            sel = be16_to_cpu(fcfile.select);
+            buf = g_malloc(len);
+            fw_cfg_read_entry(buf, sel, len);
+            if (write(STDOUT_FILENO, buf, len) != len) {
+                fprintf(stderr, "Failed to write %s to stdout\n", filename);
+                return EXIT_FAILURE;
+            }
+            return 0;;
+        }
+    }
+
+    /* requested entry not present in fw_cfg */
+    fprintf(stderr, "File %s not found in fw_cfg!\n", filename);
+    return EXIT_FAILURE;
+}
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index e92c6ab..b859e08 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -41,3 +41,5 @@ int64_t ga_get_fd_handle(GAState *s, Error **errp);
 #ifndef _WIN32
 void reopen_fd_to_null(int fd);
 #endif
+
+int ga_get_fwcfg(const char *file);
diff --git a/qga/main.c b/qga/main.c
index 9939a2b..f9c1ece 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -215,6 +215,7 @@ static void usage(const char *cmd)
 #endif
 "  -b, --blacklist   comma-separated list of RPCs to disable (no spaces, \"?\"\n"
 "                    to list available RPCs)\n"
+"  -g, --get-fwcfg   dump the content of a given fw_cfg file to stdout\n"
 "  -h, --help        display this help and exit\n"
 "\n"
 "Report bugs to <mdroth@linux.vnet.ibm.com>\n"
@@ -923,7 +924,7 @@ static void ga_print_cmd(QmpCommand *cmd, void *opaque)
 
 int main(int argc, char **argv)
 {
-    const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
+    const char *sopt = "hVvdm:p:l:f:F::b:s:t:g:";
     const char *method = NULL, *path = NULL;
     const char *log_filepath = NULL;
     const char *pid_filepath;
@@ -951,6 +952,7 @@ int main(int argc, char **argv)
         { "service", 1, NULL, 's' },
 #endif
         { "statedir", 1, NULL, 't' },
+        { "get-fwcfg", 1, NULL, 'g' },
         { NULL, 0, NULL, 0 }
     };
     int opt_ind = 0, ch, daemonize = 0, i, j, len;
@@ -1042,6 +1044,8 @@ int main(int argc, char **argv)
             }
             break;
 #endif
+        case 'g':
+            return ga_get_fwcfg(optarg);
         case 'h':
             usage(argv[0]);
             return 0;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 6/6] qga: RFC: guest-side retrieval of fw_cfg file

[Laszlo Ersek]

comments below

On 03/16/15 15:15, Gabriel L. Somlo wrote:
> Add -g (--get-fwcfg) client-mode option to qemu-ga, causing
> the named fw_cfg file to be retrieved and written to stdout.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> First off, I have NOT forgotten the suggestion to make this a
> standalone binary, and will do so when I submit it "for real".
> It's just more comfortable this way for quick-n-dirty testing :)
> 
> Two main issues I need help with before this would be ready to
> go upstream:
> 
>   1. I can't for the life of me figure out how to stop gcc -O2
>      from assuming the if() test below is ALWAYS FALSE, and thus
>      optimizing it out completely. For now I've forced -O0 on
>      the entire function, but for some reason fw_cfg_read(&fcfile, ...)
>      does not appear to count as potentially modifying fcfile...
> 
>   2. I'm toying with the idea of writing a kernel driver for fw_cfg
>      and thus having all this functionality reduced to
>      "cat /sys/firmware/fw_cfg/<filename> | grep <something>" :)
> 
>      Of course, I have no idea how that would work on Windows, so maybe
>      a binary spitting out a file is still the more portable way to go.
>      (not to mention that most of the code for that is already written
>      below).

Ignore windows, go for the Linux driver. Matt wants the kernel driver:
http://thread.gmane.org/gmane.comp.emulators.qemu/321875/focus=322263

Of course, for testing the feature, anything will do in the short term.
For the longer term, consider writing a test case. "tests/fw_cfg-test.c"
already exists, maybe you could extend that.

In any case, the "-g" option introduced here (for a one-shot invocation,
if I understand correctly) doesn't seem to match qga very well. Normally
you'd add a JSON/QMP API that would allow callers to interrogate the qga
daemon. But, again, the kernel driver is likely superior anyway.

> 
> Thanks much for any additional clue...
>   Gabriel
> 
>  qga/Makefile.objs      |  1 +
>  qga/get-fwcfg.c        | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/guest-agent-core.h |  2 ++
>  qga/main.c             |  6 +++-
>  4 files changed, 100 insertions(+), 1 deletion(-)
>  create mode 100644 qga/get-fwcfg.c
> 
> diff --git a/qga/Makefile.objs b/qga/Makefile.objs
> index 1c5986c..ef53841 100644
> --- a/qga/Makefile.objs
> +++ b/qga/Makefile.objs
> @@ -4,5 +4,6 @@ qga-obj-$(CONFIG_WIN32) += commands-win32.o channel-win32.o service-win32.o
>  qga-obj-$(CONFIG_WIN32) += vss-win32.o
>  qga-obj-y += qapi-generated/qga-qapi-types.o qapi-generated/qga-qapi-visit.o
>  qga-obj-y += qapi-generated/qga-qmp-marshal.o
> +qga-obj-y += get-fwcfg.o
>  
>  qga-vss-dll-obj-$(CONFIG_QGA_VSS) += vss-win32/
> diff --git a/qga/get-fwcfg.c b/qga/get-fwcfg.c
> new file mode 100644
> index 0000000..1928698
> --- /dev/null
> +++ b/qga/get-fwcfg.c
> @@ -0,0 +1,92 @@
> +/*
> + * QEMU Guest Agent: retrieve blob from fw_cfg device by name
> + *
> + * Copyright Carnegie Mellon University 2015

Hm? I thought... Well, nevermind, that's for another discussion. :)

> + *
> + * Author:
> + *  Gabriel L. Somlo  <somlo@cmu.edu>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <sys/io.h>
> +#include "qemu/bswap.h"
> +#include "hw/nvram/fw_cfg.h"
> +#include "qga/guest-agent-core.h"
> +
> +#define PORT_FW_CFG_CTL       0x0510
> +#define PORT_FW_CFG_DATA      0x0511
> +
> +static void
> +fw_cfg_select(uint16_t f)
> +{
> +    outw(f, PORT_FW_CFG_CTL);
> +}

This won't fly for an ARM guest, of course. But, the kernel driver
should take over this burden anyway.

> +
> +static void
> +fw_cfg_read(void *buf, int len)
> +{
> +    insb(PORT_FW_CFG_DATA, buf, len);
> +}
> +
> +static void
> +fw_cfg_read_entry(void *buf, int e, int len)
> +{
> +    fw_cfg_select(e);
> +    fw_cfg_read(buf, len);
> +}
> +
> +int
> +__attribute__((optimize("O0"))) //FIXME: "gcc -O2" wrongfully optimizes "if"!!!
> +ga_get_fwcfg(const char *filename)
> +{
> +    int i;
> +    uint32_t count, len = 0;
> +    uint16_t sel;
> +    uint8_t sig[] = "QEMU";
> +    FWCfgFile fcfile;
> +    void *buf;
> +
> +    /* ensure access to the fw_cfg device */
> +    if (ioperm(PORT_FW_CFG_CTL, 2, 1) != 0) {
> +        perror("ioperm failed");
> +        return EXIT_FAILURE;
> +    }
> +
> +    /* verify presence of fw_cfg device */
> +    fw_cfg_select(FW_CFG_SIGNATURE);
> +    for (i = 0; i < sizeof(sig) - 1; i++) {
> +        sig[i] = inb(PORT_FW_CFG_DATA);
> +    }
> +    if (memcmp(sig, "QEMU", sizeof(sig)) != 0) {
> +        fprintf(stderr, "fw_cfg signature not found!\n");
> +        return EXIT_FAILURE;
> +    }
> +
> +    /* read number of fw_cfg entries, then scan for requested entry by name */
> +    fw_cfg_read_entry(&count, FW_CFG_FILE_DIR, sizeof(count));
> +    count = be32_to_cpu(count);
> +    for (i = 0; i < count; i++) {
> +        fw_cfg_read(&fcfile, sizeof(fcfile));
> +        //FIXME: why does gcc -O2 optimize away the whole if {} block below?!?
> +        if (!strcmp(fcfile.name, filename)) {

So, according to your description at the top, and here, this test is
"always false", according to gcc, which means that strcmp() always
returns nonzero (according to gcc), meaning the two strings always differ.

My idea is that gcc optimizes this block away because it thinks it
detects undefined behavior somewhere. For example, if you omitted the
fw_cfg_read() call, that would indeed be the case, because the contents
of "fcfile" would be indeterminate then.

Hmmm. Look at this:

http://man7.org/linux/man-pages/man2/outb.2.html

       You must compile with -O or -O2 or similar.  The functions are
       defined as inline macros, and will not be substituted in without
       optimization enabled, causing unresolved references at link time.

I think fw_cfg_read() is inlined under -O2, and the insb() from that
function is somehow confusing gcc.

>From "/usr/include/sys/io.h", on my RHEL-7.1 laptop:

static __inline void
insb (unsigned short int __port, void *__addr, unsigned long int __count)
{
  __asm__ __volatile__ ("cld ; rep ; insb":"=D" (__addr), "=c" (__count)
                        :"d" (__port), "0" (__addr), "1" (__count));
}

Honestly, I don't understand the output operand list. INSB takes the
port in DX (which is '"d" (__port)' I think, in the input operand list).
For the REP prefix, CX should be set to the number of bytes, and that's
'"1" (__count)' in the input list (because #1 refers to the earlier
"=c"). The address is passed in ES:DI, ie. '"0" (__addr)', where #0
refers back to the earlier "=D".

But that doesn't explain why __addr and __count are in the *output*
operand list at all! If anything, CX and DI should be on the *clobber*
list instead (which is completely absent here). It makes no sense to
write the changed values of CX and DI back to the __addr and __count
function parameters (which are supposed to be local variables, sure, but
this function is also inlined, and that could cause some confusion).

... Now this is interesting! The Linux kernel used to use the same
definition for insb *until 2002*. Then it was rewritten. You won't even
find that commit in Linus's git repo -- you can find it in Thomas
Gleixner's "history" repo, which is -- as far as I remember -- a git
repo constructed from some other version control system's history, and
it carries patches that predate the kernel's move to git. Check it out:

http://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/?id=6b1ad46e

Notice how the prepatch code had matched what we have in glibc to this
day, and it was replaced with the following operand lists:

output:  "+D"(addr), "+c"(count)
input:   "d"(port)

According to the gcc documentation
<https://gcc.gnu.org/onlinedocs/gcc/Modifiers.html>, there's a *big*
difference between the constraint modifiers "=" and "+". Namely:

    ‘=’ identifies an operand which is only written;
    ‘+’ identifies an operand that is both read and written

Which means that the glibc version is *broken*. If ES:DI were never
read, then the processor wouldn't know where in memory to place the
datum from the port, and *that* seems consistent with gcc thinking that
"fcfile" is never modified (and remains indeterminate therefore)!

In short, I think you caught either a gcc or (much more likely) a glibc
bug. The glibc version dates back to glibc commit 40ff7949, 2002.

What do you think, Paolo?

Thanks
Laszlo


> +            len = be32_to_cpu(fcfile.size);
> +            sel = be16_to_cpu(fcfile.select);
> +            buf = g_malloc(len);
> +            fw_cfg_read_entry(buf, sel, len);
> +            if (write(STDOUT_FILENO, buf, len) != len) {
> +                fprintf(stderr, "Failed to write %s to stdout\n", filename);
> +                return EXIT_FAILURE;
> +            }
> +            return 0;;
> +        }
> +    }
> +
> +    /* requested entry not present in fw_cfg */
> +    fprintf(stderr, "File %s not found in fw_cfg!\n", filename);
> +    return EXIT_FAILURE;
> +}
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index e92c6ab..b859e08 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -41,3 +41,5 @@ int64_t ga_get_fd_handle(GAState *s, Error **errp);
>  #ifndef _WIN32
>  void reopen_fd_to_null(int fd);
>  #endif
> +
> +int ga_get_fwcfg(const char *file);
> diff --git a/qga/main.c b/qga/main.c
> index 9939a2b..f9c1ece 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -215,6 +215,7 @@ static void usage(const char *cmd)
>  #endif
>  "  -b, --blacklist   comma-separated list of RPCs to disable (no spaces, \"?\"\n"
>  "                    to list available RPCs)\n"
> +"  -g, --get-fwcfg   dump the content of a given fw_cfg file to stdout\n"
>  "  -h, --help        display this help and exit\n"
>  "\n"
>  "Report bugs to <mdroth@linux.vnet.ibm.com>\n"
> @@ -923,7 +924,7 @@ static void ga_print_cmd(QmpCommand *cmd, void *opaque)
>  
>  int main(int argc, char **argv)
>  {
> -    const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> +    const char *sopt = "hVvdm:p:l:f:F::b:s:t:g:";
>      const char *method = NULL, *path = NULL;
>      const char *log_filepath = NULL;
>      const char *pid_filepath;
> @@ -951,6 +952,7 @@ int main(int argc, char **argv)
>          { "service", 1, NULL, 's' },
>  #endif
>          { "statedir", 1, NULL, 't' },
> +        { "get-fwcfg", 1, NULL, 'g' },
>          { NULL, 0, NULL, 0 }
>      };
>      int opt_ind = 0, ch, daemonize = 0, i, j, len;
> @@ -1042,6 +1044,8 @@ int main(int argc, char **argv)
>              }
>              break;
>  #endif
> +        case 'g':
> +            return ga_get_fwcfg(optarg);
>          case 'h':
>              usage(argv[0]);
>              return 0;
> 

Re: [Qemu-devel] [PATCH 6/6] qga: RFC: guest-side retrieval of fw_cfg file

[Gabriel L. Somlo]

On Tue, Mar 17, 2015 at 01:38:06PM +0100, Laszlo Ersek wrote:
> Ignore windows, go for the Linux driver. Matt wants the kernel driver:
> http://thread.gmane.org/gmane.comp.emulators.qemu/321875/focus=322263

Me too, after all, I suggested it. And I always wanted an excuse to
write a linux device driver... :)

But my problem is that lots of the guest VMs I'm doing this for in
the first place are Windows, so as much as I'd like to ignore them,
they just won't go away...

Which is why I've decided to prioritize the host-side bits, and give
myself time to think about how I'm going to optimize happiness for
all parties involved on the guest side of things :)

> In short, I think you caught either a gcc or (much more likely) a glibc
> bug. The glibc version dates back to glibc commit 40ff7949, 2002.
> 
> What do you think, Paolo?

I wonder if it's a known bug or a shiny new one :) I could try to
maybe write a simplified test to demonstrate it, and then file it
somewhere...

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH 6/6] qga: RFC: guest-side retrieval of fw_cfg file

[Kevin O'Connor]

On Tue, Mar 17, 2015 at 01:38:06PM +0100, Laszlo Ersek wrote:
> On 03/16/15 15:15, Gabriel L. Somlo wrote:
> >   1. I can't for the life of me figure out how to stop gcc -O2
> >      from assuming the if() test below is ALWAYS FALSE, and thus
> >      optimizing it out completely. For now I've forced -O0 on
> >      the entire function, but for some reason fw_cfg_read(&fcfile, ...)
> >      does not appear to count as potentially modifying fcfile...
[...]
> > +static void
> > +fw_cfg_read(void *buf, int len)
> > +{
> > +    insb(PORT_FW_CFG_DATA, buf, len);
> > +}
[...]
> I think fw_cfg_read() is inlined under -O2, and the insb() from that
> function is somehow confusing gcc.
> 
> From "/usr/include/sys/io.h", on my RHEL-7.1 laptop:
> 
> static __inline void
> insb (unsigned short int __port, void *__addr, unsigned long int __count)
> {
>   __asm__ __volatile__ ("cld ; rep ; insb":"=D" (__addr), "=c" (__count)
>                         :"d" (__port), "0" (__addr), "1" (__count));
> }

My read of this is that gcc knows it must emit the instruction, and it
knows that __addr and __count can change.  But, it doesn't know that
the memory at *__addr can change.  I'd see if a barrier() fixes it.

See the section on "clobber" at:

https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

In particular:

  You can use a trick to avoid this if the size of the memory being
  accessed is known at compile time. For example, if accessing ten bytes
  of a string, use a memory input like:

  {"m"( ({ struct { char x[10]; } *p = (void *)ptr ; *p; }) )}.

-Kevin

Re: [Qemu-devel] [PATCH 6/6] qga: RFC: guest-side retrieval of fw_cfg file

[Laszlo Ersek]

On 03/19/15 19:27, Kevin O'Connor wrote:
> On Tue, Mar 17, 2015 at 01:38:06PM +0100, Laszlo Ersek wrote:
>> On 03/16/15 15:15, Gabriel L. Somlo wrote:
>>>   1. I can't for the life of me figure out how to stop gcc -O2
>>>      from assuming the if() test below is ALWAYS FALSE, and thus
>>>      optimizing it out completely. For now I've forced -O0 on
>>>      the entire function, but for some reason fw_cfg_read(&fcfile, ...)
>>>      does not appear to count as potentially modifying fcfile...
> [...]
>>> +static void
>>> +fw_cfg_read(void *buf, int len)
>>> +{
>>> +    insb(PORT_FW_CFG_DATA, buf, len);
>>> +}
> [...]
>> I think fw_cfg_read() is inlined under -O2, and the insb() from that
>> function is somehow confusing gcc.
>>
>> From "/usr/include/sys/io.h", on my RHEL-7.1 laptop:
>>
>> static __inline void
>> insb (unsigned short int __port, void *__addr, unsigned long int __count)
>> {
>>   __asm__ __volatile__ ("cld ; rep ; insb":"=D" (__addr), "=c" (__count)
>>                         :"d" (__port), "0" (__addr), "1" (__count));
>> }
> 
> My read of this is that gcc knows it must emit the instruction, and it
> knows that __addr and __count can change.  But, it doesn't know that
> the memory at *__addr can change.  I'd see if a barrier() fixes it.
> 
> See the section on "clobber" at:
> 
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
> 
> In particular:
> 
>   You can use a trick to avoid this if the size of the memory being
>   accessed is known at compile time. For example, if accessing ten bytes
>   of a string, use a memory input like:
> 
>   {"m"( ({ struct { char x[10]; } *p = (void *)ptr ; *p; }) )}.

I think you nailed it, thanks.

Laszlo

Re: [Qemu-devel] [PATCH 0/6] fw-cfg: documentation, cleanup, and proposed feature

[Patchew Tool]

This series passed Patchew automatic testing, but there are some warnings.

Find the log fragments below, or open the following URL to see the full log:

http://qemu.patchew.org/testing/log/<1426515305-17766-1-git-send-email-somlo@cmu.edu>

----------8<---------

docker run --net=none -v /var/tmp/patchew-tester/tmpBjk7tO:/var/tmp/patchew-test patchew:fedora-20 timeout 3600 /var/tmp/patchew-test/qemu-devel.sh /var/tmp/patchew-test

*** Testing 'coding style check' ***

Checking 0001-fw_cfg--add-documentation-file--docs-specs-fw_cfg-txt-.patch
total: 0 errors, 0 warnings, 167 lines checked

0001-fw_cfg--add-documentation-file--docs-specs-fw_cfg-txt-.patch has no obvious style problems and is ready for submission.

Checking 0002-fw_cfg--remove-support-for-guest-side-data-writes.patch
command failed with exit code 0
$@
ERROR: code indent should never use tabs
#203: FILE: include/hw/nvram/fw_cfg.h:43:
+#define FW_CFG_WRITE_CHANNEL    0x4000^I/* reserved (not implemented) */$

total: 1 errors, 0 warnings, 172 lines checked

0002-fw_cfg--remove-support-for-guest-side-data-writes.patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking 0003-fw_cfg--assertion-to-detect-memory-leak-when-adding-new-data-blob.patch
total: 0 errors, 0 warnings, 7 lines checked

0003-fw_cfg--assertion-to-detect-memory-leak-when-adding-new-data-blob.patch has no obvious style problems and is ready for submission.

Checking 0004-fw_cfg--exit-with-error-when-dupe-fw_cfg-file-name-inserted.patch
total: 0 errors, 0 warnings, 24 lines checked

0004-fw_cfg--exit-with-error-when-dupe-fw_cfg-file-name-inserted.patch has no obvious style problems and is ready for submission.

Checking 0005-fw_cfg--insert-fw_cfg-file-blobs-via-qemu-cmdline.patch
total: 0 errors, 0 warnings, 143 lines checked

0005-fw_cfg--insert-fw_cfg-file-blobs-via-qemu-cmdline.patch has no obvious style problems and is ready for submission.

Checking 0006-qga--RFC--guest-side-retrieval-of-fw_cfg-file.patch
command failed with exit code 0
$@
ERROR: do not use C99 // comments
#99: FILE: qga/get-fwcfg.c:44:
+__attribute__((optimize("O0"))) //FIXME: "gcc -O2" wrongfully optimizes "if"!!!

ERROR: do not use C99 // comments
#130: FILE: qga/get-fwcfg.c:75:
+        //FIXME: why does gcc -O2 optimize away the whole if {} block below?!?

total: 2 errors, 0 warnings, 133 lines checked

0006-qga--RFC--guest-side-retrieval-of-fw_cfg-file.patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.


*** Testing 'configure' ***

Install prefix    /usr/local
BIOS directory    /usr/local/share/qemu
binary directory  /usr/local/bin
library directory /usr/local/lib
module directory  /usr/local/lib/qemu
libexec directory /usr/local/libexec