Bug 1763477

Can you please share your debug logs from 'virsh domcapabilities' invocation?
The commit you're referring you only made libvirt report unique paths (I know you don't claim it's the one causing regression).
Anyway, I've just tested this locally and it works just fine:

libvirt.git $ git clean -fxd; ./autogen.sh --system --with-loader-nvram="/usr/share/edk2-ovmf/OVMF_CODE.fd:/usr/share/edk2-ovmf/OVMF_VARS.fd:/usr/share/edk2-ovmf/OVMF_CODE.secboot.fd:/usr/share/edk2-ovmf/OVMF_VARS.secboot.fd"
libvirt.git $ make -j10 all syntax-check check
libvirt.git # ./tools/virsh domcapabilities | grep -i ovmf
      <value>/usr/share/edk2-ovmf/OVMF_CODE.fd</value>
      <value>/usr/share/edk2-ovmf/OVMF_CODE.secboot.fd</value>


Note that only existing paths are reported in domain capabilities XML, so if yours is empty then maybe those files aren't there?

Hi Michal,

The files are definitely there:

$ ls -l /usr/share/ovmf/x64/
total 2048
-rw-r--r-- 1 root root 1966080 Jul  9 02:25 OVMF_CODE.fd
-rw-r--r-- 1 root root  131072 Jul  9 02:25 OVMF_VARS.fd

And just to confirm they have been "compiled in" on my system:

$ strings /usr/lib/libvirt/connection-driver/libvirt_driver_qemu.so | grep -i ovmf
/usr/share/ovmf/x64/OVMF_CODE.fd:/usr/share/ovmf/x64/OVMF_VARS.fd

Yet there is no output from:
$ virsh domcapabilities | grep -i ovmf
(no output)

The only way to get the correct output is to create one of those new JSON descriptor files and add it to /usr/share/qemu/firmware:

$ virsh domcapabilities | grep -i ovmf
      <value>/usr/share/ovmf/x64/OVMF_CODE.fd</value>

Is it possible your test is picking up a JSON file from somewhere on your system?

Bisected to [e9d51a221c1871da246ae8dbc5b5f71191f48be2] qemu: Use FW descriptors to report FW image paths

steps to reproduce:
ls -l /usr/share/ovmf/x64/
 total 2.0M
 -rw-r--r-- 1 root root 1.9M Jul  8 19:25 OVMF_CODE.fd
 -rw-r--r-- 1 root root 128K Jul  8 19:25 OVMF_VARS.fd

cd /var/tmp
git clone https://github.com/libvirt/libvirt.git
cd libvirt
git bisect start
git bisect bad
git checkout v5.6.0
git bisect good

# START
cd /var/tmp/libvirt
git clean -fxd 
mkdir build && cd build 
../autogen.sh --system --with-loader-nvram="/usr/share/ovmf/x64/OVMF_CODE.fd:/usr/share/ovmf/x64/OVMF_VARS.fd" 
make -j12 all syntax-check check 
if ./tools/virsh domcapabilities | grep -i ovmf ; then
 git bisect good
else
git bisect bad
# goto START

System - current Arch Linux, same as @Toolybird.

(In reply to Toolybird from comment #2)
> Is it possible your test is picking up a JSON file from somewhere on your
> system?

Actually, I'm starting to think it's your system which has a JSON file somewhere. Can you please share the libvirtd log? That will help us move forward. The way domcapabilities work is following:

Libvirt tries to fetch JSON files. If a single one was found, it is assumed that sysadmin wants to use JSON descriptors and even if no descriptor fits into domcaps arguments (machine type + arch) success is signalized and compile time / qmeu.conf list of NVRAM pairs is not taken into consideration. This can be seen in the code here:

1) in qemuFirmwareGetSupported() - towars the end:

    if (fws && !*fws && nfirmwares &&
        VIR_REALLOC_N(*fws, 0) < 0)
        return -1;

here, fws != NULL (simply because the function is called this way), *fws can be NULL if no FW descriptor that fits was found in the loop above, nfirmwares matches number of ALL FW descriptor files found

https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_firmware.c;h=be4a10689e0b774268747509a762ea733c57d48a;hb=HEAD#l1402


2) the result above is then propagated into virQEMUCapsFillDomainOSCaps() as can be seen here:

https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_capabilities.c;h=fc6473651c8db5afa89d08cdf09a2134ac98c25e;hb=HEAD#l5213

and since you're not seeing any FW paths in domcaps it means that virQEMUCapsFillDomainLoaderCaps() was passed no paths (or some non-existent ones, which is not the case as you've shown). But this can happen only if firmwaresAlt != NULL and nfirmwaresAlt == 0, which means that the condition from 1) must have evaluated to true which in turn is possible if and only if nfirwmares wasn't 0 which happens if at least single JSON file was found.

What JSON files libvirt found can be seen in the debug log too, for instance this is on my system (up to date Gentoo):

2019-11-10 09:48:36.228+0000: 74880: debug : qemuInteropFetchConfigs:168 : firmware description path '/usr/share/qemu/firmware/50-edk2-i386-secure.json' len=716
2019-11-10 09:48:36.228+0000: 74880: debug : qemuInteropFetchConfigs:168 : firmware description path '/usr/share/qemu/firmware/50-edk2-x86_64-secure.json' len=741
2019-11-10 09:48:36.228+0000: 74880: debug : qemuInteropFetchConfigs:168 : firmware description path '/usr/share/qemu/firmware/60-edk2-aarch64.json' len=622
2019-11-10 09:48:36.228+0000: 74880: debug : qemuInteropFetchConfigs:168 : firmware description path '/usr/share/qemu/firmware/60-edk2-arm.json' len=610
2019-11-10 09:48:36.228+0000: 74880: debug : qemuInteropFetchConfigs:168 : firmware description path '/usr/share/qemu/firmware/60-edk2-i386.json' len=667
2019-11-10 09:48:36.228+0000: 74880: debug : qemuInteropFetchConfigs:168 : firmware description path '/usr/share/qemu/firmware/60-edk2-x86_64.json' len=692


FW descriptors are looked for in /usr/share/qemu/firmware/, /etc/qemu/firmware/ and $HOME/.config/qemu/firmware/. Are you positive there is no JSON file there at all in your system?

(In reply to Michal Privoznik from comment #4)

> FW descriptors are looked for in /usr/share/qemu/firmware/,
> /etc/qemu/firmware/ and $HOME/.config/qemu/firmware/. Are you positive there
> is no JSON file there at all in your system?

Unfortunately, they exists:

ls -l /usr/share/qemu/firmware/
 -rw-r--r-- 1 root root 716 Nov  7 23:11 50-edk2-i386-secure.json
 -rw-r--r-- 1 root root 741 Nov  7 23:11 50-edk2-x86_64-secure.json
 -rw-r--r-- 1 root root 622 Nov  7 23:11 60-edk2-aarch64.json
 -rw-r--r-- 1 root root 610 Nov  7 23:11 60-edk2-arm.json
 -rw-r--r-- 1 root root 667 Nov  7 23:11 60-edk2-i386.json
 -rw-r--r-- 1 root root 692 Nov  7 23:11 60-edk2-x86_64.json

pacman --query --owns /usr/share/qemu/firmware/
/usr/share/qemu/firmware/ is owned by qemu 4.1.0-4

Ah, so they exists, point to a non-existent FW images and thus domcapabilities output is empty which breaks virt-manager/virt-install. Well, this looks like a packaging issue to me then. I mean, if you have some idea what libvirt upstream can do better, I'm happy to discuss it, but if your package manager installs FW descriptors (for what reason btw?) which are invalid then I guess it's also package maintainer's responsibility to fix the paths contained in them (btw: "/usr/share/ovmf/x64/OVMF_CODE.fd" doesn't look correct to me neither).

I won't close this bug just yet, just in case there's something you want done on libvirt upstream part.

(In reply to Michal Privoznik from comment #6)
> Ah, so they exists, point to a non-existent FW images and thus
> domcapabilities output is empty which breaks virt-manager/virt-install.
> Well, this looks like a packaging issue to me then. I mean, if you have some
> idea what libvirt upstream can do better, I'm happy to discuss it, but if
> your package manager installs FW descriptors (for what reason btw?) which
> are invalid then I guess it's also package maintainer's responsibility to
> fix the paths contained in them (btw: "/usr/share/ovmf/x64/OVMF_CODE.fd"
> doesn't look correct to me neither).

But FW descriptors looks valid: 

grep -H filename /usr/share/qemu/firmware/*.json
/usr/share/qemu/firmware/50-edk2-i386-secure.json:            "filename": "/usr/share/qemu/edk2-i386-secure-code.fd",
/usr/share/qemu/firmware/50-edk2-i386-secure.json:            "filename": "/usr/share/qemu/edk2-i386-vars.fd",
/usr/share/qemu/firmware/50-edk2-x86_64-secure.json:            "filename": "/usr/share/qemu/edk2-x86_64-secure-code.fd",
/usr/share/qemu/firmware/50-edk2-x86_64-secure.json:            "filename": "/usr/share/qemu/edk2-i386-vars.fd",
/usr/share/qemu/firmware/60-edk2-aarch64.json:            "filename": "/usr/share/qemu/edk2-aarch64-code.fd",
/usr/share/qemu/firmware/60-edk2-aarch64.json:            "filename": "/usr/share/qemu/edk2-arm-vars.fd",
/usr/share/qemu/firmware/60-edk2-arm.json:            "filename": "/usr/share/qemu/edk2-arm-code.fd",
/usr/share/qemu/firmware/60-edk2-arm.json:            "filename": "/usr/share/qemu/edk2-arm-vars.fd",
/usr/share/qemu/firmware/60-edk2-i386.json:            "filename": "/usr/share/qemu/edk2-i386-code.fd",
/usr/share/qemu/firmware/60-edk2-i386.json:            "filename": "/usr/share/qemu/edk2-i386-vars.fd",
/usr/share/qemu/firmware/60-edk2-x86_64.json:            "filename": "/usr/share/qemu/edk2-x86_64-code.fd",
/usr/share/qemu/firmware/60-edk2-x86_64.json:            "filename": "/usr/share/qemu/edk2-i386-vars.fd",

And those FW images exists:

ls -l /usr/share/qemu/*.fd
-rw-r--r-- 1 root root  64M Nov  7 23:11 /usr/share/qemu/edk2-aarch64-code.fd
-rw-r--r-- 1 root root  64M Nov  7 23:11 /usr/share/qemu/edk2-arm-code.fd
-rw-r--r-- 1 root root  64M Nov  7 23:11 /usr/share/qemu/edk2-arm-vars.fd
-rw-r--r-- 1 root root 3.5M Nov  7 23:11 /usr/share/qemu/edk2-i386-code.fd
-rw-r--r-- 1 root root 3.5M Nov  7 23:11 /usr/share/qemu/edk2-i386-secure-code.fd
-rw-r--r-- 1 root root 528K Nov  7 23:11 /usr/share/qemu/edk2-i386-vars.fd
-rw-r--r-- 1 root root 3.5M Nov  7 23:11 /usr/share/qemu/edk2-x86_64-code.fd
-rw-r--r-- 1 root root 3.5M Nov  7 23:11 /usr/share/qemu/edk2-x86_64-secure-code.fd

/usr/share/ovmf/x64/*.fd FW images belongs to ovmf package from tianocore.org, this FW images are used for passthrough a pci card to VM.
https://wiki.archlinux.org/index.php/PCI_passthrough_via_OVMF.

> I mean, if you have some  idea what libvirt upstream can do better, I'm happy to discuss it.
Maybe libvirt can append/override FW images list from /etc/libvirt/qemu.conf to domcapabilities ?
Anyway, its always  possible drop custom json FW descriptor to /usr/share/qemu/firmware/60-edk2-x86_64-custom.json, whish points to ovmf firmware pair.

> If you have some  idea what libvirt upstream can do better, I'm happy to discuss it.
Another idea came.
Maybe just add a warning to /etc/libvirt/qemu.conf near 'nvram = []' something like this:
"Note: If corresponding FW description files exists in /usr/share/qemu/firmware/, /etc/qemu/firmware/ or $HOME/.config/qemu/firmware/  , this setting will be ignored"
That would give a hint at least why nvram = [...,..,..] is ignored.

(In reply to Michal Privoznik from comment #4)

> Libvirt tries to fetch JSON files. If a single one was found, it is assumed
> that sysadmin wants to use JSON descriptors

This strikes me as a fairly bold assumption and not very intuitive. I also couldn't find it documented anywhere.

The "sysadmin" in this case is simply the qemu `make install' which defaults to installing the blobs and JSON files. (sidenote: I don't understand why qemu ships pre-built blobs at all).


In conclusion, Arch needs to do either (or both) of the following:

1. follow Fedora (from the rpm spec):

# Provided by package edk2
rm -rf %{buildroot}%{_datadir}/%{name}/edk2*
rm -rf %{buildroot}%{_datadir}/%{name}/firmware/*edk2*.json

2. ship an appropriate FW descriptor with the OVMF package.


Agree with @edacval that a note in qemu.conf would be useful.

Anyway, if libvirt is working as designed then all good. Thank you Michal for looking into it and providing detailed explanations.

(In reply to edacval from comment #7)
> (In reply to Michal Privoznik from comment #6)
> > Ah, so they exists, point to a non-existent FW images and thus
> > domcapabilities output is empty which breaks virt-manager/virt-install.
> > Well, this looks like a packaging issue to me then. I mean, if you have some
> > idea what libvirt upstream can do better, I'm happy to discuss it, but if
> > your package manager installs FW descriptors (for what reason btw?) which
> > are invalid then I guess it's also package maintainer's responsibility to
> > fix the paths contained in them (btw: "/usr/share/ovmf/x64/OVMF_CODE.fd"
> > doesn't look correct to me neither).
> 
> But FW descriptors looks valid: 
> 
> grep -H filename /usr/share/qemu/firmware/*.json
> /usr/share/qemu/firmware/50-edk2-i386-secure.json:            "filename":
> "/usr/share/qemu/edk2-i386-secure-code.fd",
> /usr/share/qemu/firmware/50-edk2-i386-secure.json:            "filename":
> "/usr/share/qemu/edk2-i386-vars.fd",
> /usr/share/qemu/firmware/50-edk2-x86_64-secure.json:            "filename":
> "/usr/share/qemu/edk2-x86_64-secure-code.fd",
> /usr/share/qemu/firmware/50-edk2-x86_64-secure.json:            "filename":
> "/usr/share/qemu/edk2-i386-vars.fd",
> /usr/share/qemu/firmware/60-edk2-aarch64.json:            "filename":
> "/usr/share/qemu/edk2-aarch64-code.fd",
> /usr/share/qemu/firmware/60-edk2-aarch64.json:            "filename":
> "/usr/share/qemu/edk2-arm-vars.fd",
> /usr/share/qemu/firmware/60-edk2-arm.json:            "filename":
> "/usr/share/qemu/edk2-arm-code.fd",
> /usr/share/qemu/firmware/60-edk2-arm.json:            "filename":
> "/usr/share/qemu/edk2-arm-vars.fd",
> /usr/share/qemu/firmware/60-edk2-i386.json:            "filename":
> "/usr/share/qemu/edk2-i386-code.fd",
> /usr/share/qemu/firmware/60-edk2-i386.json:            "filename":
> "/usr/share/qemu/edk2-i386-vars.fd",
> /usr/share/qemu/firmware/60-edk2-x86_64.json:            "filename":
> "/usr/share/qemu/edk2-x86_64-code.fd",
> /usr/share/qemu/firmware/60-edk2-x86_64.json:            "filename":
> "/usr/share/qemu/edk2-i386-vars.fd",
> 
> And those FW images exists:
> 
> ls -l /usr/share/qemu/*.fd
> -rw-r--r-- 1 root root  64M Nov  7 23:11 /usr/share/qemu/edk2-aarch64-code.fd
> -rw-r--r-- 1 root root  64M Nov  7 23:11 /usr/share/qemu/edk2-arm-code.fd
> -rw-r--r-- 1 root root  64M Nov  7 23:11 /usr/share/qemu/edk2-arm-vars.fd
> -rw-r--r-- 1 root root 3.5M Nov  7 23:11 /usr/share/qemu/edk2-i386-code.fd
> -rw-r--r-- 1 root root 3.5M Nov  7 23:11
> /usr/share/qemu/edk2-i386-secure-code.fd
> -rw-r--r-- 1 root root 528K Nov  7 23:11 /usr/share/qemu/edk2-i386-vars.fd
> -rw-r--r-- 1 root root 3.5M Nov  7 23:11 /usr/share/qemu/edk2-x86_64-code.fd
> -rw-r--r-- 1 root root 3.5M Nov  7 23:11
> /usr/share/qemu/edk2-x86_64-secure-code.fd
> 
> /usr/share/ovmf/x64/*.fd FW images belongs to ovmf package from
> tianocore.org, this FW images are used for passthrough a pci card to VM.
> https://wiki.archlinux.org/index.php/PCI_passthrough_via_OVMF.

Not necessarily, but more important question is, since you have some FW descriptors and also FW images, how come neither is printed in domain capabilities? Can you please share debug log?

> 
> > I mean, if you have some  idea what libvirt upstream can do better, I'm happy to discuss it.
> Maybe libvirt can append/override FW images list from /etc/libvirt/qemu.conf
> to domcapabilities ?

The idea is that users won't have to deal with paths at all. Nor sysadmins. I mean, FW descriptors are superior to nvram=[] setting and therefore when I wrote the code fore FW descriptors I made nvram=[] obsolete. A FW descriptor can live in three locations and this is so that each role can have their own, exclusive place where to store them / write new ones: /usr/share/qemu/firmware/ for packagers, /etc/qemu/firmware/ for sysadmins, and finally $HOME/.config/qemu/firmware for users (note that from this POV running a domain under system wide connection qemu:///system falls into sysadmin role).
Anyway, individuals from whatever role can write their own FW descriptors (and while I agree it's not user friendly, it offers the same functionality as nvram=[] plus more).

> Anyway, its always  possible drop custom json FW descriptor to
> /usr/share/qemu/firmware/60-edk2-x86_64-custom.json, whish points to ovmf
> firmware pair.

I still don't quite understand why you don't want to move to newer approach.

(In reply to edacval from comment #8)
> > If you have some  idea what libvirt upstream can do better, I'm happy to discuss it.
> Another idea came.
> Maybe just add a warning to /etc/libvirt/qemu.conf near 'nvram = []'
> something like this:
> "Note: If corresponding FW description files exists in
> /usr/share/qemu/firmware/, /etc/qemu/firmware/ or
> $HOME/.config/qemu/firmware/  , this setting will be ignored"
> That would give a hint at least why nvram = [...,..,..] is ignored.

Well, current formulation is this:

# This configuration option is obsolete. Libvirt will follow the
# QEMU firmware metadata specification to automatically locate
# firmware images. See docs/interop/firmware.json in the QEMU
# source tree. These metadata files are distributed alongside any
# firmware images intended for use with QEMU.

(introduced by v5.3.0~1)

which I think says the same thing? Or maybe it doesn't for everybody, and it does so only to me, since I wrote it.


(In reply to Toolybird from comment #9)
> (In reply to Michal Privoznik from comment #4)
> 
> > Libvirt tries to fetch JSON files. If a single one was found, it is assumed
> > that sysadmin wants to use JSON descriptors
> 
> This strikes me as a fairly bold assumption and not very intuitive. I also
> couldn't find it documented anywhere.

As I'm saying a few lines above, qemu.conf comment says it. At least, in my sight.

> 
> The "sysadmin" in this case is simply the qemu `make install' which defaults
> to installing the blobs and JSON files. (sidenote: I don't understand why
> qemu ships pre-built blobs at all).
> 
> 
> In conclusion, Arch needs to do either (or both) of the following:
> 
> 1. follow Fedora (from the rpm spec):
> 
> # Provided by package edk2
> rm -rf %{buildroot}%{_datadir}/%{name}/edk2*
> rm -rf %{buildroot}%{_datadir}/%{name}/firmware/*edk2*.json
> 
> 2. ship an appropriate FW descriptor with the OVMF package.

I think the best would be to go with 2) and ship FW descriptors (which seems like you're already doing by mistake) and let libvirt chose the best FW for domains automatically. No user intervention needed.

> 
> 
> Agree with @edacval that a note in qemu.conf would be useful.
> 
> Anyway, if libvirt is working as designed then all good. Thank you Michal
> for looking into it and providing detailed explanations.

No problem.

(In reply to Michal Privoznik from comment #10)
> (In reply to edacval from comment #7)
> Well, current formulation is this:
> 
> # This configuration option is obsolete. Libvirt will follow the
> # QEMU firmware metadata specification to automatically locate
> # firmware images. See docs/interop/firmware.json in the QEMU
> # source tree. These metadata files are distributed alongside any
> # firmware images intended for use with QEMU.
> 
> (introduced by v5.3.0~1)
> 
> which I think says the same thing? Or maybe it doesn't for everybody, and it
> does so only to me, since I wrote it.

Saying it is obsolete is probably insufficient - it makes it clear that it is not the modern best practice, but it doesn't tell users it will be ignored if FW descriptors are present.

Perhaps we should extend it a little to say


 # This configuration option is obsolete.
 # 
 # Libvirt will follow the QEMU firmware metadata specification to 
 # automatically locate firmware images. See docs/interop/firmware.json
 # in the QEMU source tree. These metadata files are distributed alongside
 # any firmware images intended for use with QEMU
 #
 # NOTE: if ANY firmware metadata files are detected, this setting
 # will be COMPLETELY IGNORED.

Perhaps we could also make libvirtd warn at startup if it finds 'nvram' has been explicitly set by the admin, and FW metadata files are also found ?

> > In conclusion, Arch needs to do either (or both) of the following:
> > 
> > 1. follow Fedora (from the rpm spec):
> > 
> > # Provided by package edk2
> > rm -rf %{buildroot}%{_datadir}/%{name}/edk2*
> > rm -rf %{buildroot}%{_datadir}/%{name}/firmware/*edk2*.json
> > 
> > 2. ship an appropriate FW descriptor with the OVMF package.
> 
> I think the best would be to go with 2) and ship FW descriptors (which seems
> like you're already doing by mistake) and let libvirt chose the best FW for
> domains automatically. No user intervention needed.

Note that it is reasonably likely that libvirt will delete support for the qemu.conf nvrams setting at some point, such that we only support the FW descriptors long term.

(In reply to Toolybird from comment #9)

> The "sysadmin" in this case is simply the qemu `make install' which
> defaults to installing the blobs and JSON files. (sidenote: I don't
> understand why qemu ships pre-built blobs at all).

User / developer convenience. Some people want to use, or hack on, QEMU,
and launch guests, without having to build guest firmware themselves.
This applies to a lot more than just edk2 binaries (in fact edk2
binaries were bundled with QEMU way later than everything else in the
same category).

If you don't want "make install" to install firmware blobs, run
"configure" with "--disable-blobs".

(In reply to Daniel Berrangé from comment #11)
> (In reply to Michal Privoznik from comment #10)
> > (In reply to edacval from comment #7)
> > Well, current formulation is this:
> >
> > # This configuration option is obsolete. Libvirt will follow the
> > # QEMU firmware metadata specification to automatically locate
> > # firmware images. See docs/interop/firmware.json in the QEMU
> > # source tree. These metadata files are distributed alongside any
> > # firmware images intended for use with QEMU.
> >
> > (introduced by v5.3.0~1)
> >
> > which I think says the same thing? Or maybe it doesn't for
> > everybody, and it does so only to me, since I wrote it.
>
> Saying it is obsolete is probably insufficient - it makes it clear
> that it is not the modern best practice, but it doesn't tell users it
> will be ignored if FW descriptors are present.
>
> Perhaps we should extend it a little to say
>
>
>  # This configuration option is obsolete.
>  #
>  # Libvirt will follow the QEMU firmware metadata specification to
>  # automatically locate firmware images. See docs/interop/firmware.json
>  # in the QEMU source tree. These metadata files are distributed alongside
>  # any firmware images intended for use with QEMU
>  #
>  # NOTE: if ANY firmware metadata files are detected, this setting
>  # will be COMPLETELY IGNORED.

Personally I'd appreciate this highly -- I've been wondering myself how
both configuration "routes" combine into one. The information given by
Michal in comment 4 is new to me as well.

It's the right approach, but "obsolete" doesn't quite explain it.
(Obsolete can stand for both "don't do this anymore, but if you do,
we'll still adhere to it", and "don't do this anymore, and if you do, we
will ignore it".)

Thanks!

Patch proposed upstream:

https://www.redhat.com/archives/libvir-list/2019-November/msg00351.html

Pushed upstream:

75597f022a qemu: Warn verbosely if using old loader:nvram pairs

v5.9.0-65-g75597f022a