Re: [PATCH 08/13] ci: Drop Ubuntu 20.04
On Tue, May 07, 2024 at 01:58:07PM GMT, Daniel P. Berrangé wrote: > On Tue, May 07, 2024 at 02:37:05PM +0200, Michal Prívozník wrote: > > On 5/7/24 12:11, Daniel P. Berrangé wrote: > > > FYI, I'd really *not* splitting out the removal and addition into > > > separate jobs. If you remove 20.04 and add 24.04 in the same > > > commit, then git shows the rename and we get a tiny diff so we > > > can see the interesting changes. > > > > Yeah, and that's how I've started. But then I realized I needed to bump > > glib version and Ubuntu 20.04 doesn't have it, but without the bump > > Ubuntu 24.04 build fails. But maybe Fedora and AlmaLinux can be done > > this way. > > I'd suggest just temporarily disabling -Werror warnings, or disabling > UBSAN. That way you can > > * squash the forthcoming warning > * update all the distros new/old in one go > * update glib & re-enable the warning Is this dance really necessary? We obviously care about bisectability of the code itself, but IMO it's fine if the CI pipeline stops making sense for a bit in the middle of a series, as long as things are once again working by the end of it. -- Andrea Bolognani / Red Hat / Virtualization ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 30/35] virshtest: Adapt 'virsh-read-bufsiz' and 'virsh-read-non-seekable'
On Fri, Mar 22, 2024 at 06:56:08PM GMT, Peter Krempa wrote:
> +static void
> +testPipeFeeder(void *opaque)
> +{
> +/* feed more than observed buffer size which was historically 128k in the
> + * test this was adapted from */
> +size_t emptyspace = 140 * 1024;
This test seems to fail consistently at least on ppc64le, among other
less common architectures. This can be seen both in Debian[1] and
Fedora[2]. It runs for a while, then it hits the timeout gets
terminated by meson.
I've reproduced it locally and this is the output:
# LIBVIRT_DEBUG=1 VIR_TEST_DEBUG=1 VIR_TEST_RANGE=71 ./tests/virshtest
ninja: no work to do.
TEST: virshtest
71) read-big-pipe
... 2024-05-07 16:43:17.099+: 69735: info : libvirt version:
10.4.0
2024-05-07 16:43:17.099+: 69735: debug : virThreadJobSet:96 :
Thread 69735 is now running job testPipeFeeder
2024-05-07 16:43:17.099+: 69734: debug : virCommandRunAsync:2657
: About to run LANG=C /root/libvirt/build/tools/virsh --connect
test:///default 'define /tmp/libvirt_virshtest_XUTXGN2/pipe ; list
--all'
2024-05-07 16:43:17.099+: 69735: debug : virThreadJobClear:121 :
Thread 69735 finished job testPipeFeeder with ret=0
2024-05-07 16:43:17.099+: 69734: debug : virCommandRunAsync:2659
: Command result 0, with PID 69736
I've bumped the size of emptyspace to 1024*1024 and that causes the
test to pass. 1023*1024 doesn't. Could it be something about the
fifo's capacity being different across architectures?
[1]
https://buildd.debian.org/status/fetch.php?pkg=libvirt&arch=ppc64el&ver=10.3.0-2&stamp=1715074703&raw=0
[2] https://koji.fedoraproject.org/koji/taskinfo?taskID=117156020
--
Andrea Bolognani / Red Hat / Virtualization
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 30/35] virshtest: Adapt 'virsh-read-bufsiz' and 'virsh-read-non-seekable'
On Tue, May 07, 2024 at 08:14:18PM GMT, Daniel P. Berrangé wrote:
> On Tue, May 07, 2024 at 04:56:00PM +0000, Andrea Bolognani wrote:
> > On Fri, Mar 22, 2024 at 06:56:08PM GMT, Peter Krempa wrote:
> > > +static void
> > > +testPipeFeeder(void *opaque)
> > > +{
> > > +/* feed more than observed buffer size which was historically 128k
> > > in the
> > > + * test this was adapted from */
> > > +size_t emptyspace = 140 * 1024;
> >
> > This test seems to fail consistently at least on ppc64le, among other
> > less common architectures. This can be seen both in Debian[1] and
> > Fedora[2]. It runs for a while, then it hits the timeout gets
> > terminated by meson.
> >
> > I've reproduced it locally and this is the output:
> >
> > # LIBVIRT_DEBUG=1 VIR_TEST_DEBUG=1 VIR_TEST_RANGE=71 ./tests/virshtest
> > ninja: no work to do.
> > TEST: virshtest
> > 71) read-big-pipe
> > ... 2024-05-07 16:43:17.099+: 69735: info : libvirt version:
> > 10.4.0
> > 2024-05-07 16:43:17.099+: 69735: debug : virThreadJobSet:96 :
> > Thread 69735 is now running job testPipeFeeder
> > 2024-05-07 16:43:17.099+: 69734: debug : virCommandRunAsync:2657
> > : About to run LANG=C /root/libvirt/build/tools/virsh --connect
> > test:///default 'define /tmp/libvirt_virshtest_XUTXGN2/pipe ; list
> > --all'
> > 2024-05-07 16:43:17.099+: 69735: debug : virThreadJobClear:121 :
> > Thread 69735 finished job testPipeFeeder with ret=0
> > 2024-05-07 16:43:17.099+: 69734: debug : virCommandRunAsync:2659
> > : Command result 0, with PID 69736
> >
> > I've bumped the size of emptyspace to 1024*1024 and that causes the
> > test to pass. 1023*1024 doesn't. Could it be something about the
> > fifo's capacity being different across architectures?
>
> I had multiple builds fail in Fedora, but today a ppc64
> build magically passed. So even on ppc64 it is racy :-(
>
> The virFileReadAll method reads in BUFSIZ chunks but that
> hasn't changed in years. The new test is pretty trivial,
> and I struggle to see why changing the buffer size would
> affect it, given the old test this replaced did largely
> the same thing.
Maybe the shell writes to the fifo differently than libvirt does?
Also "read XML from a fifo" and "read a really big XML file" were two
separate test cases before. Some unexpected interaction between
buffer size and fifo handling, perhaps?
--
Andrea Bolognani / Red Hat / Virtualization
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 1/2] tests: fix hang in virshtest 'read-big-pipe' case
On Wed, May 08, 2024 at 01:11:30PM GMT, Daniel P. Berrangé wrote: > * On ppc64, testPipeFeeder opens R+W, tries to write 140kb >and write() succeeds because the larger 64kb page size >resulted in greater buffer capacity for the pipe. It thus >quickly closes the pipe, removing the writer, and triggering >discard of all the unread data. Now virsh starts up, tries >to open the pipe for O_RDONLY and blocks waiting for a new >writer to open it, which will never happen. Meson kills it >the test after 30 seconds. s/kills it/kills/ Thanks a lot for looking into this! Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 2/2] tests: fix two off-by-1 errors in read-big-pipe test
On Wed, May 08, 2024 at 01:11:31PM GMT, Daniel P. Berrangé wrote: > When testPipeFeeder copies the XML document into the padded buffer, it > tells virStrcpy that 'xmlsize' bytes are available. This is under > reporting size by 1 byte, and as a result it fails to copy the trailing > '\n' replacing it when '\0'. The return value of virStrcpy wasn't *replacing it with > checked, but was reporting this truncation. > > When testPipeFeeder then sends the padded buffer down the pipe, it askes *asks > to send 'emptyspace + xmlsize + 1' bytes, which means it sends the data, > as well as the trailing '\0' terminator. > > Both bugs combined mean it is sending '\0\0' as the last bytes, instead > of '\n' which was intended. When virFileReadAll reads data from the > pipe, it ends up adding another '\0' resulting in in a very NUL *resulting in a very Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v3 2/5] qemu: Introduce shared_filesystems configuration option
On Thu, May 09, 2024 at 01:58:21PM GMT, Peter Krempa wrote: > On Thu, May 02, 2024 at 19:39:39 +0200, Andrea Bolognani wrote: > > +# libvirt will normally prevent migration if the storage backing the VM is > > not > > +# on a shared filesystems. Sometimes, however, the storage *is* shared > > despite > > +# not being detected as such: for example, this is the case when one of the > > +# hosts involved in the migration is exporting its local storage to the > > other > > +# one via NFS. > > I wanted to suggest using VIR_MIGRATE_UNSAFE flag to bypass this check, > but doing so without disabling dynamic ownership on the image itself > would still make the security driver cut off access to this file. This was suggested in the past, but the argument against it was that it's too vague: we want to skip the check on whether the storage is shared, and that one only, while "unsafe migration" could potentially cover a lot of unrelated scenarios. The other argument against using a migration flag was that it's an all-or-nothing proposition: either you perform the check for all paths, or you skip it for all paths. A configuration option allows us more granular control, where we can designate exactly which paths should not be subject to the check, retaining the default behavior for all the other ones. > > +# Any directory listed here will be assumed to live on a shared filesystem, > > +# making migration possible in scenarios such as the one described above. > > +# > > +# Due to how label remembering is implemented, it will generally be > > necessary > > +# to set remember_owner=0 *on both sides of the migration* as well. > > So IIUC the problem here is that on the box where the storage is local > this will create the XATTR entries used for remembering the lablels > (which also includes a refcount), which given that older NFS doesn't > support security labelling would then be leaked? Yeah, the expectation when remembering is enabled is that both sides of the migration will keep the record updated. This works when XATTRs/SELinux labels are supported by the shared filesystem or are *not* supported and both sides access it through it, in which case it all ends up being no-op and works by omission :) In our scenario, migrating from local to NFS will work but then migrating back won't, because the existing information will lead libvirt to believe that the image is already in use - which it is, but not in the way that it expects :) In general, all the bookkeeping and relabeling is pretty much broken by design when migration is involved, and things only appear to work because requests are silently ignored. > Looking at the code we seem to be calling 'qemuSecurityRestoreAllLabel' > with the 'migrated' flag set when migrating at all times. This should > make it theoretically possible to simply clear out the XATTRs for any > file which resides on the paths in question if we're migrating away > rather than ignoring them, which would solve the issue. This would involve touching the file's attributes on the source side after the VM is already running on the destination side, which is something that the existing code understandably steers very clear of. I've tried fairly hard to make things work "just so" and by now I'm convinced that there is no solution which gets it right without adding a lot of additional complexity/fragility and introducing its own trade-offs. Asking users to disable owner remembering is not too unreasonable IMO. Especially when we take into account the fact that I'm targeting KubeVirt's needs specifically with this feature, and they *already* do that anyway. > This would go together with the 'syntax-sugar'-ish flavor to this > feature. Additionally it would help in case, when the user configures > these paths after the VM was started and thus the XATTRs are already > applied. Those would not then be cleared on migration. That sounds like a corner case of a corner case, and a direct result of the admin failing to configure the host correctly in the first place. I wouldn't be too concerned about it. > I also wanted to suggest to modify this paragraph to suggest a lesser > hammer to disable ownersip remembering, as that will disable it also for > the non-shared paths, but IIRC we don't have flags to disable it > individually (in contrast with the 'dynamic_ownership' flag which can be > disabled per-image via the 'relabel' attribute). I'm not aware of anything of the sort either. -- Andrea Bolognani / Red Hat / Virtualization ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v3 3/5] qemu: Propagate shared_filesystems
On Thu, May 09, 2024 at 02:17:21PM GMT, Peter Krempa wrote: > I'd go with 'exportedFilesystems' instead of 'sharedFilesystems' > throughout this patch(set) for any case when the list contains only the > paths configured by user (from previous commit). Isn't that all cases? We never build a list where we add the user-provided paths to those that have been detected as shared via other means AFAICT. > That way it'd be IMO more clear that the list contains only filesystems > which are local on this host, rather than having also anything that's > consiedered shared. > > It IMO would make sense to consider that e.g. also for the name of the > config option. I can certainly change the name, but if the goal is to make things clearer to someone looking at a function in isolation I'm not entirely sure "exportedFilesystems" will help much. What about "userSharedFilesystems"? As in, filesystems that are to be considered as shared specifically because the user told us to. > > @@ -1611,20 +1612,23 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm, > > } > > > > static bool > > -qemuMigrationSrcIsSafe(virDomainDef *def, > > - virQEMUCaps *qemuCaps, > > +qemuMigrationSrcIsSafe(virDomainObj *vm, > > size_t nmigrate_disks, > > const char **migrate_disks, > > unsigned int flags) > > Sneaky refactor :) Do you want me to split it off to a separate patch? I can do that. -- Andrea Bolognani / Red Hat / Virtualization ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v3 4/5] utils: Use overrides in virFileIsSharedFS()
On Thu, May 09, 2024 at 02:28:15PM GMT, Peter Krempa wrote:
> On Thu, May 02, 2024 at 19:39:41 +0200, Andrea Bolognani wrote:
> > +static bool
> > +virFileIsSharedFSOverride(const char *path,
> > + char *const *overrides)
> > +{
> > +g_autofree char *dirpath = NULL;
> > +char *p = NULL;
> > +
> > +if (!path || path[0] != '/' || !overrides)
> > +return false;
>
> Per my comment on canonicalizing paths only when they're about to be
> used.
Gotcha.
> I think you can also modify the algorithm to avoid the truncate&compare
> operations to:
>
>
> foreach override in overrides:
>
> pc = canonicalize(path);
> po = canonicalize(override);
>
> if (STRPREFIX(pc, po))
> return true;
I'll give it a try.
> Checking the full prefix on canonicalized paths is IIUC equivalent to
> what you do below. (Okay perhaps except the case when user declares a
> full to a single file as an exported override).
That isn't supposed to work anyway... If the current code allows it
then it will need to be fixed.
--
Andrea Bolognani / Red Hat / Virtualization
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v3 2/5] qemu: Introduce shared_filesystems configuration option
On Thu, May 09, 2024 at 05:10:50PM GMT, Peter Krempa wrote: > Now things I see as problem in case when NFS not supporting xattr is > used. This means that the remote VM can set XATTRs and must use > 'virt_use_nfs' sebool. I must be confused about the purpose of the virt_use_nfs sebool, and I can't seem to find decent documentation about it. Do you have any handy? Have you actually been able to use either SELinux or (trusted) XATTRs on an NFS-mounted filesystem? If so, how? > IMO the only proper option to do this across the XATTR boundary will be > to have an additional step in the finalizing phase of migration that > will unref the libvirt labels. In case when the last reference is gone > it'd need to also restore the label, same as it does now. During > migration there'll need to be a period while two refs are on the libvirt > xattrs. This sounds fairly attractive from a high-level point of view, though I'll admit that I'm concerned about things going out of sync and unintentionally cutting off file access to the target host as a consequence of that. > As said I'll need to actually check what's really happening in regards > of the selinux labels. Please do. Hopefully you'll get further than I was able to :) -- Andrea Bolognani / Red Hat / Virtualization ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 0/4] rpm: Some ssh-proxy improvements
CI pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/1292193155 Andrea Bolognani (4): rpm: Drop weak dependency on ssh-proxy from client rpm: Only Recommend ssh-proxy rpm: Move dependency on ssh-proxy to QEMU driver rpm: Drop with_ssh_proxy define libvirt.spec.in | 23 ++- 1 file changed, 2 insertions(+), 21 deletions(-) -- 2.45.0
[PATCH 1/4] rpm: Drop weak dependency on ssh-proxy from client
The ssh-proxy feature works independently of the clients,
just like the NSS plugin does.
Moreover, ssh-proxy only works for local VMs, while clients
are routinely used to manage remote hypervisors.
Signed-off-by: Andrea Bolognani
---
libvirt.spec.in | 3 ---
1 file changed, 3 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index f7c128d809..5cb19fa433 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1025,9 +1025,6 @@ Summary: Client side utilities of the libvirt library
Requires: libvirt-libs = %{version}-%{release}
# Needed by virt-pki-validate script.
Requires: gnutls-utils
-%if %{with_ssh_proxy}
-Recommends: libvirt-ssh-proxy = %{version}-%{release}
-%endif
# Ensure smooth upgrades
Obsoletes: libvirt-bash-completion < 7.3.0
--
2.45.0
[PATCH 3/4] rpm: Move dependency on ssh-proxy to QEMU driver
This way we can avoid repeating it twice.
Signed-off-by: Andrea Bolognani
---
libvirt.spec.in | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 329b923e8f..0d6f15460d 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -810,6 +810,9 @@ Summary: QEMU driver plugin for the libvirtd daemon
Requires: libvirt-daemon-common = %{version}-%{release}
Requires: libvirt-daemon-log = %{version}-%{release}
Requires: libvirt-libs = %{version}-%{release}
+%if %{with_ssh_proxy}
+Recommends: libvirt-ssh-proxy = %{version}-%{release}
+%endif
Requires: /usr/bin/qemu-img
# For image compression
Requires: gzip
@@ -904,9 +907,6 @@ Requires: libvirt-daemon-driver-nodedev =
%{version}-%{release}
Requires: libvirt-daemon-driver-nwfilter = %{version}-%{release}
Requires: libvirt-daemon-driver-secret = %{version}-%{release}
Requires: libvirt-daemon-driver-storage = %{version}-%{release}
-%if %{with_ssh_proxy}
-Recommends: libvirt-ssh-proxy = %{version}-%{release}
-%endif
Requires: qemu
%description daemon-qemu
@@ -935,9 +935,6 @@ Requires: libvirt-daemon-driver-nodedev =
%{version}-%{release}
Requires: libvirt-daemon-driver-nwfilter = %{version}-%{release}
Requires: libvirt-daemon-driver-secret = %{version}-%{release}
Requires: libvirt-daemon-driver-storage = %{version}-%{release}
-%if %{with_ssh_proxy}
-Recommends: libvirt-ssh-proxy = %{version}-%{release}
-%endif
Requires: qemu-kvm
%description daemon-kvm
--
2.45.0
[PATCH 4/4] rpm: Drop with_ssh_proxy define
As a general rule, we use defines for features that can only be
enabled on a subset of the platforms that we target, and we
don't offer fine-grained control over every single possible
meson configuration knob at the RPM level.
In the case of ssh-proxy, we are enabling it everywhere already,
so having a define for it is unnecessary.
Signed-off-by: Andrea Bolognani
---
libvirt.spec.in | 15 +--
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 0d6f15460d..b6f9bf86f3 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -91,7 +91,6 @@
# Other optional features
%define with_numactl 0%{!?_without_numactl:1}
%define with_userfaultfd_sysctl 0%{!?_without_userfaultfd_sysctl:1}
-%define with_ssh_proxy0%{!?_without_ssh_proxy:1}
# A few optional bits off by default, we enable later
%define with_fuse 0
@@ -810,9 +809,7 @@ Summary: QEMU driver plugin for the libvirtd daemon
Requires: libvirt-daemon-common = %{version}-%{release}
Requires: libvirt-daemon-log = %{version}-%{release}
Requires: libvirt-libs = %{version}-%{release}
-%if %{with_ssh_proxy}
Recommends: libvirt-ssh-proxy = %{version}-%{release}
-%endif
Requires: /usr/bin/qemu-img
# For image compression
Requires: gzip
@@ -1104,14 +1101,12 @@ Requires: libvirt-daemon-driver-network =
%{version}-%{release}
Libvirt plugin for NSS for translating domain names into IP addresses.
%endif
-%if %{with_ssh_proxy}
%package ssh-proxy
Summary: Libvirt SSH proxy
Requires: libvirt-libs = %{version}-%{release}
%description ssh-proxy
Allows SSH into domains via VSOCK without need for network.
-%endif
%if %{with_mingw32}
%package -n mingw32-libvirt
@@ -1304,12 +1299,6 @@ exit 1
%define arg_userfaultfd_sysctl -Duserfaultfd_sysctl=disabled
%endif
-%if %{with_ssh_proxy}
-%define arg_ssh_proxy -Dssh_proxy=enabled
-%else
-%define arg_ssh_proxy -Dssh_proxy=disabled
-%endif
-
%define when %(date +"%%F-%%T")
%define where %(hostname)
%define who %{?packager}%{!?packager:Unknown}
@@ -1391,7 +1380,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y'
%{_specdir}/libvirt.spec)
-Dtls_priority=%{tls_priority} \
-Dsysctl_config=enabled \
%{?arg_userfaultfd_sysctl} \
- %{?arg_ssh_proxy} \
+ -Dssh_proxy=enabled \
%{?enable_werror} \
-Dexpensive_tests=enabled \
-Dinit_script=systemd \
@@ -2447,11 +2436,9 @@ exit 0
%{_libdir}/libnss_libvirt.so.2
%{_libdir}/libnss_libvirt_guest.so.2
-%if %{with_ssh_proxy}
%files ssh-proxy
%config(noreplace) %{_sysconfdir}/ssh/ssh_config.d/30-libvirt-ssh-proxy.conf
%{_libexecdir}/libvirt-ssh-proxy
-%endif
%if %{with_lxc}
%files login-shell
--
2.45.0
[PATCH 2/4] rpm: Only Recommend ssh-proxy
The way things are implemented, installing the package not
only makes the feature available but also enables it.
Some admins might not want that to happen, so let's make the
dependency a weak one to offer them a way out.
Signed-off-by: Andrea Bolognani
---
libvirt.spec.in | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 5cb19fa433..329b923e8f 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -905,7 +905,7 @@ Requires: libvirt-daemon-driver-nwfilter =
%{version}-%{release}
Requires: libvirt-daemon-driver-secret = %{version}-%{release}
Requires: libvirt-daemon-driver-storage = %{version}-%{release}
%if %{with_ssh_proxy}
-Requires: libvirt-ssh-proxy = %{version}-%{release}
+Recommends: libvirt-ssh-proxy = %{version}-%{release}
%endif
Requires: qemu
@@ -936,7 +936,7 @@ Requires: libvirt-daemon-driver-nwfilter =
%{version}-%{release}
Requires: libvirt-daemon-driver-secret = %{version}-%{release}
Requires: libvirt-daemon-driver-storage = %{version}-%{release}
%if %{with_ssh_proxy}
-Requires: libvirt-ssh-proxy = %{version}-%{release}
+Recommends: libvirt-ssh-proxy = %{version}-%{release}
%endif
Requires: qemu-kvm
--
2.45.0
Re: [PATCH 4/4] rpm: Drop with_ssh_proxy define
On Thu, May 16, 2024 at 10:03:02AM GMT, Daniel P. Berrangé wrote: > On Thu, May 16, 2024 at 10:24:22AM +0200, Andrea Bolognani wrote: > > As a general rule, we use defines for features that can only be > > enabled on a subset of the platforms that we target, and we > > don't offer fine-grained control over every single possible > > meson configuration knob at the RPM level. > > > > In the case of ssh-proxy, we are enabling it everywhere already, > > so having a define for it is unnecessary. > > The only reason for a conditional would be if some older distro lacks > support for this SSH proxy'ing feature. Assuming RHEL-9 / Ubuntu 22.04 > have it, then this is indeed redundant We don't need to worry about Ubuntu in the spec file ;) IIUC requirements are mostly on the guest OS side, and on the host OS side we just need the ssh ProxyCommand feature which would have been available since forever. Michal, can you please confirm that this is accurate? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 4/4] rpm: Drop with_ssh_proxy define
On Thu, May 16, 2024 at 10:26:28AM GMT, Daniel P. Berrangé wrote: > On Thu, May 16, 2024 at 02:23:13AM -0700, Andrea Bolognani wrote: > > IIUC requirements are mostly on the guest OS side, and on the host OS > > side we just need the ssh ProxyCommand feature which would have been > > available since forever. Michal, can you please confirm that this is > > accurate? > > We need ProxyUseFdpass too, and I wasn't sure how far back that one > went. According to [1] it was introduced in OpenSSH 6.5 (2014), so we should be good. [1] https://www.openssh.com/releasenotes.html -- Andrea Bolognani / Red Hat / Virtualization
[PATCH 0/3] qemu: Use TPM 2.0 on RISC-V
Andrea Bolognani (3): tests: Add TPM coverage to default-models tests tests: Delete some redundant test cases qemu: Only allow TPM 2.0 for RISC-V guests src/qemu/qemu_domain.c| 1 + src/qemu/qemu_validate.c | 10 +-- ...aarch64-tpm-wrong-model.aarch64-latest.err | 2 +- .../aarch64-tpm.aarch64-latest.args | 34 - .../aarch64-tpm.aarch64-latest.xml| 29 tests/qemuxmlconfdata/aarch64-tpm.xml | 15 ...ault-models.aarch64-latest.abi-update.args | 3 + ...fault-models.aarch64-latest.abi-update.xml | 3 + ...64-virt-default-models.aarch64-latest.args | 3 + ...h64-virt-default-models.aarch64-latest.xml | 3 + .../aarch64-virt-default-models.xml | 3 + .../loongarch64-virt-default-models.xml | 1 + ...efault-models.ppc64-latest.abi-update.args | 3 + ...default-models.ppc64-latest.abi-update.xml | 4 ++ ...4-pseries-default-models.ppc64-latest.args | 3 + ...64-pseries-default-models.ppc64-latest.xml | 4 ++ .../ppc64-pseries-default-models.xml | 3 + ...ault-models.riscv64-latest.abi-update.args | 3 + ...fault-models.riscv64-latest.abi-update.xml | 3 + ...64-virt-default-models.riscv64-latest.args | 3 + ...v64-virt-default-models.riscv64-latest.xml | 3 + .../riscv64-virt-default-models.xml | 3 + .../s390x-ccw-default-models.xml | 1 + .../tpm-emulator-spapr.ppc64-latest.args | 45 .../tpm-emulator-spapr.ppc64-latest.xml | 1 - tests/qemuxmlconfdata/tpm-emulator-spapr.xml | 70 --- ...fault-models.x86_64-latest.abi-update.args | 3 + ...efault-models.x86_64-latest.abi-update.xml | 3 + ...86_64-pc-default-models.x86_64-latest.args | 3 + ...x86_64-pc-default-models.x86_64-latest.xml | 3 + .../x86_64-pc-default-models.xml | 3 + ...fault-models.x86_64-latest.abi-update.args | 3 + ...efault-models.x86_64-latest.abi-update.xml | 3 + ...6_64-q35-default-models.x86_64-latest.args | 3 + ...86_64-q35-default-models.x86_64-latest.xml | 3 + .../x86_64-q35-default-models.xml | 3 + tests/qemuxmlconftest.c | 2 - 37 files changed, 87 insertions(+), 201 deletions(-) delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.args delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.xml delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.xml delete mode 100644 tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.args delete mode 12 tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.xml delete mode 100644 tests/qemuxmlconfdata/tpm-emulator-spapr.xml -- 2.45.1
[PATCH 1/3] tests: Add TPM coverage to default-models tests
We have a non-trivial amount of architecture-specific logic
dealing with TPM, so it's good to have coverage for it.
Note that TPM supports seems to be currently missing from s390x
and loongarch64 QEMU builds. I'm not entirely sure whether
there's a good reason for that or it's simply an oversight, but
either way we have to skip them for now.
Signed-off-by: Andrea Bolognani
---
...aarch64-virt-default-models.aarch64-latest.abi-update.args | 3 +++
.../aarch64-virt-default-models.aarch64-latest.abi-update.xml | 3 +++
.../aarch64-virt-default-models.aarch64-latest.args | 3 +++
.../aarch64-virt-default-models.aarch64-latest.xml| 3 +++
tests/qemuxmlconfdata/aarch64-virt-default-models.xml | 3 +++
tests/qemuxmlconfdata/loongarch64-virt-default-models.xml | 1 +
.../ppc64-pseries-default-models.ppc64-latest.abi-update.args | 3 +++
.../ppc64-pseries-default-models.ppc64-latest.abi-update.xml | 4
.../ppc64-pseries-default-models.ppc64-latest.args| 3 +++
.../ppc64-pseries-default-models.ppc64-latest.xml | 4
tests/qemuxmlconfdata/ppc64-pseries-default-models.xml| 3 +++
...riscv64-virt-default-models.riscv64-latest.abi-update.args | 3 +++
.../riscv64-virt-default-models.riscv64-latest.abi-update.xml | 3 +++
.../riscv64-virt-default-models.riscv64-latest.args | 3 +++
.../riscv64-virt-default-models.riscv64-latest.xml| 3 +++
tests/qemuxmlconfdata/riscv64-virt-default-models.xml | 3 +++
tests/qemuxmlconfdata/s390x-ccw-default-models.xml| 1 +
.../x86_64-pc-default-models.x86_64-latest.abi-update.args| 3 +++
.../x86_64-pc-default-models.x86_64-latest.abi-update.xml | 3 +++
.../x86_64-pc-default-models.x86_64-latest.args | 3 +++
.../x86_64-pc-default-models.x86_64-latest.xml| 3 +++
tests/qemuxmlconfdata/x86_64-pc-default-models.xml| 3 +++
.../x86_64-q35-default-models.x86_64-latest.abi-update.args | 3 +++
.../x86_64-q35-default-models.x86_64-latest.abi-update.xml| 3 +++
.../x86_64-q35-default-models.x86_64-latest.args | 3 +++
.../x86_64-q35-default-models.x86_64-latest.xml | 3 +++
tests/qemuxmlconfdata/x86_64-q35-default-models.xml | 3 +++
27 files changed, 79 insertions(+)
diff --git
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
index 0c4acf800f..a503f45d0c 100644
---
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
+++
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
@@ -38,6 +38,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config
\
-device
'{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.2","addr":"0x0"}'
\
-chardev pty,id=charserial0 \
-serial chardev:charserial0 \
+-chardev socket,id=chrtpm,path=/dev/test \
+-tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \
+-device '{"driver":"tpm-tis-device","tpmdev":"tpm-tpm0","id":"tpm0"}' \
-audiodev '{"id":"audio1","driver":"none"}' \
-device
'{"driver":"virtio-gpu-pci","id":"video0","max_outputs":1,"bus":"pci.5","addr":"0x0"}'
\
-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
diff --git
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
index 87be062c89..bbe1dd931d 100644
---
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
+++
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
@@ -69,6 +69,9 @@
+
+
+
diff --git
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
index 0c4acf800f..a503f45d0c 100644
--- a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
+++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
@@ -38,6 +38,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config
\
-device
'{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.2","addr":"0x0"}'
\
-chardev pty,id=charserial0 \
-serial chardev:charserial0 \
+-chardev socket
[PATCH 3/3] qemu: Only allow TPM 2.0 for RISC-V guests
We've made similar changes for aarch64 a few years back (see
d8a1c059e0ed and previous commits), and the rationale is the
same: the architecture is new enough that TPM 2.0 predates it,
so TPM 1.2 support was never considered and will just not work.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_domain.c | 1 +
src/qemu/qemu_validate.c | 10 ++
.../aarch64-tpm-wrong-model.aarch64-latest.err | 2 +-
...4-virt-default-models.riscv64-latest.abi-update.xml | 2 +-
.../riscv64-virt-default-models.riscv64-latest.xml | 2 +-
5 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index bda62f2e5c..6bb18ad5a8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6182,6 +6182,7 @@ qemuDomainTPMDefPostParse(virDomainTPMDef *tpm,
tpm->data.emulator.version == VIR_DOMAIN_TPM_VERSION_DEFAULT) {
if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR ||
tpm->model == VIR_DOMAIN_TPM_MODEL_CRB ||
+qemuDomainIsRISCVVirt(def) ||
qemuDomainIsARMVirt(def))
tpm->data.emulator.version = VIR_DOMAIN_TPM_VERSION_2_0;
else
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index ac1940cb31..7b871be05f 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4765,10 +4765,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
_("TPM 1.2 is not supported with the SPAPR
device model"));
return -1;
}
-/* TPM 1.2 + ARM does not work */
-if (qemuDomainIsARMVirt(def)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("TPM 1.2 is not supported on ARM"));
+/* TPM 1.2 does not work on certain modern architectures */
+if (qemuDomainIsARMVirt(def) ||
+qemuDomainIsRISCVVirt(def)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("TPM 1.2 is not supported on architecture
'%1$s'"),
+ virArchToString(def->os.arch));
return -1;
}
break;
diff --git a/tests/qemuxmlconfdata/aarch64-tpm-wrong-model.aarch64-latest.err
b/tests/qemuxmlconfdata/aarch64-tpm-wrong-model.aarch64-latest.err
index a3a82fdcf5..44c6e7372b 100644
--- a/tests/qemuxmlconfdata/aarch64-tpm-wrong-model.aarch64-latest.err
+++ b/tests/qemuxmlconfdata/aarch64-tpm-wrong-model.aarch64-latest.err
@@ -1 +1 @@
-unsupported configuration: TPM 1.2 is not supported on ARM
+unsupported configuration: TPM 1.2 is not supported on architecture 'aarch64'
diff --git
a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
index a3a701b8e4..6712c2d831 100644
---
a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
+++
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
@@ -59,7 +59,7 @@
-
+
diff --git
a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
index a3a701b8e4..6712c2d831 100644
--- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
+++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
@@ -59,7 +59,7 @@
-
+
--
2.45.1
[PATCH 2/3] tests: Delete some redundant test cases
The default-models tests provide coverage for these scenarios
now.
Signed-off-by: Andrea Bolognani
---
.../aarch64-tpm.aarch64-latest.args | 34 -
.../aarch64-tpm.aarch64-latest.xml| 29
tests/qemuxmlconfdata/aarch64-tpm.xml | 15
.../tpm-emulator-spapr.ppc64-latest.args | 45
.../tpm-emulator-spapr.ppc64-latest.xml | 1 -
tests/qemuxmlconfdata/tpm-emulator-spapr.xml | 70 ---
tests/qemuxmlconftest.c | 2 -
7 files changed, 196 deletions(-)
delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.args
delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.xml
delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.xml
delete mode 100644 tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.args
delete mode 12 tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.xml
delete mode 100644 tests/qemuxmlconfdata/tpm-emulator-spapr.xml
diff --git a/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.args
b/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.args
deleted file mode 100644
index 729d0cae53..00
--- a/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.args
+++ /dev/null
@@ -1,34 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/var/lib/libvirt/qemu/domain--1-aarch64test \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-aarch64test/.local/share \
-XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-aarch64test/.cache \
-XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-aarch64test/.config \
-/usr/bin/qemu-system-aarch64 \
--name guest=aarch64test,debug-threads=on \
--S \
--object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-aarch64test/master-key.aes"}'
\
--machine
virt,usb=off,gic-version=2,dump-guest-core=off,memory-backend=mach-virt.ram,acpi=off
\
--accel tcg \
--cpu cortex-a15 \
--m size=1048576k \
--object
'{"qom-type":"memory-backend-ram","id":"mach-virt.ram","size":1073741824}' \
--overcommit mem-lock=off \
--smp 1,sockets=1,cores=1,threads=1 \
--uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \
--display none \
--no-user-config \
--nodefaults \
--chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
--mon chardev=charmonitor,id=monitor,mode=control \
--rtc base=utc \
--no-shutdown \
--boot strict=on \
--chardev socket,id=chrtpm,path=/dev/test \
--tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \
--device '{"driver":"tpm-tis-device","tpmdev":"tpm-tpm0","id":"tpm0"}' \
--audiodev '{"id":"audio1","driver":"none"}' \
--sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
--msg timestamp=on
diff --git a/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.xml
b/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.xml
deleted file mode 100644
index e97f39aec3..00
--- a/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.xml
+++ /dev/null
@@ -1,29 +0,0 @@
-
- aarch64test
- 496d7ea8-9739-544b-4ebd-ef08be936e8b
- 1048576
- 1048576
- 1
-
-hvm
-
-
-
-
-
-
-cortex-a15
-
-
- destroy
- restart
- destroy
-
-/usr/bin/qemu-system-aarch64
-
-
-
-
-
-
-
diff --git a/tests/qemuxmlconfdata/aarch64-tpm.xml
b/tests/qemuxmlconfdata/aarch64-tpm.xml
deleted file mode 100644
index b22dbee71e..00
--- a/tests/qemuxmlconfdata/aarch64-tpm.xml
+++ /dev/null
@@ -1,15 +0,0 @@
-
- aarch64test
- 496d7ea8-9739-544b-4ebd-ef08be936e8b
- 1048576
- 1
-
-hvm
-
-
-/usr/bin/qemu-system-aarch64
-
-
-
-
-
diff --git a/tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.args
b/tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.args
deleted file mode 100644
index cba1950e2d..00
--- a/tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.args
+++ /dev/null
@@ -1,45 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/var/lib/libvirt/qemu/domain--1-TPM-VM \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-TPM-VM/.local/share \
-XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-TPM-VM/.cache \
-XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-TPM-VM/.config \
-/usr/bin/qemu-system-ppc64 \
--name guest=TPM-VM,debug-threads=on \
--S \
--object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-TPM-VM/master-key.aes"}'
\
--machine pseries-5.0,usb=off,dump-guest-core=off,memory-backend=ppc_spapr.ram \
--accel tcg \
--cpu POWER9 \
--m size=2097152k \
--object
'{"qom-type":"memory-backend-ram","id":"ppc_spapr.ram","size":214
Re: [PATCH 1/3] tests: Add TPM coverage to default-models tests
On Tue, May 28, 2024 at 08:59:46AM GMT, Peter Krempa wrote: > On Mon, May 27, 2024 at 19:31:34 +0200, Andrea Bolognani wrote: > > Note that TPM supports seems to be currently missing from s390x > > and loongarch64 QEMU builds. I'm not entirely sure whether > > there's a good reason for that or it's simply an oversight, but > > either way we have to skip them for now. > > I presume you mean that TPM support was not built into the QEMU builds > used to capture the capability dumps, right? That's just the thing: I don't know :) In both cases the default configuration disables it, but it's unclear to me whether that is something that could be addressed with a simple patch or there are factors at play that make TPM inherently incompatible with these architectures. > > +++ b/tests/qemuxmlconfdata/s390x-ccw-default-models.xml > > @@ -14,6 +14,7 @@ > > > > > > > > + For s390x, Thomas has confirmed that TPM is just not a thing and will never be a thing, so while the comment is technically accurate I agree that stronger language should be used to reflect the situation. Thomas, please correct me if I got this wrong :) > > +++ b/tests/qemuxmlconfdata/loongarch64-virt-default-models.xml > > @@ -14,6 +14,7 @@ > > > > > > > > + For loongarch64, maybe Xianglai Li can provide some insight. Is TPM something that could be flipped on at the QEMU level? -- Andrea Bolognani / Red Hat / Virtualization
[PATCH 0/3] meson: Improve default firewall backend configuration
Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/1308917840 See 1/3 for details. Andrea Bolognani (3): meson: Improve default firewall backend configuration meson: Include firewall backend selection in summary rpm: Configure firewall backends explicitly libvirt.spec.in | 1 + meson.build | 21 ++--- meson_options.txt| 3 +-- src/network/bridge_driver_conf.c | 6 +- src/network/meson.build | 6 -- src/network/network.conf.in | 13 +++-- 6 files changed, 32 insertions(+), 18 deletions(-) -- 2.45.1
[PATCH 1/3] meson: Improve default firewall backend configuration
The current implementation requires users to configure the
preference as such:
-Dfirewall_backend_default_1=iptables
-Dfirewall_backend_default_2=nftables
In addition to being more verbose than one would hope, there
are several things that could go wrong.
First of all, meson performs no validation on the provided
values, so mistakes will only be caught by the compiler.
Additionally, it's entirely possible to provide nonsensical
combinations, such as repeating the same value twice.
Change things so that the preference can now be configured
as such:
-Dfirewall_backend_priority=iptables,nftables
Checks have been added to prevent invalid values from being
accepted.
Signed-off-by: Andrea Bolognani
---
meson.build | 16 +---
meson_options.txt| 3 +--
src/network/bridge_driver_conf.c | 6 +-
src/network/meson.build | 6 --
src/network/network.conf.in | 13 +++--
5 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/meson.build b/meson.build
index f67c3d2724..ed0e9686f8 100644
--- a/meson.build
+++ b/meson.build
@@ -1635,15 +1635,17 @@ endif
if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD')
conf.set('WITH_NETWORK', 1)
- firewall_backend_default_1 = get_option('firewall_backend_default_1')
- firewall_backend_default_conf = firewall_backend_default_1
- firewall_backend_default_1 = 'VIR_FIREWALL_BACKEND_' +
firewall_backend_default_1.to_upper()
- conf.set('FIREWALL_BACKEND_DEFAULT_1', firewall_backend_default_1)
- firewall_backend_default_2 = get_option('firewall_backend_default_2')
- firewall_backend_default_2 = 'VIR_FIREWALL_BACKEND_' +
firewall_backend_default_2.to_upper()
- conf.set('FIREWALL_BACKEND_DEFAULT_2', firewall_backend_default_2)
+ firewall_backend_priority = get_option('firewall_backend_priority')
+ if (not firewall_backend_priority.contains('nftables') or
+ not firewall_backend_priority.contains('iptables') or
+ firewall_backend_priority.length() != 2)
+error('invalid value for firewall_backend_priority option')
+ endif
+ conf.set('FIREWALL_BACKEND_PRIORITY_0', 'VIR_FIREWALL_BACKEND_' +
firewall_backend_priority[0].to_upper())
+ conf.set('FIREWALL_BACKEND_PRIORITY_1', 'VIR_FIREWALL_BACKEND_' +
firewall_backend_priority[1].to_upper())
+ conf.set('FIREWALL_BACKEND_PRIORITY_NUM', firewall_backend_priority.length())
elif get_option('driver_network').enabled()
error('libvirtd must be enabled to build the network driver')
endif
diff --git a/meson_options.txt b/meson_options.txt
index ad354a8668..8723d13231 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -115,8 +115,7 @@ option('dtrace', type: 'feature', value: 'auto',
description: 'use dtrace for st
option('firewalld', type: 'feature', value: 'auto', description: 'firewalld
support')
# dep:firewalld
option('firewalld_zone', type: 'feature', value: 'auto', description: 'whether
to install firewalld libvirt zone')
-option('firewall_backend_default_1', type: 'string', value: 'nftables',
description: 'first firewall backend to try when none is specified')
-option('firewall_backend_default_2', type: 'string', value: 'iptables',
description: 'second firewall backend to try when none is specified (and first
is unavailable)')
+option('firewall_backend_priority', type: 'array', choices: ['nftables',
'iptables'], description: 'firewall backends to try, preferred ones first')
option('host_validate', type: 'feature', value: 'auto', description: 'build
virt-host-validate')
option('init_script', type: 'combo', choices: ['systemd', 'openrc', 'check',
'none'], value: 'check', description: 'Style of init script to install')
option('loader_nvram', type: 'string', value: '', description: 'Pass list of
pairs of : paths. Both pairs and list items are separated by a
colon.')
diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c
index 8f4956dace..e2f3613a41 100644
--- a/src/network/bridge_driver_conf.c
+++ b/src/network/bridge_driver_conf.c
@@ -67,8 +67,12 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg
G_GNUC_UNUSED,
g_autofree char *fwBackendStr = NULL;
bool fwBackendSelected = false;
size_t i;
-int fwBackends[] = { FIREWALL_BACKEND_DEFAULT_1,
FIREWALL_BACKEND_DEFAULT_2 };
+int fwBackends[] = {
+
[PATCH 2/3] meson: Include firewall backend selection in summary
Signed-off-by: Andrea Bolognani
---
meson.build | 5 +
1 file changed, 5 insertions(+)
diff --git a/meson.build b/meson.build
index ed0e9686f8..e98ab0d5ac 100644
--- a/meson.build
+++ b/meson.build
@@ -2382,6 +2382,11 @@ misc_summary = {
'sysctl config': conf.has('WITH_SYSCTL'),
'userfaultfd sysctl': conf.has('WITH_USERFAULTFD_SYSCTL'),
}
+if conf.has('WITH_NETWORK')
+ misc_summary += {
+'firewall backends': firewall_backend_priority,
+ }
+endif
summary(misc_summary, section: 'Miscellaneous', bool_yn: true, list_sep: ' ')
devtools_summary = {
--
2.45.1
[PATCH 3/3] rpm: Configure firewall backends explicitly
Signed-off-by: Andrea Bolognani
---
libvirt.spec.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 6fb223c74a..4381dbe30c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1387,6 +1387,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y'
%{_specdir}/libvirt.spec)
%{?enable_werror} \
-Dexpensive_tests=enabled \
-Dinit_script=systemd \
+ -Dfirewall_backend_priority=nftables,iptables \
-Ddocs=enabled \
-Dtests=enabled \
-Drpath=disabled \
--
2.45.1
Re: [PATCH 1/3] meson: Improve default firewall backend configuration
On Tue, May 28, 2024 at 12:50:51PM GMT, Laine Stump wrote:
> On 5/28/24 12:31 PM, Pavel Hrdina wrote:
> > On Tue, May 28, 2024 at 05:49:19PM +0200, Andrea Bolognani wrote:
> > > + if (not firewall_backend_priority.contains('nftables') or
> > > + not firewall_backend_priority.contains('iptables') or
> > > + firewall_backend_priority.length() != 2)
> >
> > No need to have a check for specific values. Meson will already check if
> > they are from the list of choices defined in meson_options.txt .
>
> But we don't just need to check that the values in the list are all valid
> options; we also want to make sure that nobody has entered the same value
> multiple times (which this ends up doing by making sure that there is at
> least one entry for each valid value, and that the list is the same length
> as the number of valid values).
Yes, that was exactly the idea.
> Or do we not care if someone repeats the same value? Maybe somebody wants to
> include iptables support in the build, but not look for it automatically
> (instead only use it if it's explicitly requested in network.conf). One way
> of doing that would be to sent firewall_backend_priority = nftables,nftables
>
> (that does seem a bit obtuse; perhaps it would be better to allow limiting
> the length of the option list to 1)
If that's something that we want to allow, then we should include
explicit support for it rather than make it possible through obscure
runes :)
I'm not sure we really need to bother, but I don't feel strongly
either way so I could be persuaded to look into it. Perhaps as an
after-release follow up, though?
> > > +option('firewall_backend_priority', type: 'array', choices: ['nftables',
> > > 'iptables'], description: 'firewall backends to try, preferred ones
> > > first')
> >
> > What about "order of firewall backends to try"? The part "preferred ones
> > first" sounds strange to me.
Sure, that works too.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 3/3] rpm: Configure firewall backends explicitly
On Mon, Jun 03, 2024 at 10:57:15AM GMT, Daniel P. Berrangé wrote:
> On Tue, May 28, 2024 at 05:49:21PM +0200, Andrea Bolognani wrote:
> > +++ b/libvirt.spec.in
> > @@ -1387,6 +1387,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y'
> > %{_specdir}/libvirt.spec)
> > %{?enable_werror} \
> > -Dexpensive_tests=enabled \
> > -Dinit_script=systemd \
> > + -Dfirewall_backend_priority=nftables,iptables \
>
> This change isn't right as it doesn't match earlier logic in the spec
> which is version specific
>
> %if 0%{?rhel} >= 10 || 0%{?fedora} >= 41
> Requires: nftables
> %else
> Requires: iptables
> %endif
>
> ie, we should *not* be preferring nftables on existing RHEL and Fedora,
> only future, otherwise users will get a functional behaviour change
> on their existing distro.
You're right. I'll fix it. Unfortunately 10.4.0 has already been
tagged :(
--
Andrea Bolognani / Red Hat / Virtualization
[PATCH] rpm: Don't default to nftables on existing distros
Signed-off-by: Andrea Bolognani
---
libvirt.spec.in | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 4381dbe30c..5ca7b95e6c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -205,6 +205,18 @@
%define with_modular_daemons 1
%endif
+# Prefer nftables for future OS releases but keep using iptables
+# for existing ones
+%if 0%{?rhel} >= 10 || 0%{?fedora} >= 41
+%define prefer_nftables 1
+%define firewall_backend_priority nftables,iptables
+%else
+%define prefer_nftables 0
+%define firewall_backend_priority iptables,nftables
+%endif
+
+
+
# Force QEMU to run as non-root
%define qemu_user qemu
%define qemu_group qemu
@@ -592,7 +604,7 @@ Summary: Network driver plugin for the libvirtd daemon
Requires: libvirt-daemon-common = %{version}-%{release}
Requires: libvirt-libs = %{version}-%{release}
Requires: dnsmasq >= 2.41
-%if 0%{?rhel} >= 10 || 0%{?fedora} >= 41
+%if %{prefer_nftables}
Requires: nftables
%else
Requires: iptables
@@ -1387,7 +1399,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y'
%{_specdir}/libvirt.spec)
%{?enable_werror} \
-Dexpensive_tests=enabled \
-Dinit_script=systemd \
- -Dfirewall_backend_priority=nftables,iptables \
+ -Dfirewall_backend_priority=%{firewall_backend_priority} \
-Ddocs=enabled \
-Dtests=enabled \
-Drpath=disabled \
--
2.45.1
Re: [PATCH 3/3] qemu: Only allow TPM 2.0 for RISC-V guests
On Mon, Jun 03, 2024 at 10:50:40AM GMT, Daniel P. Berrangé wrote:
> On Mon, May 27, 2024 at 07:31:36PM +0200, Andrea Bolognani wrote:
> > +/* TPM 1.2 does not work on certain modern architectures */
> > +if (qemuDomainIsARMVirt(def) ||
> > +qemuDomainIsRISCVVirt(def)) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("TPM 1.2 is not supported on architecture
> > '%1$s'"),
> > + virArchToString(def->os.arch));
> > return -1;
> > }
>
> Hmm, what architectures /do/ allow 1.2 ? x86, s390x, ppc ? Should
> we consider just doing an "allow list" for arches, given that going
> forward nothing new should be allowed.
ppc64 defaults to 2.0 already and s390x doesn't do TPM. Flipping
things around so that 1.2 becomes the special case and is only
allowed for x86 would make sense.
The only remaining question mark is loongarch64. I assume that, just
like riscv64 and aarch64 before it, it wouldn't bother with 1.2 at
all, but I'm not 100% sure. On the other hand, TPM support is
currently compiled out by default in the QEMU system binary for that
architecture, so we could go ahead with the change under that
assumption and revisit things later if necessary. Does that sound
good?
--
Andrea Bolognani / Red Hat / Virtualization
[PATCH v2 0/4] qemu: Use TPM 2.0 in most scenarios
Changes from [v1] * use TPM 2.0 more; * reject TPM 1.2 more; * add better comments to loongarch64 and s390x test cases. [v1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/YZMV33UPKZRRQBN4XCCAW25IRV46LX57/ Andrea Bolognani (4): tests: Add TPM coverage to default-models tests tests: Delete some redundant test cases qemu: Default to TPM 2.0 in most scenarios qemu: Reject TPM 1.2 in most scenarios src/qemu/qemu_domain.c| 13 ++-- src/qemu/qemu_validate.c | 22 +++--- ...aarch64-tpm-wrong-model.aarch64-latest.err | 2 +- .../aarch64-tpm.aarch64-latest.args | 34 - .../aarch64-tpm.aarch64-latest.xml| 29 tests/qemuxmlconfdata/aarch64-tpm.xml | 15 ...ault-models.aarch64-latest.abi-update.args | 3 + ...fault-models.aarch64-latest.abi-update.xml | 3 + ...64-virt-default-models.aarch64-latest.args | 3 + ...h64-virt-default-models.aarch64-latest.xml | 3 + .../aarch64-virt-default-models.xml | 3 + .../loongarch64-virt-default-models.xml | 3 + ...efault-models.ppc64-latest.abi-update.args | 3 + ...default-models.ppc64-latest.abi-update.xml | 4 ++ ...4-pseries-default-models.ppc64-latest.args | 3 + ...64-pseries-default-models.ppc64-latest.xml | 4 ++ .../ppc64-pseries-default-models.xml | 3 + ...ault-models.riscv64-latest.abi-update.args | 3 + ...fault-models.riscv64-latest.abi-update.xml | 3 + ...64-virt-default-models.riscv64-latest.args | 3 + ...v64-virt-default-models.riscv64-latest.xml | 3 + .../riscv64-virt-default-models.xml | 3 + .../s390x-ccw-default-models.xml | 2 + .../tpm-emulator-spapr.ppc64-latest.args | 45 .../tpm-emulator-spapr.ppc64-latest.xml | 1 - tests/qemuxmlconfdata/tpm-emulator-spapr.xml | 70 --- ...fault-models.x86_64-latest.abi-update.args | 3 + ...efault-models.x86_64-latest.abi-update.xml | 3 + ...86_64-pc-default-models.x86_64-latest.args | 3 + ...x86_64-pc-default-models.x86_64-latest.xml | 3 + .../x86_64-pc-default-models.xml | 3 + ...fault-models.x86_64-latest.abi-update.args | 3 + ...efault-models.x86_64-latest.abi-update.xml | 3 + ...6_64-q35-default-models.x86_64-latest.args | 3 + ...86_64-q35-default-models.x86_64-latest.xml | 3 + .../x86_64-q35-default-models.xml | 3 + tests/qemuxmlconftest.c | 2 - 37 files changed, 100 insertions(+), 215 deletions(-) delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.args delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.xml delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.xml delete mode 100644 tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.args delete mode 12 tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.xml delete mode 100644 tests/qemuxmlconfdata/tpm-emulator-spapr.xml -- 2.45.1
[PATCH v2 2/4] tests: Delete some redundant test cases
The default-models tests provide coverage for these scenarios
now.
Signed-off-by: Andrea Bolognani
Reviewed-by: Peter Krempa
---
.../aarch64-tpm.aarch64-latest.args | 34 -
.../aarch64-tpm.aarch64-latest.xml| 29
tests/qemuxmlconfdata/aarch64-tpm.xml | 15
.../tpm-emulator-spapr.ppc64-latest.args | 45
.../tpm-emulator-spapr.ppc64-latest.xml | 1 -
tests/qemuxmlconfdata/tpm-emulator-spapr.xml | 70 ---
tests/qemuxmlconftest.c | 2 -
7 files changed, 196 deletions(-)
delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.args
delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.xml
delete mode 100644 tests/qemuxmlconfdata/aarch64-tpm.xml
delete mode 100644 tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.args
delete mode 12 tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.xml
delete mode 100644 tests/qemuxmlconfdata/tpm-emulator-spapr.xml
diff --git a/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.args
b/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.args
deleted file mode 100644
index 729d0cae53..00
--- a/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.args
+++ /dev/null
@@ -1,34 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/var/lib/libvirt/qemu/domain--1-aarch64test \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-aarch64test/.local/share \
-XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-aarch64test/.cache \
-XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-aarch64test/.config \
-/usr/bin/qemu-system-aarch64 \
--name guest=aarch64test,debug-threads=on \
--S \
--object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-aarch64test/master-key.aes"}'
\
--machine
virt,usb=off,gic-version=2,dump-guest-core=off,memory-backend=mach-virt.ram,acpi=off
\
--accel tcg \
--cpu cortex-a15 \
--m size=1048576k \
--object
'{"qom-type":"memory-backend-ram","id":"mach-virt.ram","size":1073741824}' \
--overcommit mem-lock=off \
--smp 1,sockets=1,cores=1,threads=1 \
--uuid 496d7ea8-9739-544b-4ebd-ef08be936e8b \
--display none \
--no-user-config \
--nodefaults \
--chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
--mon chardev=charmonitor,id=monitor,mode=control \
--rtc base=utc \
--no-shutdown \
--boot strict=on \
--chardev socket,id=chrtpm,path=/dev/test \
--tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \
--device '{"driver":"tpm-tis-device","tpmdev":"tpm-tpm0","id":"tpm0"}' \
--audiodev '{"id":"audio1","driver":"none"}' \
--sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
--msg timestamp=on
diff --git a/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.xml
b/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.xml
deleted file mode 100644
index e97f39aec3..00
--- a/tests/qemuxmlconfdata/aarch64-tpm.aarch64-latest.xml
+++ /dev/null
@@ -1,29 +0,0 @@
-
- aarch64test
- 496d7ea8-9739-544b-4ebd-ef08be936e8b
- 1048576
- 1048576
- 1
-
-hvm
-
-
-
-
-
-
-cortex-a15
-
-
- destroy
- restart
- destroy
-
-/usr/bin/qemu-system-aarch64
-
-
-
-
-
-
-
diff --git a/tests/qemuxmlconfdata/aarch64-tpm.xml
b/tests/qemuxmlconfdata/aarch64-tpm.xml
deleted file mode 100644
index b22dbee71e..00
--- a/tests/qemuxmlconfdata/aarch64-tpm.xml
+++ /dev/null
@@ -1,15 +0,0 @@
-
- aarch64test
- 496d7ea8-9739-544b-4ebd-ef08be936e8b
- 1048576
- 1
-
-hvm
-
-
-/usr/bin/qemu-system-aarch64
-
-
-
-
-
diff --git a/tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.args
b/tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.args
deleted file mode 100644
index cba1950e2d..00
--- a/tests/qemuxmlconfdata/tpm-emulator-spapr.ppc64-latest.args
+++ /dev/null
@@ -1,45 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/var/lib/libvirt/qemu/domain--1-TPM-VM \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-TPM-VM/.local/share \
-XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-TPM-VM/.cache \
-XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-TPM-VM/.config \
-/usr/bin/qemu-system-ppc64 \
--name guest=TPM-VM,debug-threads=on \
--S \
--object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-TPM-VM/master-key.aes"}'
\
--machine pseries-5.0,usb=off,dump-guest-core=off,memory-backend=ppc_spapr.ram \
--accel tcg \
--cpu POWER9 \
--m size=2097152k \
--object
'{"qom-type":"memory-backend-ram","id":"ppc_spapr.ram
[PATCH v2 3/4] qemu: Default to TPM 2.0 in most scenarios
TPM 1.2 is a pretty bad default these days, especially for
architectures which were introduced when TPM 2.0 already existed.
We're already carving out exceptions for several scenarios, but
that's basically backwards: at this point, using TPM 1.2 is the
exception.
Restructure the code so that it reflects reality and we don't
have to remember to update it every time a new architecture is
introduced.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_domain.c | 13 -
...irt-default-models.riscv64-latest.abi-update.xml | 2 +-
.../riscv64-virt-default-models.riscv64-latest.xml | 2 +-
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index bda62f2e5c..7ba2ea4a5e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6180,12 +6180,15 @@ qemuDomainTPMDefPostParse(virDomainTPMDef *tpm,
/* TPM 1.2 and 2 are not compatible, so we choose a specific version here
*/
if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR &&
tpm->data.emulator.version == VIR_DOMAIN_TPM_VERSION_DEFAULT) {
-if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR ||
-tpm->model == VIR_DOMAIN_TPM_MODEL_CRB ||
-qemuDomainIsARMVirt(def))
-tpm->data.emulator.version = VIR_DOMAIN_TPM_VERSION_2_0;
-else
+/* tpm-tis on x86 defaults to TPM 1.2 to preserve the
+ * historical behavior, but in all other scenarios we want
+ * TPM 2.0 instead */
+if (tpm->model == VIR_DOMAIN_TPM_MODEL_TIS &&
+ARCH_IS_X86(def->os.arch)) {
tpm->data.emulator.version = VIR_DOMAIN_TPM_VERSION_1_2;
+} else {
+tpm->data.emulator.version = VIR_DOMAIN_TPM_VERSION_2_0;
+}
}
return 0;
diff --git
a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
index a3a701b8e4..6712c2d831 100644
---
a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
+++
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.abi-update.xml
@@ -59,7 +59,7 @@
-
+
diff --git
a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
index a3a701b8e4..6712c2d831 100644
--- a/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
+++ b/tests/qemuxmlconfdata/riscv64-virt-default-models.riscv64-latest.xml
@@ -59,7 +59,7 @@
-
+
--
2.45.1
[PATCH v2 1/4] tests: Add TPM coverage to default-models tests
We have a non-trivial amount of architecture-specific logic
dealing with TPM, so it's good to have coverage for it.
Note that two architectures currently don't have support for
TPM devices enabled by default in QEMU: loongarch64 and s390x.
The situation might change for the former, but that's unlikely
to happen for the latter.
Signed-off-by: Andrea Bolognani
---
...aarch64-virt-default-models.aarch64-latest.abi-update.args | 3 +++
.../aarch64-virt-default-models.aarch64-latest.abi-update.xml | 3 +++
.../aarch64-virt-default-models.aarch64-latest.args | 3 +++
.../aarch64-virt-default-models.aarch64-latest.xml| 3 +++
tests/qemuxmlconfdata/aarch64-virt-default-models.xml | 3 +++
tests/qemuxmlconfdata/loongarch64-virt-default-models.xml | 3 +++
.../ppc64-pseries-default-models.ppc64-latest.abi-update.args | 3 +++
.../ppc64-pseries-default-models.ppc64-latest.abi-update.xml | 4
.../ppc64-pseries-default-models.ppc64-latest.args| 3 +++
.../ppc64-pseries-default-models.ppc64-latest.xml | 4
tests/qemuxmlconfdata/ppc64-pseries-default-models.xml| 3 +++
...riscv64-virt-default-models.riscv64-latest.abi-update.args | 3 +++
.../riscv64-virt-default-models.riscv64-latest.abi-update.xml | 3 +++
.../riscv64-virt-default-models.riscv64-latest.args | 3 +++
.../riscv64-virt-default-models.riscv64-latest.xml| 3 +++
tests/qemuxmlconfdata/riscv64-virt-default-models.xml | 3 +++
tests/qemuxmlconfdata/s390x-ccw-default-models.xml| 2 ++
.../x86_64-pc-default-models.x86_64-latest.abi-update.args| 3 +++
.../x86_64-pc-default-models.x86_64-latest.abi-update.xml | 3 +++
.../x86_64-pc-default-models.x86_64-latest.args | 3 +++
.../x86_64-pc-default-models.x86_64-latest.xml| 3 +++
tests/qemuxmlconfdata/x86_64-pc-default-models.xml| 3 +++
.../x86_64-q35-default-models.x86_64-latest.abi-update.args | 3 +++
.../x86_64-q35-default-models.x86_64-latest.abi-update.xml| 3 +++
.../x86_64-q35-default-models.x86_64-latest.args | 3 +++
.../x86_64-q35-default-models.x86_64-latest.xml | 3 +++
tests/qemuxmlconfdata/x86_64-q35-default-models.xml | 3 +++
27 files changed, 82 insertions(+)
diff --git
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
index 0c4acf800f..a503f45d0c 100644
---
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
+++
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
@@ -38,6 +38,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config
\
-device
'{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.2","addr":"0x0"}'
\
-chardev pty,id=charserial0 \
-serial chardev:charserial0 \
+-chardev socket,id=chrtpm,path=/dev/test \
+-tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \
+-device '{"driver":"tpm-tis-device","tpmdev":"tpm-tpm0","id":"tpm0"}' \
-audiodev '{"id":"audio1","driver":"none"}' \
-device
'{"driver":"virtio-gpu-pci","id":"video0","max_outputs":1,"bus":"pci.5","addr":"0x0"}'
\
-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
diff --git
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
index 87be062c89..bbe1dd931d 100644
---
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
+++
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
@@ -69,6 +69,9 @@
+
+
+
diff --git
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
index 0c4acf800f..a503f45d0c 100644
--- a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
+++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
@@ -38,6 +38,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config
\
-device
'{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:09:a4:37","bus":"pci.2","addr":"0x0"}'
\
-chardev pty,id=charserial0 \
-serial chardev:charserial0 \
+-chardev socket,id=chrtpm,pa
[PATCH v2 4/4] qemu: Reject TPM 1.2 in most scenarios
Everywhere we use TPM 2.0 as our default, the chances of TPM
1.2 being supported by the guest OS are very slim. Just reject
such configurations outright.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_validate.c | 22 ---
...aarch64-tpm-wrong-model.aarch64-latest.err | 2 +-
2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index c08e1538f9..95af93d606 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4755,23 +4755,19 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
switch (version) {
case VIR_DOMAIN_TPM_VERSION_1_2:
-/* TPM 1.2 + CRB do not work */
-if (tpm->model == VIR_DOMAIN_TPM_MODEL_CRB) {
+/* Only tpm-tis supports TPM 1.2, and even that is only
+ * on x86: for all other models and architectures, we
+ * want TPM 2.0 */
+if (tpm->model != VIR_DOMAIN_TPM_MODEL_TIS) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Unsupported interface '%1$s' for TPM 1.2"),
+ _("TPM 1.2 is not supported for model '%1$s'"),
virDomainTPMModelTypeToString(tpm->model));
return -1;
}
-/* TPM 1.2 + SPAPR do not work with any 'type' (backend) */
-if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("TPM 1.2 is not supported with the SPAPR
device model"));
-return -1;
-}
-/* TPM 1.2 + ARM does not work */
-if (qemuDomainIsARMVirt(def)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("TPM 1.2 is not supported on ARM"));
+if (!ARCH_IS_X86(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("TPM 1.2 is not supported on architecture
'%1$s'"),
+ virArchToString(def->os.arch));
return -1;
}
break;
diff --git a/tests/qemuxmlconfdata/aarch64-tpm-wrong-model.aarch64-latest.err
b/tests/qemuxmlconfdata/aarch64-tpm-wrong-model.aarch64-latest.err
index a3a82fdcf5..44c6e7372b 100644
--- a/tests/qemuxmlconfdata/aarch64-tpm-wrong-model.aarch64-latest.err
+++ b/tests/qemuxmlconfdata/aarch64-tpm-wrong-model.aarch64-latest.err
@@ -1 +1 @@
-unsupported configuration: TPM 1.2 is not supported on ARM
+unsupported configuration: TPM 1.2 is not supported on architecture 'aarch64'
--
2.45.1
Re: [PATCH] security: AppArmor allow write when os loader readonly=no
On Tue, Jun 04, 2024 at 02:20:14PM GMT, Christian Ehrhardt wrote:
> On Tue, Jun 4, 2024 at 1:17 PM Miroslav Los via Devel
> wrote:
> > -if (ctl->def->os.loader && ctl->def->os.loader->path)
> > -if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0)
> > +if (ctl->def->os.loader && ctl->def->os.loader->path) {
> > +bool readonly = false;
> > +virTristateBoolToBool(ctl->def->os.loader->readonly, &readonly);
> > +if (vah_add_file(&buf,
> > + ctl->def->os.loader->path,
> > + readonly ? "rk" : "rwk") != 0)
>
> Not tested, but the approach looks totally reasonable and in line with
> the usual virt-aa-helper handling of such cases.
>
> Signed-off-by: Christian Ehrhardt
It looks reasonable to me too, but I'd like to see someone other than
the author take it for a spin. Christian, can you please give it a
shot? Once we have your Tested-by, I'll happily throw in my
Reviewed-by and push the patch.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Mon, Jun 10, 2024 at 09:10:08PM GMT, Roman Bogorodskiy wrote: > Laine Stump wrote: > > > This patch series enables libvirt to use nftables rules rather than > > iptables *when setting up virtual networks* (it does *not* add > > nftables support to the nwfilter driver). It accomplishes this by > > abstracting several iptables functions (from viriptables.[ch] called > > by the virtual network driver into a rudimentary "virNetfilter API" > > (in virnetfilter.[ch], having the virtual network driver call the > > virNetFilter API rather than calling the existing iptables functions > > directly, and then finally adding an equivalent virNftables backend > > that can be used instead of iptables (selected manually via a > > network.conf setting, or automatically if iptables isn't found on the > > host). > > [resend to a proper list] > > Hi, > > Apparently, I'm late to the discussion. > > I noticed that now I cannot use the bridge driver on FreeBSD as it's > failing to initialize both iptables and nftables backends (which is > expect). > > What would be a good way to address that? I see at least two options: > > 1. Add a Noop firewall driver > 2. Implement a "real" FreeBSD driver based either on pf or ipfw (that's > been on my TODO list forever, but I somehow got stuck on the very first > step on choosing between pf and ipfw). This obviously will take much > more time. > > Maybe there are other options I'm missing. > > What do you think? If I understand correctly, the new behavior might cause problems on macOS too, see [1]. I wouldn't be surprised if the other BSDs were similarly affected. I wonder how things could work before if the iptables backend is not functional. Is it possible that we used to ignore failures to initialize the backend, but now we consider them fatal instead? A proper platform-specific backend would obviously be the right approach in the long run, but the priority should be restoring the previous status quo. A noop backend might be the answer, but honestly I just don't understand enough about networking to know for sure. I thought that these firewall rules were necessary in order to give network access to VMs, but if FreeBSD has been doing fine without iptables so far clearly that's not the case? [1] https://gitlab.com/libvirt/libvirt/-/issues/642 -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Tue, Jun 11, 2024 at 02:38:58AM GMT, Andrea Bolognani wrote: > On Mon, Jun 10, 2024 at 09:10:08PM GMT, Roman Bogorodskiy wrote: > > Laine Stump wrote: > > > > > This patch series enables libvirt to use nftables rules rather than > > > iptables *when setting up virtual networks* (it does *not* add > > > nftables support to the nwfilter driver). It accomplishes this by > > > abstracting several iptables functions (from viriptables.[ch] called > > > by the virtual network driver into a rudimentary "virNetfilter API" > > > (in virnetfilter.[ch], having the virtual network driver call the > > > virNetFilter API rather than calling the existing iptables functions > > > directly, and then finally adding an equivalent virNftables backend > > > that can be used instead of iptables (selected manually via a > > > network.conf setting, or automatically if iptables isn't found on the > > > host). > > > > [resend to a proper list] > > > > Hi, > > > > Apparently, I'm late to the discussion. > > > > I noticed that now I cannot use the bridge driver on FreeBSD as it's > > failing to initialize both iptables and nftables backends (which is > > expect). > > > > What would be a good way to address that? I see at least two options: > > > > 1. Add a Noop firewall driver > > 2. Implement a "real" FreeBSD driver based either on pf or ipfw (that's > > been on my TODO list forever, but I somehow got stuck on the very first > > step on choosing between pf and ipfw). This obviously will take much > > more time. > > > > Maybe there are other options I'm missing. > > > > What do you think? > > If I understand correctly, the new behavior might cause problems on > macOS too, see [1]. I wouldn't be surprised if the other BSDs were > similarly affected. > > I wonder how things could work before if the iptables backend is not > functional. Is it possible that we used to ignore failures to > initialize the backend, but now we consider them fatal instead? > > A proper platform-specific backend would obviously be the right > approach in the long run, but the priority should be restoring the > previous status quo. A noop backend might be the answer, but honestly > I just don't understand enough about networking to know for sure. I > thought that these firewall rules were necessary in order to give > network access to VMs, but if FreeBSD has been doing fine without > iptables so far clearly that's not the case? One additional issue with this: $ PATH=/usr/bin /usr/sbin/libvirtd error : virNetworkLoadDriverConfig:146 : internal error: could not find a usable firewall backend error : virStateInitialize:672 : Initialization of bridge state driver failed: internal error: could not find a usable firewall backend error : daemonRunStateInit:617 : Driver state initialization failed This happens because nft and iptables are both in /usr/sbin, so if the user's $PATH doesn't include that directory they won't be found and the driver will fail to initialize. Not a big deal on Fedora, where /usr/sbin is part of the default $PATH for users, but that's not the case on Debian, where qemu:///session is just completely broken right now. I was testing out a patch that addressed the situation by switching backend detection to virFindFileInPathFull(), but then I realized that it's fairly pointless to look for nft/iptables when a regular user can't run them anyway. So what I think we need to do is, make the failure to detect a working backend non-fatal, unless the user has explicitly asked for a specific backend to be used. That should bring us back to the previous situation. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Tue, Jun 11, 2024 at 05:27:42PM GMT, Daniel P. Berrangé wrote: > On Tue, Jun 11, 2024 at 08:49:42AM -0700, Andrea Bolognani wrote: > > One additional issue with this: > > > > $ PATH=/usr/bin /usr/sbin/libvirtd > > error : virNetworkLoadDriverConfig:146 : internal error: could not > > find a usable firewall backend > > error : virStateInitialize:672 : Initialization of bridge state > > driver failed: internal error: could not find a usable firewall > > backend > > error : daemonRunStateInit:617 : Driver state initialization failed > > > > This happens because nft and iptables are both in /usr/sbin, so if > > the user's $PATH doesn't include that directory they won't be found > > and the driver will fail to initialize. > > > > Not a big deal on Fedora, where /usr/sbin is part of the default > > $PATH for users, but that's not the case on Debian, where > > qemu:///session is just completely broken right now. > > > > I was testing out a patch that addressed the situation by switching > > backend detection to virFindFileInPathFull(), but then I realized > > that it's fairly pointless to look for nft/iptables when a regular > > user can't run them anyway. > > > > So what I think we need to do is, make the failure to detect a > > working backend non-fatal, unless the user has explicitly asked for a > > specific backend to be used. That should bring us back to the > > previous situation. > > This is probably another reason to have a "no op" backend that merely > raises errors for every operation - see my Roman's mail about FreeBSD Is there much of a difference between having an explicit noop backend that is checked for availability after all other ones, and simply not failing to initialize the driver if a backend can't be found? Anyway, I'd be happy with either solution. I'm still unclear on how networking on FreeBSD could work at all until now. Aren't the iptables rules needed for guest connectivity? Or did I misunderstand their purpose? -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Wed, Jun 12, 2024 at 09:57:15AM GMT, Daniel P. Berrangé wrote: > On Wed, Jun 12, 2024 at 01:54:47AM -0700, Andrea Bolognani wrote: > > Is there much of a difference between having an explicit noop backend > > that is checked for availability after all other ones, and simply not > > failing to initialize the driver if a backend can't be found? > > I actually sent a patch for the latter last night Awesome, thanks! > > I'm still unclear on how networking on FreeBSD could work at all > > until now. Aren't the iptables rules needed for guest connectivity? > > Or did I misunderstand their purpose? > > It wouldn't have worked, but the problem is that we now kill the > entire libvirtd startup, instead of successfully starting a (broken) > network driver. Both are broken, but now the brokenness has spread > to the bits that do matter. I get that, it's just that I'd be extremely surprised to learn that guest network connectivity hasn't worked on FreeBSD all this time. Surely that can't be right! Roman, what am I missing? -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Wed, Jun 12, 2024 at 08:42:48AM GMT, Laine Stump wrote: > On 6/12/24 6:47 AM, Daniel P. Berrangé wrote: > > On Wed, Jun 12, 2024 at 03:27:24AM -0700, Andrea Bolognani wrote: > > > [...] I'd be extremely surprised to learn that > > > guest network connectivity hasn't worked on FreeBSD all this time. > > > Surely that can't be right! Roman, what am I missing? > > > > This is only the libvirt virtual network backend. I presume BSD hosted > > guests could just use one of the other network backend options. > > Based on the wording of Roman's initial message, I wondered if possibly > people had been using the virtual network driver with > - this wouldn't ever call any firewall functions, so it should succeed. It looks like it fails before it can even get to the point where firewall rules would be created: # virsh net-start default error: Failed to start network default error: Unable to create bridge device: Invalid argument For reference, here's what the configuration looks like: # virsh net-dumpxml default default 2bd47e50-eab7-4988-b7a5-7da41a53f9c8 > I'm > pretty sure none of the other network types are supported on BSD > (macvtap/direct, or pools of SRIOV VFs used via VFIO device assignment). Maybe works? I'm not even sure why the network driver is enabled on FreeBSD in the first place. Only the QEMU driver can use it, right? And that's compiled out by default on FreeBSD, if I'm interpreting the port[1] correctly. So, at the very least, I would expect the network driver to only be enabled when the QEMU driver is, i.e. not in the default binary package. [1] https://github.com/freebsd/freebsd-ports/blob/main/devel/libvirt/Makefile -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Wed, Jun 12, 2024 at 10:42:43AM GMT, Laine Stump wrote: > On 6/12/24 9:18 AM, Andrea Bolognani wrote: > > On Wed, Jun 12, 2024 at 08:42:48AM GMT, Laine Stump wrote: > > > On 6/12/24 6:47 AM, Daniel P. Berrangé wrote: > > > > On Wed, Jun 12, 2024 at 03:27:24AM -0700, Andrea Bolognani wrote: > > > > > [...] I'd be extremely surprised to learn that > > > > > guest network connectivity hasn't worked on FreeBSD all this time. > > > > > Surely that can't be right! Roman, what am I missing? > > > > > > > > This is only the libvirt virtual network backend. I presume BSD hosted > > > > guests could just use one of the other network backend options. > > > > > > Based on the wording of Roman's initial message, I wondered if possibly > > > people had been using the virtual network driver with > > mode='open'/> > > > - this wouldn't ever call any firewall functions, so it should succeed. > > > > It looks like it fails before it can even get to the point where > > firewall rules would be created: > > > ># virsh net-start default > >error: Failed to start network default > >error: Unable to create bridge device: Invalid argument > > Okay, then I guess I read too much into what Roman said: > > I noticed that now I cannot use the bridge driver > on FreeBSD as it's failing to initialize both > iptables and nftables backends > > I figured that meant the bridge driver (aka the network driver) had > previously been usable on FreeBSD, but if your test is typical, then that's > not the case; maybe only works, and Roman just > assumed that the network driver was needed in order for that to function. This is a bog-standard FreeBSD installation, so whatever I'm seeing is the out-of-the-box experience. Maybe there's a way to get things working with further tweaking, I don't know. > If a platform supports standard tap devices (which FreeBSD does), the three > things the network driver needs to function properly are 1) a functioning > firewall backend, 2) dnsmasq, and 3) the ability to manage a bridge device > (all the functions in virnetdevbridge.c). (1) is obviously missing, but (2) > is present on FreeBSD, and it looks like, at least for some *BSDs, (3) is > also available (there is a build-time flag WITH_BSD_BRIDGE_MGMT that is set > if certain ioctls are defined in net/if_bridgevar.h). > > Is WITH_BSD_BRIDGE_MGMT set in your FreeBSD build? Yes. > Does net/if_bridgevar.h exist? Also yes. > > I'm not even sure why the network driver is enabled on FreeBSD in the > > first place. Only the QEMU driver can use it, right? And that's > > compiled out by default on FreeBSD, if I'm interpreting the port[1] > > correctly. So, at the very least, I would expect the network driver > > to only be enabled when the QEMU driver is, i.e. not in the default > > binary package. More on this. When installing libvirt, the following message is printed out: NOTE ON CONFIGURATION: The libvirt port does not come with networking configuration enabled. The 'default' network definition is available at: /usr/local/share/examples/libvirt/networks/default.xml To enable this network please do the following: cp /usr/local/share/examples/libvirt/networks/default.xml /usr/local/etc/libvirt/qemu/networks To configure this network for autostart, execute the following: ln -s ../default.xml /usr/local/etc/libvirt/qemu/networks/autostart/default.xml If you have libvirtd already running you'll need to restart it for changes to take effect. As seen earlier, these instructions are pointless, as the default network won't be able to start. But the fact that they exist at all seems to indicate that, at some point, the default network could work on FreeBSD? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 0/2] network: fix network driver to gracefully skip startup
On Tue, Jun 11, 2024 at 05:47:56PM GMT, Daniel P. Berrangé wrote: > We should gracefully skip startup when: > > * No network.conf firewall_backend is explicitly set, and >neither iptables/nftables are present > * Running unprivileged > > The former fixes libvirtd startup on non-Linux, or minimal linux > installs without firewall tools. > > The latter skips pointless initialization that creates a driver > that cannot do anything useful > > Daniel P. Berrangé (2): > network: skip network driver init if no firewall backend is present > network: don't attempt to initialize if non-privileged > > src/network/bridge_driver.c | 14 +- > src/network/bridge_driver_conf.c | 8 > 2 files changed, 17 insertions(+), 5 deletions(-) This makes things better, in that libvirtd doesn't completely fail to start. However, we're still not handling the scenario quite gracefully: $ PATH=/usr/bin virsh -c qemu:///session net-list error: Disconnected from qemu:///session due to end of file error: Failed to list networks error: End of file while reading data: Input/output error libvirtd doesn't stick around after this, and existing sessions (if any) get forcefully disconnected. I think that having a proper null backend would allow us to handle this better, by reporting driver initialization as successful and only failing actual API calls. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Wed, Jun 12, 2024 at 07:31:51PM GMT, Laine Stump wrote: > On 6/12/24 2:32 PM, Roman Bogorodskiy wrote: > > I'm using it with the following network configuration: > > > > virsh # net-dumpxml default > > > >default > >2a1415c9-325b-41e4-82c6-e805162d8934 > > > > > > > > > > > > > > > > > > My configuration is the same (obtained from copying the file shipped as /usr/local/share/examples/libvirt/networks/default.xml, which is identical to src/network/default.xml.in in the libvirt tree) and I get an error when I try to start the network: # virsh net-start default error: Failed to start network default error: Unable to create bridge device: Invalid argument The debug log reveals the source of the error to be virNetDevBridgeCreate:474 I don't understand how that would work for you. My setup is completely vanilla, just a plain FreeBSD 14.1 install. The only thing that could possibly be making any difference is that the host's network interface is a wireless one. > > So basically all the mechanics like creating tap devices, bridges, > > serving dhcp, etc, all these work for me. On top of that I had a few > > iterations of manual firewall configurations (with both ipfw and pf) > > to implement NAT on guests. > > Okay, so essentially it was effectively (since the > only difference is the lack of libvirt-added firewall rules). > [...] > > I think even with a NULL firewall backend it still > should log an error and fail if someone tries to start a network that > requires firewall rules though (i.e. your above config would still fail, > unless it's changed to ) I agree. The fact that this was allowed in the first place is arguably a bug > > > I'm about to be offline for 3 weeks, but in the meantime if you'd like to > > > try making a NULL backend that is only an option if it's listed in > > > firewall_backend_priority (you'll need to remove the compile-time check > > > that > > > all possible backends are accounted for - I think that is the first of the > > > two G_STATIC_ASSERTS at the top of virNetworkLoadDriverConfig()), always > > > initializes successfully in bridge_driver_conf.c if it is listed in the > > > options, and then in networkAddFirewallRules add a check to log an error > > > and > > > fail if backend == NULL (something about attempting to start a network > > > type > > > that would require firewall rules, but the system not having any of the > > > supported types of firewallbackend or something - it's too late now and my > > > brain is too fried and sleepy to think of good wording :-)). As long as it > > > isn't a valid selection on Linux builds that are done with > > > firewall_backend_priority=nftables,iptables, but *is* a valid selection if > > > the setting is "firewall_backend_priority=null" that shouldn't be *too* > > > controversial. I think we can treat the null backend just like the other ones. New default: -Dfirewall_backend_priority=nftables,iptables,null Works fine on Linux (even for qemu:///session) and FreeBSD. If you don't want firewall rules to ever be created (rules out mode=nat) for whatever reason: firewall_backend = "null" Same as above, but at compile time: -Dfirewall_backend_priority=null,nftables,iptables If you changed your mind later but don't want to recompile, or disagree with the package maintainer: firewall_backend = "iptables" Seems pretty reasonable and seamless to me. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Thu, Jun 13, 2024 at 06:24:00PM GMT, Roman Bogorodskiy wrote: > Laine Stump wrote: > > On 6/12/24 2:32 PM, Roman Bogorodskiy wrote: > > > So basically all the mechanics like creating tap devices, bridges, > > > serving dhcp, etc, all these work for me. On top of that I had a few > > > iterations of manual firewall configurations (with both ipfw and pf) > > > to implement NAT on guests. > > > > Okay, so essentially it was effectively (since the > > only difference is the lack of libvirt-added firewall rules). > > Yes, I've changed it to mode='open' and it still works for me, i.e. I > have host<->guest connectivity. > > So it would make more sense to change the default to 'open', at least > until nat is not supported. I think I'd better do that on the packaging > level. If you're touching the package, can you also look into the other question that I've raised about it? Namely that, since the QEMU driver is compiled out by default, the same should probably be the case for the network driver too. If someone goes and builds the port locally with QEMU support enabled, then and only then the network driver should be included. Honestly I'm not entirely sure it makes much sense to have the network driver and especially the default network if you need to bring your own firewall rules, but that can be a separate discussion. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Thu, Jun 13, 2024 at 07:45:52PM GMT, Roman Bogorodskiy wrote: > > My configuration is the same (obtained from copying the file shipped > > as /usr/local/share/examples/libvirt/networks/default.xml, which is > > identical to src/network/default.xml.in in the libvirt tree) and I > > get an error when I try to start the network: > > > > # virsh net-start default > > error: Failed to start network default > > error: Unable to create bridge device: Invalid argument > > > > The debug log reveals the source of the error to be > > > > virNetDevBridgeCreate:474 > > > > I don't understand how that would work for you. My setup is > > completely vanilla, just a plain FreeBSD 14.1 install. The only thing > > that could possibly be making any difference is that the host's > > network interface is a wireless one. > > Looks like you don't have the if_bridge kernel module loaded. > > If you run 'virt-host-validate', it should show if something's missing. Yeah, that was it. Probably worth adding a hint to the message displayed upon package installation? -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Thu, Jun 13, 2024 at 08:00:32PM GMT, Roman Bogorodskiy wrote: > Andrea Bolognani wrote: > > Honestly I'm not entirely sure it makes much sense to have the > > network driver and especially the default network if you need to > > bring your own firewall rules, but that can be a separate discussion. > > Hm, I think the network driver is quite usable without QEMU, e.g. I use > it with bhyve. Okay, I didn't realize that was an option. Which leads me to open a different can of worms then: if libvirt networks can be used with drivers other than QEMU, wouldn't it make sense for their configuration to live in /etc/libvirt/network instead of /etc/libvirt/qemu/networks? How difficult would it be to adopt the new path without breaking existing setups? > I also find it quite useful even without firewall rules. Most of the > time internal connectivity is enough for my guests. Configuring NAT on > per-network basis is also fairly easy. For more advanced scenarios hooks > could be used, though I haven't done that specifically. VMs with no connectivity to the outside world are of very limited use IMO. At the very least, a warning about the fact that connectivity is limited could be displayed upon package installation. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/28] native support for nftables in virtual network driver
On Fri, Jun 14, 2024 at 03:34:41PM GMT, Roman Bogorodskiy wrote: > Andrea Bolognani wrote: > > On Thu, Jun 13, 2024 at 07:45:52PM GMT, Roman Bogorodskiy wrote: > > > Looks like you don't have the if_bridge kernel module loaded. > > > > > > If you run 'virt-host-validate', it should show if something's missing. > > > > Yeah, that was it. Probably worth adding a hint to the message > > displayed upon package installation? > > It's already referring to https://libvirt.org/drvbhyve.html which > mentions that additional kernel modules should be loaded, so I think > that should be good enough. I respectfully disagree, but then again I'm not going to write a patch myself either so you're absolutely entitled to disregard this suggestion :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] network: introduce a "none" firewall backend type
On Fri, Jun 14, 2024 at 03:43:53PM GMT, Daniel P. Berrangé wrote:
> meson.build | 26 +++---
> meson_options.txt | 2 +-
> src/network/bridge_driver_conf.c | 19 ++-
> src/network/bridge_driver_linux.c | 10 ++
> src/network/bridge_driver_nop.c | 15 ++-
> src/util/virfirewall.c| 6 ++
> src/util/virfirewall.h| 1 +
> 7 files changed, 65 insertions(+), 14 deletions(-)
The test suite no longer passes after applying this. At the very
least, you need to squash in the diff at the bottom of this message.
>firewall_backend_priority = get_option('firewall_backend_priority')
> - if (not firewall_backend_priority.contains('nftables') or
> - not firewall_backend_priority.contains('iptables') or
> - firewall_backend_priority.length() != 2)
> -error('invalid value for firewall_backend_priority option')
> + if firewall_backend_priority.length() == 0
> + if host_machine.system() == 'linux'
> + firewall_backend_priority = ['nftables', 'iptables']
> + else
> + # No firewall impl on non-Linux so far, so force 'none'
> + # as placeholder
> + firewall_backend_priority = ['none']
> + endif
> + else
> + if host_machine.system() != 'linux'
> + error('firewall backend priority only supported on linux hosts')
> + endif
>endif
This implementation allows things such as
-Dfirewall_backend_priority=nftables
and
-Dfirewall_backend_priority=iptables,iptables
At least
-Dfirewall_backend_priority=iptables,nftables,iptables
will be blocked, but only because it results in a compilation error:
meson will happily accept it.
Are we okay with that? It's IMO inferior to the much stricter
checking that's performed today.
> @@ -815,6 +816,11 @@ virFirewallApplyCmd(virFirewall *firewall,
> switch (virFirewallGetBackend(firewall)) {
> +case VIR_FIREWALL_BACKEND_NONE:
> +virReportError(VIR_ERR_NO_SUPPORT,
> + _("Firewall backend is not implemented"));
> +return -1;
This check (and the other ones you've introduced) are running too
late, which leads to this behavior:
$ cat default-session.xml
default-session
$ virsh -c qemu:///session net-define default-session.xml
Network default-session defined from default-session.xml
$ virsh -c qemu:///session net-start default-session
error: Failed to start network default-session
error: error creating bridge interface virbr0: Operation not permitted
Since requires firewall rules, we shouldn't
allow a network that uses it to be defined in the first place.
diff --git a/po/POTFILES b/po/POTFILES
index 4bfbb91164..1ed4086d2c 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -143,6 +143,7 @@ src/lxc/lxc_process.c
src/network/bridge_driver.c
src/network/bridge_driver_conf.c
src/network/bridge_driver_linux.c
+src/network/bridge_driver_nop.c
src/network/leaseshelper.c
src/network/network_iptables.c
src/network/network_nftables.c
diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
index 2114d521d1..323990cf17 100644
--- a/src/network/bridge_driver_nop.c
+++ b/src/network/bridge_driver_nop.c
@@ -49,8 +49,8 @@ int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
*/
if (firewallBackend != VIR_FIREWALL_BACKEND_NONE) {
virReportError(VIR_ERR_NO_SUPPORT,
- _("Firewall backend '%s' not available on this
platform"),
- virFirewallBackendTypeToString(firewallBackend));
+ _("Firewall backend '%1$s' not available on
this platform"),
+ virFirewallBackendTypeToSTring(firewallBackend));
return -1;
}
return 0;
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index d374f54b64..090dbcdbed 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -817,7 +817,7 @@ virFirewallApplyCmd(virFirewall *firewall,
switch (virFirewallGetBackend(firewall)) {
case VIR_FIREWALL_BACKEND_NONE:
-virReportError(VIR_ERR_NO_SUPPORT,
+virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("Firewall backend is not implemented"));
return -1;
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] network: introduce a "none" firewall backend type
On Fri, Jun 14, 2024 at 07:52:55PM GMT, Daniel P. Berrangé wrote:
> On Fri, Jun 14, 2024 at 12:22:50PM -0400, Andrea Bolognani wrote:
> > On Fri, Jun 14, 2024 at 03:43:53PM GMT, Daniel P. Berrangé wrote:
> > > @@ -815,6 +816,11 @@ virFirewallApplyCmd(virFirewall *firewall,
> > > switch (virFirewallGetBackend(firewall)) {
> > > +case VIR_FIREWALL_BACKEND_NONE:
> > > +virReportError(VIR_ERR_NO_SUPPORT,
> > > + _("Firewall backend is not implemented"));
> > > +return -1;
> >
> > This check (and the other ones you've introduced) are running too
> > late, which leads to this behavior:
> >
> > $ cat default-session.xml
> >
> > default-session
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > $ virsh -c qemu:///session net-define default-session.xml
> > Network default-session defined from default-session.xml
> >
> > $ virsh -c qemu:///session net-start default-session
> > error: Failed to start network default-session
> > error: error creating bridge interface virbr0: Operation not permitted
> >
> > Since requires firewall rules, we shouldn't
> > allow a network that uses it to be defined in the first place.
>
> This is the behaviour we already had before the nftables backend
> was created, and its not been a problem. There's no functional
> reason why we should refuse to allow it to be defined, if the
> binaries aren't needed until startup.
But in the case of qemu:///session, or when we're not on Linux, we
already know that there is no way for the network that's being
defined to ever work.
To me that's the same as allowing a guest to be defined, when the
corresponding QEMU binary doesn't support some of the devices. Or
when a firmware binary satisfying the constraints isn't available. We
reject such configurations for guests, and it would just be
consistent to do the same for networks.
Besides, even if we decide that we want to allow such networks to be
defined, we should report a more meaningful error for the failed
startup, one which points out that mode=nat will never work. It
shouldn't be the same error message that I was getting on FreeBSD
because of a kernel driver that wasn't loaded, as the user's ability
to make the error go away is completely different in the two
scenarios.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] network: introduce a "none" firewall backend type
On Mon, Jun 17, 2024 at 09:36:15AM GMT, Daniel P. Berrangé wrote: > On Fri, Jun 14, 2024 at 12:22:50PM -0400, Andrea Bolognani wrote: > > This implementation allows things such as > > > > -Dfirewall_backend_priority=nftables > > > > and > > > > -Dfirewall_backend_priority=iptables,iptables > > > > At least > > > > -Dfirewall_backend_priority=iptables,nftables,iptables > > > > will be blocked, but only because it results in a compilation error: > > meson will happily accept it. > > > > Are we okay with that? It's IMO inferior to the much stricter > > checking that's performed today. > > I found that if you try this with meson you'll see this > > DEPRECATION: Duplicated values in array option is deprecated. This will > become a hard error in the future. > > I think we're fine to delegate this to Meson, given its intent to turn > this into a hard error eventually, since duplication is harmless for > us in the short term. I didn't realize that. Sounds good! -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2] network: introduce a "none" firewall backend type
On Mon, Jun 17, 2024 at 01:40:16PM GMT, Daniel P. Berrangé wrote: > On Mon, Jun 17, 2024 at 04:44:01AM -0400, Andrea Bolognani wrote: > > On Fri, Jun 14, 2024 at 07:52:55PM GMT, Daniel P. Berrangé wrote: > > > This is the behaviour we already had before the nftables backend > > > was created, and its not been a problem. There's no functional > > > reason why we should refuse to allow it to be defined, if the > > > binaries aren't needed until startup. > > > > But in the case of qemu:///session, or when we're not on Linux, we > > already know that there is no way for the network that's being > > defined to ever work. > > > > To me that's the same as allowing a guest to be defined, when the > > corresponding QEMU binary doesn't support some of the devices. Or > > when a firmware binary satisfying the constraints isn't available. We > > reject such configurations for guests, and it would just be > > consistent to do the same for networks. > > FWIW, I find the QEMU behaviour really unpleasant / hostile, when I'm > working with guests that reference a self-built QEMU, as the binary > often "doesn't exist", if I've cleaned by build temporarily. That makes things slightly annoying for developers, sure, but it's much more friendly towards users IMO. Getting an error sooner (define) rather than later (start) for situations where we know in advance that things can't possibly ever work is a lot better, which is also why libvirt erroring out is more user friendly than running QEMU and let if fail instead. > Overall, there's lots that could be improved in the network driver, > but for this, I'd really just like to focus on fixing the regression, > suc that we return to the behaviour we had historically. Clearly I'm not entirely happy with the behavior you're implementing with this, but I agree that it's still much better than leaving several common configurations completely broken. Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v3] network: introduce a "none" firewall backend type
On Mon, Jun 17, 2024 at 09:39:34AM GMT, Daniel P. Berrangé wrote: > There are two scenarios identified after the recent firewall backend > selection was introduced, which result in libvirtd failing to startup > due to an inability to find either iptables/nftables > > - On Linux if running unprivileged with $PATH lacking the dir >containing iptables/nftables > - On non-Linux where iptables/nftables never existed > > In the former case, it is preferrable to restore the behaviour whereby > the driver starts successfully. Users will get an error reported when > attempting to start any virtual network, due to the lack of permissions > needed to create bridge devices. This makes the missing firewall backend > irrelevant. > > In the latter case, the network driver calls the 'nop' platform > implementation which does not attempt to implement any firewall logic, > just allowing the network to start without firewall rules. > > To solve this are number of changes are required > > * Introduce VIR_FIREWALL_BACKEND_NONE, which does nothing except >report a fatal error from virFirewallApply(). This code path >is unreachable, since we'll never create a virFirewall >object with with VIR_FIREWALL_BACKEND_NONE, so the error reporting >is just a sanity check. > > * Ignore the compile time backend defaults and assume use of >the 'none' backend if running unprivileged. > >This fixes the first regression, avoiding the failure to start >libvirtd on Linux in unprivileged context, instead allowing use >of the driver and expecting a permission denied when creating a >bridge. > > * Reject the use of compile time backend defaults no non-Linux >and hardcode the 'none' backend. The non-Linux platforms have >no firewall implementation at all currently, so there's no >reason to permit the use of 'firewall_backend_priority' >meson option. > >This fixes the second regression, avoiding the failure to start >libvirtd on non-Linux hosts due to non-existant Linux binaries. > > * Change the Linux platform backend to raise an error if the >firewall backend is 'none'. Again this code path is unreachable >by default since we'll fail to create the bridge before getting >here, but if someone modified network.conf to request the 'none' >backend, this will stop further progress. > > * Change the nop platform backend to raise an error if the >firewall backend is 'iptables' or 'nftables'. Again this code >path is unreachable, since we should already have failed to >find the iptables/nftables binaries on non-Linux hosts, so >this is just a sanity check. > > * 'none' is not permited as a value in 'firewall_backend_priority' >meson option, since it is conceptually meaningless to ask for >that on Linux. > > NB, 'firewall_backend_priority' allows repeated options temporarily, > which we don't want. Meson intends to turn this into a hard error > > DEPRECATION: Duplicated values in array option is deprecated. This will > become a hard error in the future. > > and we can live with the reduced error checking until that happens. > > Signed-off-by: Daniel P. Berrangé > --- > > Changed in v3: > > - Fix various syntax-check errors > - Added note about meson tightening up validation of duplicated >choices > > meson.build | 26 +++--- > meson_options.txt | 2 +- > po/POTFILES | 1 + > src/network/bridge_driver_conf.c | 19 ++- > src/network/bridge_driver_linux.c | 10 ++ > src/network/bridge_driver_nop.c | 15 ++- > src/util/virfirewall.c| 6 ++ > src/util/virfirewall.h| 1 + > 8 files changed, 66 insertions(+), 14 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 1/4] tests: Add TPM coverage to default-models tests
On Tue, Jun 18, 2024 at 11:57:49AM GMT, lixianglai wrote: > Hi Andrea: > > We have a non-trivial amount of architecture-specific logic > > dealing with TPM, so it's good to have coverage for it. > > > > Note that two architectures currently don't have support for > > TPM devices enabled by default in QEMU: loongarch64 and s390x. > > The situation might change for the former, but that's unlikely > > to happen for the latter. > > loongarch64 has added support in the source code of qemu using TIS TPM2.0. > Before submission, we only carried out a simple test on TPM function, > which is still in the experimental stage. Up to now, after adding tpm > device, t > here will be an exception in the startup stage of guestos. > We don't have the energy to solve this problem at the moment, so please > skip it for now. Got it, thanks for confirming! -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] security: AppArmor allow write when os loader readonly=no
On Fri, Jun 07, 2024 at 09:39:30AM GMT, Andrea Bolognani wrote:
> On Tue, Jun 04, 2024 at 02:20:14PM GMT, Christian Ehrhardt wrote:
> > On Tue, Jun 4, 2024 at 1:17 PM Miroslav Los via Devel
> > wrote:
> > > -if (ctl->def->os.loader && ctl->def->os.loader->path)
> > > -if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0)
> > > +if (ctl->def->os.loader && ctl->def->os.loader->path) {
> > > +bool readonly = false;
> > > +virTristateBoolToBool(ctl->def->os.loader->readonly, &readonly);
> > > +if (vah_add_file(&buf,
> > > + ctl->def->os.loader->path,
> > > + readonly ? "rk" : "rwk") != 0)
> >
> > Not tested, but the approach looks totally reasonable and in line with
> > the usual virt-aa-helper handling of such cases.
> >
> > Signed-off-by: Christian Ehrhardt
>
> It looks reasonable to me too, but I'd like to see someone other than
> the author take it for a spin. Christian, can you please give it a
> shot? Once we have your Tested-by, I'll happily throw in my
> Reviewed-by and push the patch.
Unfortunately this has missed 10.5.0 now. Can we get it tested and
merged? Thanks!
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] virt-aa-helper: Allow RO access to /usr/share/edk2-ovmf
On Thu, Jul 04, 2024 at 01:13:36PM GMT, Michal Privoznik wrote: > When binary version of edk2 is distributed, the files reside > under /usr/share/edk2-ovmf as can be seen from Gentoo's ebuild > [1]. Allow virt-aa-helper to generate paths under that dir. > > 1: > https://gitweb.gentoo.org/repo/gentoo.git/tree/sys-firmware/edk2-ovmf-bin/edk2-ovmf-bin-202202.ebuild > Resolves: https://bugs.gentoo.org/911786 > Signed-off-by: Michal Privoznik > --- > src/security/virt-aa-helper.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > index 402cbd9602..076b98a1d7 100644 > --- a/src/security/virt-aa-helper.c > +++ b/src/security/virt-aa-helper.c > @@ -475,6 +475,7 @@ valid_path(const char *path, const bool readonly) > "/initrd", > "/initrd.img", > "/usr/share/edk2/", > +"/usr/share/edk2-ovmf/", > "/usr/share/OVMF/", /* for OVMF images */ > "/usr/share/ovmf/", /* for OVMF images */ > "/usr/share/AAVMF/", /* for AAVMF images */ For consistency with existing entries, you could add a /* for OVMF images */ comment to the right. Either way, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
[PATCH 1/6] tests: Update firmware descriptors
Sync with the edk2-20240524-4.fc39 package from Fedora.
The only notable change is that the inteltdx variant now declares
support for Secure Boot and is a ROM image instead of a stateless
pflash one.
The latter causes it to be considered eligible for the
configuration described by the firmware-auto-efi-rw test cases,
which now passes instead of failing.
Of course that doesn't make any sense, because a ROM image by
definition cannot be read/write. So this indicates the presence
of a bug in our firmware selection algorithm, which we're going
to address with an upcoming commit.
Signed-off-by: Andrea Bolognani
---
.../firmware/60-edk2-ovmf-x64-inteltdx.json | 10 +++---
.../firmware/60-edk2-ovmf-x64-inteltdx.json | 10 +++---
tests/qemufirmwaretest.c | 2 +-
...e-auto-efi-rw.x86_64-latest.abi-update.err | 1 -
.../firmware-auto-efi-rw.x86_64-latest.args | 34 +++
.../firmware-auto-efi-rw.x86_64-latest.err| 1 -
.../firmware-auto-efi-rw.x86_64-latest.xml| 6 +++-
tests/qemuxmlconftest.c | 3 +-
8 files changed, 49 insertions(+), 18 deletions(-)
delete mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.abi-update.err
create mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.args
delete mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.err
diff --git
a/tests/qemufirmwaredata/out/usr/share/qemu/firmware/60-edk2-ovmf-x64-inteltdx.json
b/tests/qemufirmwaredata/out/usr/share/qemu/firmware/60-edk2-ovmf-x64-inteltdx.json
index 35a625b3ec..d002ec7386 100644
---
a/tests/qemufirmwaredata/out/usr/share/qemu/firmware/60-edk2-ovmf-x64-inteltdx.json
+++
b/tests/qemufirmwaredata/out/usr/share/qemu/firmware/60-edk2-ovmf-x64-inteltdx.json
@@ -3,12 +3,8 @@
"uefi"
],
"mapping": {
-"device": "flash",
-"mode": "stateless",
-"executable": {
-"filename": "/usr/share/edk2/ovmf/OVMF.inteltdx.fd",
-"format": "raw"
-}
+"device": "memory",
+"filename": "/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd"
},
"targets": [
{
@@ -19,6 +15,8 @@
}
],
"features": [
+"enrolled-keys",
+"secure-boot",
"verbose-dynamic"
]
}
diff --git
a/tests/qemufirmwaredata/usr/share/qemu/firmware/60-edk2-ovmf-x64-inteltdx.json
b/tests/qemufirmwaredata/usr/share/qemu/firmware/60-edk2-ovmf-x64-inteltdx.json
index 44993ab1f3..445eb70e03 100644
---
a/tests/qemufirmwaredata/usr/share/qemu/firmware/60-edk2-ovmf-x64-inteltdx.json
+++
b/tests/qemufirmwaredata/usr/share/qemu/firmware/60-edk2-ovmf-x64-inteltdx.json
@@ -4,12 +4,8 @@
"uefi"
],
"mapping": {
-"device": "flash",
-"mode": "stateless",
-"executable": {
-"filename": "/usr/share/edk2/ovmf/OVMF.inteltdx.fd",
-"format": "raw"
-}
+"device": "memory",
+"filename": "/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd"
},
"targets": [
{
@@ -20,7 +16,9 @@
}
],
"features": [
+"enrolled-keys",
"intel-tdx",
+"secure-boot",
"verbose-dynamic"
],
"tags": [
diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c
index c967f86d68..a5e7e2ec65 100644
--- a/tests/qemufirmwaretest.c
+++ b/tests/qemufirmwaretest.c
@@ -317,7 +317,7 @@ mymain(void)
"/usr/share/edk2/ovmf/OVMF_CODE.fd:/usr/share/edk2/ovmf/OVMF_VARS.fd:"
"/usr/share/edk2/ovmf/OVMF.secboot.fd:NULL:"
"/usr/share/edk2/ovmf/OVMF.amdsev.fd:NULL:"
- "/usr/share/edk2/ovmf/OVMF.inteltdx.fd:NULL",
+ "/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd:NULL",
VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS,
VIR_DOMAIN_OS_DEF_FIRMWARE_EFI);
DO_SUPPORTED_TEST("pc-q35-3.1", VIR_ARCH_I686, false,
diff --git
a/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.abi-update.err
b/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.abi-update.err
deleted file mode 100644
index 3edb2b3451..00
--- a/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.abi-update.err
+++ /dev/null
@@ -1 +0,0 @@
-operation failed: Unable to find 'efi' firmware that is compatible with the
current configuration
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.args
[PATCH 0/6] qemu: Fixes to firmware selection
Andrea Bolognani (6):
tests: Update firmware descriptors
tests: Add more firmware selection coverage
qemu: Filter firmware images by type
qemu: ROM firmware images are always readonly
tests: Add firmware descriptor for edk2 on riscv64
tests: Add test for UEFI autoselection on riscv64
src/qemu/qemu_firmware.c | 17 +
.../qemu_5.2.0-tcg-virt.riscv64.xml | 4 ++-
.../qemu_5.2.0-virt.riscv64.xml | 4 ++-
.../qemu_8.0.0-tcg-virt.riscv64.xml | 4 ++-
.../qemu_8.0.0-virt.riscv64.xml | 4 ++-
.../firmware/60-edk2-ovmf-x64-inteltdx.json | 10 +++---
.../qemu/firmware/50-edk2-riscv-qcow2.json| 33 +
.../firmware/60-edk2-ovmf-x64-inteltdx.json | 10 +++---
tests/qemufirmwaretest.c | 7 +++-
...efi-riscv64.riscv64-latest.abi-update.args | 34 ++
...-efi-riscv64.riscv64-latest.abi-update.xml | 28 +++
.../firmware-auto-efi-riscv64.xml | 14
...ware-auto-efi-rw-pflash.x86_64-latest.err} | 0
...mware-auto-efi-rw-pflash.x86_64-latest.xml | 35 +++
.../firmware-auto-efi-rw-pflash.xml | 18 ++
tests/qemuxmlconftest.c | 3 +-
16 files changed, 207 insertions(+), 18 deletions(-)
create mode 100644
tests/qemufirmwaredata/usr/share/qemu/firmware/50-edk2-riscv-qcow2.json
create mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-riscv64.riscv64-latest.abi-update.args
create mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-riscv64.riscv64-latest.abi-update.xml
create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-riscv64.xml
rename
tests/qemuxmlconfdata/{firmware-auto-efi-rw.x86_64-latest.abi-update.err =>
firmware-auto-efi-rw-pflash.x86_64-latest.err} (100%)
create mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.xml
create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.xml
--
2.45.2
[PATCH 2/6] tests: Add more firmware selection coverage
This new test case covers the scenario in which the user
specifically asked for a read/write pflash image.
>From the output files, we can see that the firmware selection
algorithm has picked a ROM image, which demonstrates the
presence of another bug. We're going to fix it with an upcoming
commit.
Signed-off-by: Andrea Bolognani
---
...ware-auto-efi-rw-pflash.x86_64-latest.args | 34
...mware-auto-efi-rw-pflash.x86_64-latest.xml | 39 +++
.../firmware-auto-efi-rw-pflash.xml | 18 +
tests/qemuxmlconftest.c | 1 +
4 files changed, 92 insertions(+)
create mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.args
create mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.xml
create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.xml
diff --git
a/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.args
b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.args
new file mode 100644
index 00..753ad2d4b5
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.args
@@ -0,0 +1,34 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-guest \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-guest/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-guest/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=guest,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}'
\
+-machine pc-q35-4.0,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=on \
+-accel kvm \
+-cpu qemu64 \
+-bios /usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd \
+-m size=1048576k \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-global ICH9-LPC.noreboot=off \
+-watchdog-action reset \
+-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git
a/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.xml
b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.xml
new file mode 100644
index 00..fe05e33b69
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.xml
@@ -0,0 +1,39 @@
+
+ guest
+ 63840878-0deb-4095-97e6-fc444d9bc9fa
+ 1048576
+ 1048576
+ 1
+
+hvm
+
+
+
+
+/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd
+
+
+
+
+
+
+qemu64
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-x86_64
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.xml
b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.xml
new file mode 100644
index 00..0100585031
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.xml
@@ -0,0 +1,18 @@
+
+ guest
+ 63840878-0deb-4095-97e6-fc444d9bc9fa
+ 1048576
+ 1
+
+hvm
+
+
+
+
+
+
+/usr/bin/qemu-system-x86_64
+
+
+
+
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 351c7c59eb..07454f19c2 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1421,6 +1421,7 @@ mymain(void)
DO_TEST_CAPS_LATEST_ABI_UPDATE("firmware-auto-efi");
DO_TEST_CAPS_LATEST("firmware-auto-efi-stateless");
DO_TEST_CAPS_LATEST("firmware-auto-efi-rw");
+DO_TEST_CAPS_LATEST("firmware-auto-efi-rw-pflash");
DO_TEST_CAPS_LATEST("firmware-auto-efi-loader-secure");
DO_TEST_CAPS_LATEST_ABI_UPDATE("firmware-auto-efi-loader-secure");
DO_TEST_CAPS_LATEST("firmware-auto-efi-loader-insecure");
--
2.45.2
[PATCH 3/6] qemu: Filter firmware images by type
If the configuration explicitly requests a specific type of
firmware image, be it pflash or ROM, we should ignore all images
that are not of that type.
If no specific type has been requested, of course, any type is
considered a match and the selection will be based upon the
other attributes.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_firmware.c | 12 +++
...ware-auto-efi-rw-pflash.x86_64-latest.args | 34 ---
...mware-auto-efi-rw-pflash.x86_64-latest.err | 1 +
...mware-auto-efi-rw-pflash.x86_64-latest.xml | 6 +---
tests/qemuxmlconftest.c | 2 +-
5 files changed, 15 insertions(+), 40 deletions(-)
delete mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.args
create mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.err
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
index 424b0b3217..a0b13f76b8 100644
--- a/src/qemu/qemu_firmware.c
+++ b/src/qemu/qemu_firmware.c
@@ -1280,6 +1280,12 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
if (fw->mapping.device == QEMU_FIRMWARE_DEVICE_FLASH) {
const qemuFirmwareMappingFlash *flash = &fw->mapping.data.flash;
+if (loader && loader->type &&
+loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH) {
+VIR_DEBUG("Discarding flash loader");
+return false;
+}
+
if (loader && loader->stateless == VIR_TRISTATE_BOOL_YES) {
if (flash->mode != QEMU_FIRMWARE_FLASH_MODE_STATELESS) {
VIR_DEBUG("Discarding loader without stateless flash");
@@ -1327,6 +1333,12 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
return false;
}
}
+} else if (fw->mapping.device == QEMU_FIRMWARE_DEVICE_MEMORY) {
+if (loader && loader->type &&
+loader->type != VIR_DOMAIN_LOADER_TYPE_ROM) {
+VIR_DEBUG("Discarding rom loader");
+return false;
+}
}
if (def->sec) {
diff --git
a/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.args
b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.args
deleted file mode 100644
index 753ad2d4b5..00
--- a/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.args
+++ /dev/null
@@ -1,34 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/var/lib/libvirt/qemu/domain--1-guest \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-guest/.local/share \
-XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-guest/.cache \
-XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
-/usr/bin/qemu-system-x86_64 \
--name guest=guest,debug-threads=on \
--S \
--object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}'
\
--machine pc-q35-4.0,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=on \
--accel kvm \
--cpu qemu64 \
--bios /usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd \
--m size=1048576k \
--object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
--overcommit mem-lock=off \
--smp 1,sockets=1,cores=1,threads=1 \
--uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \
--display none \
--no-user-config \
--nodefaults \
--chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
--mon chardev=charmonitor,id=monitor,mode=control \
--rtc base=utc \
--no-shutdown \
--boot strict=on \
--audiodev '{"id":"audio1","driver":"none"}' \
--global ICH9-LPC.noreboot=off \
--watchdog-action reset \
--sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
--msg timestamp=on
diff --git
a/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.err
b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.err
new file mode 100644
index 00..3edb2b3451
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.err
@@ -0,0 +1 @@
+operation failed: Unable to find 'efi' firmware that is compatible with the
current configuration
diff --git
a/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.xml
b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.xml
index fe05e33b69..3ced80f78b 100644
--- a/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.xml
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.xml
@@ -6,11 +6,7 @@
1
hvm
-
-
-
-
-/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd
+
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 07454f19c2..2c1918cb46 100644
--- a/tests/qemuxmlconftest.c
+++ b
[PATCH 4/6] qemu: ROM firmware images are always readonly
By definition. Accordingly, filter them out when looking for
a read/write image.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_firmware.c | 5 +++
.../firmware-auto-efi-rw.x86_64-latest.args | 34 ---
.../firmware-auto-efi-rw.x86_64-latest.err| 1 +
.../firmware-auto-efi-rw.x86_64-latest.xml| 6 +---
tests/qemuxmlconftest.c | 2 +-
5 files changed, 8 insertions(+), 40 deletions(-)
delete mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.args
create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.err
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
index a0b13f76b8..08ca99e1ac 100644
--- a/src/qemu/qemu_firmware.c
+++ b/src/qemu/qemu_firmware.c
@@ -1339,6 +1339,11 @@ qemuFirmwareMatchDomain(const virDomainDef *def,
VIR_DEBUG("Discarding rom loader");
return false;
}
+
+if (loader && loader->readonly == VIR_TRISTATE_BOOL_NO) {
+VIR_DEBUG("Discarding readonly loader");
+return false;
+}
}
if (def->sec) {
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.args
b/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.args
deleted file mode 100644
index 753ad2d4b5..00
--- a/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.args
+++ /dev/null
@@ -1,34 +0,0 @@
-LC_ALL=C \
-PATH=/bin \
-HOME=/var/lib/libvirt/qemu/domain--1-guest \
-USER=test \
-LOGNAME=test \
-XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-guest/.local/share \
-XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-guest/.cache \
-XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
-/usr/bin/qemu-system-x86_64 \
--name guest=guest,debug-threads=on \
--S \
--object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}'
\
--machine pc-q35-4.0,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=on \
--accel kvm \
--cpu qemu64 \
--bios /usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd \
--m size=1048576k \
--object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
--overcommit mem-lock=off \
--smp 1,sockets=1,cores=1,threads=1 \
--uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \
--display none \
--no-user-config \
--nodefaults \
--chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
--mon chardev=charmonitor,id=monitor,mode=control \
--rtc base=utc \
--no-shutdown \
--boot strict=on \
--audiodev '{"id":"audio1","driver":"none"}' \
--global ICH9-LPC.noreboot=off \
--watchdog-action reset \
--sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
--msg timestamp=on
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.err
b/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.err
new file mode 100644
index 00..3edb2b3451
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.err
@@ -0,0 +1 @@
+operation failed: Unable to find 'efi' firmware that is compatible with the
current configuration
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.xml
b/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.xml
index fe05e33b69..c2d0c33a0b 100644
--- a/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.xml
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.xml
@@ -6,11 +6,7 @@
1
hvm
-
-
-
-
-/usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd
+
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 2c1918cb46..49b4d023b6 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1420,7 +1420,7 @@ mymain(void)
DO_TEST_CAPS_LATEST("firmware-auto-efi");
DO_TEST_CAPS_LATEST_ABI_UPDATE("firmware-auto-efi");
DO_TEST_CAPS_LATEST("firmware-auto-efi-stateless");
-DO_TEST_CAPS_LATEST("firmware-auto-efi-rw");
+DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-rw");
DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-rw-pflash");
DO_TEST_CAPS_LATEST("firmware-auto-efi-loader-secure");
DO_TEST_CAPS_LATEST_ABI_UPDATE("firmware-auto-efi-loader-secure");
--
2.45.2
[PATCH 5/6] tests: Add firmware descriptor for edk2 on riscv64
It's available as part of the edk2-riscv64 Fedora package.
Signed-off-by: Andrea Bolognani
---
.../qemu_5.2.0-tcg-virt.riscv64.xml | 4 ++-
.../qemu_5.2.0-virt.riscv64.xml | 4 ++-
.../qemu_8.0.0-tcg-virt.riscv64.xml | 4 ++-
.../qemu_8.0.0-virt.riscv64.xml | 4 ++-
.../qemu/firmware/50-edk2-riscv-qcow2.json| 33 +++
tests/qemufirmwaretest.c | 5 +++
6 files changed, 50 insertions(+), 4 deletions(-)
create mode 100644
tests/qemufirmwaredata/usr/share/qemu/firmware/50-edk2-riscv-qcow2.json
diff --git a/tests/domaincapsdata/qemu_5.2.0-tcg-virt.riscv64.xml
b/tests/domaincapsdata/qemu_5.2.0-tcg-virt.riscv64.xml
index c487d467ef..e69fb88891 100644
--- a/tests/domaincapsdata/qemu_5.2.0-tcg-virt.riscv64.xml
+++ b/tests/domaincapsdata/qemu_5.2.0-tcg-virt.riscv64.xml
@@ -6,7 +6,9 @@
-
+
+ efi
+
/obviously/fake/firmware1.fd
/obviously/fake/firmware2.fd
diff --git a/tests/domaincapsdata/qemu_5.2.0-virt.riscv64.xml
b/tests/domaincapsdata/qemu_5.2.0-virt.riscv64.xml
index b0e4aafcd5..e9a1883a14 100644
--- a/tests/domaincapsdata/qemu_5.2.0-virt.riscv64.xml
+++ b/tests/domaincapsdata/qemu_5.2.0-virt.riscv64.xml
@@ -5,7 +5,9 @@
riscv64
-
+
+ efi
+
/obviously/fake/firmware1.fd
/obviously/fake/firmware2.fd
diff --git a/tests/domaincapsdata/qemu_8.0.0-tcg-virt.riscv64.xml
b/tests/domaincapsdata/qemu_8.0.0-tcg-virt.riscv64.xml
index e4bb90c929..3915f789fe 100644
--- a/tests/domaincapsdata/qemu_8.0.0-tcg-virt.riscv64.xml
+++ b/tests/domaincapsdata/qemu_8.0.0-tcg-virt.riscv64.xml
@@ -6,7 +6,9 @@
-
+
+ efi
+
/obviously/fake/firmware1.fd
/obviously/fake/firmware2.fd
diff --git a/tests/domaincapsdata/qemu_8.0.0-virt.riscv64.xml
b/tests/domaincapsdata/qemu_8.0.0-virt.riscv64.xml
index 265274aa65..ddcbac7192 100644
--- a/tests/domaincapsdata/qemu_8.0.0-virt.riscv64.xml
+++ b/tests/domaincapsdata/qemu_8.0.0-virt.riscv64.xml
@@ -5,7 +5,9 @@
riscv64
-
+
+ efi
+
/obviously/fake/firmware1.fd
/obviously/fake/firmware2.fd
diff --git
a/tests/qemufirmwaredata/usr/share/qemu/firmware/50-edk2-riscv-qcow2.json
b/tests/qemufirmwaredata/usr/share/qemu/firmware/50-edk2-riscv-qcow2.json
new file mode 100644
index 00..eb1930da49
--- /dev/null
+++ b/tests/qemufirmwaredata/usr/share/qemu/firmware/50-edk2-riscv-qcow2.json
@@ -0,0 +1,33 @@
+{
+"description": "UEFI firmware for RISC-V virtual machines",
+"interface-types": [
+"uefi"
+],
+"mapping": {
+"device": "flash",
+"mode" : "split",
+"executable": {
+"filename": "/usr/share/edk2/riscv/RISCV_VIRT_CODE.qcow2",
+"format": "qcow2"
+},
+"nvram-template": {
+"filename": "/usr/share/edk2/riscv/RISCV_VIRT_VARS.qcow2",
+"format": "qcow2"
+}
+},
+"targets": [
+{
+"architecture": "riscv64",
+"machines": [
+"virt",
+"virt-*"
+]
+}
+],
+"features": [
+
+],
+"tags": [
+
+]
+}
diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c
index a5e7e2ec65..f16ea526ff 100644
--- a/tests/qemufirmwaretest.c
+++ b/tests/qemufirmwaretest.c
@@ -93,6 +93,7 @@ testFWPrecedence(const void *opaque G_GNUC_UNUSED)
PREFIX "/share/qemu/firmware/50-edk2-loongarch64.json",
PREFIX "/share/qemu/firmware/50-edk2-ovmf-4m-qcow2-x64-nosb.json",
PREFIX "/share/qemu/firmware/50-edk2-ovmf-x64-microvm.json",
+PREFIX "/share/qemu/firmware/50-edk2-riscv-qcow2.json",
PREFIX "/share/qemu/firmware/51-edk2-aarch64-raw.json",
PREFIX "/share/qemu/firmware/51-edk2-ovmf-2m-raw-x64-nosb.json",
PREFIX "/share/qemu/firmware/52-edk2-aarch64-verbose-qcow2.json",
@@ -272,6 +273,7 @@ mymain(void)
DO_PARSE_TEST("usr/share/qemu/firmware/50-edk2-loongarch64.json");
DO_PARSE_TEST("usr/share/qemu/firmware/50-edk2-ovmf-4m-qcow2-x64-nosb.json");
DO_PARSE_TEST("usr/share/qemu/firmware/50-edk2-ovmf-x64-microvm.json");
+DO_PARSE_TEST("usr/share/qemu/firmware/50-edk2-riscv-qcow2.json");
DO_PARSE_TEST("usr/share/qemu/firmware/51-edk2-aarch64-raw.json");
DO_PARSE_TEST("usr/share/qemu/firmware/51-edk2-ovmf-2m-raw-x64-nosb.json");
DO_PARSE_TEST("usr/share/qemu/firmware/52-edk2-aarch64-verbose-qcow2.json");
@@ -332,6 +334,9 @@
[PATCH 6/6] tests: Add test for UEFI autoselection on riscv64
This scenario is going to be ever more popular, especially now
that virt-manager has started using UEFI by default on riscv64
(see https://github.com/virt-manager/virt-manager/pull/670/).
Signed-off-by: Andrea Bolognani
---
...efi-riscv64.riscv64-latest.abi-update.args | 34 +++
...-efi-riscv64.riscv64-latest.abi-update.xml | 28 +++
.../firmware-auto-efi-riscv64.xml | 14
tests/qemuxmlconftest.c | 1 +
4 files changed, 77 insertions(+)
create mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-riscv64.riscv64-latest.abi-update.args
create mode 100644
tests/qemuxmlconfdata/firmware-auto-efi-riscv64.riscv64-latest.abi-update.xml
create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-riscv64.xml
diff --git
a/tests/qemuxmlconfdata/firmware-auto-efi-riscv64.riscv64-latest.abi-update.args
b/tests/qemuxmlconfdata/firmware-auto-efi-riscv64.riscv64-latest.abi-update.args
new file mode 100644
index 00..942be6d73d
--- /dev/null
+++
b/tests/qemuxmlconfdata/firmware-auto-efi-riscv64.riscv64-latest.abi-update.args
@@ -0,0 +1,34 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-guest \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-guest/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-guest/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
+/usr/bin/qemu-system-riscv64 \
+-name guest=guest,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}'
\
+-blockdev
'{"driver":"file","filename":"/usr/share/edk2/riscv/RISCV_VIRT_CODE.qcow2","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}'
\
+-blockdev
'{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"qcow2","file":"libvirt-pflash0-storage"}'
\
+-blockdev
'{"driver":"file","filename":"/var/lib/libvirt/qemu/nvram/guest_VARS.qcow2","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}'
\
+-blockdev
'{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"qcow2","file":"libvirt-pflash1-storage"}'
\
+-machine
virt,usb=off,dump-guest-core=off,memory-backend=riscv_virt_board.ram,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format
\
+-accel tcg \
+-m size=1048576k \
+-object
'{"qom-type":"memory-backend-ram","id":"riscv_virt_board.ram","size":1073741824}'
\
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git
a/tests/qemuxmlconfdata/firmware-auto-efi-riscv64.riscv64-latest.abi-update.xml
b/tests/qemuxmlconfdata/firmware-auto-efi-riscv64.riscv64-latest.abi-update.xml
new file mode 100644
index 00..45f581214e
--- /dev/null
+++
b/tests/qemuxmlconfdata/firmware-auto-efi-riscv64.riscv64-latest.abi-update.xml
@@ -0,0 +1,28 @@
+
+ guest
+ 63840878-0deb-4095-97e6-fc444d9bc9fa
+ 1048576
+ 1048576
+ 1
+
+hvm
+
+
+
+
+/usr/share/edk2/riscv/RISCV_VIRT_CODE.qcow2
+/var/lib/libvirt/qemu/nvram/guest_VARS.qcow2
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-riscv64
+
+
+
+
+
+
diff --git a/tests/qemuxmlconfdata/firmware-auto-efi-riscv64.xml
b/tests/qemuxmlconfdata/firmware-auto-efi-riscv64.xml
new file mode 100644
index 00..9904a50e25
--- /dev/null
+++ b/tests/qemuxmlconfdata/firmware-auto-efi-riscv64.xml
@@ -0,0 +1,14 @@
+
+ guest
+ 63840878-0deb-4095-97e6-fc444d9bc9fa
+ 1048576
+ 1
+
+hvm
+
+
+/usr/bin/qemu-system-riscv64
+
+
+
+
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 49b4d023b6..a215faae72 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -1437,6 +1437,7 @@ mymain(void)
DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("firmware-auto-efi-aarch64",
"aarch64");
DO_TEST_CAPS_ARCH_LATEST("firmware-auto-efi-loongarch64", "loongarch64");
DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("firmware-auto-efi-loongarch64",
"loongarch64");
+DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE("firmware-auto-efi-riscv64",
"riscv64");
DO_TEST_CAPS_LATEST("firmware-auto-efi-nvram-path");
DO_TEST_CAPS_LATEST("firmware-auto-efi-nvram-template");
DO_TEST_CAPS_LATEST_FAILURE("firmware-auto-efi-nvram-template-nonstandard");
--
2.45.2
Re: [PATCH] security: AppArmor allow write when os loader readonly=no
On Fri, Jul 05, 2024 at 12:38:36PM GMT, Christian Ehrhardt wrote:
> That is due to the check for valid_path having /usr/share/OVMF/ in
> restricted_rw and therefore blocking this. Now the question is,
> was this meant to allow using the system files RW - then I think
> it is rightfully blocked. Instead I'm going to assume this was meant for
> separate files.
>
> # TEST ONLY - DO NOT DO THIS
> $ cp /usr/share/OVMF/OVMF_CODE_4M.fd /opt/OVMF_CODE_4M.fd
> $ chmod 666 /opt/OVMF_CODE_4M.fd
>
> root@o:~# virsh dumpxml testdomain | grep OVMF; uuid=$(virsh domuuid
> testdomain); cat /etc/apparmor.d/libvirt/libvirt-${uuid}.files | grep OVM
> /opt/OVMF_CODE_4M.fd
> "/opt/OVMF_CODE_4M.fd" rwk,
>
> So we can see the code achieves what it is intended to do while not
> regressing the former flow.
>
> But it can not be used on the normal paths, that is probably good to avoid
> accidentally overwriting these.
We definitely don't want the shared CODE files to be used in
read/write mode, as that would certainly cause corruption.
In fact, I would have expected such a request on the user's part to
be outright rejected by libvirt, but when I tried I realized that
wasn't the case, at least in some scenarios. I've just posted some
patches[1] that should improve the situation.
> But I think along the patch we should hear a little bit more about the use
> case
> to ensure that it has a practical use?
>
> @Miroslav: how exactly would you make use of that now?
Yes, it would be nice to get confirmation.
[1]
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/2NR3NUK7H3IUQBGM5NHWOHFV5TPT6QGE/
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] security: AppArmor allow write when os loader readonly=no
On Tue, Jul 09, 2024 at 08:41:17AM GMT, mirlos--- via Devel wrote: > My reply by email has not arrived by now, hence I'll post it via > the archive site. Sorry for the potential double post. No double post as far as I can tell :) > Older bootloaders were not split into separate _CODE.fd and _VARS.fd, > i.e. there was no separate nvram for the latter file to create. The guest > could write to the single bootloader, which then must not be shared. > > You mark such bootloaders readonly=no and make a copy of the pflash, > e.g. next to the VM's disk files, as you did in your TEST ONLY run. > > It is a mode of operation supported by the formatdomain documentation > on the loader element and intended to work as described. This patch > makes such combination of parameters actually succeed on Ubuntu, > which I find should be useful to the project. > > In the VMs we use this for, they do not actually write anything to the loader. > This meant that we never noticed the bug, which was present in focal and > configured qemu to open the loader read-only anyway. But it failed on AA > in noble since the bug is now fixed in newer libvirt. So the behavior changed between libvirt 6.0.0 in Ubuntu 20.04 and libvirt 10.0.0 in Ubuntu 24.04. That's not surprising, since I've done a lot of work to fix and improve firmware handling around the 9.2.0 timeframe. You seem to identify the change of behavior as a bug fix, which matches my understanding too. > As a workaround, we've in the mean time switched to marking the loader > stateless and read-only, which is now allowed in libvirt, and also works > without requiring a separate nvram. Of course, this only works because > the VM does not make any writes and would fail in case it needed to. Right, that ought to do the trick. As far as I'm concerned, I'm satisfied with the explanation you've provided so the patch gets my Reviewed-by: Andrea Bolognani Christian, any objection to pushing it? -- Andrea Bolognani / Red Hat / Virtualization
[PATCH 0/2] qemu: Add support for pauth Arm CPU feature
Andrea Bolognani (2): cpu_map: Add pauth Arm CPU feature tests: Add coverage for pauth Arm CPU feature src/cpu_map/arm_features.xml | 3 ++ ...aarch64-features-pauth.aarch64-latest.args | 31 +++ .../aarch64-features-pauth.aarch64-latest.xml | 28 + .../aarch64-features-pauth.xml| 17 ++ tests/qemuxmlconftest.c | 1 + 5 files changed, 80 insertions(+) create mode 100644 tests/qemuxmlconfdata/aarch64-features-pauth.aarch64-latest.args create mode 100644 tests/qemuxmlconfdata/aarch64-features-pauth.aarch64-latest.xml create mode 100644 tests/qemuxmlconfdata/aarch64-features-pauth.xml -- 2.45.2
[PATCH 1/2] cpu_map: Add pauth Arm CPU feature
This CPU feature can be used to explicitly enable or disable support for pointer authentication. By default, it will be enabled if the host supports it. https://issues.redhat.com/browse/RHEL-7044 Signed-off-by: Andrea Bolognani --- src/cpu_map/arm_features.xml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cpu_map/arm_features.xml b/src/cpu_map/arm_features.xml index 8a53384463..fc80a3b5bd 100644 --- a/src/cpu_map/arm_features.xml +++ b/src/cpu_map/arm_features.xml @@ -19,4 +19,7 @@ + + + -- 2.45.2
[PATCH 2/2] tests: Add coverage for pauth Arm CPU feature
Signed-off-by: Andrea Bolognani
---
...aarch64-features-pauth.aarch64-latest.args | 31 +++
.../aarch64-features-pauth.aarch64-latest.xml | 28 +
.../aarch64-features-pauth.xml| 17 ++
tests/qemuxmlconftest.c | 1 +
4 files changed, 77 insertions(+)
create mode 100644
tests/qemuxmlconfdata/aarch64-features-pauth.aarch64-latest.args
create mode 100644
tests/qemuxmlconfdata/aarch64-features-pauth.aarch64-latest.xml
create mode 100644 tests/qemuxmlconfdata/aarch64-features-pauth.xml
diff --git a/tests/qemuxmlconfdata/aarch64-features-pauth.aarch64-latest.args
b/tests/qemuxmlconfdata/aarch64-features-pauth.aarch64-latest.args
new file mode 100644
index 00..41714ae696
--- /dev/null
+++ b/tests/qemuxmlconfdata/aarch64-features-pauth.aarch64-latest.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-guest \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-guest/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-guest/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config \
+/usr/bin/qemu-system-aarch64 \
+-name guest=guest,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-guest/master-key.aes"}'
\
+-machine
virt,usb=off,gic-version=3,dump-guest-core=off,memory-backend=mach-virt.ram,acpi=off
\
+-accel kvm \
+-cpu host,pauth=off \
+-m size=1048576k \
+-object
'{"qom-type":"memory-backend-ram","id":"mach-virt.ram","size":1073741824}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxmlconfdata/aarch64-features-pauth.aarch64-latest.xml
b/tests/qemuxmlconfdata/aarch64-features-pauth.aarch64-latest.xml
new file mode 100644
index 00..be7676bba4
--- /dev/null
+++ b/tests/qemuxmlconfdata/aarch64-features-pauth.aarch64-latest.xml
@@ -0,0 +1,28 @@
+
+ guest
+ 1ccfd97d-5eb4-478a-bbe6-88d254c16db7
+ 1048576
+ 1048576
+ 1
+
+hvm
+
+
+
+
+
+
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-aarch64
+
+
+
+
+
+
diff --git a/tests/qemuxmlconfdata/aarch64-features-pauth.xml
b/tests/qemuxmlconfdata/aarch64-features-pauth.xml
new file mode 100644
index 00..5dcede8781
--- /dev/null
+++ b/tests/qemuxmlconfdata/aarch64-features-pauth.xml
@@ -0,0 +1,17 @@
+
+ guest
+ 1ccfd97d-5eb4-478a-bbe6-88d254c16db7
+ 1048576
+ 1
+
+hvm
+
+
+
+
+
+/usr/bin/qemu-system-aarch64
+
+
+
+
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 389d31800b..392bb6c0ff 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -2601,6 +2601,7 @@ mymain(void)
DO_TEST_CAPS_ARCH_LATEST("aarch64-features-sve", "aarch64");
DO_TEST_CAPS_ARCH_LATEST("aarch64-features-ras", "aarch64");
+DO_TEST_CAPS_ARCH_LATEST("aarch64-features-pauth", "aarch64");
DO_TEST_CAPS_ARCH_LATEST("clock-timer-armvtimer", "aarch64");
--
2.45.2
[PATCH 3/2] news: Mention pauth Arm CPU feature
Signed-off-by: Andrea Bolognani --- NEWS.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 775f5904ea..2fdb52c607 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,8 @@ v10.6.0 (unreleased) * **New features** + * qemu: Add support for the 'pauth' Arm CPU feature + * **Improvements** * **Bug fixes** -- 2.45.2
Re: [PATCH 0/2] qemu: Add support for pauth Arm CPU feature
On Mon, Jul 15, 2024 at 10:45:10AM GMT, Michal Prívozník wrote: > On 7/15/24 10:32, Michal Prívozník wrote: > > On 7/12/24 16:21, Andrea Bolognani wrote: > >> Andrea Bolognani (2): > >> cpu_map: Add pauth Arm CPU feature > >> tests: Add coverage for pauth Arm CPU feature > > > > Sorry for letting this fall through cracks. > > Actually, I've misread the date this was sent in. It was sent only 3 > days ago, so not sorry then :-P O:-) I just realized that I didn't update the release notes, so I've just posted a small patch for that too (messing up the threading in the process). Can I consider your R-b valid for patch 3/2 too? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] security: AppArmor allow write when os loader readonly=no
On Thu, Jul 11, 2024 at 06:26:31AM GMT, Andrea Bolognani wrote: > On Tue, Jul 09, 2024 at 08:41:17AM GMT, mirlos--- via Devel wrote: > > My reply by email has not arrived by now, hence I'll post it via > > the archive site. Sorry for the potential double post. > > No double post as far as I can tell :) > > > Older bootloaders were not split into separate _CODE.fd and _VARS.fd, > > i.e. there was no separate nvram for the latter file to create. The guest > > could write to the single bootloader, which then must not be shared. > > > > You mark such bootloaders readonly=no and make a copy of the pflash, > > e.g. next to the VM's disk files, as you did in your TEST ONLY run. > > > > It is a mode of operation supported by the formatdomain documentation > > on the loader element and intended to work as described. This patch > > makes such combination of parameters actually succeed on Ubuntu, > > which I find should be useful to the project. > > > > In the VMs we use this for, they do not actually write anything to the > > loader. > > This meant that we never noticed the bug, which was present in focal and > > configured qemu to open the loader read-only anyway. But it failed on AA > > in noble since the bug is now fixed in newer libvirt. > > So the behavior changed between libvirt 6.0.0 in Ubuntu 20.04 and > libvirt 10.0.0 in Ubuntu 24.04. That's not surprising, since I've > done a lot of work to fix and improve firmware handling around the > 9.2.0 timeframe. > > You seem to identify the change of behavior as a bug fix, which > matches my understanding too. > > > As a workaround, we've in the mean time switched to marking the loader > > stateless and read-only, which is now allowed in libvirt, and also works > > without requiring a separate nvram. Of course, this only works because > > the VM does not make any writes and would fail in case it needed to. > > Right, that ought to do the trick. > > As far as I'm concerned, I'm satisfied with the explanation you've > provided so the patch gets my > > Reviewed-by: Andrea Bolognani > > Christian, any objection to pushing it? Since I got no reply, I'm going to assume Christian is okay with me pushing the patch. We still have a bit of time to revert it before 10.6.0 is tagged if needs be. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/5] conf: Introduce pstore device
On Mon, Jul 22, 2024 at 02:23:30PM GMT, Michal Privoznik wrote: > +Pstore > +~ > + > +Pstore is an oops/panic logger that writes its logs to a block device and > +non-block device before the system crashes. Currently only ACPI Error Record > +Serialization Table, ERST, is supported. This feature is designed for storing > +error records in persistent storage for future reference and/or debugging. > +:since:`Since v10.6.0` > + > +:: > + > + ... > + > +/tmp/guest_acpi_esrt > +8 > + function='0x0'/> > + > + ... > + > +The ``pstore`` element has one mandatory attribute ``backend`` which selects > +desired backend (only ``acpi-erst`` is accepted for now). Then it has the > +following child elements: > + > +``path`` > + Represents a path in the host that backs the pstore device in the guest. It > + is mandatory. I know, very late feedback, but couldn't we fill this in for the user if no value has been provided, e.g. /var/lib/libvirt/qemu/nvram/mydomain_PSTORE.raw or something like that? I suppose using a well-known directory would help avoid file permission issues and SELinux denials. Also IIUC we're leaving creation of the file to QEMU instead of handling that part ourselves, which is what we do in other cases such as UEFI VARS storage. Are we okay with that? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 8/8] qemu: migration: Don't remember seclabel for images shared from current host
On Wed, Jul 24, 2024 at 05:34:32PM GMT, Peter Krempa wrote:
> +static void
> +qemuMigrationDstPrepareDiskSeclabels(virDomainObj *vm,
> + size_t nmigrate_disks,
> + const char **migrate_disks,
> + unsigned int flags)
> +{
> +qemuDomainObjPrivate *priv = vm->privateData;
> +g_autoptr(virQEMUDriverConfig) cfg =
> virQEMUDriverGetConfig(priv->driver);
> +size_t i;
> +
> +/* In case when storage is exported from this host, security label
> + * remembering would behave differently compared to the host which mounts
> + * the exported filesystem. Specifically for incoming migration
> remembering
> + * a seclabel would remember a seclabel already allowing access to the
> image,
> + * which is not desired. Thus we skip remembering of seclabels for images
> + * which are local to this host but accessed in a shared way from another
> + * host.
> + */
> +if (!cfg->sharedFilesystems ||
> +cfg->sharedFilesystems[0] == NULL)
> +return;
> +
> +for (i = 0; i < vm->def->ndisks; i++) {
> +virDomainDiskDef *disk = vm->def->disks[i];
This only iterates over disks, so it's probably not surprising that
it fails when other types of storage are involved.
For example, if the domain uses UEFI, local -> remote migration works
but when attempting to migrate back I get
error: Requested operation is not valid: Setting different SELinux
label on /var/lib/libvirt/qemu/nvram/cirros-efi_VARS.fd which is
already in use
If I add TPM into the mix, local -> remote migration fails with
error: unable to lock
/var/lib/libvirt/swtpm/50601047-47f7-4f45-9b41-9112dfaa3539/tpm2/.lock
for metadata change: Resource temporarily unavailable
Setting remember_owner=0 in qemu.conf makes all these errors go away,
but of course that's a very big hammer and the idea here is to use a
much smaller, more targeted one.
I think it might be enough to extend this logic beyond disks so that
it covers UEFI and TPM too, but I'm not entirely sure those use a
virStorageSource behind the scenes. Hopefully that's the case and the
changes needed on top are minimal, but I figure you're in a better
position than me to look into it.
> +/* We care only about existing local storage */
> +if (virStorageSourceIsEmpty(disk->src) ||
> +!virStorageSourceIsLocalStorage(disk->src))
> +continue;
> +
> +/* Only paths which are on local filesystem but shared elsewhere are
> + * relevant */
> +if (!virFileIsSharedFSOverride(disk->src->path,
> cfg->sharedFilesystems))
> +continue;
> +
> +/* Any storage that was migrated via NBD is technically fully local
> so
> + * we want seclabels remembered */
> +if (flags & (VIR_MIGRATE_NON_SHARED_DISK |
> VIR_MIGRATE_NON_SHARED_INC)) {
> +if (qemuMigrationAnyCopyDisk(disk, nmigrate_disks,
> migrate_disks))
> +continue;
> +}
> +
> +disk->src->seclabelSkipRemeber = true;
You misspelled "remember" here.
> @@ -3154,6 +3201,8 @@ qemuMigrationDstPrepareActive(virQEMUDriver *driver,
> dataFD[0])))
> goto error;
>
> +qemuMigrationDstPrepareDiskSeclabels(vm, nmigrate_disks, migrate_disks,
> flags);
> +
> if (qemuProcessPrepareDomain(driver, vm, startFlags) < 0)
> goto error;
One thing that concerns me is that the changes appear to only be on
the destination side of the migration. So only the remote -> local
path is affected. But what about the local -> remote path?
If we assume remember_owner=1, then obviously the various XATTRs
related to remembering will be set on domain startup. What will take
care of collecting that garbage once local -> remote migration has
been performed? I think things might be fine based on the fact that
the XATTRs seems to be dropped for disks even in the current state,
but I thought I'd mention this specifically just in case.
Thanks a lot for looking into this!
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 8/8] qemu: migration: Don't remember seclabel for images shared from current host
On Mon, Jul 29, 2024 at 03:09:55PM GMT, Peter Krempa wrote: > On Fri, Jul 26, 2024 at 07:47:57 -0700, Andrea Bolognani wrote: > > For example, if the domain uses UEFI, local -> remote migration works > > but when attempting to migrate back I get > > > > error: Requested operation is not valid: Setting different SELinux > > label on /var/lib/libvirt/qemu/nvram/cirros-efi_VARS.fd which is > > already in use > > So I've kind of forgot about this one. That's mainly as sharing this > path is not actually needed for migration to work, as the contents are > shared inside the migration stream automatically. > > Given though that users might want to share this one, so that they can > start the VM withouth migration on a different host and also we allow > setting the path to a more reasonable directory for sharing > (users should not share /var/lib/libvirt/qemu/nvram/, or anything > belonging to libvirt in the first place) What's wrong with sharing this? It's just another type of storage after all. And it's true that in some cases you might be able to start the domain even with the NVRAM file being missing, but that's not true in the general case. Or are you saying that it's okay to share the directory where NVRAM files are stored, as long as it's not under /var/lib/libvirt? > > If I add TPM into the mix, local -> remote migration fails with > > > > error: unable to lock > > /var/lib/libvirt/swtpm/50601047-47f7-4f45-9b41-9112dfaa3539/tpm2/.lock > > for metadata change: Resource temporarily unavailable > > This makes it look like the file is still locked, that might be > indication of a different problem as libvirt will not keep the lock > after updating the XATTRs. I think there were some interesting timing issues with swtpm attempting to lock the file while libvirt was trying to perform its own metadata bookkeeping. I need to revisit that because honestly I've forgotten almost everything about it. > > One thing that concerns me is that the changes appear to only be on > > the destination side of the migration. So only the remote -> local > > path is affected. But what about the local -> remote path? > > On the source side, at least for disks we simply nuke the > metadata(xattrs). In fact we do that in most cases when stopping the VM, > not only for migration. > > See qemuProcessStop(), in the loop which calls > qemuBlockRemoveImageMetadata(). That call removes the metadata from all > disk top level sources. I see, thanks for the pointer. > > If we assume remember_owner=1, then obviously the various XATTRs > > related to remembering will be set on domain startup. What will take > > care of collecting that garbage once local -> remote migration has > > been performed? I think things might be fine based on the fact that > > the XATTRs seems to be dropped for disks even in the current state, > > but I thought I'd mention this specifically just in case. > > So apart from the code I've mentioned above which explicitly drops the > xattrs, they are also not shared via NFS. Thus the remote side itself > would not get them. > > If we'd leak them though it would mean that a re-start or incoming > migration on the source of > the migration would no longer work, so we need the same treatment for > the other resources as well. Right, that's exactly my concern. I have encountered at least one case in which XATTRs not being cleaned up correctly on (failed?) migration resulted in having to perform manual fixup tasks before the domain would start again. > I'll post another version adding the missing treatment for uefi, but I > don't think I care enough about TPMs to go digging how that stuff is > plumbed in or why it complains about locks being held which don't seem > to be held by libvirt. I will try to look into it myself on top of your updated patches. I'm pretty sure TPM is a requirement for KubeVirt, so we definitely need it to be handled correctly or the only recourse from their side will be to set remember_owner=0 - and we've already established that we don't want that to happen :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 1/2] qemu: Autofill pstore path if missing
On Mon, Jul 29, 2024 at 11:31:35AM GMT, Michal Privoznik wrote: > Introduced only a couple of commits ago (in > v10.5.0-84-g90e50e67c6) the pstore device acts as a nonvolatile > storage, where guest kernel can store information about crashes. > This device, however, expects a file in the host from which the > crash data is read. So far, we expected users to provide a path, > but we can autogenerate one if missing. Just put it next to > per-domain's _NVRAM stores. Either s/_NVRAM/_VARS/ or lose the leading underscore. You also need to squash in the diff below. With the latter taken care of, Reviewed-by: Andrea Bolognani diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 860ef17d7b..c56b739b23 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -8683,8 +8683,7 @@ desired backend (only ``acpi-erst`` is accepted for now). Then it has the following child elements: ``path`` - Represents a path in the host that backs the pstore device in the guest. It - is mandatory. + Represents a path in the host that backs the pstore device in the guest. ``size`` Configures the size of the persistent storage available to the guest. It is diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 6fcee2a70c..7d58dce465 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -6261,9 +6261,11 @@ acpi-erst - - - + + + + + -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/2] qemu: Pre-create pstore device file
On Mon, Jul 29, 2024 at 11:31:36AM GMT, Michal Privoznik wrote:
> +static int
> +qemuProcessPreparePstore(virDomainObj *vm)
> +{
> +virDomainPstoreDef *pstore = vm->def->pstore;
> +VIR_AUTOCLOSE fd = -1;
> +
> +if (!pstore)
> +return 0;
> +
> +switch (pstore->backend) {
> +case VIR_DOMAIN_PSTORE_BACKEND_ACPI_ERST:
> +if ((fd = open(pstore->path, O_WRONLY | O_CREAT, 0600)) < 0) {
> +virReportSystemError(errno,
> + _("cannot create file '%1$s'"),
> + pstore->path);
> +return -1;
> +}
> +
> +if (ftruncate(fd, pstore->size) < 0) {
> +virReportSystemError(errno,
> + _("Failed to truncate file '%1$s'"),
> + pstore->path);
> +return -1;
> +}
The size is stored in KiB but ftruncate(2) expect it in bytes, so you
need to multiply it by 1024 or QEMU will (correctly) complain about
the size mismatch on startup.
> +if (VIR_CLOSE(fd) < 0) {
> +virReportSystemError(errno,
> + _("Unable to save '%1$s'"),
> + pstore->path);
> +return -1;
> +}
> +
> +
> +break;
Unnecessary empty line.
Even with the fix mentioned above applied, I still get an error on
startup on my SELinux-enabled system. Running `setenforce 0` makes it
go away, but clearly that's not acceptable.
The diff below seems to do the trick, but I'm not entirely confident
that it's the right fix rather than a mere kludge.
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index ba0ce8fb9d..31df4d22db 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -3341,7 +3341,7 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr,
if (def->pstore &&
virSecuritySELinuxSetFilecon(mgr, def->pstore->path,
- data->content_context, true) < 0)
+ secdef->imagelabel, true) < 0)
return -1;
return 0;
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 for v10.6.0 0/2] Two pstore related fixes
On Tue, Jul 30, 2024 at 05:36:38PM GMT, Michal Privoznik wrote: > This is a v2 of the following patch: > > https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/PP4WO2ZYB7UXMV6WQ2N4B33KCUUDIQFK/ > > NB, 1/2 from the original series is pushed already. > > diff to v1: > - adjusted args to ftruncate() > - empty line cleanup > - new patch 2/2 (thanks Andrea for testing!) > > Michal Prívozník (2): > qemu: Pre-create pstore device file > security: Allow RW access to pstore device > > src/qemu/qemu_process.c | 45 + > src/security/security_selinux.c | 2 +- > 2 files changed, 46 insertions(+), 1 deletion(-) Series Reviewed-by: Andrea Bolognani and pushed for your convenience. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 1/2] qemu_domain: Strip from s390(x) definitions
it's > + * unsupported, as we don't fixup x86(_64) and AARCH64) */ > +if (stripACPI && > +virQEMUCapsMachineSupportsACPI(qemuCaps, def->virtType, > def->os.machine) != VIR_TRISTATE_BOOL_YES) > +def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_TRISTATE_SWITCH_ABSENT; Clever safety measure. Regarding the disadvantage you mention in the commit message, how about we implement a middle-ground solution instead? If we only actually strip the feature when !(parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) then we still silently fix migration, but we also prevent new s390x domains with ACPI (supposedly) enabled from being defined, and generally limit the scope of the workaround as much as possible. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/2] qemuxmlconftest: Add tests for the ACPI stripping hack on s390
On Wed, Jul 31, 2024 at 01:02:27PM GMT, Peter Krempa wrote:
> +++ b/tests/qemuxmlconfdata/aarch64-nousb-acpi.xml
> @@ -0,0 +1,18 @@
> +
> + aarch64test
> + 6ba410c5-1e5c-4d57-bee7-2228e7ffa32f
> + 1048576
> + 1
> +
> +
> +hvm
> +
The relationship between having implicit USB and being able to use
ACPI is not explained. I could probably figure it out by looking at
the code, but I think it would be better if the comment was expanded
to include this information.
> +++ b/tests/qemuxmlconfdata/riscv64-virt-acpi.xml
> @@ -0,0 +1,15 @@
> +
> + guest
> + 1ccfd97d-5eb4-478a-bbe6-88d254c16db7
> + 4194304
> + 4
> +
> +
> +
> +
> +hvm
> +
> +
> +/usr/bin/qemu-system-riscv64
> +
> +
Here and in a few other input files you've put the element
before the element, which of course our parser is perfectly
capable of handling but it just looks... Off to the human eye :)
For this input file in particular, you could add
which would make for slighly smaller output files. I'd personally
just include that (as well as the USB equivalent) in every file, just
to ensure minimal hardware while making it easier to compare them.
> +++ b/tests/qemuxmlconftest.c
> @@ -1732,7 +1732,18 @@ mymain(void)
>
> DO_TEST_CAPS_LATEST("input-usbmouse");
> DO_TEST_CAPS_LATEST("input-usbtablet");
> -DO_TEST_CAPS_LATEST("misc-acpi");
> +
> +/* tests for ACPI support handling:
> + * - x86(_64) and aarch attempt
Incomplete sentence? Also it's aarch64.
> + * - other architectures base the decision based on how qemu reports
> + *the support for ACPI
> + * - s390x has hack to strip ACPI to preserve migration of old configs
> */
> +DO_TEST_CAPS_LATEST("x86_64-q35-acpi");
> + DO_TEST_CAPS_ARCH_LATEST_PARSE_ERROR("aarch64-nousb-acpi", "aarch64");
We should have a positive test for aarch64 which uses the virt
machine type and has ACPI enabled.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 1/2] qemu_domain: Strip from s390(x) definitions
On Thu, Aug 01, 2024 at 12:03:52PM GMT, Peter Krempa wrote: > On Thu, Aug 01, 2024 at 02:35:59 -0700, Andrea Bolognani wrote: > > This looks unnecessarily verbose. I kinda get the idea, but > > considering that we're unlikely to go back and do anything about most > > of the architectures that we're currently leaving alone, does it > > really make sense to have all this code instead of just > > > > bool stripACPI = false; > > > > /* S390 never supported ACPI, but some users enabled it regardless in > > their > >* configs for some reason. In order to fix incoming migration we'll > > strip > >* ACPI from those definitions, so that user configs are fixed by updating > >* only the destination libvirt installation */ > > if (def->os.arch == VIR_ARCH_S390 || def->os.arch == VIR_ARCH_S390X) > > stripACPI = true; > > > > ? Maybe there's some motivation that I'm not seeing. > > > Well, I tend to prefer the switch statement because it forces you to > consider every code path when adding another member into the enum. In > this case though, you're right that we won't ever want to add anything > to the case when stripping is allowed. > > On the other hand it clearly differentiates the legacy arches where > theoretically the same bug could be considered to be fixed and the > modern arches where that stuff must never be done. > > Also note that yes this is more lines, but I'd consider the "verbosity" > to be at the same level, because you then don't have to consider what > other options are around when reading the code. And yes, all of that > could be replaced by a comment. I generally prefer the switch approach as well, it just seems overkill in this specific case. But it's perfectly valid, so feel free to just ignore this bit of feedback. > > Regarding the disadvantage you mention in the commit message, how > > about we implement a middle-ground solution instead? If we only > > actually strip the feature when > > > > !(parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) > > > > then we still silently fix migration, but we also prevent new s390x > > domains with ACPI (supposedly) enabled from being defined, and > > generally limit the scope of the workaround as much as possible. > > I've considered this, but in case when you migrate the persistent XML > along with the migration or use offline migration, you won't be able to > start the VM. Yes it will force you to fix the config but it feels wrong > to allow migration but then have a definition which no longer works. > > If the s390 folks consider this important for their usage and want to > forgo the fact that we'll be stripping instead of them fixing > the configs in the first place, we should do that always to prevent > having weird intermediate states. > > If we want to go the route of not allowing 'defining' a new config with > acpi enabled, we'll need another VIR_DOMAIN_DEF_PARSE_ flag asserted on > all migration related XML parsing where the modification will be > performed and on none of the normal code paths, but none of the existing > flags allow that granularity. So offline migration sets ABI_UPDATE? I guess that makes sense to some extent, as live migration obviously has the strictest possible requirements, but still it feels weird that the same domain migrated between the same two hosts could end up with slightly different ABI depending on whether or not it's migrated while it's running. I can tell you for certain that the presence of ABI_UPDATE is (mis)interpreted as a witness for "this is a newly-defined domain" in several places. I've introduced a few myself. Having a separate flag that actually carries that meaning would probably be a very good idea. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/2] qemuxmlconftest: Add tests for the ACPI stripping hack on s390
On Thu, Aug 01, 2024 at 12:06:57PM GMT, Peter Krempa wrote:
> On Thu, Aug 01, 2024 at 02:46:34 -0700, Andrea Bolognani wrote:
> > On Wed, Jul 31, 2024 at 01:02:27PM GMT, Peter Krempa wrote:
> > > +
> > > +hvm
> >
> > The relationship between having implicit USB and being able to use
> > ACPI is not explained. I could probably figure it out by looking at
> > the code, but I think it would be better if the comment was expanded
> > to include this information.
>
> The comment is a leftover from copying aarch64-nousb-minimal. I should
> have removed the nousb part and went with a specific machine type name.
That explains it.
> > > +
> > > +
> > > +
> > > +
> > > +hvm
> > > +
> >
> > Here and in a few other input files you've put the element
> > before the element, which of course our parser is perfectly
> > capable of handling but it just looks... Off to the human eye :)
>
> Welp. I've added it manually. I can move it or we can say it is testing
> the XML parser.
I have a mild preference for the former. Keep in mind that there's a
very real chance that, in a while, I will forget this conversation,
re-discover the out-of-order elements, and post a patch shuffling
them around :D
> > For this input file in particular, you could add
> >
> >
>
> Once again, I've copied riscv64-virt-minimal. Seems like it's missing
> there in the first place.
In that case it's very intentional: the *-minimal test cases are
supposed to show what kind of controllers and devices libvirt will
automatically add when presented with an XML that doesn't contain
any. The fact that some of the output files have memballoon and some
don't is exactly the point.
> > > +DO_TEST_CAPS_LATEST("x86_64-q35-acpi");
> > > +DO_TEST_CAPS_ARCH_LATEST_PARSE_ERROR("aarch64-nousb-acpi",
> > > "aarch64");
> >
> > We should have a positive test for aarch64 which uses the virt
> > machine type and has ACPI enabled.
>
> We do have that already in the code which is testing also the
> appripriate UEFI firmware selection for it, which is the exact reason I
> didn't duplicate it here. I can add a comment if you feel like that.
Makes sense. A small comment would be appreciated.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 1/2] qemu_domain: Strip from s390(x) definitions
On Thu, Aug 01, 2024 at 03:52:57PM GMT, Peter Krempa wrote: > The s390(x) machines never supported ACPI. That didn't stop users > enabling ACPI in their config. As of libvirt-9.2 (98c4e3d073) with new > enough qemu we reject configs which require ACPI, but qemu can't satisfy > it. > > This breaks migration of existing VMs with the old wrong configs to new > libvirt installations. > > To address this introduce a post-parse fixup removing the ACPI flag > specifically for s390 machines which do enable it in the definition. > > The advantage of doing it in post-parse, rather than simply relaxing the > ABI stability check to allow users providing an fixed XML when migrating > (allowing change of the ACPI flag for s390 in ABI stability check, as it > doesn't impact ABI), is that only the destination installation needs to > be patched in order to preserve migration. > > To mitigate the disadvantage of simply stripping it from all s390(x) > configs the hack is not applied when defining or starting a new domain > from the XML, to preserve the error about unsupported configuration. > > Resolves: https://issues.redhat.com/browse/RHEL-49516 > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_domain.c | 47 ++++++++++++++ > 1 file changed, 47 insertions(+) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 2/2] qemuxmlconftest: Add tests for the ACPI stripping hack on s390
On Thu, Aug 01, 2024 at 03:52:58PM GMT, Peter Krempa wrote:
> +++ b/tests/qemuxmlconfdata/riscv64-virt-acpi.xml
> @@ -0,0 +1,15 @@
> +
> + guest
> + 1ccfd97d-5eb4-478a-bbe6-88d254c16db7
> + 4194304
> + 4
> +
> +hvm
> +
> +
> +
> +
> +
> +/usr/bin/qemu-system-riscv64
> +
> +
This (and s390x-ccw-acpi) could still do with explicitly dropping the
memballoon IMO, but it's fine either way.
> +/* tests for ACPI support handling:
> + * - existing positive test cases enabling ACPI for
> aarch64/x86_64/loongarch:
> + * - firmware-manual-efi-acpi-q35
> + * - firmware-manual-efi-acpi-aarch64
> + * - firmware-auto-efi-loongarch64
> + *
> + * - negative case for aarch64 with 'borzoi' machine not supporting ACPI
> + *
> + * - s390x has hack to strip ACPI to preserve migration of old configs,
> + *but should produce error when ABI_UPDATE is requested
> + */
> +DO_TEST_CAPS_ARCH_LATEST_PARSE_ERROR("aarch64-noacpi-acpi", "aarch64");
> +DO_TEST_CAPS_ARCH_LATEST("riscv64-virt-acpi", "riscv64");
> +DO_TEST_CAPS_ARCH_LATEST("s390x-ccw-acpi", "s390x");
> +DO_TEST_CAPS_ARCH_LATEST_ABI_UPDATE_PARSE_ERROR("s390x-ccw-acpi",
> "s390x");
Oh, you managed to increase coverage while at the same time reducing
the number of additional files! And the comment is quite detailed and
useful. Very nice indeed :)
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 8/8] qemu: migration: Don't remember seclabel for images shared from current host
On Tue, Jul 30, 2024 at 12:21:56PM GMT, Peter Krempa wrote: > On Mon, Jul 29, 2024 at 08:29:42 -0700, Andrea Bolognani wrote: > > On Mon, Jul 29, 2024 at 03:09:55PM GMT, Peter Krempa wrote: > > > Given though that users might want to share this one, so that they can > > > start the VM withouth migration on a different host and also we allow > > > setting the path to a more reasonable directory for sharing > > > (users should not share /var/lib/libvirt/qemu/nvram/, or anything > > > belonging to libvirt in the first place) > > > > What's wrong with sharing this? It's just another type of storage > > after all. > > Well, in case of this specific directory it'd be okay. But we had users > wanting to share the XML directory too. In general I'd suggest user not > share anything that belongs to libvirt. > > In worst case they'd share /var/lib/libvirt, which also contains stuff > that must not be shared, such as the master keys for qemu, dnsmasq data > files etc. > > Similarly, sharing /var/lib/libvirt/qemu as whole is a bad idea. Right, I agree with you that indiscriminately sharing libvirt-private directories would be a terrible idea. I was talking about the NVRAM location and that one only. > > I think there were some interesting timing issues with swtpm > > attempting to lock the file while libvirt was trying to perform its > > own metadata bookkeeping. I need to revisit that because honestly > > I've forgotten almost everything about it. > > IIRC libvirt uses a specific block to apply the lock on so that it's > locking it only for internal purposes. If 'swtpm' is locking the same > block it might cause problems. IIRC libvirt wanted to acquire the lock opportunistically and specifically for the scope of the relabel operation, while swtpm owns the storage and so it wants to have a long-term exclusive lock on it. > > I have encountered at least one > > case in which XATTRs not being cleaned up correctly on (failed?) > > migration resulted in having to perform manual fixup tasks before the > > domain would start again. > > It'd be great to know when that happened. Once I start looking into this again, on top of your updated patches, I'll probably manage to hit the issue whether I like it or not. When that happens, I'll make sure to write down some details this time around. -- Andrea Bolognani / Red Hat / Virtualization
[PATCH] apparmor: Allow more paths for qemu-bridge-helper
The QEMU package in Debian has recently moved the
qemu-bridge-helper binary under /usr/libexec/qemu. Update the
AppArmor profile accordingly.
https://bugs.debian.org/1077915
Signed-off-by: Andrea Bolognani
---
src/security/apparmor/usr.sbin.libvirtd.in | 4 ++--
src/security/apparmor/usr.sbin.virtqemud.in | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/security/apparmor/usr.sbin.libvirtd.in
b/src/security/apparmor/usr.sbin.libvirtd.in
index 1601d73d47..5fa5c7842c 100644
--- a/src/security/apparmor/usr.sbin.libvirtd.in
+++ b/src/security/apparmor/usr.sbin.libvirtd.in
@@ -116,7 +116,7 @@ profile libvirtd @sbindir@/libvirtd
flags=(attach_disconnected) {
# allow changing to our UUID-based named profiles
change_profile ->
@{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*,
- /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper Cx ->
qemu_bridge_helper,
+ /usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper Cx ->
qemu_bridge_helper,
# child profile for bridge helper process
profile qemu_bridge_helper {
#include
@@ -137,7 +137,7 @@ profile libvirtd @sbindir@/libvirtd
flags=(attach_disconnected) {
/etc/qemu/** r,
owner @{PROC}/*/status r,
- /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix,
+ /usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper rmix,
}
@BEGIN_APPARMOR_3@
diff --git a/src/security/apparmor/usr.sbin.virtqemud.in
b/src/security/apparmor/usr.sbin.virtqemud.in
index 6b9c5d32d9..ff2967c6eb 100644
--- a/src/security/apparmor/usr.sbin.virtqemud.in
+++ b/src/security/apparmor/usr.sbin.virtqemud.in
@@ -110,7 +110,7 @@ profile virtqemud @sbindir@/virtqemud
flags=(attach_disconnected) {
# allow changing to our UUID-based named profiles
change_profile ->
@{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*,
- /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper Cx ->
qemu_bridge_helper,
+ /usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper Cx ->
qemu_bridge_helper,
# child profile for bridge helper process
profile qemu_bridge_helper {
#include
@@ -130,7 +130,7 @@ profile virtqemud @sbindir@/virtqemud
flags=(attach_disconnected) {
/etc/qemu/** r,
owner @{PROC}/*/status r,
- /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix,
+ /usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper rmix,
}
@BEGIN_APPARMOR_3@
--
2.45.2
Re: [PATCH v3 0/6] qemu: Introduce the ability to disable the built-in PS/2 controller
On Wed, Aug 21, 2024 at 03:36:03PM GMT, Kamil Szczęk wrote: > On Wednesday, August 21st, 2024 at 17:15, Michal Prívozník > wrote: > > > We require that we're able to compile and tests pass after each commit > > (it's easier when we need to bisect something in the future). Therefore, > > I'm squashing 3/6 into 2/6. > > Makes sense, will keep that in mind. > > > and merged. Congratulations in your first libvirt contribution! > > Awesome, thanks. Looking forward to contributing further! I think the availability of this new feature should be advertised in the domain capabilities XML. That way, management applications such as virt-manager will be able to detect its presence and decide to ditch PS/2 completely for headless VMs, where it's not necessary to have any input devices. For VMs with graphics, they could add a USB keyboard to go along with the USB tablet that is already added, instead of the current combo of PS/2 keyboard and mouse *plus* USB tablet, for a slightly cleaner overall configuration matching that of other architectures that are not beholden to the same legacy as x86. -- Andrea Bolognani / Red Hat / Virtualization
[PATCH 0/4] qemu: Fill in panic model automatically on aarch64
Back when pvpanic-pci support was introduced 1.5 years ago (!), we
required the user to manually provide the model name. This is
incovenient and doesn't match the behavior seen on other
architectures. Make things more user friendly.
Andrea Bolognani (4):
tests: Add coverage for panic on riscv64
qemu: Refactor default panic model
qemu: Sometimes the default panic model doesn't exist
qemu: Use pvpanic by default on aarch64
src/qemu/qemu_domain.c| 36 ---
src/qemu/qemu_validate.c | 6 +++-
.../aarch64-panic-no-model.aarch64-latest.err | 1 -
...ault-models.aarch64-latest.abi-update.args | 1 +
...fault-models.aarch64-latest.abi-update.xml | 3 ++
...64-virt-default-models.aarch64-latest.args | 1 +
...h64-virt-default-models.aarch64-latest.xml | 3 ++
.../aarch64-virt-default-models.xml | 2 +-
.../riscv64-panic-no-model.riscv64-latest.err | 1 +
...o-model.xml => riscv64-panic-no-model.xml} | 4 +--
tests/qemuxmlconftest.c | 2 +-
11 files changed, 42 insertions(+), 18 deletions(-)
delete mode 100644
tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err
create mode 100644
tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err
rename tests/qemuxmlconfdata/{aarch64-panic-no-model.xml =>
riscv64-panic-no-model.xml} (65%)
--
2.46.0
[PATCH 1/4] tests: Add coverage for panic on riscv64
It merely duplicates the existing aarch64 coverage right now,
but it will become actually useful with the upcoming changes.
Signed-off-by: Andrea Bolognani
---
.../riscv64-panic-no-model.riscv64-latest.err | 1 +
tests/qemuxmlconfdata/riscv64-panic-no-model.xml| 13 +
tests/qemuxmlconftest.c | 1 +
3 files changed, 15 insertions(+)
create mode 100644
tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err
create mode 100644 tests/qemuxmlconfdata/riscv64-panic-no-model.xml
diff --git a/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err
b/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err
new file mode 100644
index 00..8e3f2c194d
--- /dev/null
+++ b/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: the QEMU binary does not support the ISA panic
device
diff --git a/tests/qemuxmlconfdata/riscv64-panic-no-model.xml
b/tests/qemuxmlconfdata/riscv64-panic-no-model.xml
new file mode 100644
index 00..9731a997ea
--- /dev/null
+++ b/tests/qemuxmlconfdata/riscv64-panic-no-model.xml
@@ -0,0 +1,13 @@
+
+ guest
+ 1ccfd97d-5eb4-478a-bbe6-88d254c16db7
+ 4194304
+ 4
+
+hvm
+
+
+/usr/bin/qemu-system-riscv64
+
+
+
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index f7c0cf4ad0..e97d0e7bdc 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -2649,6 +2649,7 @@ mymain(void)
DO_TEST_CAPS_LATEST("panic-double");
DO_TEST_CAPS_LATEST("panic-no-address");
DO_TEST_CAPS_ARCH_LATEST_PARSE_ERROR("aarch64-panic-no-model", "aarch64");
+DO_TEST_CAPS_ARCH_LATEST_PARSE_ERROR("riscv64-panic-no-model", "riscv64");
DO_TEST_CAPS_LATEST("pvpanic-pci-x86_64");
DO_TEST_CAPS_ARCH_LATEST("pvpanic-pci-aarch64", "aarch64");
--
2.46.0
[PATCH 2/4] qemu: Refactor default panic model
Perform decisions based on the architecture and machine type
in a single place instead of duplicating them.
This technically adds new behavior for MODEL_ISA in
qemuDomainDefAddDefaultDevices(), but it doesn't make any
difference functionally since we don't set addPanicDevice
outside of ppc64(le) and s390(x). If we did, the lack of
handling for that value would be a latent bug.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_domain.c | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d1767c326d..e460e48fce 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4150,6 +4150,19 @@ qemuDomainGetSCSIControllerModel(const virDomainDef *def,
}
+static virDomainPanicModel
+qemuDomainDefaultPanicModel(const virDomainDef *def)
+{
+if (qemuDomainIsPSeries(def))
+return VIR_DOMAIN_PANIC_MODEL_PSERIES;
+
+if (ARCH_IS_S390(def->os.arch))
+return VIR_DOMAIN_PANIC_MODEL_S390;
+
+return VIR_DOMAIN_PANIC_MODEL_ISA;
+}
+
+
static int
qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
virDomainDef *def,
@@ -4397,13 +4410,12 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver,
return -1;
if (addPanicDevice) {
+virDomainPanicModel defaultModel = qemuDomainDefaultPanicModel(def);
size_t j;
+
for (j = 0; j < def->npanics; j++) {
if (def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT ||
-(ARCH_IS_PPC64(def->os.arch) &&
- def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_PSERIES)
||
-(ARCH_IS_S390(def->os.arch) &&
- def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_S390))
+def->panics[j]->model == defaultModel)
break;
}
@@ -6100,14 +6112,8 @@ static int
qemuDomainDevicePanicDefPostParse(virDomainPanicDef *panic,
const virDomainDef *def)
{
-if (panic->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) {
-if (qemuDomainIsPSeries(def))
-panic->model = VIR_DOMAIN_PANIC_MODEL_PSERIES;
-else if (ARCH_IS_S390(def->os.arch))
-panic->model = VIR_DOMAIN_PANIC_MODEL_S390;
-else
-panic->model = VIR_DOMAIN_PANIC_MODEL_ISA;
-}
+if (panic->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT)
+panic->model = qemuDomainDefaultPanicModel(def);
return 0;
}
--
2.46.0
[PATCH 3/4] qemu: Sometimes the default panic model doesn't exist
Right now the fallback behavior is to use MODEL_ISA if we
haven't been able to find a better match, but that's not very
useful as we're still going to hit an error later, when
QEMU_CAPS_DEVICE_PANIC is not found at Validate time.
Instead of doing that, allow the MODEL_DEFAULT to get all the
way to Validate and report an error upon encountering it.
The reported error changes slightly, but other than that the
set of configurations that allow and block remains the same.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_domain.c | 5 -
src/qemu/qemu_validate.c| 6 +-
.../aarch64-panic-no-model.aarch64-latest.err | 2 +-
.../riscv64-panic-no-model.riscv64-latest.err | 2 +-
4 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e460e48fce..0cc335eb30 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4159,7 +4159,10 @@ qemuDomainDefaultPanicModel(const virDomainDef *def)
if (ARCH_IS_S390(def->os.arch))
return VIR_DOMAIN_PANIC_MODEL_S390;
-return VIR_DOMAIN_PANIC_MODEL_ISA;
+if (ARCH_IS_X86(def->os.arch))
+return VIR_DOMAIN_PANIC_MODEL_ISA;
+
+return VIR_DOMAIN_PANIC_MODEL_DEFAULT;
}
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index f74c538efe..43418033f6 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1048,8 +1048,12 @@ qemuValidateDomainDefPanic(const virDomainDef *def,
}
break;
-/* default model value was changed before in post parse */
case VIR_DOMAIN_PANIC_MODEL_DEFAULT:
+/* PostParse couldn't figure out a sensible default model */
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("no panic model provided, and no default for the
architecture and machine type"));
+return -1;
+
case VIR_DOMAIN_PANIC_MODEL_LAST:
break;
}
diff --git a/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err
b/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err
index 8e3f2c194d..139249bbc5 100644
--- a/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err
+++ b/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err
@@ -1 +1 @@
-unsupported configuration: the QEMU binary does not support the ISA panic
device
+unsupported configuration: no panic model provided, and no default for the
architecture and machine type
diff --git a/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err
b/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err
index 8e3f2c194d..139249bbc5 100644
--- a/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err
+++ b/tests/qemuxmlconfdata/riscv64-panic-no-model.riscv64-latest.err
@@ -1 +1 @@
-unsupported configuration: the QEMU binary does not support the ISA panic
device
+unsupported configuration: no panic model provided, and no default for the
architecture and machine type
--
2.46.0
[PATCH 4/4] qemu: Use pvpanic by default on aarch64
pvpanic-pci is the only reasonable implementation of a panic
device for aarch64/virt guests. Right now we're asking users to
provide the model name manually, but we can be more helpful and
fill it in automatically instead.
With this change, the aarch64-panic-no-model test no longer
fails and so it's no longer useful to us. Instead, we can amend
the aarch64-virt-default-models test case to include panic
coverage, something that until now wasn't possible.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_domain.c | 3 +++
.../aarch64-panic-no-model.aarch64-latest.err | 1 -
tests/qemuxmlconfdata/aarch64-panic-no-model.xml| 13 -
...rt-default-models.aarch64-latest.abi-update.args | 1 +
...irt-default-models.aarch64-latest.abi-update.xml | 3 +++
.../aarch64-virt-default-models.aarch64-latest.args | 1 +
.../aarch64-virt-default-models.aarch64-latest.xml | 3 +++
.../qemuxmlconfdata/aarch64-virt-default-models.xml | 2 +-
tests/qemuxmlconftest.c | 1 -
9 files changed, 12 insertions(+), 16 deletions(-)
delete mode 100644
tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err
delete mode 100644 tests/qemuxmlconfdata/aarch64-panic-no-model.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0cc335eb30..4b3b5077e0 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4162,6 +4162,9 @@ qemuDomainDefaultPanicModel(const virDomainDef *def)
if (ARCH_IS_X86(def->os.arch))
return VIR_DOMAIN_PANIC_MODEL_ISA;
+if (qemuDomainIsARMVirt(def))
+return VIR_DOMAIN_PANIC_MODEL_PVPANIC;
+
return VIR_DOMAIN_PANIC_MODEL_DEFAULT;
}
diff --git a/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err
b/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err
deleted file mode 100644
index 139249bbc5..00
--- a/tests/qemuxmlconfdata/aarch64-panic-no-model.aarch64-latest.err
+++ /dev/null
@@ -1 +0,0 @@
-unsupported configuration: no panic model provided, and no default for the
architecture and machine type
diff --git a/tests/qemuxmlconfdata/aarch64-panic-no-model.xml
b/tests/qemuxmlconfdata/aarch64-panic-no-model.xml
deleted file mode 100644
index 5207e48bbd..00
--- a/tests/qemuxmlconfdata/aarch64-panic-no-model.xml
+++ /dev/null
@@ -1,13 +0,0 @@
-
- guest
- 1ccfd97d-5eb4-478a-bbe6-88d254c16db7
- 4194304
- 4
-
-hvm
-
-
-/usr/bin/qemu-system-aarch64
-
-
-
diff --git
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
index a503f45d0c..96fb251d80 100644
---
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
+++
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.args
@@ -44,4 +44,5 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config
\
-audiodev '{"id":"audio1","driver":"none"}' \
-device
'{"driver":"virtio-gpu-pci","id":"video0","max_outputs":1,"bus":"pci.5","addr":"0x0"}'
\
-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-device '{"driver":"pvpanic-pci","bus":"pcie.0","addr":"0x2"}' \
-msg timestamp=on
diff --git
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
index bbe1dd931d..f27e7e1522 100644
---
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
+++
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.abi-update.xml
@@ -78,5 +78,8 @@
+
+
+
diff --git
a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
index a503f45d0c..96fb251d80 100644
--- a/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
+++ b/tests/qemuxmlconfdata/aarch64-virt-default-models.aarch64-latest.args
@@ -44,4 +44,5 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-guest/.config
\
-audiodev '{"id":"audio1","driver":"none"}' \
-device
'{"driver":"virtio-gpu-pci","id":"video0","max_outputs":1,"bus":"pci.5","addr":"0x0"}'
\
-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-device '{"driver":"pvpanic-pci","bus":"pcie.0","addr":"0x2"}' \
-msg timestamp=on
diff --git
a/tests/qemuxmlconfdata/aarch64-virt-de
[PATCH FOR-10.7.0 0/3] qemu: Expose availability of PS/2 feature in domcaps
Andrea Bolognani (3): qemu: Export a few functions qemu: Change signature for virQEMUCapsSupportsI8042Toggle() qemu: Expose availability of PS/2 feature in domcaps src/conf/domain_capabilities.c| 1 + src/conf/domain_capabilities.h| 1 + src/conf/schemas/domaincaps.rng | 9 +++ src/qemu/qemu_capabilities.c | 24 +++ src/qemu/qemu_capabilities.h | 3 ++- src/qemu/qemu_domain.c| 14 +++ src/qemu/qemu_domain.h| 6 + src/qemu/qemu_validate.c | 2 +- .../domaincapsdata/qemu_5.2.0-q35.x86_64.xml | 1 + .../qemu_5.2.0-tcg-virt.riscv64.xml | 1 + .../domaincapsdata/qemu_5.2.0-tcg.x86_64.xml | 1 + .../qemu_5.2.0-virt.aarch64.xml | 1 + .../qemu_5.2.0-virt.riscv64.xml | 1 + tests/domaincapsdata/qemu_5.2.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_5.2.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_5.2.0.s390x.xml | 1 + tests/domaincapsdata/qemu_5.2.0.x86_64.xml| 1 + .../domaincapsdata/qemu_6.0.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_6.0.0-tcg.x86_64.xml | 1 + .../qemu_6.0.0-virt.aarch64.xml | 1 + tests/domaincapsdata/qemu_6.0.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_6.0.0.s390x.xml | 1 + tests/domaincapsdata/qemu_6.0.0.x86_64.xml| 1 + .../domaincapsdata/qemu_6.1.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_6.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_6.1.0.x86_64.xml| 1 + .../domaincapsdata/qemu_6.2.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml | 1 + .../qemu_6.2.0-virt.aarch64.xml | 1 + tests/domaincapsdata/qemu_6.2.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_6.2.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_6.2.0.x86_64.xml| 1 + .../qemu_7.0.0-hvf.aarch64+hvf.xml| 1 + .../domaincapsdata/qemu_7.0.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_7.0.0-tcg.x86_64.xml | 1 + .../qemu_7.0.0-virt.aarch64.xml | 1 + tests/domaincapsdata/qemu_7.0.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_7.0.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_7.0.0.x86_64.xml| 1 + .../domaincapsdata/qemu_7.1.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_7.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.1.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_7.1.0.x86_64.xml| 1 + .../qemu_7.2.0-hvf.x86_64+hvf.xml | 1 + .../domaincapsdata/qemu_7.2.0-q35.x86_64.xml | 1 + .../qemu_7.2.0-tcg.x86_64+hvf.xml | 1 + .../domaincapsdata/qemu_7.2.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.2.0.ppc.xml | 1 + tests/domaincapsdata/qemu_7.2.0.x86_64.xml| 1 + .../domaincapsdata/qemu_8.0.0-q35.x86_64.xml | 1 + .../qemu_8.0.0-tcg-virt.riscv64.xml | 1 + .../domaincapsdata/qemu_8.0.0-tcg.x86_64.xml | 1 + .../qemu_8.0.0-virt.riscv64.xml | 1 + tests/domaincapsdata/qemu_8.0.0.x86_64.xml| 1 + .../domaincapsdata/qemu_8.1.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_8.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_8.1.0.s390x.xml | 1 + tests/domaincapsdata/qemu_8.1.0.x86_64.xml| 1 + .../domaincapsdata/qemu_8.2.0-q35.x86_64.xml | 1 + .../qemu_8.2.0-tcg-virt.loongarch64.xml | 1 + .../domaincapsdata/qemu_8.2.0-tcg.x86_64.xml | 1 + .../qemu_8.2.0-virt.aarch64.xml | 1 + .../qemu_8.2.0-virt.loongarch64.xml | 1 + tests/domaincapsdata/qemu_8.2.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_8.2.0.armv7l.xml| 1 + tests/domaincapsdata/qemu_8.2.0.s390x.xml | 1 + tests/domaincapsdata/qemu_8.2.0.x86_64.xml| 1 + .../domaincapsdata/qemu_9.0.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_9.0.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_9.0.0.sparc.xml | 1 + tests/domaincapsdata/qemu_9.0.0.x86_64.xml| 1 + .../domaincapsdata/qemu_9.1.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_9.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_9.1.0.x86_64.xml| 1 + 74 files changed, 115 insertions(+), 11 deletions(-) -- 2.46.0
[PATCH FOR-10.7.0 1/3] qemu: Export a few functions
We're going to need them in a minute.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_domain.c | 14 ++
src/qemu/qemu_domain.h | 6 ++
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d1767c326d..aa4a2b50bb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9045,8 +9045,9 @@ qemuFindAgentConfig(virDomainDef *def)
return NULL;
}
-
-static bool
+/* You should normally avoid this function and use
+ * qemuDomainMachineIsQ35() instead. */
+bool
qemuDomainMachineIsQ35(const char *machine,
const virArch arch)
{
@@ -9062,7 +9063,9 @@ qemuDomainMachineIsQ35(const char *machine,
}
-static bool
+/* You should normally avoid this function and use
+ * qemuDomainMachineIsI440FX() instead. */
+bool
qemuDomainMachineIsI440FX(const char *machine,
const virArch arch)
{
@@ -9179,7 +9182,10 @@ qemuDomainMachineIsMipsMalta(const char *machine,
return false;
}
-static bool
+
+/* You should normally avoid this function and use
+ * qemuDomainMachineIsXenFV() instead. */
+bool
qemuDomainMachineIsXenFV(const char *machine,
const virArch arch)
{
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 1179b0d6dc..d799f6c086 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -823,10 +823,16 @@ virDomainChrDef *qemuFindAgentConfig(virDomainDef *def);
/* You should normally avoid these functions and use the variant that
* doesn't have "Machine" in the name instead. */
+bool qemuDomainMachineIsQ35(const char *machine,
+const virArch arch);
+bool qemuDomainMachineIsI440FX(const char *machine,
+ const virArch arch);
bool qemuDomainMachineIsARMVirt(const char *machine,
const virArch arch);
bool qemuDomainMachineIsPSeries(const char *machine,
const virArch arch);
+bool qemuDomainMachineIsXenFV(const char *machine,
+ const virArch arch);
bool qemuDomainMachineHasBuiltinIDE(const char *machine,
const virArch arch);
--
2.46.0
[PATCH FOR-10.7.0 2/3] qemu: Change signature for virQEMUCapsSupportsI8042Toggle()
We will soon need to use it in a context where we don't have
a virDomainDef handy.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_capabilities.c | 11 ++-
src/qemu/qemu_capabilities.h | 3 ++-
src/qemu/qemu_validate.c | 2 +-
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2d53e87ff3..bde28ad083 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -6032,15 +6032,16 @@ virQEMUCapsSupportsI8042(virQEMUCaps *qemuCaps,
bool
virQEMUCapsSupportsI8042Toggle(virQEMUCaps *qemuCaps,
- const virDomainDef *def)
+ const char *machine,
+ const virArch arch)
{
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_I8042_OPT))
return false;
-return qemuDomainIsI440FX(def) ||
-qemuDomainIsQ35(def) ||
-qemuDomainIsXenFV(def) ||
-STREQ(def->os.machine, "isapc");
+return qemuDomainMachineIsI440FX(machine, arch) ||
+ qemuDomainMachineIsQ35(machine, arch) ||
+ qemuDomainMachineIsXenFV(machine, arch) ||
+ STREQ(machine, "isapc");
}
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 9c577c1505..5036d49aab 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -723,7 +723,8 @@ bool virQEMUCapsSupportsI8042(virQEMUCaps *qemuCaps,
const virDomainDef *def);
bool virQEMUCapsSupportsI8042Toggle(virQEMUCaps *qemuCaps,
-const virDomainDef *def);
+const char *machine,
+const virArch arch);
const char *virQEMUCapsGetBinary(virQEMUCaps *qemuCaps);
virArch virQEMUCapsGetArch(virQEMUCaps *qemuCaps);
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index f74c538efe..3c40f76c12 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -258,7 +258,7 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
}
if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
-!virQEMUCapsSupportsI8042Toggle(qemuCaps, def)) {
+!virQEMUCapsSupportsI8042Toggle(qemuCaps, def->os.machine,
def->os.arch)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("ps2 feature state cannot be controlled with
this QEMU binary"));
return -1;
--
2.46.0
[PATCH FOR-10.7.0 3/3] qemu: Expose availability of PS/2 feature in domcaps
This advertises the feature only for the architectures and machine types where it can actually be used. Signed-off-by: Andrea Bolognani --- src/conf/domain_capabilities.c | 1 + src/conf/domain_capabilities.h | 1 + src/conf/schemas/domaincaps.rng | 9 + src/qemu/qemu_capabilities.c| 13 + tests/domaincapsdata/qemu_5.2.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_5.2.0-tcg-virt.riscv64.xml | 1 + tests/domaincapsdata/qemu_5.2.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_5.2.0-virt.aarch64.xml| 1 + tests/domaincapsdata/qemu_5.2.0-virt.riscv64.xml| 1 + tests/domaincapsdata/qemu_5.2.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_5.2.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_5.2.0.s390x.xml | 1 + tests/domaincapsdata/qemu_5.2.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_6.0.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_6.0.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_6.0.0-virt.aarch64.xml| 1 + tests/domaincapsdata/qemu_6.0.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_6.0.0.s390x.xml | 1 + tests/domaincapsdata/qemu_6.0.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_6.1.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_6.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_6.1.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_6.2.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_6.2.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_6.2.0-virt.aarch64.xml| 1 + tests/domaincapsdata/qemu_6.2.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_6.2.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_6.2.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.0.0-hvf.aarch64+hvf.xml | 1 + tests/domaincapsdata/qemu_7.0.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.0.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.0.0-virt.aarch64.xml| 1 + tests/domaincapsdata/qemu_7.0.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_7.0.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_7.0.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.1.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.1.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_7.1.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.2.0-hvf.x86_64+hvf.xml | 1 + tests/domaincapsdata/qemu_7.2.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.2.0-tcg.x86_64+hvf.xml | 1 + tests/domaincapsdata/qemu_7.2.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_7.2.0.ppc.xml | 1 + tests/domaincapsdata/qemu_7.2.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_8.0.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_8.0.0-tcg-virt.riscv64.xml | 1 + tests/domaincapsdata/qemu_8.0.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_8.0.0-virt.riscv64.xml| 1 + tests/domaincapsdata/qemu_8.0.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_8.1.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_8.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_8.1.0.s390x.xml | 1 + tests/domaincapsdata/qemu_8.1.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_8.2.0-q35.x86_64.xml | 1 + .../qemu_8.2.0-tcg-virt.loongarch64.xml | 1 + tests/domaincapsdata/qemu_8.2.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_8.2.0-virt.aarch64.xml| 1 + .../domaincapsdata/qemu_8.2.0-virt.loongarch64.xml | 1 + tests/domaincapsdata/qemu_8.2.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_8.2.0.armv7l.xml | 1 + tests/domaincapsdata/qemu_8.2.0.s390x.xml | 1 + tests/domaincapsdata/qemu_8.2.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_9.0.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_9.0.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_9.0.0.sparc.xml | 1 + tests/domaincapsdata/qemu_9.0.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_9.1.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_9.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_9.1.0.x86_64.xml | 1 + 70 files changed, 90 insertions(+) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 199bac006b..30540de326 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -42,6 +42,7 @@ VIR_ENUM_IMPL(virDomainCapsFeature, "backup", "async-teardown", "s390-pv", + "ps2", ); static virClass *virDomainCapsClass; diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 3fe62ce211..2a4596ac14 100644 --- a/src/conf/domain_capabilities.h +++ b/src/con
Re: [PATCH v3 0/6] qemu: Introduce the ability to disable the built-in PS/2 controller
On Tue, Aug 27, 2024 at 05:14:21AM GMT, Andrea Bolognani wrote: > On Wed, Aug 21, 2024 at 03:36:03PM GMT, Kamil Szczęk wrote: > > On Wednesday, August 21st, 2024 at 17:15, Michal Prívozník > > wrote: > > > We require that we're able to compile and tests pass after each commit > > > (it's easier when we need to bisect something in the future). Therefore, > > > I'm squashing 3/6 into 2/6. > > > > Makes sense, will keep that in mind. > > > > > and merged. Congratulations in your first libvirt contribution! > > > > Awesome, thanks. Looking forward to contributing further! > > I think the availability of this new feature should be advertised in > the domain capabilities XML. > > That way, management applications such as virt-manager will be able > to detect its presence and decide to ditch PS/2 completely for > headless VMs, where it's not necessary to have any input devices. > > For VMs with graphics, they could add a USB keyboard to go along with > the USB tablet that is already added, instead of the current combo of > PS/2 keyboard and mouse *plus* USB tablet, for a slightly cleaner > overall configuration matching that of other architectures that are > not beholden to the same legacy as x86. Patches here: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/MY7APHGASWQ2SNGK2T3DFRKYWEYXBKTS/ Ignore the fact that hyperkitty-- is currently completely broken and only shows the cover letter :( -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH FOR-10.7.0 0/3] qemu: Expose availability of PS/2 feature in domcaps
On Wed, Aug 28, 2024 at 01:49:22PM GMT, Kamil Szczęk wrote:
> Hi Andrea,
>
> I was actually working on the same thing today, but haven't gotten around to
> posting the patches yet.
> Do you think it would make sense to make this feature indeterminate for
> non-i8042 machines?
>
> Like so:
>
> static void
> virQEMUCapsFillDomainFeaturePS2Caps(virQEMUCaps *qemuCaps,
> virDomainCaps *domCaps)
> {
> if (!virQEMUCapsMachineSupportsI8042(qemuCaps, domCaps->machine,
> domCaps->arch))
> return;
>
> if (virQEMUCapsMachineSupportsI8042Toggle(qemuCaps, domCaps->machine,
> domCaps->arch))
> domCaps->features[VIR_DOMAIN_CAPS_FEATURE_PS2] =
> VIR_TRISTATE_BOOL_YES;
> else
> domCaps->features[VIR_DOMAIN_CAPS_FEATURE_PS2] = VIR_TRISTATE_BOOL_NO;
> }
I had something like that initially, but then I realized that it
doesn't really make sense. Reporting
on e.g. aarch64 is entirely accurate after all.
--
Andrea Bolognani / Red Hat / Virtualization
[PATCH FOR-10.7.0 4/3] docs: Document presence of PS/2 feature in domcaps
Signed-off-by: Andrea Bolognani --- docs/formatdomaincaps.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/docs/formatdomaincaps.rst b/docs/formatdomaincaps.rst index be45de8996..712d8b44f8 100644 --- a/docs/formatdomaincaps.rst +++ b/docs/formatdomaincaps.rst @@ -754,6 +754,12 @@ element in the domain XML `__. For more details on the Protected Virtualization feature please see `Protected Virtualization on s390 `__. +ps2 capability +^^ + +Reports whether it is possible to disable the machine's built-in PS/2 +controller. + SEV capabilities -- 2.46.0
Re: [PATCH FOR-10.7.0 3/3] qemu: Expose availability of PS/2 feature in domcaps
On Wed, Aug 28, 2024 at 04:59:33PM GMT, Michal Prívozník wrote: > On 8/28/24 15:08, Andrea Bolognani wrote: > > This advertises the feature only for the architectures and > > machine types where it can actually be used. > > Don't forget to put a few words about this new element into > docs/formatdomaincaps.rst Good point. Posted as 4/3, let me know if I can push it together with the rest. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH TRIVIAL] qemu: Fix a few comments
On Thu, Aug 29, 2024 at 10:25:19AM GMT, Kamil Szczęk wrote: > /* You should normally avoid this function and use > - * qemuDomainMachineIsQ35() instead. */ > + * qemuDomainIsQ35() instead. */ > bool > qemuDomainMachineIsQ35(const char *machine, > const virArch arch) Well that's embarrassing :) Thanks for catching it. Reviewed-by: Andrea Bolognani and pushed. -- Andrea Bolognani / Red Hat / Virtualization
[PATCH v6 00/13] qemu: Introduce shared_filesystems configuration option
The need to have something like this in the first place is driven by KubeVirt (see [1] and [2]). A draft version of this series has been integrated into KubeVirt and it has been confirmed that it was effective in removing the need to use LD_PRELOAD hacks in the storage provider. Changes from [v5]: * make migration of domains with TPM work (patches 12 and 13); * fixed all typos for "remember"; * added R-bs for Peter's patches. Changes from [v4] (v5 was posted by Peter): * added patch 7 cleaning up a helper function (noticed just while reading the code) * added patch 8 properly unrefing security labels in dac/selinux drivers on outgoing migration * patch 11: added handling of the 'nvram' image file (and refactored the function to allow reuse) Changes from [v3] (v4 was posted by Peter): * patch 2/8 was modified to change the docs for the new option. * patches 1-5 will get an R-b by me as I've adopted them. * patches 6, 9-11 are new. * patches 7, 8 were not part of v3 Changes from [v2]: * added canonicalization for user-provided paths; * fixed compilation issues when AppArmor support is enabled. Changes from [v1]: * documented more explicitly that the newly introduced option is intended for very specific scenarios and not general usage; as part of this, the NEWS update has been dropped too; * made a few tweaks and addressed a few oversight based on review feedback; * several preparatory cleanup patches have been pushed. Changes from [v0]: * reworked approach. [v5] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/HNF576CP4LSJJTSNP5MKG32MCBTCCDQ6/ [v4] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/FWR7YCZJUHBZH33EX465GSE4EQI6KRWA/ [v3] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/PISBZCI5MAQQWPN7NMMEGV4VPLJKGEFJ/ [v2] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/XPCPYID6ZS5NXQCAYCUHFMCXJFL6C3TP/ [v1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/XEISMPGRJHFRT4LZ3MJ3L3XR7OPOQKPM/ [v0] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/MMKVR54LD3SDG5CMSXUECV7I57LMJJTH/ [1] https://issues.redhat.com/browse/CNV-34322 [2] https://issues.redhat.com/browse/CNV-39370 Andrea Bolognani (7): security: Fix alignment qemu: Introduce shared_filesystems configuration option qemu: Propagate shared_filesystems utils: Use overrides in virFileIsSharedFS() qemu: Always set labels for TPM state security: Always forget labels for TPM state directory qemu: Don't lock TPM state directory for incoming migration Peter Krempa (6): virFileIsSharedFSOverride: Export virParseOwnershipIds: Refactor virSecuritySELinuxRestoreImageLabelInt: Move FD image relabeling after 'migrated' check security_(dac|selinux): Unref remembered security labels on outgoing migration storage_source: Add field for skipping seclabel remembering qemu: migration: Don't remember seclabel for images shared from current host src/conf/storage_source_conf.c | 3 + src/conf/storage_source_conf.h | 9 ++ src/libvirt_private.syms | 1 + src/lxc/lxc_controller.c | 3 +- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_process.c | 4 +- src/qemu/libvirtd_qemu.aug | 3 + src/qemu/qemu.conf.in | 26 + src/qemu/qemu_conf.c | 31 ++ src/qemu/qemu_conf.h | 2 + src/qemu/qemu_domain.c | 7 +- src/qemu/qemu_extdevice.c | 2 +- src/qemu/qemu_migration.c | 86 ++--- src/qemu/qemu_security.c | 95 +- src/qemu/qemu_security.h | 6 +- src/qemu/qemu_tpm.c| 50 ++ src/qemu/qemu_tpm.h| 10 +- src/qemu/test_libvirtd_qemu.aug.in | 5 + src/security/security_apparmor.c | 8 +- src/security/security_dac.c| 53 +-- src/security/security_driver.h | 8 +- src/security/security_manager.c| 33 +-- src/security/security_manager.h| 9 +- src/security/security_nop.c| 5 + src/security/security_selinux.c| 148 +++-- src/security/security_stack.c | 32 +-- src/util/virfile.c | 63 +++- src/util/virfile.h | 5 +- src/util/virutil.c | 20 ++-- tests/securityselinuxlabeltest.c | 2 +- tests/virfiletest.c| 2 +- 31 files changed, 594 insertions(+), 139 deletions(-) -- 2.46.0
