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(a)redhat.com&gt; >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >>   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... 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?