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

[Laszlo Ersek <lersek@redhat.com>]

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);
>