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 <so...@cmu.edu>
> > ---
>
> 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
+@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..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)
