From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53471) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YzK8z-0002jS-0p for qemu-devel@nongnu.org; Mon, 01 Jun 2015 03:28:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YzK8u-0005Cj-SQ for qemu-devel@nongnu.org; Mon, 01 Jun 2015 03:28:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46489) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YzK8u-0005CK-Jw for qemu-devel@nongnu.org; Mon, 01 Jun 2015 03:28:16 -0400 Date: Mon, 1 Jun 2015 09:28:12 +0200 From: "Michael S. Tsirkin" Message-ID: <20150601092645-mutt-send-email-mst@redhat.com> References: <1430320913-20737-1-git-send-email-somlo@cmu.edu> <1430320913-20737-5-git-send-email-somlo@cmu.edu> <20150531181048.GC5268@redhat.com> <556C046B.9070704@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <556C046B.9070704@redhat.com> Subject: Re: [Qemu-devel] [PATCH V4 4/4] fw_cfg: insert fw_cfg file blobs via qemu cmdline List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: "Gabriel L. Somlo" , matt.fleming@intel.com, kraxel@redhat.com, qemu-devel@nongnu.org, gsomlo@gmail.com On Mon, Jun 01, 2015 at 09:06:19AM +0200, Laszlo Ersek wrote: > On 05/31/15 20:10, Michael S. Tsirkin wrote: > > On Wed, Apr 29, 2015 at 11:21:53AM -0400, 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 > >> Reviewed-by: Laszlo Ersek > > > > Would it make sense to make an illegal prefix a hard failure > > instead? > > If we don't, we'll be unable to add new file names without > > fear of conflicts in the future. > > We discussed this earlier. The idea was "mechanism, not policy", and > that if a user violates the convention (not rule) / ignores the warning > at some point, he's on his own when it breaks in the next version. > > I don't feel overly strongly about it; just "mechanism, not policy" > looks like a good tradition (well, good excuse anyway). > > Thanks > Laszlo Most users never see warnings. We ship it, we support it. If we don't want to support it, let's not ship it. > >> --- > >> 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=],file= > >> + > >> +where is the fw_cfg item name, and 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(). > >> diff --git a/qemu-options.hx b/qemu-options.hx > >> index 319d971..aa386b4 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=],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 74c2681..b02b2d4 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); > >> @@ -4233,6 +4291,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) > >> -- > >> 2.1.0 > >>