| Message ID | 1426515305-17766-6-git-send-email-somlo@cmu.edu |
|---|---|
| State | New |
| Headers | show |
Hi, > +static struct FWCfgOption { > + const char *name; > + const char *file; > +} *fw_cfg_options; > +static int fw_cfg_num_options; > +void fw_cfg_option_add(QemuOpts *opts) > +static void fw_cfg_options_insert(FWCfgState *s) Hmm, when looking at this (and the existing fw_cfg init order issues we have) I get the feeling that we should simply separate the fw_cfg storage and the fw_cfg device, with the storage being initialized early as global service ... cheers, Gerd
On Mon, 2015-03-16 at 10:15 -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 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(+) [...] > +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); > + } Just because I know I'm going to get this wrong when I start using it, can this error message mention that the fw_cfg argument is invalid, rather than being ambiguous?
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
Hi, > 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. Basically fw_cfg init picks up information from a bunch of places, instead of the having the places where the information is generated store the information in fw_cfg. Which would be nice as it would make the fw_cfg dependency more visible in the code. Due to parsing and using command line switches being separated (via QemuOpts) this doesn't create that many init order issues though. 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. cheers, Gerd
Matt, On Tue, Mar 17, 2015 at 10:55:30AM +0000, Matt Fleming wrote: > > +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); > > + } > > Just because I know I'm going to get this wrong when I start using it, > can this error message mention that the fw_cfg argument is invalid, > rather than being ambiguous? With a command line containing e.g. "-fw_cfg file=,name=" to trigger that error, the full error message ends up reading like this: qemu-system-x86_64: -fw_cfg file=,name=: invalid argument value So I figured anything more specific would end up repeating "fw_cfg" too many times on the same line of output... :) What do you think ? Thanks, --Gabriel
On Tue, Mar 17, 2015 at 12:49:50PM +0100, Gerd Hoffmann wrote: > Hi, > > > 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. > > Basically fw_cfg init picks up information from a bunch of places, > instead of the having the places where the information is generated > store the information in fw_cfg. Which would be nice as it would make > the fw_cfg dependency more visible in the code. > > Due to parsing and using command line switches being separated (via > QemuOpts) this doesn't create that many init order issues though. > > 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. +static struct FWCfgOption { + const char *name; + const char *file; +} *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); + } + + 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++; +} So basically, you're saying "don't process 'opts' here with qemu_opt_get(), just stash them as is, then process them later" ? (e.g. during fw_cfg_options_insert() below) ? Is there an existing example where that's currently done that I use for inspiration? Laszlo's suggestion was almost the opposite of this, i.e. allocate memory for all the blobs during option processing, them just call fw_cfg_add_file() during fw_cfg_options_insert(). +static void fw_cfg_options_insert(FWCfgState *s) +{ + size_t i; + int filesize; + 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); + } +} + static void fw_cfg_init1(DeviceState *dev) { @@ -584,6 +631,8 @@ static void fw_cfg_init1(DeviceState *dev) qdev_init_nofail(dev); + fw_cfg_options_insert(s); + 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)); Personally, I don't think there are *any* init order issues at all. It's an error to insert a dupe file name either way, whether the user's command line blob goes in first and we error out at the programmatically inserted dupe, or the other way around. BTW, regarding that suggestion (to allocate blobs during option_add and just call add_file() from options_insert(): I did think of that, but decided against it because users could pathologically provide the same fw_cfg file name multiple times on the command line, and I didn't like the idea of either allocating RAM for each one blindly, nor did I like the idea of adding checks for dupes within user-supplied blobs, so delaying everything until the moment fw_cfg_add_file would enforce uniqueness on my behalf sounded like the least amount of trouble. Of course, that's an unlikely scenario, and so what if we allocate lots of RAM only to exit with an error shortly after :) Anyway, I'm going to try and figure out the bits about stashing QemuOpts from fw_cfg_option_add(), then probably fall back to preallocating blobs during the same function. If I (very likely) misunderstood something, please let me know :) Thanks, --Gabriel
On Tue, Mar 17, 2015 at 12:28:20PM +0100, Laszlo Ersek wrote: > > + > > +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. I don't think that's applicable here. fw_cfg_option_add() is called from v.c: @@ -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); + 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); ...and, as such, I'm exactly in the appropriate location for error_report() to work as expected. In fact, in an earlier reply to Matt, I quoted an example of what the error message looks like: qemu-system-x86_64: -fw_cfg file=,name=: invalid argument value ...which shows it works the way we'd want it to. > (CC'ing Markus.) > > > +static void fw_cfg_options_insert(FWCfgState *s) > > +{ > > + int i, filesize; > > + 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); Now, *this* is where I could use the stashed copy of 'QemuOpts *opts' from fw_cfg_add_option() to tie the error report back to the "bad" command line component :) That would work if we decided it's OK to delay everything, including parsing '*opts' with qemu_opt_get(), until this point. > > + exit(1); > > + } > > + fw_cfg_add_file(s, fw_cfg_options[i].name, filebuf, filesize); > > + } > > +} Guess I'm going to wait on some feedback on whether to stash or not to stash QemuOpts first... Thanks much, --Gabriel
Hi, > + case QEMU_OPTION_fwcfg: > + opts = qemu_opts_parse(qemu_find_opts("fw_cfg"), optarg, 0); > + if (opts == NULL) { > + exit(1); > + } That is fine here. > + fw_cfg_option_add(opts); That should be called later. There is qemu_opts_foreach() to loop over all QemuOpts added, you'll find other calls of this in main(). And when called late enough (after fw_cfg init) you don't need the separate insert function any more. Testcase: (1) qemu -fw_cfg $options -writeconfig fw_cfg_test.cfg (2) qemu -readconfig fw_cfg_test.cfg Both (1) and (2) should give the same result. cheers, Gerd
"Gabriel L. Somlo" <gsomlo@gmail.com> writes: > On Tue, Mar 17, 2015 at 12:28:20PM +0100, Laszlo Ersek wrote: >> > + >> > +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. > > I don't think that's applicable here. fw_cfg_option_add() is called > from v.c: > > @@ -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); > + 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); > > ...and, as such, I'm exactly in the appropriate location for > error_report() to work as expected. In fact, in an earlier reply to > Matt, I quoted an example of what the error message looks like: > > qemu-system-x86_64: -fw_cfg file=,name=: invalid argument value > > ...which shows it works the way we'd want it to. Yup. >> (CC'ing Markus.) >> >> > +static void fw_cfg_options_insert(FWCfgState *s) >> > +{ >> > + int i, filesize; >> > + 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); > > Now, *this* is where I could use the stashed copy of 'QemuOpts *opts' > from fw_cfg_add_option() to tie the error report back to the "bad" > command line component :) That would work if we decided it's OK to > delay everything, including parsing '*opts' with qemu_opt_get(), until > this point. error_report() prints the "current location". This is actually the top of a location stack. The stack initially contains just one LOC_NONE entry, which makes error_report() print no location. main()'s option processing puts the current option's location on the stack. So do qemu_config_parse() and qemu_opt_foreach(). If you need to report an error later on, you have to do it yourself (unless you're using qemu_opt_foreach()). Sadly, we too often can't be bothered. The general pattern is push a location in one of the several ways do stuff, maybe call error_report() pop the location with loc_pop() To find concrete examples, search for loc_pop(). I can see two involving QemuOpts: scsi_bus_legacy_handle_cmdline() and pc_system_flash_init(). The former is easier to understand, because it does much less. A helper error_report_for_opts() could save you the monkeying around with the location stack. Maybe when we have more than two potential users. For a non-QemuOpts example, see foreach_device_config(). [...]
On 03/18/15 21:06, Gabriel L. Somlo wrote: > On Tue, Mar 17, 2015 at 12:49:50PM +0100, Gerd Hoffmann wrote: [snip] >> 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. [snip] > So basically, you're saying "don't process 'opts' here with qemu_opt_get(), > just stash them as is, then process them later" ? (e.g. during > fw_cfg_options_insert() below) ? > > Is there an existing example where that's currently done that I use > for inspiration? > > Laszlo's suggestion was almost the opposite of this, i.e. allocate > memory for all the blobs during option processing, them just call > fw_cfg_add_file() during fw_cfg_options_insert(). I was wrong, simply. :) I forgot about -readconfig completely. (I do recall that I had been annoyed earlier by some option not working as expected with -readconfig; perhaps -smp? Not sure.) Meta: You should always take my qemu reviews with a grain of salt. They're mostly good for pulling in reviewers who know what they're doing. ;) I reviewed your v1 quickly for two reasons: - I didn't want you to regress fw_cfg accidentally, especially code I wrote, or code UEFI depends on. - I'm a nice person (lol) and wanted to get the review gears rolling for you (see above). Cheers, Laszlo
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; + +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); + } + + 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++; +} + +static void fw_cfg_options_insert(FWCfgState *s) +{ + int i, filesize; + 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); + } +} + 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); + 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 +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); + 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);
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(+)