[PATCH] docs: Discourage users from using fwcfg

Michal Privoznik posted 1 patch 3 years, 6 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/4b8abba831f4ba316668d261d631c86dd27c0346.1599486496.git.mprivozn@redhat.com
docs/formatdomain.rst | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
[PATCH] docs: Discourage users from using fwcfg
Posted by Michal Privoznik 3 years, 6 months ago
Even though this was brought up in upstream discussion [1] it
missed my patches: users should prefer <oemStrings/> over fwcfg.
The reason is that fwcfg is considered somewhat internal to QEMU
and it has limited number of slots and neither of these applies
to <oemStrings/>.

While I'm at it, I'm fixing the example too (because it contains
incorrect element name) and clarifying sysfs/ exposure.

1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 docs/formatdomain.rst | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 1979dfb8d3..821ffe8d60 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -509,18 +509,22 @@ layout of sub-elements, with supported values of:
    Some hypervisors provide unified way to tweak how firmware configures itself,
    or may contain tables to be installed for the guest OS, for instance boot
    order, ACPI, SMBIOS, etc. It even allows users to define their own config
-   blobs. In case of QEMU, these then appear under domain's sysfs, under
+   blobs. In case of QEMU, these then appear under domain's sysfs (if the guest
+   kernel has FW_CFG_SYSFS config option enabled), under
    ``/sys/firmware/qemu_fw_cfg``. Note, that these values apply regardless the
    <smbios/> mode under <os/>. :since:`Since 6.5.0`
 
+   **Please note that because of limited number of data slots use of fwcfg is
+   strongly discouraged and <oemStrings/> should be used instead**.
+
    ::
 
-        <smbios type='fwcfg'>
+        <sysinfo type='fwcfg'>
           <entry name='opt/com.example/name'>example value</entry>
-          <entry name='opt/com.coreos/config' file='/tmp/provision.ign'/>
-        </smbios>
+          <entry name='opt/com.example/config' file='/tmp/provision.ign'/>
+        </sysinfo>
 
-   The ``smbios`` element can have multiple ``entry`` child elements. Each
+   The ``sysinfo`` element can have multiple ``entry`` child elements. Each
    element then has mandatory ``name`` attribute, which defines the name of the
    blob and must begin with ``"opt/"`` and to avoid clashing with other names is
    advised to be in form ``"opt/$RFQDN/$name"`` where ``$RFQDN`` is a reverse
-- 
2.26.2

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Martin Kletzander 3 years, 6 months ago
On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>Even though this was brought up in upstream discussion [1] it
>missed my patches: users should prefer <oemStrings/> over fwcfg.
>The reason is that fwcfg is considered somewhat internal to QEMU
>and it has limited number of slots and neither of these applies
>to <oemStrings/>.
>
>While I'm at it, I'm fixing the example too (because it contains
>incorrect element name) and clarifying sysfs/ exposure.
>
>1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>
>Reported-by: Laszlo Ersek <lersek@redhat.com>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> docs/formatdomain.rst | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
>diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>index 1979dfb8d3..821ffe8d60 100644
>--- a/docs/formatdomain.rst
>+++ b/docs/formatdomain.rst
>@@ -509,18 +509,22 @@ layout of sub-elements, with supported values of:
>    Some hypervisors provide unified way to tweak how firmware configures itself,
>    or may contain tables to be installed for the guest OS, for instance boot
>    order, ACPI, SMBIOS, etc. It even allows users to define their own config
>-   blobs. In case of QEMU, these then appear under domain's sysfs, under
>+   blobs. In case of QEMU, these then appear under domain's sysfs (if the guest
>+   kernel has FW_CFG_SYSFS config option enabled), under
>    ``/sys/firmware/qemu_fw_cfg``. Note, that these values apply regardless the
>    <smbios/> mode under <os/>. :since:`Since 6.5.0`
>
>+   **Please note that because of limited number of data slots use of fwcfg is
>+   strongly discouraged and <oemStrings/> should be used instead**.
>+

It's nice that you say that, but people who would like to use fw_cfg for passing
in a huge blob, which is saved in a file, will read this, go to <oemStrings/>
and see that there is no way to pass a file as an input.  Should that be dealt
with somehow?  Or would that be discouraged as well?
Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Michal Privoznik 3 years, 6 months ago
On 9/7/20 3:57 PM, Martin Kletzander wrote:
> On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>> Even though this was brought up in upstream discussion [1] it
>> missed my patches: users should prefer <oemStrings/> over fwcfg.
>> The reason is that fwcfg is considered somewhat internal to QEMU
>> and it has limited number of slots and neither of these applies
>> to <oemStrings/>.
>>
>> While I'm at it, I'm fixing the example too (because it contains
>> incorrect element name) and clarifying sysfs/ exposure.
>>
>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>
>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> docs/formatdomain.rst | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>


> It's nice that you say that, but people who would like to use fw_cfg for 
> passing
> in a huge blob, which is saved in a file, will read this, go to 
> <oemStrings/>
> and see that there is no way to pass a file as an input.  Should that be 
> dealt
> with somehow?  Or would that be discouraged as well?

Unfortunately, QEMU doesn't allow reading OEM strings from a file (at 
least quick glance over hw/smbios/smbios.c doesn't show any signs it's 
allowed).

Michal

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Daniel P. Berrangé 3 years, 6 months ago
On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
> On 9/7/20 3:57 PM, Martin Kletzander wrote:
> > On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
> > > Even though this was brought up in upstream discussion [1] it
> > > missed my patches: users should prefer <oemStrings/> over fwcfg.
> > > The reason is that fwcfg is considered somewhat internal to QEMU
> > > and it has limited number of slots and neither of these applies
> > > to <oemStrings/>.
> > > 
> > > While I'm at it, I'm fixing the example too (because it contains
> > > incorrect element name) and clarifying sysfs/ exposure.
> > > 
> > > 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
> > > 
> > > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > ---
> > > docs/formatdomain.rst | 14 +++++++++-----
> > > 1 file changed, 9 insertions(+), 5 deletions(-)
> > > 
> 
> 
> > It's nice that you say that, but people who would like to use fw_cfg for
> > passing
> > in a huge blob, which is saved in a file, will read this, go to
> > <oemStrings/>
> > and see that there is no way to pass a file as an input.  Should that be
> > dealt
> > with somehow?  Or would that be discouraged as well?
> 
> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at least
> quick glance over hw/smbios/smbios.c doesn't show any signs it's allowed).

Indeed, that is an RFE I've never got around to

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Martin Kletzander 3 years, 6 months ago
On Mon, Sep 07, 2020 at 03:38:23PM +0100, Daniel P. Berrangé wrote:
>On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
>> > On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>> > > Even though this was brought up in upstream discussion [1] it
>> > > missed my patches: users should prefer <oemStrings/> over fwcfg.
>> > > The reason is that fwcfg is considered somewhat internal to QEMU
>> > > and it has limited number of slots and neither of these applies
>> > > to <oemStrings/>.
>> > >
>> > > While I'm at it, I'm fixing the example too (because it contains
>> > > incorrect element name) and clarifying sysfs/ exposure.
>> > >
>> > > 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>> > >
>> > > Reported-by: Laszlo Ersek <lersek@redhat.com>
>> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> > > ---
>> > > docs/formatdomain.rst | 14 +++++++++-----
>> > > 1 file changed, 9 insertions(+), 5 deletions(-)
>> > >
>>
>>
>> > It's nice that you say that, but people who would like to use fw_cfg for
>> > passing
>> > in a huge blob, which is saved in a file, will read this, go to
>> > <oemStrings/>
>> > and see that there is no way to pass a file as an input.  Should that be
>> > dealt
>> > with somehow?  Or would that be discouraged as well?
>>
>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at least
>> quick glance over hw/smbios/smbios.c doesn't show any signs it's allowed).
>
>Indeed, that is an RFE I've never got around to
>

Do you mean RFE for QEMU or libvirt?

Are there any limitations for the oemStrings?  Libvirt could read the file and
pass the data to QEMU as it is not something that is supposed to be writeable or
shareable.  I understand it could be the first time we do something like this,
but it might not be that bad of a precedence.

Just so we are on the same page, I'm not against this patch, I just want to save
our users the frustration that I, myself, experienced so many times.  There's
something deep hurting when you finally find exactly what you're looking for,
only to learn that it is deprecated with another option which does not provide
the same functionality.  Sure, you can have a start hook that reads a file and
changes the data, etc.  But you get the point.  At least we could mention that?

>Regards,
>Daniel
>-- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Daniel P. Berrangé 3 years, 6 months ago
On Tue, Sep 08, 2020 at 08:37:03AM +0200, Martin Kletzander wrote:
> On Mon, Sep 07, 2020 at 03:38:23PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
> > > On 9/7/20 3:57 PM, Martin Kletzander wrote:
> > > > On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
> > > > > Even though this was brought up in upstream discussion [1] it
> > > > > missed my patches: users should prefer <oemStrings/> over fwcfg.
> > > > > The reason is that fwcfg is considered somewhat internal to QEMU
> > > > > and it has limited number of slots and neither of these applies
> > > > > to <oemStrings/>.
> > > > >
> > > > > While I'm at it, I'm fixing the example too (because it contains
> > > > > incorrect element name) and clarifying sysfs/ exposure.
> > > > >
> > > > > 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
> > > > >
> > > > > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > > > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > > > ---
> > > > > docs/formatdomain.rst | 14 +++++++++-----
> > > > > 1 file changed, 9 insertions(+), 5 deletions(-)
> > > > >
> > > 
> > > 
> > > > It's nice that you say that, but people who would like to use fw_cfg for
> > > > passing
> > > > in a huge blob, which is saved in a file, will read this, go to
> > > > <oemStrings/>
> > > > and see that there is no way to pass a file as an input.  Should that be
> > > > dealt
> > > > with somehow?  Or would that be discouraged as well?
> > > 
> > > Unfortunately, QEMU doesn't allow reading OEM strings from a file (at least
> > > quick glance over hw/smbios/smbios.c doesn't show any signs it's allowed).
> > 
> > Indeed, that is an RFE I've never got around to
> > 
> 
> Do you mean RFE for QEMU or libvirt?

Both.

> Are there any limitations for the oemStrings?  Libvirt could read the file and
> pass the data to QEMU as it is not something that is supposed to be writeable or
> shareable.  I understand it could be the first time we do something like this,
> but it might not be that bad of a precedence.

The problem is the size of the command line you end up with. You need to have
QMEU support for reading from the file.

> Just so we are on the same page, I'm not against this patch, I just want to save
> our users the frustration that I, myself, experienced so many times.  There's
> something deep hurting when you finally find exactly what you're looking for,
> only to learn that it is deprecated with another option which does not provide
> the same functionality.  Sure, you can have a start hook that reads a file and
> changes the data, etc.  But you get the point.  At least we could mention that?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Martin Kletzander 3 years, 6 months ago
On Tue, Sep 08, 2020 at 08:03:46AM +0100, Daniel P. Berrangé wrote:
>On Tue, Sep 08, 2020 at 08:37:03AM +0200, Martin Kletzander wrote:
>> On Mon, Sep 07, 2020 at 03:38:23PM +0100, Daniel P. Berrangé wrote:
>> > On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>> > > On 9/7/20 3:57 PM, Martin Kletzander wrote:
>> > > > On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>> > > > > Even though this was brought up in upstream discussion [1] it
>> > > > > missed my patches: users should prefer <oemStrings/> over fwcfg.
>> > > > > The reason is that fwcfg is considered somewhat internal to QEMU
>> > > > > and it has limited number of slots and neither of these applies
>> > > > > to <oemStrings/>.
>> > > > >
>> > > > > While I'm at it, I'm fixing the example too (because it contains
>> > > > > incorrect element name) and clarifying sysfs/ exposure.
>> > > > >
>> > > > > 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>> > > > >
>> > > > > Reported-by: Laszlo Ersek <lersek@redhat.com>
>> > > > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> > > > > ---
>> > > > > docs/formatdomain.rst | 14 +++++++++-----
>> > > > > 1 file changed, 9 insertions(+), 5 deletions(-)
>> > > > >
>> > >
>> > >
>> > > > It's nice that you say that, but people who would like to use fw_cfg for
>> > > > passing
>> > > > in a huge blob, which is saved in a file, will read this, go to
>> > > > <oemStrings/>
>> > > > and see that there is no way to pass a file as an input.  Should that be
>> > > > dealt
>> > > > with somehow?  Or would that be discouraged as well?
>> > >
>> > > Unfortunately, QEMU doesn't allow reading OEM strings from a file (at least
>> > > quick glance over hw/smbios/smbios.c doesn't show any signs it's allowed).
>> >
>> > Indeed, that is an RFE I've never got around to
>> >
>>
>> Do you mean RFE for QEMU or libvirt?
>
>Both.
>
>> Are there any limitations for the oemStrings?  Libvirt could read the file and
>> pass the data to QEMU as it is not something that is supposed to be writeable or
>> shareable.  I understand it could be the first time we do something like this,
>> but it might not be that bad of a precedence.
>
>The problem is the size of the command line you end up with.

Good point.

>You need to have QEMU support for reading from the file.
>

Or from an FD, ideally.

>> Just so we are on the same page, I'm not against this patch, I just want to save
>> our users the frustration that I, myself, experienced so many times.  There's
>> something deep hurting when you finally find exactly what you're looking for,
>> only to learn that it is deprecated with another option which does not provide
>> the same functionality.  Sure, you can have a start hook that reads a file and
>> changes the data, etc.  But you get the point.  At least we could mention that?
>
>
>Regards,
>Daniel
>-- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Laszlo Ersek 3 years, 6 months ago
On 09/08/20 08:37, Martin Kletzander wrote:
> On Mon, Sep 07, 2020 at 03:38:23PM +0100, Daniel P. Berrangé wrote:
>> On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
>>> > On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>>> > > Even though this was brought up in upstream discussion [1] it
>>> > > missed my patches: users should prefer <oemStrings/> over fwcfg.
>>> > > The reason is that fwcfg is considered somewhat internal to QEMU
>>> > > and it has limited number of slots and neither of these applies
>>> > > to <oemStrings/>.
>>> > >
>>> > > While I'm at it, I'm fixing the example too (because it contains
>>> > > incorrect element name) and clarifying sysfs/ exposure.
>>> > >
>>> > > 1:
>>> https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>> > >
>>> > > Reported-by: Laszlo Ersek <lersek@redhat.com>
>>> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> > > ---
>>> > > docs/formatdomain.rst | 14 +++++++++-----
>>> > > 1 file changed, 9 insertions(+), 5 deletions(-)
>>> > >
>>>
>>>
>>> > It's nice that you say that, but people who would like to use
>>> fw_cfg for
>>> > passing
>>> > in a huge blob, which is saved in a file, will read this, go to
>>> > <oemStrings/>
>>> > and see that there is no way to pass a file as an input.  Should
>>> that be
>>> > dealt
>>> > with somehow?  Or would that be discouraged as well?
>>>
>>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at
>>> least
>>> quick glance over hw/smbios/smbios.c doesn't show any signs it's
>>> allowed).
>>
>> Indeed, that is an RFE I've never got around to
>>
> 
> Do you mean RFE for QEMU or libvirt?
> 
> Are there any limitations for the oemStrings?  Libvirt could read the
> file and
> pass the data to QEMU as it is not something that is supposed to be
> writeable or
> shareable.  I understand it could be the first time we do something like
> this,
> but it might not be that bad of a precedence.
> 
> Just so we are on the same page, I'm not against this patch, I just want
> to save
> our users the frustration that I, myself, experienced so many times. 
> There's
> something deep hurting when you finally find exactly what you're looking
> for,
> only to learn that it is deprecated with another option which does not
> provide
> the same functionality.

This description is not entirely accurate. I would put it like this: you
stumble upon a mis-use of a feature that was originally meant for
something else. It seems that the original consumers of the feature have
always been unhappy about the mis-use (it presents a risk for the
subsystem they are responsible for), in spite of the mis-use possibly
scratching your itch too.

That shouldn't hurt your feelings -- the point is that it's not
"deprecation"; the original thing has never been condoned for this
particular creative kind of abuse. Instead, the feature that could
scratch your itch has never existed.

Thanks,
Laszlo


> Sure, you can have a start hook that reads a
> file and
> changes the data, etc.  But you get the point.  At least we could
> mention that?
> 
>> Regards,
>> Daniel
>> -- 
>> |: https://berrange.com      -o-   
>> https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-           
>> https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-   
>> https://www.instagram.com/dberrange :|

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Martin Kletzander 3 years, 6 months ago
On Tue, Sep 08, 2020 at 10:22:34AM +0200, Laszlo Ersek wrote:
>On 09/08/20 08:37, Martin Kletzander wrote:
>> On Mon, Sep 07, 2020 at 03:38:23PM +0100, Daniel P. Berrangé wrote:
>>> On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>>>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
>>>> > On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>>>> > > Even though this was brought up in upstream discussion [1] it
>>>> > > missed my patches: users should prefer <oemStrings/> over fwcfg.
>>>> > > The reason is that fwcfg is considered somewhat internal to QEMU
>>>> > > and it has limited number of slots and neither of these applies
>>>> > > to <oemStrings/>.
>>>> > >
>>>> > > While I'm at it, I'm fixing the example too (because it contains
>>>> > > incorrect element name) and clarifying sysfs/ exposure.
>>>> > >
>>>> > > 1:
>>>> https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>> > >
>>>> > > Reported-by: Laszlo Ersek <lersek@redhat.com>
>>>> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> > > ---
>>>> > > docs/formatdomain.rst | 14 +++++++++-----
>>>> > > 1 file changed, 9 insertions(+), 5 deletions(-)
>>>> > >
>>>>
>>>>
>>>> > It's nice that you say that, but people who would like to use
>>>> fw_cfg for
>>>> > passing
>>>> > in a huge blob, which is saved in a file, will read this, go to
>>>> > <oemStrings/>
>>>> > and see that there is no way to pass a file as an input.  Should
>>>> that be
>>>> > dealt
>>>> > with somehow?  Or would that be discouraged as well?
>>>>
>>>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at
>>>> least
>>>> quick glance over hw/smbios/smbios.c doesn't show any signs it's
>>>> allowed).
>>>
>>> Indeed, that is an RFE I've never got around to
>>>
>>
>> Do you mean RFE for QEMU or libvirt?
>>
>> Are there any limitations for the oemStrings?  Libvirt could read the
>> file and
>> pass the data to QEMU as it is not something that is supposed to be
>> writeable or
>> shareable.  I understand it could be the first time we do something like
>> this,
>> but it might not be that bad of a precedence.
>>
>> Just so we are on the same page, I'm not against this patch, I just want
>> to save
>> our users the frustration that I, myself, experienced so many times. 
>> There's
>> something deep hurting when you finally find exactly what you're looking
>> for,
>> only to learn that it is deprecated with another option which does not
>> provide
>> the same functionality.
>
>This description is not entirely accurate. I would put it like this: you
>stumble upon a mis-use of a feature that was originally meant for
>something else. It seems that the original consumers of the feature have
>always been unhappy about the mis-use (it presents a risk for the
>subsystem they are responsible for), in spite of the mis-use possibly
>scratching your itch too.
>

That is *also* true, it's just a view from the other perspective.

>That shouldn't hurt your feelings -- the point is that it's not
>"deprecation"; the original thing has never been condoned for this
>particular creative kind of abuse. Instead, the feature that could
>scratch your itch has never existed.
>

This works for empathic readers only ;-)

>Thanks,
>Laszlo
>
>
>> Sure, you can have a start hook that reads a
>> file and
>> changes the data, etc.  But you get the point.  At least we could
>> mention that?
>>
>>> Regards,
>>> Daniel
>>> -- 
>>> |: https://berrange.com      -o-   
>>> https://www.flickr.com/photos/dberrange :|
>>> |: https://libvirt.org         -o-           
>>> https://fstop138.berrange.com :|
>>> |: https://entangle-photo.org    -o-   
>>> https://www.instagram.com/dberrange :|
>
Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Daniel P. Berrangé 3 years, 6 months ago
On Tue, Sep 08, 2020 at 10:22:34AM +0200, Laszlo Ersek wrote:
> On 09/08/20 08:37, Martin Kletzander wrote:
> > On Mon, Sep 07, 2020 at 03:38:23PM +0100, Daniel P. Berrangé wrote:
> >> On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
> >>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
> >>> > On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
> >>> > > Even though this was brought up in upstream discussion [1] it
> >>> > > missed my patches: users should prefer <oemStrings/> over fwcfg.
> >>> > > The reason is that fwcfg is considered somewhat internal to QEMU
> >>> > > and it has limited number of slots and neither of these applies
> >>> > > to <oemStrings/>.
> >>> > >
> >>> > > While I'm at it, I'm fixing the example too (because it contains
> >>> > > incorrect element name) and clarifying sysfs/ exposure.
> >>> > >
> >>> > > 1:
> >>> https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
> >>> > >
> >>> > > Reported-by: Laszlo Ersek <lersek@redhat.com>
> >>> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >>> > > ---
> >>> > > docs/formatdomain.rst | 14 +++++++++-----
> >>> > > 1 file changed, 9 insertions(+), 5 deletions(-)
> >>> > >
> >>>
> >>>
> >>> > It's nice that you say that, but people who would like to use
> >>> fw_cfg for
> >>> > passing
> >>> > in a huge blob, which is saved in a file, will read this, go to
> >>> > <oemStrings/>
> >>> > and see that there is no way to pass a file as an input.  Should
> >>> that be
> >>> > dealt
> >>> > with somehow?  Or would that be discouraged as well?
> >>>
> >>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at
> >>> least
> >>> quick glance over hw/smbios/smbios.c doesn't show any signs it's
> >>> allowed).
> >>
> >> Indeed, that is an RFE I've never got around to
> >>
> > 
> > Do you mean RFE for QEMU or libvirt?
> > 
> > Are there any limitations for the oemStrings?  Libvirt could read the
> > file and
> > pass the data to QEMU as it is not something that is supposed to be
> > writeable or
> > shareable.  I understand it could be the first time we do something like
> > this,
> > but it might not be that bad of a precedence.
> > 
> > Just so we are on the same page, I'm not against this patch, I just want
> > to save
> > our users the frustration that I, myself, experienced so many times. 
> > There's
> > something deep hurting when you finally find exactly what you're looking
> > for,
> > only to learn that it is deprecated with another option which does not
> > provide
> > the same functionality.
> 
> This description is not entirely accurate. I would put it like this: you
> stumble upon a mis-use of a feature that was originally meant for
> something else. It seems that the original consumers of the feature have
> always been unhappy about the mis-use (it presents a risk for the
> subsystem they are responsible for), in spite of the mis-use possibly
> scratching your itch too.
> 
> That shouldn't hurt your feelings -- the point is that it's not
> "deprecation"; the original thing has never been condoned for this
> particular creative kind of abuse. Instead, the feature that could
> scratch your itch has never existed.

I understand why you wish that was the case, but I don't think that
is the reality today.

The QEMU manual page has explicitly documented -fw_cfg, even going
as far as illustrating its usage for passing application specific
data to the guest:

[quote]
     Add named fw_cfg entry with contents from string str.
     
     The terminating NUL character of the contents of str will
     not be included as part of the fw_cfg item data. To insert
     contents with embedded NUL characters, you have to use the
     file parameter.

     The fw_cfg entries are passed by QEMU through to the guest.

     Example:
         -fw_cfg name=opt/com.mycompany/blob,file=./my_blob.bin

     creates an fw_cfg entry named opt/com.mycompany/blob with
     contents from ./my_blob.bin.
[/quote]

Similarly the -help output merely lists -fw_cfg as a "debug/expert"
option, and doesn't give any indication it is not for application
usage.

IOW, saying that this is not for general application usage is
directly contradicting QEMU's own documentation that exists today.

If you want to change such that this option is marked not for
application usage, then that is certainly a deprecation of an
existing documented feature in QEMU.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Laszlo Ersek 3 years, 6 months ago
On 09/08/20 11:05, Daniel P. Berrangé wrote:
> On Tue, Sep 08, 2020 at 10:22:34AM +0200, Laszlo Ersek wrote:
>> On 09/08/20 08:37, Martin Kletzander wrote:
>>> On Mon, Sep 07, 2020 at 03:38:23PM +0100, Daniel P. Berrangé wrote:
>>>> On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>>>>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
>>>>>> On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>>>>>>> Even though this was brought up in upstream discussion [1] it
>>>>>>> missed my patches: users should prefer <oemStrings/> over fwcfg.
>>>>>>> The reason is that fwcfg is considered somewhat internal to QEMU
>>>>>>> and it has limited number of slots and neither of these applies
>>>>>>> to <oemStrings/>.
>>>>>>>
>>>>>>> While I'm at it, I'm fixing the example too (because it contains
>>>>>>> incorrect element name) and clarifying sysfs/ exposure.
>>>>>>>
>>>>>>> 1:
>>>>> https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>>>>>
>>>>>>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>>>>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>>>>> ---
>>>>>>> docs/formatdomain.rst | 14 +++++++++-----
>>>>>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>
>>>>>
>>>>>> It's nice that you say that, but people who would like to use
>>>>> fw_cfg for
>>>>>> passing
>>>>>> in a huge blob, which is saved in a file, will read this, go to
>>>>>> <oemStrings/>
>>>>>> and see that there is no way to pass a file as an input.  Should
>>>>> that be
>>>>>> dealt
>>>>>> with somehow?  Or would that be discouraged as well?
>>>>>
>>>>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at
>>>>> least
>>>>> quick glance over hw/smbios/smbios.c doesn't show any signs it's
>>>>> allowed).
>>>>
>>>> Indeed, that is an RFE I've never got around to
>>>>
>>>
>>> Do you mean RFE for QEMU or libvirt?
>>>
>>> Are there any limitations for the oemStrings?  Libvirt could read the
>>> file and
>>> pass the data to QEMU as it is not something that is supposed to be
>>> writeable or
>>> shareable.  I understand it could be the first time we do something like
>>> this,
>>> but it might not be that bad of a precedence.
>>>
>>> Just so we are on the same page, I'm not against this patch, I just want
>>> to save
>>> our users the frustration that I, myself, experienced so many times. 
>>> There's
>>> something deep hurting when you finally find exactly what you're looking
>>> for,
>>> only to learn that it is deprecated with another option which does not
>>> provide
>>> the same functionality.
>>
>> This description is not entirely accurate. I would put it like this: you
>> stumble upon a mis-use of a feature that was originally meant for
>> something else. It seems that the original consumers of the feature have
>> always been unhappy about the mis-use (it presents a risk for the
>> subsystem they are responsible for), in spite of the mis-use possibly
>> scratching your itch too.
>>
>> That shouldn't hurt your feelings -- the point is that it's not
>> "deprecation"; the original thing has never been condoned for this
>> particular creative kind of abuse. Instead, the feature that could
>> scratch your itch has never existed.
> 
> I understand why you wish that was the case, but I don't think that
> is the reality today.
> 
> The QEMU manual page has explicitly documented -fw_cfg, even going
> as far as illustrating its usage for passing application specific
> data to the guest:
> 
> [quote]
>      Add named fw_cfg entry with contents from string str.
>      
>      The terminating NUL character of the contents of str will
>      not be included as part of the fw_cfg item data. To insert
>      contents with embedded NUL characters, you have to use the
>      file parameter.
> 
>      The fw_cfg entries are passed by QEMU through to the guest.
> 
>      Example:
>          -fw_cfg name=opt/com.mycompany/blob,file=./my_blob.bin
> 
>      creates an fw_cfg entry named opt/com.mycompany/blob with
>      contents from ./my_blob.bin.
> [/quote]
> 
> Similarly the -help output merely lists -fw_cfg as a "debug/expert"
> option, and doesn't give any indication it is not for application
> usage.
> 
> IOW, saying that this is not for general application usage is
> directly contradicting QEMU's own documentation that exists today.
> 
> If you want to change such that this option is marked not for
> application usage, then that is certainly a deprecation of an
> existing documented feature in QEMU.

... fair enough I guess.

Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Laszlo Ersek 3 years, 6 months ago
On 09/07/20 16:38, Daniel P. Berrangé wrote:
> On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
>>> On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>>>> Even though this was brought up in upstream discussion [1] it
>>>> missed my patches: users should prefer <oemStrings/> over fwcfg.
>>>> The reason is that fwcfg is considered somewhat internal to QEMU
>>>> and it has limited number of slots and neither of these applies
>>>> to <oemStrings/>.
>>>>
>>>> While I'm at it, I'm fixing the example too (because it contains
>>>> incorrect element name) and clarifying sysfs/ exposure.
>>>>
>>>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>>
>>>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>> docs/formatdomain.rst | 14 +++++++++-----
>>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>>
>>
>>
>>> It's nice that you say that, but people who would like to use fw_cfg for
>>> passing
>>> in a huge blob, which is saved in a file, will read this, go to
>>> <oemStrings/>
>>> and see that there is no way to pass a file as an input.  Should that be
>>> dealt
>>> with somehow?  Or would that be discouraged as well?
>>
>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at least
>> quick glance over hw/smbios/smbios.c doesn't show any signs it's allowed).
> 
> Indeed, that is an RFE I've never got around to

Yes, I remember that -- I remember that we discussed this, and also that
you didn't have time for it. Which is entirely justifiable; other stuff
has been (and continues to be) more important.

For reference, here's the launchpad ticket that tracks the RFE for QEMU:

  https://bugs.launchpad.net/qemu/+bug/1826200

Thanks,
Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Daniel P. Berrangé 3 years, 6 months ago
On Tue, Sep 08, 2020 at 10:17:08AM +0200, Laszlo Ersek wrote:
> On 09/07/20 16:38, Daniel P. Berrangé wrote:
> > On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
> >> On 9/7/20 3:57 PM, Martin Kletzander wrote:
> >>> On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
> >>>> Even though this was brought up in upstream discussion [1] it
> >>>> missed my patches: users should prefer <oemStrings/> over fwcfg.
> >>>> The reason is that fwcfg is considered somewhat internal to QEMU
> >>>> and it has limited number of slots and neither of these applies
> >>>> to <oemStrings/>.
> >>>>
> >>>> While I'm at it, I'm fixing the example too (because it contains
> >>>> incorrect element name) and clarifying sysfs/ exposure.
> >>>>
> >>>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
> >>>>
> >>>> Reported-by: Laszlo Ersek <lersek@redhat.com>
> >>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >>>> ---
> >>>> docs/formatdomain.rst | 14 +++++++++-----
> >>>> 1 file changed, 9 insertions(+), 5 deletions(-)
> >>>>
> >>
> >>
> >>> It's nice that you say that, but people who would like to use fw_cfg for
> >>> passing
> >>> in a huge blob, which is saved in a file, will read this, go to
> >>> <oemStrings/>
> >>> and see that there is no way to pass a file as an input.  Should that be
> >>> dealt
> >>> with somehow?  Or would that be discouraged as well?
> >>
> >> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at least
> >> quick glance over hw/smbios/smbios.c doesn't show any signs it's allowed).
> > 
> > Indeed, that is an RFE I've never got around to
> 
> Yes, I remember that -- I remember that we discussed this, and also that
> you didn't have time for it. Which is entirely justifiable; other stuff
> has been (and continues to be) more important.
> 
> For reference, here's the launchpad ticket that tracks the RFE for QEMU:
> 
>   https://bugs.launchpad.net/qemu/+bug/1826200

I spent some time today (more than expected in fact!) implementing
this....

  https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg03023.html

I hit a possible EDK2 bug when SMBIOS 3.0 where it hangs when the
size exceeds about 128 KB, but I don't intend to investigate it
further.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Laszlo Ersek 3 years, 6 months ago
On 09/08/20 19:26, Daniel P. Berrangé wrote:
> On Tue, Sep 08, 2020 at 10:17:08AM +0200, Laszlo Ersek wrote:
>> On 09/07/20 16:38, Daniel P. Berrangé wrote:
>>> On Mon, Sep 07, 2020 at 04:20:02PM +0200, Michal Privoznik wrote:
>>>> On 9/7/20 3:57 PM, Martin Kletzander wrote:
>>>>> On Mon, Sep 07, 2020 at 03:48:16PM +0200, Michal Privoznik wrote:
>>>>>> Even though this was brought up in upstream discussion [1] it
>>>>>> missed my patches: users should prefer <oemStrings/> over fwcfg.
>>>>>> The reason is that fwcfg is considered somewhat internal to QEMU
>>>>>> and it has limited number of slots and neither of these applies
>>>>>> to <oemStrings/>.
>>>>>>
>>>>>> While I'm at it, I'm fixing the example too (because it contains
>>>>>> incorrect element name) and clarifying sysfs/ exposure.
>>>>>>
>>>>>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>>>>
>>>>>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>>>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>>>> ---
>>>>>> docs/formatdomain.rst | 14 +++++++++-----
>>>>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>>
>>>>
>>>>
>>>>> It's nice that you say that, but people who would like to use fw_cfg for
>>>>> passing
>>>>> in a huge blob, which is saved in a file, will read this, go to
>>>>> <oemStrings/>
>>>>> and see that there is no way to pass a file as an input.  Should that be
>>>>> dealt
>>>>> with somehow?  Or would that be discouraged as well?
>>>>
>>>> Unfortunately, QEMU doesn't allow reading OEM strings from a file (at least
>>>> quick glance over hw/smbios/smbios.c doesn't show any signs it's allowed).
>>>
>>> Indeed, that is an RFE I've never got around to
>>
>> Yes, I remember that -- I remember that we discussed this, and also that
>> you didn't have time for it. Which is entirely justifiable; other stuff
>> has been (and continues to be) more important.
>>
>> For reference, here's the launchpad ticket that tracks the RFE for QEMU:
>>
>>   https://bugs.launchpad.net/qemu/+bug/1826200
> 
> I spent some time today (more than expected in fact!) implementing
> this....
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg03023.html

That was quick! Thank you!

> I hit a possible EDK2 bug when SMBIOS 3.0 where it hangs when the
> size exceeds about 128 KB, but I don't intend to investigate it
> further.

I'll look into it.

Thanks!
Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Laszlo Ersek 3 years, 6 months ago
On 09/07/20 15:48, Michal Privoznik wrote:
> Even though this was brought up in upstream discussion [1] it
> missed my patches: users should prefer <oemStrings/> over fwcfg.
> The reason is that fwcfg is considered somewhat internal to QEMU
> and it has limited number of slots and neither of these applies
> to <oemStrings/>.
>
> While I'm at it, I'm fixing the example too (because it contains
> incorrect element name) and clarifying sysfs/ exposure.
>
> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  docs/formatdomain.rst | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 1979dfb8d3..821ffe8d60 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -509,18 +509,22 @@ layout of sub-elements, with supported values of:
>     Some hypervisors provide unified way to tweak how firmware configures itself,
>     or may contain tables to be installed for the guest OS, for instance boot
>     order, ACPI, SMBIOS, etc. It even allows users to define their own config
> -   blobs. In case of QEMU, these then appear under domain's sysfs, under
> +   blobs. In case of QEMU, these then appear under domain's sysfs (if the guest
> +   kernel has FW_CFG_SYSFS config option enabled), under
>     ``/sys/firmware/qemu_fw_cfg``. Note, that these values apply regardless the
>     <smbios/> mode under <os/>. :since:`Since 6.5.0`
>
> +   **Please note that because of limited number of data slots use of fwcfg is
> +   strongly discouraged and <oemStrings/> should be used instead**.

please replace:

  strongly discouraged

with:

  strongly discouraged for configuring any guest-side component other
  than the firmware

(

Consider for example the following feature:

  https://bugzilla.tianocore.org/show_bug.cgi?id=2681

Namely, the following QEMU switches:

  -fw_cfg name=opt/org.tianocore/IPv4PXESupport,string=[yn]
  -fw_cfg name=opt/org.tianocore/IPv6PXESupport,string=[yn]

alter the behavior of OVMF and ArmVirtQemu. These flags are meant to be
stable. They do not need dedicated QEMU or libvirtd enablement. They
influence firmware behavior. So <sysinfo type='fwcfg'> is perfectly fine
(even ideal!) for tweaking them, through the domain XML. What's not fine
is configuring any random guest payload via <sysinfo type='fwcfg'>.

The point is that people who parse new fw_cfg files in edk2 such as
"opt/org.tianocore/IPv6PXESupport" are conscious of the slot count in
QEMU. They *can* bump the "x-file-slots" property in QEMU, for new
machine types, they just need to be aware of the property.

)

> +
>     ::
>
> -        <smbios type='fwcfg'>
> +        <sysinfo type='fwcfg'>
>            <entry name='opt/com.example/name'>example value</entry>

I suggest (according to the above):

- name: opt/org.tianocore/IPv4PXESupport
- value: n

> -          <entry name='opt/com.coreos/config' file='/tmp/provision.ign'/>
> -        </smbios>
> +          <entry name='opt/com.example/config' file='/tmp/provision.ign'/>

We have a functional -- working, stable -- example for name+file as
well:

- name: etc/edk2/https/cacerts
- file: /etc/pki/ca-trust/extracted/edk2/cacerts.bin

(This is documented in "OvmfPkg/README" in edk2, but it's not really
relevant here.)

> +        </sysinfo>
>
> -   The ``smbios`` element can have multiple ``entry`` child elements. Each
> +   The ``sysinfo`` element can have multiple ``entry`` child elements. Each
>     element then has mandatory ``name`` attribute, which defines the name of the
>     blob and must begin with ``"opt/"`` and to avoid clashing with other names is
>     advised to be in form ``"opt/$RFQDN/$name"`` where ``$RFQDN`` is a reverse
>

It's hard to express this cleanly.

- The opt/RFQDN notation is a mitigation for users that are hell-bent on
  using fw-cfg files of their own purposes (not heeding our advice about
  not using fw-cfg for such purposes at all). So the idea is, "if you
  ignore our request, then (a) be prepared to run out of slots, and (b)
  *at least* use a name pattern (opt/RFQDN) that minimizes conflicts
  with other, similar-minded users / projects"

- For "officially supported" knobs that the firmware looks at, it's fine
  to use any names -- they avoid conflicts with the above "rogue" files.
  Examples:

  - opt/ovmf/          -- reserved for historical reasons

  - opt/org.tianocore/ -- should never conflict due to RFQDN

  - etc/edk2/https/... -- should never conflict due to being outside of
                          opt/

So I guess the short rule is, "Feel free to refer to any fw_cfg file
name that your firmware officially supports. When defining other fw_cfg
file names (i.e., for your own purposes), then prepare for breakage in
the long-term, and then at least use the opt/RFQDN/ name pattern".

Thank you,
Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Daniel P. Berrangé 3 years, 6 months ago
On Tue, Sep 08, 2020 at 11:02:10AM +0200, Laszlo Ersek wrote:
> On 09/07/20 15:48, Michal Privoznik wrote:
> > Even though this was brought up in upstream discussion [1] it
> > missed my patches: users should prefer <oemStrings/> over fwcfg.
> > The reason is that fwcfg is considered somewhat internal to QEMU
> > and it has limited number of slots and neither of these applies
> > to <oemStrings/>.
> >
> > While I'm at it, I'm fixing the example too (because it contains
> > incorrect element name) and clarifying sysfs/ exposure.
> >
> > 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
> >
> > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> >  docs/formatdomain.rst | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > index 1979dfb8d3..821ffe8d60 100644
> > --- a/docs/formatdomain.rst
> > +++ b/docs/formatdomain.rst
> > @@ -509,18 +509,22 @@ layout of sub-elements, with supported values of:
> >     Some hypervisors provide unified way to tweak how firmware configures itself,
> >     or may contain tables to be installed for the guest OS, for instance boot
> >     order, ACPI, SMBIOS, etc. It even allows users to define their own config
> > -   blobs. In case of QEMU, these then appear under domain's sysfs, under
> > +   blobs. In case of QEMU, these then appear under domain's sysfs (if the guest
> > +   kernel has FW_CFG_SYSFS config option enabled), under
> >     ``/sys/firmware/qemu_fw_cfg``. Note, that these values apply regardless the
> >     <smbios/> mode under <os/>. :since:`Since 6.5.0`
> >
> > +   **Please note that because of limited number of data slots use of fwcfg is
> > +   strongly discouraged and <oemStrings/> should be used instead**.
> 
> please replace:
> 
>   strongly discouraged
> 
> with:
> 
>   strongly discouraged for configuring any guest-side component other
>   than the firmware
> 
> (
> 
> Consider for example the following feature:
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=2681
> 
> Namely, the following QEMU switches:
> 
>   -fw_cfg name=opt/org.tianocore/IPv4PXESupport,string=[yn]
>   -fw_cfg name=opt/org.tianocore/IPv6PXESupport,string=[yn]
> 
> alter the behavior of OVMF and ArmVirtQemu. These flags are meant to be
> stable. They do not need dedicated QEMU or libvirtd enablement. They
> influence firmware behavior. So <sysinfo type='fwcfg'> is perfectly fine
> (even ideal!) for tweaking them, through the domain XML. What's not fine
> is configuring any random guest payload via <sysinfo type='fwcfg'>.
> 
> The point is that people who parse new fw_cfg files in edk2 such as
> "opt/org.tianocore/IPv6PXESupport" are conscious of the slot count in
> QEMU. They *can* bump the "x-file-slots" property in QEMU, for new
> machine types, they just need to be aware of the property.

I'd suggest that QEMU always sets x-file-slots to be 10 entries larger
than the number of officially known slots. That leads some scope for
app usage without risk of hitting the limits.


> > -          <entry name='opt/com.coreos/config' file='/tmp/provision.ign'/>
> > -        </smbios>
> > +          <entry name='opt/com.example/config' file='/tmp/provision.ign'/>
> 
> We have a functional -- working, stable -- example for name+file as
> well:
> 
> - name: etc/edk2/https/cacerts
> - file: /etc/pki/ca-trust/extracted/edk2/cacerts.bin

I don't think we should document that in libvirt, since it is something
libvirt will set silently behind the scenes. The ignition example is a
acceptable real world example of expected usage in libvirt.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Laszlo Ersek 3 years, 6 months ago
On 09/08/20 11:12, Daniel P. Berrangé wrote:
> On Tue, Sep 08, 2020 at 11:02:10AM +0200, Laszlo Ersek wrote:
>> On 09/07/20 15:48, Michal Privoznik wrote:
>>> Even though this was brought up in upstream discussion [1] it
>>> missed my patches: users should prefer <oemStrings/> over fwcfg.
>>> The reason is that fwcfg is considered somewhat internal to QEMU
>>> and it has limited number of slots and neither of these applies
>>> to <oemStrings/>.
>>>
>>> While I'm at it, I'm fixing the example too (because it contains
>>> incorrect element name) and clarifying sysfs/ exposure.
>>>
>>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>
>>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  docs/formatdomain.rst | 14 +++++++++-----
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>> index 1979dfb8d3..821ffe8d60 100644
>>> --- a/docs/formatdomain.rst
>>> +++ b/docs/formatdomain.rst
>>> @@ -509,18 +509,22 @@ layout of sub-elements, with supported values of:
>>>     Some hypervisors provide unified way to tweak how firmware configures itself,
>>>     or may contain tables to be installed for the guest OS, for instance boot
>>>     order, ACPI, SMBIOS, etc. It even allows users to define their own config
>>> -   blobs. In case of QEMU, these then appear under domain's sysfs, under
>>> +   blobs. In case of QEMU, these then appear under domain's sysfs (if the guest
>>> +   kernel has FW_CFG_SYSFS config option enabled), under
>>>     ``/sys/firmware/qemu_fw_cfg``. Note, that these values apply regardless the
>>>     <smbios/> mode under <os/>. :since:`Since 6.5.0`
>>>
>>> +   **Please note that because of limited number of data slots use of fwcfg is
>>> +   strongly discouraged and <oemStrings/> should be used instead**.
>>
>> please replace:
>>
>>   strongly discouraged
>>
>> with:
>>
>>   strongly discouraged for configuring any guest-side component other
>>   than the firmware
>>
>> (
>>
>> Consider for example the following feature:
>>
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=2681
>>
>> Namely, the following QEMU switches:
>>
>>   -fw_cfg name=opt/org.tianocore/IPv4PXESupport,string=[yn]
>>   -fw_cfg name=opt/org.tianocore/IPv6PXESupport,string=[yn]
>>
>> alter the behavior of OVMF and ArmVirtQemu. These flags are meant to be
>> stable. They do not need dedicated QEMU or libvirtd enablement. They
>> influence firmware behavior. So <sysinfo type='fwcfg'> is perfectly fine
>> (even ideal!) for tweaking them, through the domain XML. What's not fine
>> is configuring any random guest payload via <sysinfo type='fwcfg'>.
>>
>> The point is that people who parse new fw_cfg files in edk2 such as
>> "opt/org.tianocore/IPv6PXESupport" are conscious of the slot count in
>> QEMU. They *can* bump the "x-file-slots" property in QEMU, for new
>> machine types, they just need to be aware of the property.
> 
> I'd suggest that QEMU always sets x-file-slots to be 10 entries larger
> than the number of officially known slots. That leads some scope for
> app usage without risk of hitting the limits.

Right, at least I believe we've tried maintaining a "safety margin". The
"officially known slots" is sort of a fuzzy count one can deduce from
grepping the QEMU, SeaBIOS and edk2 source trees. So when we'd like to
introduce a new file, we redo those greps, and compare with the current
x-file-slots value (for the latest machine types, that is).

> 
> 
>>> -          <entry name='opt/com.coreos/config' file='/tmp/provision.ign'/>
>>> -        </smbios>
>>> +          <entry name='opt/com.example/config' file='/tmp/provision.ign'/>
>>
>> We have a functional -- working, stable -- example for name+file as
>> well:
>>
>> - name: etc/edk2/https/cacerts
>> - file: /etc/pki/ca-trust/extracted/edk2/cacerts.bin
> 
> I don't think we should document that in libvirt, since it is something
> libvirt will set silently behind the scenes. The ignition example is a
> acceptable real world example of expected usage in libvirt.

OK. I wasn't sure if specific libvirt enablement was going to occur for
the CA certs passing.

Thanks,
Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Michal Privoznik 3 years, 6 months ago
On 9/8/20 11:02 AM, Laszlo Ersek wrote:
> On 09/07/20 15:48, Michal Privoznik wrote:
>> Even though this was brought up in upstream discussion [1] it
>> missed my patches: users should prefer <oemStrings/> over fwcfg.
>> The reason is that fwcfg is considered somewhat internal to QEMU
>> and it has limited number of slots and neither of these applies
>> to <oemStrings/>.
>>
>> While I'm at it, I'm fixing the example too (because it contains
>> incorrect element name) and clarifying sysfs/ exposure.
>>
>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>
>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   docs/formatdomain.rst | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index 1979dfb8d3..821ffe8d60 100644
>> --- a/docs/formatdomain.rst
>> +++ b/docs/formatdomain.rst
>> @@ -509,18 +509,22 @@ layout of sub-elements, with supported values of:
>>      Some hypervisors provide unified way to tweak how firmware configures itself,
>>      or may contain tables to be installed for the guest OS, for instance boot
>>      order, ACPI, SMBIOS, etc. It even allows users to define their own config
>> -   blobs. In case of QEMU, these then appear under domain's sysfs, under
>> +   blobs. In case of QEMU, these then appear under domain's sysfs (if the guest
>> +   kernel has FW_CFG_SYSFS config option enabled), under
>>      ``/sys/firmware/qemu_fw_cfg``. Note, that these values apply regardless the
>>      <smbios/> mode under <os/>. :since:`Since 6.5.0`
>>
>> +   **Please note that because of limited number of data slots use of fwcfg is
>> +   strongly discouraged and <oemStrings/> should be used instead**.
> 
> please replace:
> 
>    strongly discouraged
> 
> with:
> 
>    strongly discouraged for configuring any guest-side component other
>    than the firmware

Actually, we have a check that forbids anything else than "opt/" prefix 
and also "opt/ovmf/" and "opt/org.qemu/" are forbidden:

https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_validate.c#L780

Looks like "opt/org.tianocore" slipped through. I think the reasoning is 
that this is too low level and allows changing values of knobs that are 
exposed elsewhere (e.g. boot order). It really should be just to append 
new values, not change existing ones.

> 
> (
> 
> Consider for example the following feature:
> 
>    https://bugzilla.tianocore.org/show_bug.cgi?id=2681
> 
> Namely, the following QEMU switches:
> 
>    -fw_cfg name=opt/org.tianocore/IPv4PXESupport,string=[yn]
>    -fw_cfg name=opt/org.tianocore/IPv6PXESupport,string=[yn]
> 
> alter the behavior of OVMF and ArmVirtQemu. These flags are meant to be
> stable. They do not need dedicated QEMU or libvirtd enablement. They
> influence firmware behavior. So <sysinfo type='fwcfg'> is perfectly fine
> (even ideal!) for tweaking them, through the domain XML. What's not fine
> is configuring any random guest payload via <sysinfo type='fwcfg'>.
> 
> The point is that people who parse new fw_cfg files in edk2 such as
> "opt/org.tianocore/IPv6PXESupport" are conscious of the slot count in
> QEMU. They *can* bump the "x-file-slots" property in QEMU, for new
> machine types, they just need to be aware of the property.
> 
> )
> 
>> +
>>      ::
>>
>> -        <smbios type='fwcfg'>
>> +        <sysinfo type='fwcfg'>
>>             <entry name='opt/com.example/name'>example value</entry>
> 
> I suggest (according to the above):
> 
> - name: opt/org.tianocore/IPv4PXESupport
> - value: n
> 
>> -          <entry name='opt/com.coreos/config' file='/tmp/provision.ign'/>
>> -        </smbios>
>> +          <entry name='opt/com.example/config' file='/tmp/provision.ign'/>
> 
> We have a functional -- working, stable -- example for name+file as
> well:
> 
> - name: etc/edk2/https/cacerts
> - file: /etc/pki/ca-trust/extracted/edk2/cacerts.bin
> 
> (This is documented in "OvmfPkg/README" in edk2, but it's not really
> relevant here.)
> 
>> +        </sysinfo>
>>
>> -   The ``smbios`` element can have multiple ``entry`` child elements. Each
>> +   The ``sysinfo`` element can have multiple ``entry`` child elements. Each
>>      element then has mandatory ``name`` attribute, which defines the name of the
>>      blob and must begin with ``"opt/"`` and to avoid clashing with other names is
>>      advised to be in form ``"opt/$RFQDN/$name"`` where ``$RFQDN`` is a reverse
>>
> 
> It's hard to express this cleanly.
> 
> - The opt/RFQDN notation is a mitigation for users that are hell-bent on
>    using fw-cfg files of their own purposes (not heeding our advice about
>    not using fw-cfg for such purposes at all). So the idea is, "if you
>    ignore our request, then (a) be prepared to run out of slots, and (b)
>    *at least* use a name pattern (opt/RFQDN) that minimizes conflicts
>    with other, similar-minded users / projects"
> 
> - For "officially supported" knobs that the firmware looks at, it's fine
>    to use any names -- they avoid conflicts with the above "rogue" files.
>    Examples:
> 
>    - opt/ovmf/          -- reserved for historical reasons
> 
>    - opt/org.tianocore/ -- should never conflict due to RFQDN
> 
>    - etc/edk2/https/... -- should never conflict due to being outside of
>                            opt/
> 
> So I guess the short rule is, "Feel free to refer to any fw_cfg file
> name that your firmware officially supports. When defining other fw_cfg
> file names (i.e., for your own purposes), then prepare for breakage in
> the long-term, and then at least use the opt/RFQDN/ name pattern".

I think in the light of what I wrote above this makes more sense, 
doesn't it?

Michal

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Laszlo Ersek 3 years, 6 months ago
On 09/08/20 12:05, Michal Privoznik wrote:
> On 9/8/20 11:02 AM, Laszlo Ersek wrote:
>> On 09/07/20 15:48, Michal Privoznik wrote:
>>> Even though this was brought up in upstream discussion [1] it
>>> missed my patches: users should prefer <oemStrings/> over fwcfg.
>>> The reason is that fwcfg is considered somewhat internal to QEMU
>>> and it has limited number of slots and neither of these applies
>>> to <oemStrings/>.
>>>
>>> While I'm at it, I'm fixing the example too (because it contains
>>> incorrect element name) and clarifying sysfs/ exposure.
>>>
>>> 1: https://www.redhat.com/archives/libvir-list/2020-May/msg00957.html
>>>
>>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>   docs/formatdomain.rst | 14 +++++++++-----
>>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>> index 1979dfb8d3..821ffe8d60 100644
>>> --- a/docs/formatdomain.rst
>>> +++ b/docs/formatdomain.rst
>>> @@ -509,18 +509,22 @@ layout of sub-elements, with supported values of:
>>>      Some hypervisors provide unified way to tweak how firmware
>>> configures itself,
>>>      or may contain tables to be installed for the guest OS, for
>>> instance boot
>>>      order, ACPI, SMBIOS, etc. It even allows users to define their
>>> own config
>>> -   blobs. In case of QEMU, these then appear under domain's sysfs,
>>> under
>>> +   blobs. In case of QEMU, these then appear under domain's sysfs
>>> (if the guest
>>> +   kernel has FW_CFG_SYSFS config option enabled), under
>>>      ``/sys/firmware/qemu_fw_cfg``. Note, that these values apply
>>> regardless the
>>>      <smbios/> mode under <os/>. :since:`Since 6.5.0`
>>>
>>> +   **Please note that because of limited number of data slots use of
>>> fwcfg is
>>> +   strongly discouraged and <oemStrings/> should be used instead**.
>>
>> please replace:
>>
>>    strongly discouraged
>>
>> with:
>>
>>    strongly discouraged for configuring any guest-side component other
>>    than the firmware
> 
> Actually, we have a check that forbids anything else than "opt/" prefix
> and also "opt/ovmf/" and "opt/org.qemu/" are forbidden:
> 
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_validate.c#L780
> 
> 
> Looks like "opt/org.tianocore" slipped through. I think the reasoning is
> that this is too low level and allows changing values of knobs that are
> exposed elsewhere (e.g. boot order). It really should be just to append
> new values, not change existing ones.
> 
>>
>> (
>>
>> Consider for example the following feature:
>>
>>    https://bugzilla.tianocore.org/show_bug.cgi?id=2681
>>
>> Namely, the following QEMU switches:
>>
>>    -fw_cfg name=opt/org.tianocore/IPv4PXESupport,string=[yn]
>>    -fw_cfg name=opt/org.tianocore/IPv6PXESupport,string=[yn]
>>
>> alter the behavior of OVMF and ArmVirtQemu. These flags are meant to be
>> stable. They do not need dedicated QEMU or libvirtd enablement. They
>> influence firmware behavior. So <sysinfo type='fwcfg'> is perfectly fine
>> (even ideal!) for tweaking them, through the domain XML. What's not fine
>> is configuring any random guest payload via <sysinfo type='fwcfg'>.
>>
>> The point is that people who parse new fw_cfg files in edk2 such as
>> "opt/org.tianocore/IPv6PXESupport" are conscious of the slot count in
>> QEMU. They *can* bump the "x-file-slots" property in QEMU, for new
>> machine types, they just need to be aware of the property.
>>
>> )
>>
>>> +
>>>      ::
>>>
>>> -        <smbios type='fwcfg'>
>>> +        <sysinfo type='fwcfg'>
>>>             <entry name='opt/com.example/name'>example value</entry>
>>
>> I suggest (according to the above):
>>
>> - name: opt/org.tianocore/IPv4PXESupport
>> - value: n
>>
>>> -          <entry name='opt/com.coreos/config'
>>> file='/tmp/provision.ign'/>
>>> -        </smbios>
>>> +          <entry name='opt/com.example/config'
>>> file='/tmp/provision.ign'/>
>>
>> We have a functional -- working, stable -- example for name+file as
>> well:
>>
>> - name: etc/edk2/https/cacerts
>> - file: /etc/pki/ca-trust/extracted/edk2/cacerts.bin
>>
>> (This is documented in "OvmfPkg/README" in edk2, but it's not really
>> relevant here.)
>>
>>> +        </sysinfo>
>>>
>>> -   The ``smbios`` element can have multiple ``entry`` child
>>> elements. Each
>>> +   The ``sysinfo`` element can have multiple ``entry`` child
>>> elements. Each
>>>      element then has mandatory ``name`` attribute, which defines the
>>> name of the
>>>      blob and must begin with ``"opt/"`` and to avoid clashing with
>>> other names is
>>>      advised to be in form ``"opt/$RFQDN/$name"`` where ``$RFQDN`` is
>>> a reverse
>>>
>>
>> It's hard to express this cleanly.
>>
>> - The opt/RFQDN notation is a mitigation for users that are hell-bent on
>>    using fw-cfg files of their own purposes (not heeding our advice about
>>    not using fw-cfg for such purposes at all). So the idea is, "if you
>>    ignore our request, then (a) be prepared to run out of slots, and (b)
>>    *at least* use a name pattern (opt/RFQDN) that minimizes conflicts
>>    with other, similar-minded users / projects"
>>
>> - For "officially supported" knobs that the firmware looks at, it's fine
>>    to use any names -- they avoid conflicts with the above "rogue" files.
>>    Examples:
>>
>>    - opt/ovmf/          -- reserved for historical reasons
>>
>>    - opt/org.tianocore/ -- should never conflict due to RFQDN
>>
>>    - etc/edk2/https/... -- should never conflict due to being outside of
>>                            opt/
>>
>> So I guess the short rule is, "Feel free to refer to any fw_cfg file
>> name that your firmware officially supports. When defining other fw_cfg
>> file names (i.e., for your own purposes), then prepare for breakage in
>> the long-term, and then at least use the opt/RFQDN/ name pattern".
> 
> I think in the light of what I wrote above this makes more sense,
> doesn't it?

Are you saying that the current (upstream) language applies only to the
case when users define their own new fw_cfg file names?

Hm. That would certainly be consistent with my initial confusion upon
seeing the subject of this patch "Discourage users from using fwcfg" --
"why the blanket discouragement, since there are valid uses"? But this
would explain it, yes.

The docs say,

    Some hypervisors provide unified way to tweak how firmware
    configures itself, or may contain tables to be installed for the
    guest OS, for instance boot order, ACPI, SMBIOS, etc. It even allows
    users to define their own config blobs. In case of QEMU, these then
    appear [...]

So I guess for me what's missing is a hard paragraph break just before
"It even allows". That would clarify that the domain XML snippet belongs
to "defin[ing] their own config blobs".

With that in mind, let me re-review your patch...

Yes, I think your patch is fine then.

Please consider breaking the sentence "It even allows..." to a new
paragraph.

You could even keep the "com.coreos" example as-is.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Michal Privoznik 3 years, 6 months ago
On 9/8/20 1:45 PM, Laszlo Ersek wrote:
> Reviewed-by: Laszlo Ersek<lersek@redhat.com>

Thank you, pushed.

Michal

Re: [PATCH] docs: Discourage users from using fwcfg
Posted by Laszlo Ersek 3 years, 6 months ago
On 09/08/20 14:28, Michal Privoznik wrote:
> On 9/8/20 1:45 PM, Laszlo Ersek wrote:
>> Reviewed-by: Laszlo Ersek<lersek@redhat.com>
> 
> Thank you, pushed.

Thank *you* for the patch. :)

Laszlo