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

[Laszlo Ersek <lersek@redhat.com>]

comments below

On 03/21/15 21:23, 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. A warning message will be
> printed if the fw_cfg item name does not begin with the
> prefix "opt/", which is recommended for external, user
> provided blobs.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> I tested that object_resolve_path() always returns NULL when it can't find
> a match for the given argument (i.e., rather than triggering an assert);
> this means on architectures which do not create/initialize a fw_cfg object,
> the call to fw_cfg_find() returns NULL.

Great!

> I spent a while trying to decide whether to wrap the vl.c bits
> in #ifdef CONFIG_SOFTMMU or not. Making fw_cfg_find() an always-available
> static inline doesn't actually help, since I'm also calling fw_cfg_add_file()
> from parse_fw_cfg(), so it's either #ifdef or no #ifdef, no point in turning
> fw_cfg_find() into "static inline".
> 
> This version of the patch (without #ifdef CONFIG_SOFTMMU) actually builds
> correctly, without linking errors due to missing fw_cfg symbols, on ALL
> current qemu architectures (there's only *-softmmu and *-linux-user, BTW,
> and the latter group doesn't actually appear to be using vl.c).
> 
> The "common-obj-y += vl.o" line in the toplevel Makefile.objs is wrapped
> inside an "ifeq ($(CONFIG_SOFTMMU),y) ... endif", which would make checking
> for CONFIG_SOFTMMU inside vl.c redundant anyway, so we should be good on
> that front as well.

Cool.

> If I'm still missing anything, or something pops up that we didn't talk
> about, I'd be happy to throw out a v4, just let me know.
> 
> Thanks,
>    Gabriel
> 
>  docs/specs/fw_cfg.txt | 21 +++++++++++++++++
>  qemu-options.hx       | 11 +++++++++
>  vl.c                  | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 6accd92..74351dd 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -203,3 +203,24 @@ completes fully overwriting the item's data.
>  
>  NOTE: This function is deprecated, and will be completely removed
>  starting with QEMU v2.4.
> +
> +== Externally Provided Items ==
> +
> +As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
> +FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
> +directory structure) may be inserted via the QEMU command line, using
> +the following syntax:
> +
> +    -fw_cfg [name=]<item_name>,file=<path>
> +
> +where <item_name> is the fw_cfg item name, and <path> is the location
> +on the host file system of a file containing the data to be inserted.
> +
> +NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/"
> +when using the "-fw_cfg" command line option, to avoid conflicting with
> +item names used internally by QEMU. For instance:
> +
> +    -fw_cfg name=opt/my_item_name,file=./my_blob.bin
> +
> +Similarly, QEMU developers *SHOULD NOT* use item names prefixed with
> +"opt/" when inserting items programmatically, e.g. via fw_cfg_add_file().

Nice. Thanks.

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

I guess I should have pointed this out earlier -- you could have
bracketed the "name=" part, to communicate that "name" is
"implied_opt_name".

Anyway, don't respin just because of this.

> +    "                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..1fc047d 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,38 @@ 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) {
> +        error_report("fw_cfg device not available");
> +        return -1;
> +    }
> +    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 (strncmp(name, "opt/", 4) != 0) {
> +        error_report("WARNING: externally provided fw_cfg item names "
> +                     "should be prefixed with \"opt/\"!");
> +    }
> +    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 +2857,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 +3474,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 +4289,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)
> 

Nice.

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