From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52351) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YYePg-000254-DB for qemu-devel@nongnu.org; Thu, 19 Mar 2015 13:39:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YYePY-0003s9-Jr for qemu-devel@nongnu.org; Thu, 19 Mar 2015 13:39:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39732) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YYePY-0003rq-Ak for qemu-devel@nongnu.org; Thu, 19 Mar 2015 13:39:12 -0400 Message-ID: <550B09AD.6050108@redhat.com> Date: Thu, 19 Mar 2015 18:38:53 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1426724311-9343-1-git-send-email-somlo@cmu.edu> <1426724311-9343-6-git-send-email-somlo@cmu.edu> In-Reply-To: <1426724311-9343-6-git-send-email-somlo@cmu.edu> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 5/5] fw_cfg: insert fw_cfg file blobs via qemu cmdline List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gabriel L. Somlo" Cc: matt.fleming@intel.com, mdroth@linux.vnet.ibm.com, rjones@redhat.com, jordan.l.justen@intel.com, qemu-devel@nongnu.org, gleb@cloudius-systems.com, gsomlo@gmail.com, kraxel@redhat.com, pbonzini@redhat.com, armbru@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 > --- > > 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=,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); >