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

[Laszlo Ersek <lersek@redhat.com>]

comments below

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

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

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

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

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

(CC'ing Markus.)

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

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

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

Urgh. :)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Structurally this looks sane to me.

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

Thanks!
Laszlo