February 2022 - Devel - libvirt List Archives

[PATCH v2] docs: expand firmware descriptor to allow flash without NVRAM
by Daniel P. Berrangé 01 Feb '22

01 Feb '22

The current firmware descriptor schema for flash requires that both the executable to NVRAM template paths be provided. This is fine for the most common usage of EDK2 builds in virtualization where the separate _CODE and _VARS files are provided. With confidential computing technology like AMD SEV, persistent storage of variables may be completely disabled because the firmware requires a known clean state on every cold boot. There is no way to express this in the firmware descriptor today. Even with regular EDK2 builds it is possible to create a firmware that has both executable code and variable persistence in a single file. This hasn't been commonly used, since it would mean every guest bootup would need to clone the full firmware file, leading to redundant duplicate storage of the code portion. In some scenarios this may not matter and might even be beneficial. For example if a public cloud allows users to bring their own firmware, such that the user can pre-enroll their own secure boot keys, you're going to have this copied on disk for each tenant already. At this point the it can be simpler to just deal with a single file rather than split builds. The firmware descriptor ought to be able to express this combined firmware model too. This all points towards expanding the schema for flash with a 'mode' concept: - "split" - the current implicit behaviour with separate files for code and variables. - "combined" - the alternate behaviour where a single file contains both code and variables. - "stateless" - the confidential computing use case where storage of variables is completely disable, leaving only the code. Reviewed-by: Philippe Mathieu-Daudé <f4bug(a)amsat.org> Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- docs/interop/firmware.json | 54 ++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 8 deletions(-) In v2: - Mark 'mode' as optional field - Misc typos in docs diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json index 8d8b0be030..f5d1d0b6e7 100644 --- a/docs/interop/firmware.json +++ b/docs/interop/firmware.json @@ -210,24 +210,61 @@ 'data' : { 'filename' : 'str', 'format' : 'BlockdevDriver' } } + +## +# @FirmwareFlashType: +# +# Describes how the firmware build handles code versus variable +# persistence. +# +# @split: the executable file contains code while the NVRAM +# template provides variable storage. The executable +# must be configured read-only and can be shared between +# multiple guests. The NVRAM template must be cloned +# for each new guest and configured read-write. +# +# @combined: the executable file contains both code and +# variable storage. The executable must be cloned +# for each new guest and configured read-write. +# No NVRAM template will be specified. +# +# @stateless: the executable file contains code and variable +# storage is not persisted. The executed must +# be configured read-only and can be shared +# between multiple guests. No NVRAM template +# will be specified. +# +# Since: 7.0.0 +## +{ 'enum': 'FirmwareFlashMode', + 'data': [ 'split', 'combined', 'stateless' ] } + ## # @FirmwareMappingFlash: # # Describes loading and mapping properties for the firmware executable # and its accompanying NVRAM file, when @FirmwareDevice is @flash. # -# @executable: Identifies the firmware executable. The firmware -# executable may be shared by multiple virtual machine -# definitions. The preferred corresponding QEMU command -# line options are +# @mode: describes how the firmware build handles code versus variable +# storage. If not present, it must be treated as if it was +# configured with value ``split``. Since: 7.0.0 +# +# @executable: Identifies the firmware executable. The @mode +# indicates whether there will be an associated +# NVRAM template present. The preferred +# corresponding QEMU command line options are # -drive if=none,id=pflash0,readonly=on,file=@executable.@filename,format=@executable.@format # -machine pflash0=pflash0 -# or equivalent -blockdev instead of -drive. +# or equivalent -blockdev instead of -drive. When +# @mode is ``combined`` the executable must be +# cloned before use and configured with readonly=off. # With QEMU versions older than 4.0, you have to use # -drive if=pflash,unit=0,readonly=on,file=@executable.@filename,format=@executable.@format # # @nvram-template: Identifies the NVRAM template compatible with -# @executable. Management software instantiates an +# @executable, when @mode is set to ``split``, +# otherwise it should not be present. +# Management software instantiates an # individual copy -- a specific NVRAM file -- from # @nvram-template.@filename for each new virtual # machine definition created. @nvram-template.@filename @@ -246,8 +283,9 @@ # Since: 3.0 ## { 'struct' : 'FirmwareMappingFlash', - 'data' : { 'executable' : 'FirmwareFlashFile', - 'nvram-template' : 'FirmwareFlashFile' } } + 'data' : { '*mode': 'FirmwareFlashMode', + 'executable' : 'FirmwareFlashFile', + '*nvram-template' : 'FirmwareFlashFile' } } ## # @FirmwareMappingKernel: -- 2.34.1

3 6

[PATCH] virnwfilterobj: Unlock virNWFilterObj before triggering rebuild
by Michal Privoznik 01 Feb '22

01 Feb '22

When updating am NWFilter, rebuilding of all NWFilters is triggered. This happens in virNWFilterObjListAssignDef() by calling virNWFilterTriggerRebuild(). Now consider another thread, that's currently executing virNWFilterBindingCreateXML() over the same NWFilter. What happens is that this second thread gets all the way into virNWFilterInstantiateFilterInternal(), acquires @updateMutex and transitively calls virNWFilterObjListFindByName() which iterates over list of NWFilters, locking one by one, comparing names trying to find the desired one. Sooner or later it will try to lock the object that the other, original thread is redefining and which it holds locked. Now that thread can't continue either because it's waiting for the @updateMutex lock. So we end up in a typical deadlock situation, one thread holding lock A trying to acquire lock B, the other thread holding B and trying to acquire A. The solution is to unlock the virNWFilterObj in the first thread, just before triggering rebuild. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2044379 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- I'm not sure this is 100% correct, but hey - it make that particular bug go away. src/conf/virnwfilterobj.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 6bbdf6e6fa..d6c2e59ce8 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -352,12 +352,17 @@ virNWFilterObjListAssignDef(virNWFilterObjList *nwfilters, } obj->newDef = def; - /* trigger the update on VMs referencing the filter */ + + /* Trigger the update on VMs referencing the filter, but + * unlock the filter because rebuild will lock it again. */ + virNWFilterObjUnlock(obj); if (virNWFilterTriggerRebuild() < 0) { + virNWFilterObjLock(obj); obj->newDef = NULL; virNWFilterObjUnlock(obj); return NULL; } + virNWFilterObjLock(obj); virNWFilterDefFree(objdef); obj->def = def; -- 2.34.1

2 1

[PATCH] systemd: Use correct man page name in modular daemon service files
by Peter Krempa 01 Feb '22

01 Feb '22

The service files were copied out of the service file for libvirtd and the name of the corresponding manpage was not fixed. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2045959 Signed-off-by: Peter Krempa <pkrempa(a)redhat.com> --- src/ch/virtchd.service.in | 2 +- src/interface/virtinterfaced.service.in | 2 +- src/libxl/virtxend.service.in | 2 +- src/lxc/virtlxcd.service.in | 2 +- src/network/virtnetworkd.service.in | 2 +- src/node_device/virtnodedevd.service.in | 2 +- src/nwfilter/virtnwfilterd.service.in | 2 +- src/qemu/virtqemud.service.in | 2 +- src/remote/virtproxyd.service.in | 2 +- src/secret/virtsecretd.service.in | 2 +- src/storage/virtstoraged.service.in | 2 +- src/vbox/virtvboxd.service.in | 2 +- src/vz/virtvzd.service.in | 2 +- 13 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/ch/virtchd.service.in b/src/ch/virtchd.service.in index f08339f211..f53a12ea05 100644 --- a/src/ch/virtchd.service.in +++ b/src/ch/virtchd.service.in @@ -13,7 +13,7 @@ After=local-fs.target After=remote-fs.target After=systemd-logind.service After=systemd-machined.service -Documentation=man:libvirtd(8) +Documentation=man:virtchd(8) Documentation=https://libvirt.org [Service] diff --git a/src/interface/virtinterfaced.service.in b/src/interface/virtinterfaced.service.in index 3d944e17a9..cb860ff1c4 100644 --- a/src/interface/virtinterfaced.service.in +++ b/src/interface/virtinterfaced.service.in @@ -8,7 +8,7 @@ After=network.target After=dbus.service After=apparmor.service After=local-fs.target -Documentation=man:libvirtd(8) +Documentation=man:virtinterfaced(8) Documentation=https://libvirt.org [Service] diff --git a/src/libxl/virtxend.service.in b/src/libxl/virtxend.service.in index 2b5163e179..6b083c414f 100644 --- a/src/libxl/virtxend.service.in +++ b/src/libxl/virtxend.service.in @@ -12,7 +12,7 @@ After=local-fs.target After=remote-fs.target After=xencommons.service Conflicts=xendomains.service -Documentation=man:libvirtd(8) +Documentation=man:virtxend(8) Documentation=https://libvirt.org ConditionPathExists=/proc/xen/capabilities diff --git a/src/lxc/virtlxcd.service.in b/src/lxc/virtlxcd.service.in index d58bde9f5d..334c34db44 100644 --- a/src/lxc/virtlxcd.service.in +++ b/src/lxc/virtlxcd.service.in @@ -13,7 +13,7 @@ After=local-fs.target After=remote-fs.target After=systemd-logind.service After=systemd-machined.service -Documentation=man:libvirtd(8) +Documentation=man:virtlxcd(8) Documentation=https://libvirt.org [Service] diff --git a/src/network/virtnetworkd.service.in b/src/network/virtnetworkd.service.in index 3decfbbf1d..05ce672b73 100644 --- a/src/network/virtnetworkd.service.in +++ b/src/network/virtnetworkd.service.in @@ -11,7 +11,7 @@ After=ip6tables.service After=dbus.service After=apparmor.service After=local-fs.target -Documentation=man:libvirtd(8) +Documentation=man:virtnetworkd(8) Documentation=https://libvirt.org [Service] diff --git a/src/node_device/virtnodedevd.service.in b/src/node_device/virtnodedevd.service.in index 688cf89822..cd9de362fd 100644 --- a/src/node_device/virtnodedevd.service.in +++ b/src/node_device/virtnodedevd.service.in @@ -8,7 +8,7 @@ After=network.target After=dbus.service After=apparmor.service After=local-fs.target -Documentation=man:libvirtd(8) +Documentation=man:virtnodedevd(8) Documentation=https://libvirt.org [Service] diff --git a/src/nwfilter/virtnwfilterd.service.in b/src/nwfilter/virtnwfilterd.service.in index 36d00b58f0..ab65419e0c 100644 --- a/src/nwfilter/virtnwfilterd.service.in +++ b/src/nwfilter/virtnwfilterd.service.in @@ -8,7 +8,7 @@ After=network.target After=dbus.service After=apparmor.service After=local-fs.target -Documentation=man:libvirtd(8) +Documentation=man:virtnwfilterd(8) Documentation=https://libvirt.org [Service] diff --git a/src/qemu/virtqemud.service.in b/src/qemu/virtqemud.service.in index 551eb4d405..5ad968ace9 100644 --- a/src/qemu/virtqemud.service.in +++ b/src/qemu/virtqemud.service.in @@ -15,7 +15,7 @@ After=local-fs.target After=remote-fs.target After=systemd-logind.service After=systemd-machined.service -Documentation=man:libvirtd(8) +Documentation=man:virtqemud(8) Documentation=https://libvirt.org [Service] diff --git a/src/remote/virtproxyd.service.in b/src/remote/virtproxyd.service.in index 10e8cf7263..f9bb6b84a9 100644 --- a/src/remote/virtproxyd.service.in +++ b/src/remote/virtproxyd.service.in @@ -8,7 +8,7 @@ After=network.target After=dbus.service After=apparmor.service After=local-fs.target -Documentation=man:libvirtd(8) +Documentation=man:virtproxyd(8) Documentation=https://libvirt.org [Service] diff --git a/src/secret/virtsecretd.service.in b/src/secret/virtsecretd.service.in index cbd63fe0b2..6d298c5334 100644 --- a/src/secret/virtsecretd.service.in +++ b/src/secret/virtsecretd.service.in @@ -8,7 +8,7 @@ After=network.target After=dbus.service After=apparmor.service After=local-fs.target -Documentation=man:libvirtd(8) +Documentation=man:virtsecretd(8) Documentation=https://libvirt.org [Service] diff --git a/src/storage/virtstoraged.service.in b/src/storage/virtstoraged.service.in index f72f8426fd..eda4d86d37 100644 --- a/src/storage/virtstoraged.service.in +++ b/src/storage/virtstoraged.service.in @@ -10,7 +10,7 @@ After=iscsid.service After=apparmor.service After=local-fs.target After=remote-fs.target -Documentation=man:libvirtd(8) +Documentation=man:virtstoraged(8) Documentation=https://libvirt.org [Service] diff --git a/src/vbox/virtvboxd.service.in b/src/vbox/virtvboxd.service.in index cfdafc39d2..6f447276e9 100644 --- a/src/vbox/virtvboxd.service.in +++ b/src/vbox/virtvboxd.service.in @@ -9,7 +9,7 @@ After=dbus.service After=apparmor.service After=local-fs.target After=remote-fs.target -Documentation=man:libvirtd(8) +Documentation=man:virtvboxd(8) Documentation=https://libvirt.org [Service] diff --git a/src/vz/virtvzd.service.in b/src/vz/virtvzd.service.in index 7636bf2b9e..2b1165c92b 100644 --- a/src/vz/virtvzd.service.in +++ b/src/vz/virtvzd.service.in @@ -9,7 +9,7 @@ After=dbus.service After=apparmor.service After=local-fs.target After=remote-fs.target -Documentation=man:libvirtd(8) +Documentation=man:virtvzd(8) Documentation=https://libvirt.org [Service] -- 2.34.1

2 1

[PATCH] virnwfilterbindingobj: Fix virNWFilterBindingObjNew()
by Michal Privoznik 01 Feb '22

01 Feb '22

The idea behind virNWFilterBindingObjNew() is to create and return an object of virNWFilterBindingObjClass class. The class is virObjectLockable (and the corresponding _virNWFilterBindingObj structure has virObjectLockable parent). But for some reason plain virObjectNew() is called. This is wrong because the mutex in the parent is left uninitialized. Next, the returned object is not locked. This is wrong because in some cases the returned object is added onto a list of bindings and then passed to virNWFilterBindingObjEndAPI() which unlocks it right away. This is potentially dangerous because we might just have unlocked the object for another thread. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/conf/virnwfilterbindingobj.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/conf/virnwfilterbindingobj.c b/src/conf/virnwfilterbindingobj.c index acea240b5d..d387af68c0 100644 --- a/src/conf/virnwfilterbindingobj.c +++ b/src/conf/virnwfilterbindingobj.c @@ -57,10 +57,15 @@ VIR_ONCE_GLOBAL_INIT(virNWFilterBindingObj); virNWFilterBindingObj * virNWFilterBindingObjNew(void) { + virNWFilterBindingObj *ret; if (virNWFilterBindingObjInitialize() < 0) return NULL; - return virObjectNew(virNWFilterBindingObjClass); + if (!(ret = virObjectLockableNew(virNWFilterBindingObjClass))) + return NULL; + + virObjectLock(ret); + return ret; } -- 2.34.1

2 1

Re: [PATCH v3 1/2] hw/i386: Attach CPUs to machine
by Daniel P. Berrangé 01 Feb '22

01 Feb '22

Cc libvir-list since this will (intentionally) break compatibility with current libvirt code that looks for "/machine/unattached/device[0]" in the assumption it is the first CPU. On Tue, Feb 01, 2022 at 12:35:06AM +0100, Philippe Mathieu-Daudé wrote: > Previously CPUs were exposed in the QOM tree at a path > > /machine/unattached/device[nn] > > where the 'nn' of the first CPU is usually zero, but can > vary depending on what devices were already created. > > With this change the CPUs are now at > > /machine/cpu[nn] > > where the 'nn' of the first CPU is always zero. > > Suggested-by: Daniel P. Berrangé <berrange(a)redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug(a)amsat.org> > --- > hw/i386/x86.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com> > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index b84840a1bb9..50bf249c700 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -108,6 +108,7 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp) > { > Object *cpu = object_new(MACHINE(x86ms)->cpu_type); > > + object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu)); > if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) { > goto out; > } > -- > 2.34.1 > 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 :|

1 0