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

["Gabriel L. Somlo" <somlo@cmu.edu>]

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