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 <address@hidden>
> > ---
> 
> 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
address@hidden -fw_cfg address@hidden,address@hidden
address@hidden -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)