[Qemu-devel] [PATCH v2 0/5] fw-cfg: documentation, cleanup, and cmdline blobs

[Qemu-devel] [PATCH v2 0/5] fw-cfg: documentation, cleanup, and cmdline blobs

[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.

Changes since v1:

    - reworked documentation (1/5) as per Laszlo's feedback
      (actual changes detailed below the commit blurb in the actual patch)

    - guest-side write in 2/5 is now a no-op (as opposed to completely
      removing the write_op itself); also removed trace event.

    - reworded commit blurbs for 3/5 and 4/5; the latter now also removes
      a no longer needed trace event.

    - streamlined 5/5 (inserting fw_cfg blobs via command line)
      Specific changes and potentially remaining issues described at length
      below the commit blurb in the actual patch

    - dropped 6th patch (guest-side qga hack to retrieve fw_cfg files by
      name), as it is merely an RFC at this time, and we can continue
      talking about it in its original thread on the mailing list.

I sent out a couple of emails about patch #5 earlier this afternoon,
but please ignore that as I'm bringing up the same issues below the
commit blurb, only hopefully in a way that's a bit more eloquent, or
at least more coherent :)

Thanks for all the feedback, and please let me know what you all think.

  Gabriel

>Summary of patches in v1 of 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)


Gabriel L. Somlo (5):
  fw_cfg: add documentation file (docs/specs/fw_cfg.txt)
  fw_cfg: remove support for guest-side data writes
  fw_cfg: prevent selector key conflict
  fw_cfg: prohibit insertion of duplicate fw_cfg file names
  fw_cfg: insert fw_cfg file blobs via qemu cmdline

 docs/specs/fw_cfg.txt     | 192 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/nvram/fw_cfg.c         |  85 +++++++++++---------
 include/hw/nvram/fw_cfg.h |   6 +-
 qemu-options.hx           |  11 +++
 trace-events              |   2 -
 vl.c                      |  27 +++++++
 6 files changed, 282 insertions(+), 41 deletions(-)
 create mode 100644 docs/specs/fw_cfg.txt

-- 
2.1.0

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

[Gabriel L. Somlo]

This document covers guest-side hardware interface, as well as the
host-side programming API of QEMU's firmware configuration (fw_cfg)
device.

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

I tried to incorporate most of Laszlo's feedback; I decided to take
an example from the arm kernel doc and refer to the QEMU source for
the specifics on what each blob does and how it's formatted. As an
exception, I do still talk about the signature, revision, and directory
blobs, as they're still part of the *interface* of fw_cfg, rather
than the actual *payload*, which is what I wante to avoid getting into.

I added a section on the host-side API, and listed what each fw_cfg_add_*
function is for.

Thanks,
  Gabriel

 docs/specs/fw_cfg.txt | 192 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 192 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..7be81d5
--- /dev/null
+++ b/docs/specs/fw_cfg.txt
@@ -0,0 +1,192 @@
+QEMU Firmware Configuration (fw_cfg) Device
+===========================================
+
+= Guest-side Hardware Interface =
+
+This hardware interface allows the guest to retrieve various data items
+(blobs) that can influence how the firmware configures itself, or may
+contain tables to be installed for the guest OS. Examples include device
+boot order, ACPI and SMBIOS tables, virtual machine UUID, SMP and NUMA
+information, kernel/initrd images for direct (Linux) kernel booting, etc.
+
+== Selector (Control) Register ==
+
+* Write only
+* Location: platform dependent (IOport or MMIO)
+* Width: 16-bit
+* Endianness: little-endian (if IOport), or big-endian (if MMIO)
+
+A write to this register sets the index of a firmware configuration
+item which can subsequently be accessed via the data register.
+
+Setting the selector register will cause the data offset to be set
+to zero. The data offset impacts which data is accessed via the data
+register, and is explained below.
+
+Bit14 of the selector register indicates whether the configuration
+setting is being written. A value of 0 means the item is only being
+read, and all write access to the data port will be ignored. A value
+of 1 means the item's data can be overwritten by writes to the data
+register. In other words, configuration write mode is enabled when
+the selector value is between 0x4000-0x7fff or 0xc000-0xffff.
+
+NOTE: As of QEMU v2.4, writes to the fw_cfg data register are no
+      longer supported, and will be ignored (treated as no-ops)!
+
+Bit15 of the selector register indicates whether the configuration
+setting is architecture specific. A value of 0 means the item is a
+generic configuration item. A value of 1 means the item is specific
+to a particular architecture. In other words, generic configuration
+items are accessed with a selector value between 0x0000-0x7fff, and
+architecture specific configuration items are accessed with a selector
+value between 0x8000-0xffff.
+
+== Data Register ==
+
+* Read/Write (writes ignored as of QEMU v2.4)
+* Location: platform dependent (IOport [*] or MMIO)
+* Width: 8-bit (if IOport), 8/16/32/64-bit (if MMIO)
+* Endianness: string-preserving
+
+[*] On platforms where the data register is exposed as an IOport, its
+port number will always be one greater than the port number of the
+selector register. In other words, the two ports overlap, and can not
+be mapped separately.
+
+The data register allows access to an array of bytes for each firmware
+configuration data item. The specific item is selected by writing to
+the selector register, as described above.
+
+Initially following a write to the selector register, the data offset
+will be set to zero. Each successful access to the data register will
+increment the data offset by the appropriate access width.
+
+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.
+
+An N-byte wide read of the data register will return the next available
+N bytes of the selected firmware configuration item, as a substring, in
+increasing address order, similar to memcpy().
+
+== Register Locations ==
+
+=== x86, x86_64 Register Locations ===
+
+Selector Register IOport: 0x510
+Data Register IOport:     0x511
+
+== Firmware Configuration Items ==
+
+=== Signature (Key 0x0000, FW_CFG_SIGNATURE) ===
+
+The presence of the fw_cfg selector and data registers can be verified
+by selecting the "signature" item using key 0x0000 (FW_CFG_SIGNATURE),
+and reading four bytes from the data register. If the fw_cfg device is
+present, the four bytes read will contain the characters "QEMU".
+
+=== Revision (Key 0x0001, FW_CFG_ID) ===
+
+A 32-bit little-endian unsigned int, this item is used as an interface
+revision number, and is currently set to 1 by all QEMU architectures
+which expose a fw_cfg device.
+
+=== File Directory (Key 0x0019, FW_CFG_FILE_DIR) ===
+
+Firmware configuration items stored at selector keys 0x0020 or higher
+(FW_CFG_FILE_FIRST or higher) have an associated entry in a directory
+structure, which makes it easier for guest-side firmware to identify
+and retrieve them. The format of this file directory (from fw_cfg.h in
+the QEMU source tree) is shown here, slightly annotated for clarity:
+
+struct FWCfgFiles {		/* the entire file directory fw_cfg item */
+    uint32_t count;		/* number of entries, in big-endian format */
+    struct FWCfgFile f[];	/* array of file entries, see below */
+};
+
+struct FWCfgFile {		/* an individual file entry, 64 bytes total */
+    uint32_t size;		/* size of referenced fw_cfg item, big-endian */
+    uint16_t select;		/* selector key of fw_cfg item, big-endian */
+    uint16_t reserved;
+    char name[56];		/* fw_cfg item name, NUL-terminated ascii */
+};
+
+=== All Other Data Items ===
+
+Please consult the QEMU source for the most up-to-date and authoritative
+list of selector keys and their respective items' purpose and format.
+
+=== Ranges ===
+
+Theoretically, there may be up to 0x4000 generic firmware configuration
+items, and up to 0x4000 architecturally specific ones.
+
+Selector Reg.    Range Usage
+---------------  -----------
+0x0000 - 0x3fff  Generic (0x0000 - 0x3fff, RO)
+0x4000 - 0x7fff  Generic (0x0000 - 0x3fff, RW, ignored in QEMU v2.4+)
+0x8000 - 0xbfff  Arch. Specific (0x0000 - 0x3fff, RO)
+0xc000 - 0xffff  Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
+
+In practice, the number of allowed firmware configuration items is given
+by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
+
+= Host-side API =
+
+The following functions are available to the QEMU programmer for adding
+data to a fw_cfg device during guest initialization (see fw_cfg.h for
+each function's complete prototype):
+
+== fw_cfg_add_bytes() ==
+
+Given a selector key value, starting pointer, and size, create an item
+as a raw "blob" of the given size, available by selecting the given key.
+
+== fw_cfg_add_string() ==
+
+Instead of a starting pointer and size, this function accepts a
+pointer to a NUL-terminated ascii string, and creates an item which
+exactly fits the length of the string, including its NUL terminator.
+
+== fw_cfg_add_iXX() ==
+
+Insert an XX-bit item, where XX may be 16, 32, or 64. These functions
+will convert a 16-, 32-, or 64-bit integer to little-endian format
+before creating an item of the appropriate size available via the given
+selector key value.
+
+== fw_cfg_add_file() ==
+
+Given a file name, starting pointer, and size, create an item as a raw
+"blob" of the given size. Unlike fw_cfg_add_bytes() above, the next
+available selector key (above 0x0020, FW_CFG_FILE_FIRST) will be used,
+and a new entry will be added to the file directory structure (at key
+0x0019), containing the file name, blob size, and automatically assigned
+selector key value.
+
+== fw_cfg_add_file_callback() ==
+
+Like fw_cfg_add_file(), but additionally sets pointers to a callback
+function (and argument), which will be executed host-side by QEMU each
+time a byte is read by the guest from this particular item.
+
+== fw_cfg_modify_file() ==
+
+Given a file name, starting pointer, and size, completely replace the
+configuration item referenced by the given file name with the new
+given blob. If an existing blob is found, its callback information is
+removed, and a pointer to the old data is returned to allow the caller
+to free it, helping avoid memory leaks. If a configuration item does
+not already exist under the given file name, a new item will be created
+as with fw_cfg_add_file(), and NULL is returned to the caller.
+
+== fw_cfg_add_callback() ==
+
+Like fw_cfg_add_bytes(), but additionally sets pointers to a callback
+function (and argument), which will be executed host-side by QEMU each
+time a guest-side write operation to this particular item completes
+fully overwriting the item's data.
+
+NOTE: This function is deprecated, and will be completely removed
+starting with QEMU v2.4.
-- 
2.1.0

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

[Gabriel L. Somlo]

>From this point forward, any guest-side writes to the fw_cfg
data register will be treated as no-ops. This patch also removes
the unused host-side API function fw_cfg_add_callback(), which
allowed the registration of a callback to be executed each time
the guest completed a full overwrite of a given fw_cfg data item.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/nvram/fw_cfg.c         | 33 +--------------------------------
 include/hw/nvram/fw_cfg.h |  2 --
 trace-events              |  1 -
 3 files changed, 1 insertion(+), 35 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 78a37be..2f609b4 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;
 
@@ -232,19 +231,7 @@ static void fw_cfg_reboot(FWCfgState *s)
 
 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;
-        }
-    }
+    /* nothing, write support removed in QEMU v2.4+ */
 }
 
 static int fw_cfg_select(FWCfgState *s, uint16_t key)
@@ -458,7 +445,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 +488,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..b2e10c2 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -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,
diff --git a/trace-events b/trace-events
index 30eba92..1275b70 100644
--- a/trace-events
+++ b/trace-events
@@ -193,7 +193,6 @@ ecc_diag_mem_writeb(uint64_t addr, uint32_t val) "Write diagnostic %"PRId64" = %
 ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x"
 
 # hw/nvram/fw_cfg.c
-fw_cfg_write(void *s, uint8_t value) "%p %d"
 fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
 fw_cfg_read(void *s, uint8_t ret) "%p = %d"
 fw_cfg_add_file_dupe(void *s, char *name) "%p %s"
-- 
2.1.0

[Qemu-devel] [PATCH v2 3/5] fw_cfg: prevent selector key conflict

[Gabriel L. Somlo]

Enforce a single assignment of data for each distinct selector key.

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 2f609b4..659de4c 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -423,6 +423,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); /* avoid key conflict */
 
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = (uint32_t)len;
-- 
2.1.0

[Qemu-devel] [PATCH v2 4/5] fw_cfg: prohibit insertion of duplicate fw_cfg file names

[Gabriel L. Somlo]

Exit with an error (instead of simply logging a trace event)
whenever the same fw_cfg file name is added multiple times via
one of the fw_cfg_add_file[_callback]() host-side API calls.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/nvram/fw_cfg.c | 11 ++++++-----
 trace-events      |  1 -
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 659de4c..a5fd512 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -505,18 +505,19 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
     index = be32_to_cpu(s->files->count);
     assert(index < FW_CFG_FILE_SLOTS);
 
-    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;
+            error_report("duplicate fw_cfg file name: %s",
+                         s->files->f[index].name);
+            exit(1);
         }
     }
 
+    fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index,
+                                   callback, callback_opaque, data, len);
+
     s->files->f[index].size   = cpu_to_be32(len);
     s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
     trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
diff --git a/trace-events b/trace-events
index 1275b70..a340c5a 100644
--- a/trace-events
+++ b/trace-events
@@ -195,7 +195,6 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x
 # hw/nvram/fw_cfg.c
 fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
 fw_cfg_read(void *s, uint8_t ret) "%p = %d"
-fw_cfg_add_file_dupe(void *s, char *name) "%p %s"
 fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
 
 # hw/block/hd-geometry.c
-- 
2.1.0

[Qemu-devel] [PATCH v2 5/5] 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 programmatically added from
within the QEMU source code.

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

I belive at this point I'm addressing all of Laszlo's feedback.

fw_cfg_option_add() operates from within the "Location" set by
qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 1), and, as such,
does not require a "location switch" to print out the proper context
around the error message.

I'm allocating blobs (and reading host-side file contents) all from
within fw_cfg_option_add(), so those error messages are also printed
with the right context.

Simply traversing the command-line blob list and inserting it into
fw_cfg now happens directly from fw_cfg_init1(), immediately after
the device is initialized and ready to accept fw_cfg_add_* calls.

I'm not sure I'm addressing Gerd's concerns re. init order, but
since each blob key and each file name can only be inserted at most
once, we will exit with an error whether the user-specified blob from
the command line or the programmatically-inserted one from the arch
specific init routine gets inserted first.

And, since that's the case, I took full advantage of not having to
determine with certainty for each architecture where the "last place
any fw_cfg blobs are inserted" is, so I could add the command line
blobs *last*. Since *first* will work just as well, or exit with the
same error message, I don't have to worry about handling each arch
separately.

The one other thing I may not have comprehended was the part about

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

If the code below is still in serious need of cleaning up, please
help me figure out what I'm missing, and maybe point me toward an
example.

Thanks much,
   Gabriel

 hw/nvram/fw_cfg.c         | 40 ++++++++++++++++++++++++++++++++++++++++
 include/hw/nvram/fw_cfg.h |  4 ++++
 qemu-options.hx           | 11 +++++++++++
 vl.c                      | 27 +++++++++++++++++++++++++++
 4 files changed, 82 insertions(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index a5fd512..7036fde 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"
@@ -573,9 +574,42 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
 }
 
 
+static struct FWCfgOption {
+    const char *name;
+    gchar *data;
+    size_t size;
+} *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);
+    }
+    if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
+        error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH - 1);
+        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;
+    if (!g_file_get_contents(file, &fw_cfg_options[fw_cfg_num_options].data,
+                             &fw_cfg_options[fw_cfg_num_options].size, NULL)) {
+        error_report("can't load %s", file);
+        exit(1);
+    }
+    fw_cfg_num_options++;
+}
+
 
 static void fw_cfg_init1(DeviceState *dev)
 {
+    size_t i;
     FWCfgState *s = FW_CFG(dev);
 
     assert(!object_resolve_path(FW_CFG_PATH, NULL));
@@ -584,6 +618,12 @@ static void fw_cfg_init1(DeviceState *dev)
 
     qdev_init_nofail(dev);
 
+    /* insert file(s) from command line */
+    for (i = 0; i < fw_cfg_num_options; i++) {
+        fw_cfg_add_file(s, fw_cfg_options[i].name,
+                        fw_cfg_options[i].data, fw_cfg_options[i].size);
+    }
+
     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 b2e10c2..e6b0eeb 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 16ff72c..3879c89 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2669,6 +2669,17 @@ STEXI
 @table @option
 ETEXI
 
+DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
+    "-fw_cfg name=<name>,file=<file>\n"
+    "                add named fw_cfg entry from file\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -fw_cfg name=@var{name},file=@var{file}
+@findex -fw_cfg
+Add named fw_cfg entry from file. @var{name} determines the name of
+the entry in the fw_cfg file directory exposed to the guest.
+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..88a0b6e 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, 1);
+                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 v2 5/5] fw_cfg: insert fw_cfg file blobs via qemu cmdline

[Markus Armbruster]

[...]
> The one other thing I may not have comprehended was the part about
>
>> 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.

To test whether --readconfig works, append "--writeconfig test.cfg" to
your test command line(s), inspect the resulting file, and see whether
replacing your command arguments by "--readconfig test.cfg" works.
You'll have to retain any options that aren't implemented with QemuOpts,
such as --nodefaults.

[...]

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

[Laszlo Ersek]

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

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

arm/virt doesn't :)

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

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

    fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);

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

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

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

I'll leave up to other reviewers.

> +== fw_cfg_add_bytes() ==
> +
> +Given a selector key value, starting pointer, and size, create an item
> +as a raw "blob" of the given size, available by selecting the given key.

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

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

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

> +
> +== fw_cfg_add_iXX() ==
> +
> +Insert an XX-bit item, where XX may be 16, 32, or 64. These functions
> +will convert a 16-, 32-, or 64-bit integer to little-endian format
> +before creating an item of the appropriate size available via the given
> +selector key value.

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

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

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

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

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

> Unlike fw_cfg_add_bytes() above, the next
> +available selector key (above 0x0020, FW_CFG_FILE_FIRST) will be used,
> +and a new entry will be added to the file directory structure (at key
> +0x0019), containing the file name, blob size, and automatically assigned
> +selector key value.
> +
> +== fw_cfg_add_file_callback() ==
> +
> +Like fw_cfg_add_file(), but additionally sets pointers to a callback
> +function (and argument), which will be executed host-side by QEMU each
> +time a byte is read by the guest from this particular item.

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

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

(fw_cfg item name)

> with the new
> +given blob. If an existing blob is found, its callback information is
> +removed, and a pointer to the old data is returned to allow the caller
> +to free it, helping avoid memory leaks.

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

> If a configuration item does
> +not already exist under the given file name, a new item will be created
> +as with fw_cfg_add_file(), and NULL is returned to the caller.
> +
> +== fw_cfg_add_callback() ==
> +
> +Like fw_cfg_add_bytes(), but additionally sets pointers to a callback
> +function (and argument), which will be executed host-side by QEMU each
> +time a guest-side write operation to this particular item completes
> +fully overwriting the item's data.
> +
> +NOTE: This function is deprecated, and will be completely removed
> +starting with QEMU v2.4.
> 

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

Thanks!
Laszlo

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

[Laszlo Ersek]

On 03/19/15 01:18, Gabriel L. Somlo wrote:
> From this point forward, any guest-side writes to the fw_cfg
> data register will be treated as no-ops. This patch also removes
> the unused host-side API function fw_cfg_add_callback(), which
> allowed the registration of a callback to be executed each time
> the guest completed a full overwrite of a given fw_cfg data item.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/nvram/fw_cfg.c         | 33 +--------------------------------
>  include/hw/nvram/fw_cfg.h |  2 --
>  trace-events              |  1 -
>  3 files changed, 1 insertion(+), 35 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 78a37be..2f609b4 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;
>  
> @@ -232,19 +231,7 @@ static void fw_cfg_reboot(FWCfgState *s)
>  
>  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;
> -        }
> -    }
> +    /* nothing, write support removed in QEMU v2.4+ */
>  }
>  
>  static int fw_cfg_select(FWCfgState *s, uint16_t key)
> @@ -458,7 +445,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 +488,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..b2e10c2 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -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,
> diff --git a/trace-events b/trace-events
> index 30eba92..1275b70 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -193,7 +193,6 @@ ecc_diag_mem_writeb(uint64_t addr, uint32_t val) "Write diagnostic %"PRId64" = %
>  ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x"
>  
>  # hw/nvram/fw_cfg.c
> -fw_cfg_write(void *s, uint8_t value) "%p %d"
>  fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
>  fw_cfg_read(void *s, uint8_t ret) "%p = %d"
>  fw_cfg_add_file_dupe(void *s, char *name) "%p %s"
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/5] fw_cfg: prevent selector key conflict

[Laszlo Ersek]

On 03/19/15 01:18, Gabriel L. Somlo wrote:
> Enforce a single assignment of data for each distinct selector key.
> 
> 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 2f609b4..659de4c 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -423,6 +423,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); /* avoid key conflict */
>  
>      s->entries[arch][key].data = data;
>      s->entries[arch][key].len = (uint32_t)len;
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/5] fw_cfg: prohibit insertion of duplicate fw_cfg file names

[Laszlo Ersek]

On 03/19/15 01:18, Gabriel L. Somlo wrote:
> Exit with an error (instead of simply logging a trace event)
> whenever the same fw_cfg file name is added multiple times via
> one of the fw_cfg_add_file[_callback]() host-side API calls.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/nvram/fw_cfg.c | 11 ++++++-----
>  trace-events      |  1 -
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 659de4c..a5fd512 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -505,18 +505,19 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>      index = be32_to_cpu(s->files->count);
>      assert(index < FW_CFG_FILE_SLOTS);
>  
> -    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;
> +            error_report("duplicate fw_cfg file name: %s",
> +                         s->files->f[index].name);
> +            exit(1);
>          }
>      }
>  
> +    fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index,
> +                                   callback, callback_opaque, data, len);
> +
>      s->files->f[index].size   = cpu_to_be32(len);
>      s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
>      trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
> diff --git a/trace-events b/trace-events
> index 1275b70..a340c5a 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -195,7 +195,6 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x
>  # hw/nvram/fw_cfg.c
>  fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
>  fw_cfg_read(void *s, uint8_t ret) "%p = %d"
> -fw_cfg_add_file_dupe(void *s, char *name) "%p %s"
>  fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
>  
>  # hw/block/hd-geometry.c
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

[Laszlo Ersek]

On 03/19/15 01:18, 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 programmatically added from
> within the QEMU source code.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> I belive at this point I'm addressing all of Laszlo's feedback.
> 
> fw_cfg_option_add() operates from within the "Location" set by
> qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 1), and, as such,
> does not require a "location switch" to print out the proper context
> around the error message.
> 
> I'm allocating blobs (and reading host-side file contents) all from
> within fw_cfg_option_add(), so those error messages are also printed
> with the right context.
> 
> Simply traversing the command-line blob list and inserting it into
> fw_cfg now happens directly from fw_cfg_init1(), immediately after
> the device is initialized and ready to accept fw_cfg_add_* calls.
> 
> I'm not sure I'm addressing Gerd's concerns re. init order, but
> since each blob key and each file name can only be inserted at most
> once, we will exit with an error whether the user-specified blob from
> the command line or the programmatically-inserted one from the arch
> specific init routine gets inserted first.
> 
> And, since that's the case, I took full advantage of not having to
> determine with certainty for each architecture where the "last place
> any fw_cfg blobs are inserted" is, so I could add the command line
> blobs *last*. Since *first* will work just as well, or exit with the
> same error message, I don't have to worry about handling each arch
> separately.
> 
> The one other thing I may not have comprehended was the part about
> 
>> 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.
> 
> If the code below is still in serious need of cleaning up, please
> help me figure out what I'm missing, and maybe point me toward an
> example.

Yes, -readconfig support is the missing bit here.

I won't claim I understand everything Gerd said, but I think I can take
a shot at explaining readconfig (although I vaguely recall Markus and/or
Gerd did that already -- my explanation is bound to be weaker :))

So, the problem is that you are doing "business logic" under the
QEMU_OPTION_fwcfg case label. The fw_cfg_option_add() function call is
that business logic.

When you read options from a file with -readconfig, the option parsing
will happen, but this specific case loop in main() that iterates over
the command line arguments will *not* find '-fw_cfg' (assuming you won't
be passing it on the command line, only in the config file). The option
parsing itself will have been taken care of, in qemu_read_config_file(),
but the additional "business logic" will be missing, because the code
that calls fw_cfg_option_add() simply won't run.

Consider the command line option case:

main()
  qemu_opts_parse()
    opts_parse()
      qemu_opts_create()
        // links the new -fw_cfg QemuOpts under "qemu_fw_cfg_opts.head"
      opts_do_parse()
        // for each sub-option, ie. "name" and "file":
        opt_set()
          // validate against format specification
  fw_cfg_option_add()
    // do business, yo

versus the readconfig case (example config file snippet below):

  [fw_cfg]
    name = "etc/foo"
    file = "/tmp/foo.dat"

  [fw_cfg]
    name = "etc/bar"
    file = "/tmp/bar.dat"

which is parsed by

main()
  qemu_read_config_file()
    qemu_config_parse()
      /* group without id */ <--- finds first [fw_cfg] section
      qemu_opts_create()
        // links the new -fw_cfg QemuOpts under "qemu_fw_cfg_opts.head"

      /* arg = value */ <--- finds 'name = "etc/foo"'
      qemu_opt_set()
        opt_set()
          // validate against format specification

  // no business

That is, -readconfig will cover the same functionality as
qemu_opts_parse() does in your current patch, but nothing more.

Instead, the code under the QEMU_OPTION_fwcfg case label should only
consist of qemu_opts_parse(), and the error check. Then, *after* the
option parsing loop (which also includes the call to
qemu_read_config_file()), you should add another loop, with
qemu_opts_foreach(), that actually calls fw_cfg_option_add().

Because at that point, instances of -fw_cfg will have been linked into
"qemu_fw_cfg_opts.head" (originating from *both* the direct command line
and the config file read with -readconfig), and you can invoke the
"business logic" callback on each such instance, regardless of its
origin (cmdline or config file).

I think the simplest example that you can refer to is the two
occurrences of:

  qemu_find_opts("name")

And, as Markus said, qemu_opts_foreach() restores the Location
temporarily, before calling the callback you provide, so you can easily
use error_report() just the same.


Then, regarding the second part of Gerd's idea. Since you are adding a
separate qemu_opts_foreach() loop underneath the cmdline parsing loop,
you might as well delay it until after the

  machine_class->init(current_machine);

call in main(). That call invokes the board initialization code (for
example:
- machvirt_init() in "hw/arm/virt.c",
- pc_init_pci() in "hw/i386/pc_piix.c"),
which sets up fw_cfg (if the board has fw_cfg).

And then your callback function, invoked from the qemu_opts_foreach()
loop, can load the file from disk and add it to fw_cfg *immediately*. No
need for the "fw_cfg_options" temporary storage.

Of course, I guess, you should wrap your qemu_opts_foreach() loop with a
check to see if the machine in question actually has fw_cfg. If it
doesn't, then you might want to skip the loop, or even exit with an error.

Hmmmm... that's messy, again. fw_cfg is built into the qemu binary only
if you have CONFIG_SOFTMMU. I guess something like this should work:

#ifdef CONFIG_SOFTMMU
    /* okay, fw_cfg_find() is linked into the binary */

    if (fw_cfg_find()) {
        /* okay, the board has set up fw_cfg in its MachineClass init
         * function
         */

        if (qemu_opts_foreach(qemu_find_opts("fw_cfg"), parse_fw_cfg,
                              NULL, 1)) {
            exit(1);
        }
    }
#endif

You should definitely wait & see what Markus & Gerd think about the above.

I apologize for being wrong the first time around (well I could be wrong
again! :)), but nothing is simple in qemu, so bear with me.

Cheers
Laszlo

> 
> Thanks much,
>    Gabriel
> 
>  hw/nvram/fw_cfg.c         | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h |  4 ++++
>  qemu-options.hx           | 11 +++++++++++
>  vl.c                      | 27 +++++++++++++++++++++++++++
>  4 files changed, 82 insertions(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index a5fd512..7036fde 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"
> @@ -573,9 +574,42 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
>  }
>  
>  
> +static struct FWCfgOption {
> +    const char *name;
> +    gchar *data;
> +    size_t size;
> +} *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);
> +    }
> +    if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
> +        error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH - 1);
> +        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;
> +    if (!g_file_get_contents(file, &fw_cfg_options[fw_cfg_num_options].data,
> +                             &fw_cfg_options[fw_cfg_num_options].size, NULL)) {
> +        error_report("can't load %s", file);
> +        exit(1);
> +    }
> +    fw_cfg_num_options++;
> +}
> +
>  
>  static void fw_cfg_init1(DeviceState *dev)
>  {
> +    size_t i;
>      FWCfgState *s = FW_CFG(dev);
>  
>      assert(!object_resolve_path(FW_CFG_PATH, NULL));
> @@ -584,6 +618,12 @@ static void fw_cfg_init1(DeviceState *dev)
>  
>      qdev_init_nofail(dev);
>  
> +    /* insert file(s) from command line */
> +    for (i = 0; i < fw_cfg_num_options; i++) {
> +        fw_cfg_add_file(s, fw_cfg_options[i].name,
> +                        fw_cfg_options[i].data, fw_cfg_options[i].size);
> +    }
> +
>      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 b2e10c2..e6b0eeb 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 16ff72c..3879c89 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2669,6 +2669,17 @@ STEXI
>  @table @option
>  ETEXI
>  
> +DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
> +    "-fw_cfg name=<name>,file=<file>\n"
> +    "                add named fw_cfg entry from file\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> +@item -fw_cfg name=@var{name},file=@var{file}
> +@findex -fw_cfg
> +Add named fw_cfg entry from file. @var{name} determines the name of
> +the entry in the fw_cfg file directory exposed to the guest.
> +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..88a0b6e 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, 1);
> +                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);
> 

Re: [Qemu-devel] [PATCH v2 4/5] fw_cfg: prohibit insertion of duplicate fw_cfg file names

[Laszlo Ersek]

On 03/19/15 01:18, Gabriel L. Somlo wrote:
> Exit with an error (instead of simply logging a trace event)
> whenever the same fw_cfg file name is added multiple times via
> one of the fw_cfg_add_file[_callback]() host-side API calls.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/nvram/fw_cfg.c | 11 ++++++-----
>  trace-events      |  1 -
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 659de4c..a5fd512 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -505,18 +505,19 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>      index = be32_to_cpu(s->files->count);
>      assert(index < FW_CFG_FILE_SLOTS);
>  
> -    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;
> +            error_report("duplicate fw_cfg file name: %s",
> +                         s->files->f[index].name);
> +            exit(1);
>          }
>      }
>  
> +    fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index,
> +                                   callback, callback_opaque, data, len);
> +
>      s->files->f[index].size   = cpu_to_be32(len);
>      s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
>      trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
> diff --git a/trace-events b/trace-events
> index 1275b70..a340c5a 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -195,7 +195,6 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x
>  # hw/nvram/fw_cfg.c
>  fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
>  fw_cfg_read(void *s, uint8_t ret) "%p = %d"
> -fw_cfg_add_file_dupe(void *s, char *name) "%p %s"
>  fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
>  
>  # hw/block/hd-geometry.c
> 

Here's an idea I had this morning.

This series gives equal rank to fw_cfg file names that originate
internally and those that come from the user, via the command line.

That means that whenever qemu developers want to introduce a new fw_cfg
file, they can never be sure that the new name will not conflict with
something a user has already been passing in via the command line, for
whatever purposes. (Because, well, that's the goal of this patchset, to
empower the user to pass in fw_cfg files independently of qemu developers.)

This looks brittle. How about:

(a) advising users in the docs txt *and in the manual* to use some kind
of fw_cfg file name prefix, like "usr/" or "opt/", and then steering
clear of such prefixes in qemu, as far as developers are concerned. Or,

(b) automatically prepending "opt/" or "usr/" to all fw_cfg file names
that come via -fw_cfg (equiv. via [fw_cfg] in the config file), and, for
developers, steering clear of those prefixes in qemu's source.

The C standard and the POSIX standard define lists of identifier
prefixes (well, patterns) that are reserved for various uses. If a
program violates that, it might not compile on some platform, or with
the next release of the compiler on the same platform etc. I think we
should posit something like this.

Personally I vote (a). Document it, but don't enforce it.

(Assuming that a user-specified fw_cfg file gains traction, and becomes
popular to the point that qemu wants to expose it itself, then qemu can
just generate the same file with (eg.) an "etc/" prefix. And then
firmware (or other guest code) can start looking for the file under both
prefixes, and give priority to... well, that's another policy question;
but we're talking mechanism thus far. :))

Thoughts about (a) vs. (b) vs. neither?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v2 4/5] fw_cfg: prohibit insertion of duplicate fw_cfg file names

[Gabriel L. Somlo]

On Fri, Mar 20, 2015 at 07:51:06AM +0100, Laszlo Ersek wrote:
> Here's an idea I had this morning.
> 
> This series gives equal rank to fw_cfg file names that originate
> internally and those that come from the user, via the command line.
> 
> That means that whenever qemu developers want to introduce a new fw_cfg
> file, they can never be sure that the new name will not conflict with
> something a user has already been passing in via the command line, for
> whatever purposes. (Because, well, that's the goal of this patchset, to
> empower the user to pass in fw_cfg files independently of qemu developers.)
> 
> This looks brittle. How about:
> 
> (a) advising users in the docs txt *and in the manual* to use some kind
> of fw_cfg file name prefix, like "usr/" or "opt/", and then steering
> clear of such prefixes in qemu, as far as developers are concerned. Or,
> 
> (b) automatically prepending "opt/" or "usr/" to all fw_cfg file names
> that come via -fw_cfg (equiv. via [fw_cfg] in the config file), and, for
> developers, steering clear of those prefixes in qemu's source.
> 
> The C standard and the POSIX standard define lists of identifier
> prefixes (well, patterns) that are reserved for various uses. If a
> program violates that, it might not compile on some platform, or with
> the next release of the compiler on the same platform etc. I think we
> should posit something like this.
> 
> Personally I vote (a). Document it, but don't enforce it.
> 
> (Assuming that a user-specified fw_cfg file gains traction, and becomes
> popular to the point that qemu wants to expose it itself, then qemu can
> just generate the same file with (eg.) an "etc/" prefix. And then
> firmware (or other guest code) can start looking for the file under both
> prefixes, and give priority to... well, that's another policy question;
> but we're talking mechanism thus far. :))
> 
> Thoughts about (a) vs. (b) vs. neither?

You're basically saying it'd be nice to keep user-provided commandline
blobs in a separate *namespace* from those inserted programmatically
by QEMU, and I definitely agree!

My inner control freak's gut reaction is to vote (b), but I'm sure
theres lots of facets of the issue I haven't considered, so I could
easily be talked out of that selection :)

Either way, this would go in with patch 5/5 (where the command line
option is being added), so everything up to that point (including
generating an error on dupe fwcfg file names) should probably stay
the way it is...

Thanks much,
--Gabriel

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

[Gabriel L. Somlo]

On Thu, Mar 19, 2015 at 06:38:53PM +0100, Laszlo Ersek wrote:
> On 03/19/15 01:18, 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 programmatically added from
> > within the QEMU source code.
> > 
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> 
> Of course, I guess, you should wrap your qemu_opts_foreach() loop with a
> check to see if the machine in question actually has fw_cfg. If it
> doesn't, then you might want to skip the loop, or even exit with an error.
> 
> Hmmmm... that's messy, again. fw_cfg is built into the qemu binary only
> if you have CONFIG_SOFTMMU. I guess something like this should work:
> 
> #ifdef CONFIG_SOFTMMU
>     /* okay, fw_cfg_find() is linked into the binary */
> 
>     if (fw_cfg_find()) {
>         /* okay, the board has set up fw_cfg in its MachineClass init
>          * function
>          */
> 
>         if (qemu_opts_foreach(qemu_find_opts("fw_cfg"), parse_fw_cfg,
>                               NULL, 1)) {
>             exit(1);
>         }
>     }
> #endif
> 
> You should definitely wait & see what Markus & Gerd think about the above.

Thanks much Gerd, Markus, and Laszlo for the "QemuOpts 101" crash course!

With this change, the command-line options patch no longer has to
touch fw_cfg.c at all, which is pretty cool, IMHO.

Below is "version 2.5" of the command-line fw_cfg patch only. I'll
send out a v3 of the series once we're mostly happy with this one in
particular.

I didn't use #ifdef around fw_cfg_find() -- I think the underlying
object_resolve_path() will return NULL whether fw_cfg support isn't
compiled in or if the fw_cfg device hasn't been initialized, so
that should not be necessary [*].

[*] Oh, wait. Right now, fw_cfg_find() is a function defined inside
fw_cfg.c, so if that won't get compiled, we lose... How about turning
it into a macro or inline function in fw_cfg.h instead ? Something like

    static inline FWCfgState *fw_cfg_find(void)
    {
        return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
    }

should work nicely -- any thoughts ?

The only remaining issue in my mind is what to do about the issue
Laszlo raised, of whether we should force user-provided fw_cfg files
into a separate "namespace" from those inserted programmatically.

Thanks much,
  Gabriel


diff --git a/qemu-options.hx b/qemu-options.hx
index 319d971..138b9cd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2668,6 +2668,17 @@ STEXI
 @table @option
 ETEXI
 
+DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
+    "-fw_cfg name=<name>,file=<file>\n"
+    "                add named fw_cfg entry from file\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -fw_cfg name=@var{name},file=@var{file}
+@findex -fw_cfg
+Add named fw_cfg entry from file. @var{name} determines the name of
+the entry in the fw_cfg file directory exposed to the guest.
+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 75ec292..b8e5e4c 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
  *
@@ -2118,6 +2137,34 @@ char *qemu_find_file(int type, const char *name)
     return NULL;
 }
 
+static int parse_fw_cfg(QemuOpts *opts, void *opaque)
+{
+    gchar *buf;
+    size_t size;
+    const char *name, *file;
+
+    if (opaque == NULL) {
+        return 0;
+    }
+
+    name = qemu_opt_get(opts, "name");
+    file = qemu_opt_get(opts, "file");
+    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
+        error_report("invalid argument value");
+        return -1;
+    }
+    if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
+        error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH - 1);
+        return -1;
+    }
+    if (!g_file_get_contents(file, &buf, &size, NULL)) {
+        error_report("can't load %s", file);
+        return -1;
+    }
+    fw_cfg_add_file((FWCfgState *)opaque, name, buf, size);
+    return 0;
+}
+
 static int device_help_func(QemuOpts *opts, void *opaque)
 {
     return qdev_device_help(opts);
@@ -2806,6 +2853,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();
 
@@ -3422,6 +3470,12 @@ 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, 1);
+                if (opts == NULL) {
+                    exit(1);
+                }
+                break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse(olist, "accel=kvm", 0);
@@ -4231,6 +4285,11 @@ int main(int argc, char **argv, char **envp)
 
     numa_post_machine_init();
 
+    if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
+                          parse_fw_cfg, fw_cfg_find(), 1) != 0) {
+        exit(1);
+    }
+
     /* init USB devices */
     if (usb_enabled()) {
         if (foreach_device_config(DEV_USB, usb_parse) < 0)

Re: [Qemu-devel] [PATCH v2 4/5] fw_cfg: prohibit insertion of duplicate fw_cfg file names

[Laszlo Ersek]

On 03/20/15 15:34, Gabriel L. Somlo wrote:
> On Fri, Mar 20, 2015 at 07:51:06AM +0100, Laszlo Ersek wrote:
>> Here's an idea I had this morning.
>>
>> This series gives equal rank to fw_cfg file names that originate
>> internally and those that come from the user, via the command line.
>>
>> That means that whenever qemu developers want to introduce a new
>> fw_cfg file, they can never be sure that the new name will not
>> conflict with something a user has already been passing in via the
>> command line, for whatever purposes. (Because, well, that's the goal
>> of this patchset, to empower the user to pass in fw_cfg files
>> independently of qemu developers.)
>>
>> This looks brittle. How about:
>>
>> (a) advising users in the docs txt *and in the manual* to use some
>> kind of fw_cfg file name prefix, like "usr/" or "opt/", and then
>> steering clear of such prefixes in qemu, as far as developers are
>> concerned. Or,
>>
>> (b) automatically prepending "opt/" or "usr/" to all fw_cfg file
>> names that come via -fw_cfg (equiv. via [fw_cfg] in the config file),
>> and, for developers, steering clear of those prefixes in qemu's
>> source.
>>
>> The C standard and the POSIX standard define lists of identifier
>> prefixes (well, patterns) that are reserved for various uses. If a
>> program violates that, it might not compile on some platform, or with
>> the next release of the compiler on the same platform etc. I think we
>> should posit something like this.
>>
>> Personally I vote (a). Document it, but don't enforce it.
>>
>> (Assuming that a user-specified fw_cfg file gains traction, and
>> becomes popular to the point that qemu wants to expose it itself,
>> then qemu can just generate the same file with (eg.) an "etc/"
>> prefix. And then firmware (or other guest code) can start looking for
>> the file under both prefixes, and give priority to... well, that's
>> another policy question; but we're talking mechanism thus far. :))
>>
>> Thoughts about (a) vs. (b) vs. neither?
>
> You're basically saying it'd be nice to keep user-provided commandline
> blobs in a separate *namespace* from those inserted programmatically
> by QEMU, and I definitely agree!

Right. Dunno why I didn't say "namespace"; otherwise I keep dropping
that term twice a day. :)

> My inner control freak's gut reaction is to vote (b), but I'm sure
> theres lots of facets of the issue I haven't considered, so I could
> easily be talked out of that selection :)

Well, if you implement (b), I certainly won't protest.

> Either way, this would go in with patch 5/5 (where the command line
> option is being added), so everything up to that point (including
> generating an error on dupe fwcfg file names) should probably stay
> the way it is...

Oh yes, sure. I guess I followed up here only because this would be the
patch to catch the conflict. My R-b's stand.

Thanks
Laszlo

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

[Gabriel L. Somlo]

On Fri, Mar 20, 2015 at 02:01:25PM -0400, Gabriel L. Somlo wrote:
> On Thu, Mar 19, 2015 at 06:38:53PM +0100, Laszlo Ersek wrote:
> > Hmmmm... that's messy, again. fw_cfg is built into the qemu binary only
> > if you have CONFIG_SOFTMMU. I guess something like this should work:
> > 
> > #ifdef CONFIG_SOFTMMU
> >     /* okay, fw_cfg_find() is linked into the binary */
> > 
> >     if (fw_cfg_find()) {
> >         /* okay, the board has set up fw_cfg in its MachineClass init
> >          * function
> >          */
> > 
> >         if (qemu_opts_foreach(qemu_find_opts("fw_cfg"), parse_fw_cfg,
> >                               NULL, 1)) {
> >             exit(1);
> >         }
> >     }
> > #endif
> > 
> > You should definitely wait & see what Markus & Gerd think about the above.
> 
> I didn't use #ifdef around fw_cfg_find() -- I think the underlying
> object_resolve_path() will return NULL whether fw_cfg support isn't
> compiled in or if the fw_cfg device hasn't been initialized, so
> that should not be necessary [*].
> 
> [*] Oh, wait. Right now, fw_cfg_find() is a function defined inside
> fw_cfg.c, so if that won't get compiled, we lose... How about turning
> it into a macro or inline function in fw_cfg.h instead ? Something like
> 
>     static inline FWCfgState *fw_cfg_find(void)
>     {
>         return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>     }
> 
> should work nicely -- any thoughts ?

That patch, by the way, would look something like below. At this
point, "#ifdef CONFIG_SOFTMMU" definitely looks cleaner, although
now we'd have the (fw_cfg <-> softmmu) relationship codified *both*
here, and in hw/Makefile.objs ("devices-dirs-$(CONFIG_SOFTMMU) += nvram/"),
which is why I'm not 100% thrilled by the idea...

Thanks,
--Gabriel


diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 8d4ea25..7a7439a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -31,14 +31,10 @@
 #include "qemu/config-file.h"
 
 #define FW_CFG_SIZE 2
-#define FW_CFG_NAME "fw_cfg"
-#define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
-#define TYPE_FW_CFG     "fw_cfg"
 #define TYPE_FW_CFG_IO  "fw_cfg_io"
 #define TYPE_FW_CFG_MEM "fw_cfg_mem"
 
-#define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
 #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
 #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
 
@@ -633,11 +629,6 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
 }
 
 
-FWCfgState *fw_cfg_find(void)
-{
-    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
-}
-
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index b2e10c2..10c7771 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 "qom/object.h"
 #endif
 
 #define FW_CFG_SIGNATURE        0x00
@@ -81,7 +82,16 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
 FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
                                  uint32_t data_width);
 
-FWCfgState *fw_cfg_find(void);
+#define FW_CFG_NAME "fw_cfg"
+#define FW_CFG_PATH "/machine/" FW_CFG_NAME
+#define TYPE_FW_CFG "fw_cfg"
+
+#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
+
+static inline FWCfgState *fw_cfg_find(void)
+{
+    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
+}
 
 #endif /* NO_QEMU_PROTOS */
 

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

[Gerd Hoffmann]

On Fr, 2015-03-20 at 14:01 -0400, Gabriel L. Somlo wrote:
> Below is "version 2.5" of the command-line fw_cfg patch only. I'll
> send out a v3 of the series once we're mostly happy with this one in
> particular.

Approach taken looks good to me on a quick scan.

cheers,
  Gerd