[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] docs: Discourage users from using fwcfg



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]