Re: [Qemu-devel] [PATCH v2] fw_cfg: RFQDN rules, documentation

2016-04-07 Thread Michael S. Tsirkin 
On Thu, Apr 07, 2016 at 12:55:16PM -0400, Gabriel L. Somlo wrote:
...
> > > 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.
> 
> And that's why IMHO it's cleaner for that interface to be:
> 
>   /sys/firmware/qemu-fw-cfg/by-name//[key|name|raw|size]
> 
> I really don't think any particular instance of  could
> reasonably be called an "interface" (and therefore create expectations
> of its continued presence forever), or can it ?
> 
> Thanks,
> --Gabriel

Generally it's an interface if userspace relies on it.

> > This is unlike firmware interfaces - if these are updated
> > together with firmware, you do not need to maintain
> > old ones.

Re: [Qemu-devel] [PATCH v2] fw_cfg: RFQDN rules, documentation

2016-04-07 Thread Laszlo Ersek 
On 04/07/16 18:40, Michael S. Tsirkin wrote:
> On Thu, Apr 07, 2016 at 06:23:24PM +0200, Laszlo Ersek wrote:

>>   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.

Even for "interfaces" that are transparently passed through from
firmware / hardware? I think that shouldn't put compatibility
requirements on the kernel.

I tend to think about these sysfs (IIRC) entries similarly to ACPI
tables, SMBIOS tables, and such. Applications are allowed to see them,
yes; the kernel isn't responsible for maintaining them forever. If the
hardware changes, or the firmware changes, the applications (that care)
will see the change; and the kernel has no responsibility.

> This is unlike firmware interfaces - if these are updated
> together with firmware, you do not need to maintain
> old ones.

Anyway, I'll claim lack of jurisdiction here.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v2] fw_cfg: RFQDN rules, documentation

2016-04-07 Thread Gabriel L. Somlo 
On Thu, Apr 07, 2016 at 07:40:12PM +0300, Michael S. Tsirkin wrote:
> 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 
> > > Cc: Gabriel L. Somlo 
> > > Cc: Laszlo Ersek 
> > > Cc: Markus Armbruster 
> > > Signed-off-by: Michael S. Tsirkin 
> > > ---
> > > 
> > > 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.

And that's why IMHO it's cleaner for that interface to be:

/sys/firmware/qemu-fw-cfg/by-name//[key|name|raw|size]

I really don't think any particular instance of  could
reasonably be called an "interface" (and therefore create expectations
of its continued presence forever), or can it ?

Thanks,
--Gabriel

> 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

Re: [Qemu-devel] [PATCH v2] fw_cfg: RFQDN rules, documentation

2016-04-07 Thread Michael S. Tsirkin 
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 
> > Cc: Gabriel L. Somlo 
> > Cc: Laszlo Ersek 
> > Cc: Markus Armbruster 
> > Signed-off-by: Michael S. Tsirkin 
> > ---
> > 
> > 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

Re: [Qemu-devel] [PATCH v2] fw_cfg: RFQDN rules, documentation

2016-04-07 Thread Laszlo Ersek 
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 
> Cc: Gabriel L. Somlo 
> Cc: Laszlo Ersek 
> Cc: Markus Armbruster 
> Signed-off-by: Michael S. Tsirkin 
> ---
> 
> 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?

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.

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 

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

[Qemu-devel] [PATCH v2] fw_cfg: RFQDN rules, documentation

2016-04-07 Thread Michael S. Tsirkin 
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 
Cc: Gabriel L. Somlo 
Cc: Laszlo Ersek 
Cc: Markus Armbruster 
Signed-off-by: Michael S. Tsirkin 
---

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. "."?

 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);
+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=],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.
-
-Small enough items may be provided directly as strings on the command
-line, using the syntax:
+Or
 
 -fw_cfg [name=],string=
 
-The terminating NUL character of the content  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  and, if applicable, the content  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