On Thu, Apr 07, 2016 at 06:23:24PM +0200, Laszlo Ersek wrote: > On 04/07/16 17:38, Michael S. Tsirkin wrote: > > This requires that all -fw_cfg command line users use names of the form > > opt/RFQDN/: such names are compatible with QEMU 2.4 and 2.5 as well as > > future QEMU versions. > > > > As ability to insert fw_cfg entries in QEMU root is useful for > > firmware development, add a special prefix: unsupported/root/ that > > allows that, while making sure users are aware it's unsupported. > > > > Cc: Gerd Hoffmann <kra...@redhat.com> > > Cc: Gabriel L. Somlo <so...@cmu.edu> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Markus Armbruster <arm...@redhat.com> > > Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > > --- > > > > changes from v1: > > address comments by Laszlo Ersek. > > > > There are still things worrying me > > > > 1. there is apparently no way to tell linux guests whether it should expose > > a specific file to userspace. > > > > 2. Should we have opt/fw/ or opt/hidden/ for firmware use? > > Alternatively, agree to hide files and/or directories > > starting with e.g. "."? > > Hm, is #2 an idea for addressing #1? > > For interpreting #2, I again have to reach back to the three groups of > people you identified -- QEMU developers, QEMU firmware developers, and > users. > > Since you say "for firmware use", I guess the point would be to enable > QEMU firmware developers to create such settings, either for > (a) population by QEMU, or for > (b) population by firmware end-users, > that the guest kernel would be prevented from seeing. > > Furthermore, since your examples both start with opt/, *and* we have > language saying > > QEMU developers MUST NOT use item names prefixed with "opt/" when > inserting items programmatically > > I determine that option (a) must not be your intent. Therefore, the > question is, I think: > > Should we allow QEMU firmware developers to create special settings, > to be populated manually by their end-users, that the guest kernel > would be prevented from seeing?
Exactly. > I don't think so. Namely, in practice, new firmware settings (that are > to be populated manually by users) will go under "opt/org.seabios/" and > "opt/org.tianocore.edk2.ovmf/". I couldn't care less if a guest kernel > user looks at such files. After all, the names *explicitly carry* the > RFQDN of the intended consumer. If a user violates it, that's his > problem. (It may become the problem of his downstream users too, but > that's the same thing.) > > So, as long as I understood your question right, I don't think it's > necessary. It's not a question we need to ask ourselves as hardware/qemu designers. It's a question for the guest kernel - once that exposes interfaces to applications, it has to maintain them forever. This is unlike firmware interfaces - if these are updated together with firmware, you do not need to maintain old ones. > I have one other comment below: > > > vl.c | 44 ++++++++++++++++++++++++++++++++++++++++---- > > docs/specs/fw_cfg.txt | 34 +++++++++++++++++----------------- > > qemu-options.hx | 38 +++++++++++++++++++++++++++++++++----- > > 3 files changed, 90 insertions(+), 26 deletions(-) > > > > diff --git a/vl.c b/vl.c > > index 2200e62..aec8a94 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -2296,8 +2296,11 @@ static int parse_fw_cfg(void *opaque, QemuOpts > > *opts, Error **errp) > > { > > gchar *buf; > > size_t size; > > - const char *name, *file, *str; > > + const char *name, *file, *str, *slash, *dot; > > FWCfgState *fw_cfg = (FWCfgState *) opaque; > > + static const char qemu_prefix[] = "opt/org.qemu"; > > + static const char ovmf_prefix[] = "opt/ovmf/"; > > + static const char unsupported_root_prefix[] = "unsupported/root/"; > > > > if (fw_cfg == NULL) { > > error_report("fw_cfg device not available"); > > @@ -2320,9 +2323,42 @@ static int parse_fw_cfg(void *opaque, QemuOpts > > *opts, Error **errp) > > 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/\""); > > + /* > > + * Look for and strip unsupported_root_prefix, which is useful for > > firmware > > + * development, but warn users. > > + */ > > + if (!strncmp(name, unsupported_root_prefix, > > + sizeof(unsupported_root_prefix) - 1)) { > > + error_report("warning: removing prefix \"%s\". " > > + "Guest or QEMU may crash. " > > + "Names must be prefixed with \"opt/RFQDN/\"", > > + unsupported_root_prefix); > > + name += strlen(unsupported_root_prefix); > > I think here you missed my separate comment about the sizeof > replacement. I'm not insisting on it, of course, but in v2 you did > replace all other strlen()s with sizeof, so I think this was an oversight. > > If you fix it: > > Reviewed-by: Laszlo Ersek <ler...@redhat.com> > > Thanks > Laszlo > > > + if (!nonempty_str(name)) { > > + error_report("invalid argument(s)"); > > + return -1; > > + } > > + } else if (!strncmp(name, ovmf_prefix, sizeof(ovmf_prefix) - 1)) { > > + /* Allow the prefix used historically with ovmf. */ > > + } else { > > + /* > > + * Don't attempt to validate a valid RFQDN in name, as that's not > > easy: > > + * we do validate that it includes '.' . > > + */ > > + if (strncmp(name, "opt/", 4) || > > + !(dot = strchr(name + 4, '.')) || > > + !(slash = strchr(name + 4, '/')) || > > + dot > slash) { > > + error_report("error: externally provided fw_cfg item names " > > + "must be prefixed with \"opt/RFQDN/\""); > > + return -1; > > + } > > + if (!strncmp(name, qemu_prefix, sizeof(qemu_prefix) - 1)) { > > + error_report("error: externally provided fw_cfg item names " > > + "must not use the reserved prefix \"%s\"", > > + qemu_prefix); > > + return -1; > > + } > > } > > if (nonempty_str(str)) { > > size = strlen(str); /* NUL terminator NOT included in fw_cfg blob > > */ > > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt > > index 5414140..41ce9ca 100644 > > --- a/docs/specs/fw_cfg.txt > > +++ b/docs/specs/fw_cfg.txt > > @@ -210,29 +210,29 @@ the following syntax: > > > > -fw_cfg [name=]<item_name>,file=<path> > > > > -where <item_name> is the fw_cfg item name, and <path> is the location > > -on the host file system of a file containing the data to be inserted. > > - > > -Small enough items may be provided directly as strings on the command > > -line, using the syntax: > > +Or > > > > -fw_cfg [name=]<item_name>,string=<string> > > > > -The terminating NUL character of the content <string> will NOT be > > -included as part of the fw_cfg item data, which is consistent with > > -the absence of a NUL terminator for items inserted via the file option. > > +See QEMU man page for more documentation. > > > > -Both <item_name> and, if applicable, the content <string> are passed > > -through by QEMU without any interpretation, expansion, or further > > -processing. Any such processing (potentially performed e.g., by the shell) > > -is outside of QEMU's responsibility; as such, using plain ASCII characters > > -is recommended. > > +Using item_name with plain ASCII characters only is recommended. > > > > -NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/" > > +Users MUST choose item names beginning with the prefix "opt/RFQDN/" > > when using the "-fw_cfg" command line option, to avoid conflicting with > > -item names used internally by QEMU. For instance: > > +item names used internally by QEMU, or by firmware. For instance: > > > > - -fw_cfg name=opt/my_item_name,file=./my_blob.bin > > + -fw_cfg name=opt/com.mycompany/guestagent/guestblob,file=./my_blob.bin > > > > -Similarly, QEMU developers *SHOULD NOT* use item names prefixed with > > +Similarly, QEMU developers MUST NOT use item names prefixed with > > "opt/" when inserting items programmatically, e.g. via fw_cfg_add_file(). > > + > > +For historical reasons "opt/ovmf/" is reserved for use with the OVMF > > firmware. > > + > > +To simplify guest firmware development, the prefix > > +unsupported/root/ is automatically stripped from paths, which > > +allows creating fw_cfg files in the root QEMU directory. This interface is > > +strictly for use by developers, its use can cause guest or QEMU crashes, is > > +unsupported and can be removed at any point. > > + > > +Any use of the prefix "opt/org.qemu" is reserved for future use. > > diff --git a/qemu-options.hx b/qemu-options.hx > > index a770086..a5abf98 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -2860,18 +2860,46 @@ ETEXI > > > > DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg, > > "-fw_cfg [name=]<name>,file=<file>\n" > > - " add named fw_cfg entry from file\n" > > + " add named fw_cfg entry with content from file\n" > > "-fw_cfg [name=]<name>,string=<str>\n" > > - " add named fw_cfg entry from string\n", > > + " add named fw_cfg entry with content from string\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. > > +Add named fw_cfg entry with content from file @var{file}. > > > > @item -fw_cfg [name=]@var{name},string=@var{str} > > -Add named fw_cfg entry from string. > > +Add named fw_cfg entry with content from string @var{str}, up to the first > > NUL character. > > + > > +The terminating NUL character of the content @var{str} will NOT be > > +included as part of the fw_cfg item data. To insert content including > > +the NUL character, store it in file and insert the item via > > +the @var{file} option. > > + > > +Both the name and the content are passed by QEMU through to the guest, > > where: > > +@table @option > > +@item @var{name} determines the name of the entry in the fw_cfg file > > directory exposed to the guest. > > + > > +@var{name} must be in the format opt/RFQDN/<item_name>. > > + > > +Any processing of @var{name} values (potentially performed e.g., by the > > shell) > > +is outside of QEMU's responsibility; as such, using plain ASCII characters > > is > > +recommended. > > +@end table > > + > > +Example: > > +@example > > + -fw_cfg name=opt/com.mycompany/guestagent/guestblob,file=./my_blob.bin > > +@end example > > + > > +To simplify guest firmware development, the prefix > > +unsupported/root/ is automatically stripped from paths, which > > +allows creating fw_cfg files in the root QEMU directory. This interface is > > +strictly for use by developers, its use can cause guest or QEMU crashes, is > > +unsupported and can be removed at any point. > > + > > ETEXI > > > > DEF("serial", HAS_ARG, QEMU_OPTION_serial, \ > >