[PATCHv2] ch_domain: Add handler for virDomainDeviceDefValidateCallback
Instead of trying to match devices passed in based on the monitor
detecting the number of devices that were used in the domain
definition, use the deviceValidateCallback to evaluate if
unsupported devices are used.
This allows the compiler to detect when new device types are added
that need to be checked.
Signed-off-by: William Douglas
---
The only change from the previous version was to switch to use the XML
validation callback based on the great explanations from Michal and Peter.
---
src/ch/ch_domain.c | 120 +++
src/ch/ch_monitor.c | 122
2 files changed, 120 insertions(+), 122 deletions(-)
diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
index f9a6f3f31d..3495ee22ff 100644
--- a/src/ch/ch_domain.c
+++ b/src/ch/ch_domain.c
@@ -197,7 +197,127 @@ virCHDomainDefPostParse(virDomainDef *def,
return 0;
}
+static int
+chValidateDomainDeviceDef(const virDomainDeviceDef *dev,
+ const virDomainDef *def G_GNUC_UNUSED,
+ void *opaque G_GNUC_UNUSED,
+ void *parseOpaque G_GNUC_UNUSED)
+{
+int ret = -1;
+
+switch ((virDomainDeviceType)dev->type) {
+case VIR_DOMAIN_DEVICE_DISK:
+ret = 0;
+break;
+case VIR_DOMAIN_DEVICE_LEASE:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support lease"));
+break;
+case VIR_DOMAIN_DEVICE_FS:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support fs"));
+break;
+case VIR_DOMAIN_DEVICE_NET:
+ret = 0;
+break;
+case VIR_DOMAIN_DEVICE_INPUT:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support input"));
+break;
+case VIR_DOMAIN_DEVICE_SOUND:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support sound"));
+break;
+case VIR_DOMAIN_DEVICE_VIDEO:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support video"));
+break;
+case VIR_DOMAIN_DEVICE_HOSTDEV:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support hostdev"));
+break;
+case VIR_DOMAIN_DEVICE_WATCHDOG:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support watchdog"));
+break;
+case VIR_DOMAIN_DEVICE_CONTROLLER:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support controller"));
+break;
+case VIR_DOMAIN_DEVICE_GRAPHICS:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support graphics"));
+break;
+case VIR_DOMAIN_DEVICE_HUB:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support hub"));
+break;
+case VIR_DOMAIN_DEVICE_REDIRDEV:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support redirdev"));
+break;
+case VIR_DOMAIN_DEVICE_SMARTCARD:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support smartcard"));
+break;
+case VIR_DOMAIN_DEVICE_CHR:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support chr"));
+break;
+case VIR_DOMAIN_DEVICE_MEMBALLOON:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support memballoon"));
+break;
+case VIR_DOMAIN_DEVICE_NVRAM:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support nvram"));
+break;
+case VIR_DOMAIN_DEVICE_RNG:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support rng"));
+break;
+case VIR_DOMAIN_DEVICE_SHMEM:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support shmem"));
+break;
+case VIR_DOMAIN_DEVICE_TPM:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support tpm"));
+break;
+case VIR_DOMAIN_DEVICE_PANIC:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support panic"));
+break;
+case VIR_DOMAIN_DEVICE_MEMORY:
+ret = 0;
+break;
+case VIR_DOMAIN_DEVICE_IOMMU:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+
Re: [libvirt PATCH 0/4] remote: switch to modular daemons by default
On 6/15/21 2:42 AM, Daniel P. Berrangé wrote:
On Mon, Jun 14, 2021 at 05:22:22PM -0600, Jim Fehlig wrote:
On 6/10/21 7:43 AM, Daniel P. Berrangé wrote:
This series first improves driver probing when using modular daemons.
Currently when URI is NULL, we connect to virtproxyd and it looks
at which UNIX sockets exist and what binaries exist, to decide which
modular hypervisor daemon to connect to.
This means the common case results in all traffic going via virtproxyd.
Moving the logic out of virtproxyd into the remote client means we can
avoid using virtproxyd by default.
With this, we can now switch to the modular daemons by default. The
latter change primarily impacts how autostart works
When running as root we simply connect to whatever UNIX socket exists
and rely on systemd to autostart if needed. Whether the UNIX sockets
are for the modular daemon or libvirt doesn't matter - we'll look for
both. Defaults are dependent on the distros' systemd presets. I intend
to get Fedora / RHEL-9 presets changed to use the modular daemons.
I'll need to do the same for the SUSE presets, along with adjusting zypper
patterns that include libvirtd, and other downstream tweaks. Additional
testing may uncover other issues I haven't considered. I don't _think_
apparmor will prevent things from working since there are no profiles for
the modular daemons. But yes, I'll need to work on some profiles :-).
FWIW, with SELinux we have just copied the existing libvirtd profile
to the modular daemons. That is not optimal of course, but it is as
least no worse than current system. Over time we can refine the profile
to be more strict.
I started with the approach of copying the libvirtd profile to
virt{lxc,qemu,xen}d and removing the obvious stuff from each
https://listman.redhat.com/archives/libvir-list/2021-June/msg00456.html
The xen one in particular can be further reduced. I'm working on that and
addressing other comments for V2.
Also note if you're not ready to switch SUSE, you can just pass the
-Dremote_default_mode=legacy option to meson, which will retain
current behaviour when autostarting.
Nod. I'll make the change after gaining more confidence at the packaging level,
e.g. upgrades, etc.
BTW, I've been testing the apparmor work on top of this series and haven't
noticed any problems beyond the s/libxl/xen/ issue you already fixed. I didn't
review the changes thoroughly but can certainly give a
Tested-by: Jim Fehlig
Regards,
Jim
Re: [PATCH] remote: fix prefix for libxl Xen driver
On 6/18/21 8:09 AM, Daniel P. Berrangé wrote:
The libxl driver supports xen:///system URLs and the daemon socket
uses 'virtxend' as the socket prefix.
Reported-by: Jim Fehlig
Signed-off-by: Daniel P. Berrangé
---
src/remote/remote_daemon_dispatch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Jim Fehlig
Regards,
Jim
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 838f4a925f..dd797a81f7 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -2021,7 +2021,7 @@ remoteDispatchProbeURI(bool readonly,
* calls in daemonInitialize */
const char *drivers[] = {
# ifdef WITH_LIBXL
-"libxl",
+"xen",
# endif
# ifdef WITH_QEMU
"qemu",
Re: [PATCH 0/2] Xen: Fallout from minimum supported version bump
On Thu, Jun 17, 2021 at 02:30:27PM -0600, Jim Fehlig wrote: > Patch 1 removes the use of LIBXL_HAVE_* that are present in Xen >= 4.9. > Patch 2 mentions the version bump in News. > > Thanks danpb for a little prodding to take a closer look at potential > code reduction! After following through with the exersice, it proved > to be a bit more than I expected. > > Jim Fehlig (2): > Xen: Remove unneeded LIBXL_HAVE_* ifdefs > News: Mention bump in minimum supported Xen version Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 0/2] Xen: Fallout from minimum supported version bump
Am Thu, 17 Jun 2021 14:30:27 -0600 schrieb Jim Fehlig : > Patch 1 removes the use of LIBXL_HAVE_* that are present in Xen >= 4.9. Thanks, this compiles for, and looks good to me. Olaf pgpsQtL1UiylY.pgp Description: Digitale Signatur von OpenPGP
Re: Add options to device xml to skip reattach of pci passthrough devices.
On Fri, 18 Jun 2021 10:43:07 -0400 Laine Stump wrote: > On 6/16/21 4:15 PM, Daniel Henrique Barboza wrote: > > > > > > On 6/9/21 4:38 PM, Manish Mishra wrote: > >> Hi Everyone, > >> > >> We want to add extra options to device xml to skip reattach of pci > >> passthrough devices. Following is xml format for pci passthrough > >> devices added to domain as of now. > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> When we pass managed=’yes’ flag through xml, libvirt takes > >> responsibility of detaching device on domain(guest VM) start and > >> reattaching on domain shutdown. We observed some issues where guest VM > >> shutdown may take long time, blocked for reattach operation on pci > >> passthrough device. As domain lock is held during this time it also > >> makes libvirt mostly inactive as it blocks even basic operations like > >> (virsh list). Reattaching of device to host can block due to reasons > >> like buggy driver or initialization of device itself can take long > >> time in some cases. > > > > I am more interested in hearing about the problem with this faulty buggy > > driver holding domain lock during device reattach and compromising 'virsh' > > operations, and see if there's something to do to mitigate that, instead > > of creating a XML workaround for a driver problem. > > > >> > >> We want to pass following extra options to resolve this: > >> > >> 1. *skipReAttach*(optional flag) > >> > >> In some cases we do not need to reattach device to host as it may be > >> reserved only for guests, with this flag we can skip reattach > >> operation on host. We do not want to modify managed flag to avoid > >> regression, so thinking of adding new optional flag. > >> > >> 2. *reAttachDriverName*(optional flag) > >> > >> Name of driver to which we want to attach instead of default, to avoid > >> reattaching to buggy driver. Currently libvirt asks host to auto > >> selects driver for device. > >> > >> Yes we can use managed=’no’ but in that case user has to take > >> responsibility of detaching device before starting domain which we do > >> not want. Please let us know your views on this. > > > > The case you mentioned above, "we do not need to reattach device to host > > as it may be reserved only for guests", is one of the most common uses > > we have for managed='no' AFAIK. The user/sysadm must detach the device > > from the host, but it's only one time. After that the device can remain > > detached from the host, and guests can use it freely as long as you > > don't reboot the host (or reattach the device back). This scenario > > you described fit the managed='no' mechanics fine IMO. > > > > If you want to automate the detach process, you can use a Libvirt QEMU > > hook (/etc/libvirt/hooks/qemu) to make the device detach when starting > > the domain, in case the device isn't already detached. Note that > > this has the same effect of the "skipReAttach" option you proposed. > > > > Making a design around faulty drivers isn't ideal. If the driver you're > > using starts to have problems with the detach operation as well, > > 'skipReAttach' > > will do you no good. You'll have to fall back to 'managed=no' to circumvent > > that. > > > > Even if we discard the motivation, I'm not sure about the utility of having > > more forms of PCI assignment management (e.g > > managed=yes|no|detach|reattach). > > managed=yes|no seems to cover most use cases where the device driver works > > properly. > > > > > > Laine, what do you think? > > > I have a vague memory of someone (may even have been me) proposing > something similar several years ago, and the idea was shot down. I don't > remember the exact context or naming, but the general idea was to have > something like managed='detach-only' in order to have the advantage of > all configuration being within libvirt, but eliminating the potential > bad behavior associated with repeated re-binding of devices to drivers. > I unfortunately also don't recall the reason the idea was nixed. Dan or > Alex - do either of you have any memory of this? Sounds very vaguely familiar, but not enough to remember the arguments. > As for myself, 1) I agree with Daniel's suggestion that it is important > to find the root cause of the long delay rather than just covering it up > with another obscure option that will need to be re-discovered by anyone > who encounters the problem, and 2) every new bit that we add in makes > the code more complex and so more prone to errors, and also makes > configuration more complex and also more prone to errors. So while new > options like this could be useful, they could also be a net loss (or > not, it's hard to know without actually doing it, but once it's done it > can't be "un-done" :-)) AFAIK, the generally recommendation for "devices used only for assignment" is to use driverctl to automatically attach these devices to vfio-pci on boot/attach and
Re: Add options to device xml to skip reattach of pci passthrough devices.
On 6/16/21 4:15 PM, Daniel Henrique Barboza wrote: On 6/9/21 4:38 PM, Manish Mishra wrote: Hi Everyone, We want to add extra options to device xml to skip reattach of pci passthrough devices. Following is xml format for pci passthrough devices added to domain as of now. When we pass managed=’yes’ flag through xml, libvirt takes responsibility of detaching device on domain(guest VM) start and reattaching on domain shutdown. We observed some issues where guest VM shutdown may take long time, blocked for reattach operation on pci passthrough device. As domain lock is held during this time it also makes libvirt mostly inactive as it blocks even basic operations like (virsh list). Reattaching of device to host can block due to reasons like buggy driver or initialization of device itself can take long time in some cases. I am more interested in hearing about the problem with this faulty buggy driver holding domain lock during device reattach and compromising 'virsh' operations, and see if there's something to do to mitigate that, instead of creating a XML workaround for a driver problem. We want to pass following extra options to resolve this: 1. *skipReAttach*(optional flag) In some cases we do not need to reattach device to host as it may be reserved only for guests, with this flag we can skip reattach operation on host. We do not want to modify managed flag to avoid regression, so thinking of adding new optional flag. 2. *reAttachDriverName*(optional flag) Name of driver to which we want to attach instead of default, to avoid reattaching to buggy driver. Currently libvirt asks host to auto selects driver for device. Yes we can use managed=’no’ but in that case user has to take responsibility of detaching device before starting domain which we do not want. Please let us know your views on this. The case you mentioned above, "we do not need to reattach device to host as it may be reserved only for guests", is one of the most common uses we have for managed='no' AFAIK. The user/sysadm must detach the device from the host, but it's only one time. After that the device can remain detached from the host, and guests can use it freely as long as you don't reboot the host (or reattach the device back). This scenario you described fit the managed='no' mechanics fine IMO. If you want to automate the detach process, you can use a Libvirt QEMU hook (/etc/libvirt/hooks/qemu) to make the device detach when starting the domain, in case the device isn't already detached. Note that this has the same effect of the "skipReAttach" option you proposed. Making a design around faulty drivers isn't ideal. If the driver you're using starts to have problems with the detach operation as well, 'skipReAttach' will do you no good. You'll have to fall back to 'managed=no' to circumvent that. Even if we discard the motivation, I'm not sure about the utility of having more forms of PCI assignment management (e.g managed=yes|no|detach|reattach). managed=yes|no seems to cover most use cases where the device driver works properly. Laine, what do you think? I have a vague memory of someone (may even have been me) proposing something similar several years ago, and the idea was shot down. I don't remember the exact context or naming, but the general idea was to have something like managed='detach-only' in order to have the advantage of all configuration being within libvirt, but eliminating the potential bad behavior associated with repeated re-binding of devices to drivers. I unfortunately also don't recall the reason the idea was nixed. Dan or Alex - do either of you have any memory of this? As for myself, 1) I agree with Daniel's suggestion that it is important to find the root cause of the long delay rather than just covering it up with another obscure option that will need to be re-discovered by anyone who encounters the problem, and 2) every new bit that we add in makes the code more complex and so more prone to errors, and also makes configuration more complex and also more prone to errors. So while new options like this could be useful, they could also be a net loss (or not, it's hard to know without actually doing it, but once it's done it can't be "un-done" :-))
[PATCH] remote: fix prefix for libxl Xen driver
The libxl driver supports xen:///system URLs and the daemon socket
uses 'virtxend' as the socket prefix.
Reported-by: Jim Fehlig
Signed-off-by: Daniel P. Berrangé
---
src/remote/remote_daemon_dispatch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 838f4a925f..dd797a81f7 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -2021,7 +2021,7 @@ remoteDispatchProbeURI(bool readonly,
* calls in daemonInitialize */
const char *drivers[] = {
# ifdef WITH_LIBXL
-"libxl",
+"xen",
# endif
# ifdef WITH_QEMU
"qemu",
--
2.31.1
Re: [PATCH v2] Add SELinux policy for virt
On 24. 05. 21 14:36, Daniel P. Berrangé wrote:
On Mon, May 24, 2021 at 05:25:19AM -0700, Andrea Bolognani wrote:
On Fri, May 21, 2021 at 03:37:00PM +0100, Daniel P. Berrangé wrote:
On Fri, May 21, 2021 at 04:22:59PM +0200, Vit Mojzis wrote:
On 4/30/21 10:28 PM, Vit Mojzis wrote:
On 4/26/21 7:31 PM, Daniel P. Berrangé wrote:
On Wed, Apr 07, 2021 at 06:14:58AM -0700, Vit Mojzis wrote:
Sorry for the long delay. This is our first request to ship a
policy for
multiple selinux stores (targeted, mls and minimum).
Changes:
* Replace all selinux-policy-%{policytype} dependencies with
selinux-policy-base
* Add Ghost files representing installed policy modules in all
policy stores
* Rewrite policy compilation script in python
* Compile the policy module twice (1 version for
targeted/minimum - with
enable_mcs, and 1 for mls - with enable_mls)
* Manage policy (un)installation using triggers based on which policy
type is available
The new policy was only tested in "targeted" mode so far and
we'll need to make
sure it works properly in "mls". As for "minimum", we know it will not
work properly (as is the case of the current policy) by default (some
other "contrib" policy modules need to be enabled).
I'd argue there is no point trying to get it to work in "minimum",
mostly because it (minimum) will be retired soon.
I'm wondering how SELinux is supposed to integrate with containers when
using a modular policy.
Right now you can install RPMs in a container, and use selinux
enforcement
on that container because the host OS policy provides all the rules
in the
monolithic blob.
If we take this policy into libvirt, then when you install libvirt in a
container, there will be no selinux policy available.
Users can't install libvirt-selinux inside the container, as it
needs to be
built against the main policy in the host.
User likely won't install libvirt-selinux outside the container as that
defeats the purpose of using containers for their deployment mechanism.
Container based deployment of libvirt is important for both OpenStack
and KubeVirt.
So from discussions with respective developers i got the following:
KubeVirt runs the libvirt containers with a custom policy
https://github.com/kubevirt/kubevirt/blob/81cb9f79e0144af0e6e43c439eab7f8dac81de31/cmd/virt-handler/virt_launcher.cil,
that depends on libvirt module (uses svirt_sandbox_domain). Libvirt is only
installed inside the container and there is no bind mount of
/sys/fs/selinux. So they will need to install libvirt-daemon-selinux on the
host.
With OpenStack I believe their deployment tool manages the config of
the entire host, so installing the libvirt-daemon-selinux package
ought to be reasonably straightforward for them.
I worry about KubeVirt though. IIUC in their deployment, the hosts
in use are all provisioned by OpenShift upfront & when KubeVirt is
deployed, the only pieces they're deploying live inside the host.
IOW, it seems like libvirt-daemon-selinux would have to be provided
ahead of time by OpenShift if it is to be used, and I'm not sure
if that's a practical requirement.
I think we need to get explicit confirmation from KubeVirt that
a requirement to installing RPMs directly on the host is going
to be acceptable.
I'm afraid that's not going to fly for KubeVirt.
Adding Roman and Vladik so they can provide more information.
For context, the discussion is about shipping the SELinux policy
for libvirt as part of a sub-package of libvirt instead of the main
selinux-policy package.
Reading again, I realize Vit links to a URL above that shows
virt-handler includes a custom selinux policy.
How does that get deployed, and can the libvirt-daemon-selinux
stuff be deployed in the same way ?
Based on a quick look at virt-handler it seems like the policy is
installed by installPolicy in cmd/virt-handler/virt-handler.go, which
just calls "semodule -i".
Shipping the policy is much more straight-forward in this case, since
it's in "cil" format, which means it does not need to be compiled before
installation.
I expect that it would be easier to include virt-daemon-selinux as a
dependency, instead of managing the virt policy.
Vit
Regards,
Daniel
Re: [libvirt PATCH 0/3] wait longer for virtiofsd to exit (virtio-fs epopee)
On 6/18/21 1:45 PM, Ján Tomko wrote: > See patch 2/3. > > Ján Tomko (3): > Introduce virPidFileForceCleanupPathDelay > qemu: wait more for virtiofsd to exit > util: fix typo > > src/libvirt_private.syms | 1 + > src/qemu/qemu_virtiofs.c | 2 +- > src/util/virpidfile.c| 16 +++- > src/util/virpidfile.h| 2 ++ > src/util/virprocess.c| 2 +- > 5 files changed, 20 insertions(+), 3 deletions(-) > Reviewed-by: Michal Privoznik Michal
Re: [libvirt PATCH 2/3] qemu: wait more for virtiofsd to exit
On 6/18/21 1:45 PM, Ján Tomko wrote:
> In some cases, such as doing intense I/O on slow filesystems,
> it can take virtiofsd as long as 42 seconds to exit.
>
> Add a delay of extra 45 seconds before we forcefully kill it.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1940276
This is inaccessible :-( We should look into making it accessible.
>
> Signed-off-by: Ján Tomko
> ---
> src/qemu/qemu_virtiofs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
> index edaedf0304..3f99f0caa4 100644
> --- a/src/qemu/qemu_virtiofs.c
> +++ b/src/qemu/qemu_virtiofs.c
> @@ -281,7 +281,7 @@ qemuVirtioFSStop(virQEMUDriver *driver G_GNUC_UNUSED,
> if (!(pidfile = qemuVirtioFSCreatePidFilename(vm, fs->info.alias)))
> goto cleanup;
>
Please put a comment here that the wait is 30 + 15 seconds. Otherwise 30
is misleading.
> -if (virPidFileForceCleanupPath(pidfile) < 0) {
> +if (virPidFileForceCleanupPathDelay(pidfile, 30) < 0) {
> VIR_WARN("Unable to kill virtiofsd process");
> } else {
> if (QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock)
>
Michal
Re: [PATCH 3/4] virISCSIDirectUpdateTargets: Rework to simplify cleanup and return GStrv
On 6/18/21 3:04 PM, Peter Krempa wrote:
> Count the elements in advance rather than using VIR_APPEND_ELEMENT and
> ensure that there's a NULL terminator for the string list so it's GStrv
> compatible.
>
> Signed-off-by: Peter Krempa
> ---
> src/storage/storage_backend_iscsi_direct.c | 29 --
> 1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/src/storage/storage_backend_iscsi_direct.c
> b/src/storage/storage_backend_iscsi_direct.c
> index 263db835ae..2073a6df38 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -420,37 +420,30 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
> size_t *ntargets,
> char ***targets)
> {
> -int ret = -1;
> struct iscsi_discovery_address *addr;
> struct iscsi_discovery_address *tmp_addr;
> -size_t tmp_ntargets = 0;
> -char **tmp_targets = NULL;
> +
> +*ntargets = 0;
>
> if (!(addr = iscsi_discovery_sync(iscsi))) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to discover session: %s"),
> iscsi_get_error(iscsi));
> -return ret;
> +return -1;
> }
>
> -for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
> -g_autofree char *target = NULL;
> -
> -target = g_strdup(tmp_addr->target_name);
> +for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
> +(*ntargets)++;
>
> -if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0)
> -goto cleanup;
> -}
> +*targets = g_new0(char *, ntargets + 1);
I'm afraid ntargets + 1 will be too big on many systems. Please use:
*ntargets + 1
> +*ntargets = 0;
>
> -*targets = g_steal_pointer(_targets);
> -*ntargets = tmp_ntargets;
> -tmp_ntargets = 0;
> +for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
> +*targets[(*ntargets)++] = g_strdup(tmp_addr->target_name);
>
Re-using *ntargets makes this a bit harder to follow, consider using 'i' and
only
filling *ntargets once.
Jano
> -ret = 0;
> - cleanup:
> iscsi_free_discovery_data(iscsi, addr);
> -virStringListFreeCount(tmp_targets, tmp_ntargets);
> -return ret;
> +
> +return 0;
> }
>
> static int
>
Re: [PATCH v2 1/4] conf: refactor launch security to allow more types
On Fri, Jun 18, 2021 at 15:20:24 +0200, Boris Fiuczynski wrote: > To allow other types of launch security the SEV type specific > parameters like e.g. policy need to be optional and be separated > from other new launch security types. A test is added to ensure > the previously required and now optional launch security policy > remains required when launch security type is SEV. > > Signed-off-by: Boris Fiuczynski > Reviewed-by: Daniel Henrique Barboza > --- > docs/schemas/domaincommon.rng | 12 +- > src/conf/domain_conf.c| 156 +++--- > src/conf/domain_conf.h| 13 +- > src/conf/virconftypes.h | 2 + > src/qemu/qemu_cgroup.c| 4 +- > src/qemu/qemu_command.c | 43 - > src/qemu/qemu_driver.c| 2 +- > src/qemu/qemu_firmware.c | 8 +- > src/qemu/qemu_namespace.c | 20 ++- > src/qemu/qemu_process.c | 35 +++- > src/qemu/qemu_validate.c | 22 ++- > src/security/security_dac.c | 4 +- > ...urity-sev-missing-policy.x86_64-2.12.0.err | 1 + > .../launch-security-sev-missing-policy.xml| 34 > tests/qemuxml2argvtest.c | 1 + There's a bit too much going on in this single commit. Please split it into appropriate parts.
Re: [PATCH 0/4] storage_backend_iscsi_direct: Refactor string list use and cleanup
On 6/18/21 3:04 PM, Peter Krempa wrote: > Peter Krempa (4): > conf: storage: Introduce virStoragePoolSourceListFree > virStorageBackendISCSIDirectFindPoolSources: Use allocated > virStoragePoolSourceList > virISCSIDirectUpdateTargets: Rework to simplify cleanup and return > GStrv > virStorageBackendISCSIDirectFindPoolSources: Rework cleanup > > src/conf/storage_conf.c| 16 > src/conf/storage_conf.h| 5 ++ > src/libvirt_private.syms | 1 + > src/storage/storage_backend_iscsi_direct.c | 87 +- > 4 files changed, 56 insertions(+), 53 deletions(-) > Reviewed-by: Ján Tomko Jano
[PATCH v2 3/4] conf: add s390-pv as launch security type
Add launch security type 's390-pv' as well as some tests.
Signed-off-by: Boris Fiuczynski
Reviewed-by: Daniel Henrique Barboza
---
docs/schemas/domaincommon.rng | 1 +
src/conf/domain_conf.c| 8 +
src/conf/domain_conf.h| 1 +
src/qemu/qemu_command.c | 26 ++
src/qemu/qemu_namespace.c | 1 +
src/qemu/qemu_process.c | 1 +
src/qemu/qemu_validate.c | 9 +
.../launch-security-s390-pv-ignore-policy.xml | 24 +
.../launch-security-s390-pv.xml | 18 ++
.../launch-security-s390-pv-ignore-policy.xml | 1 +
tests/genericxml2xmltest.c| 2 ++
...ty-s390-pv-ignore-policy.s390x-latest.args | 35 +++
.../launch-security-s390-pv-ignore-policy.xml | 33 +
.../launch-security-s390-pv.s390x-latest.args | 35 +++
.../launch-security-s390-pv.xml | 30
tests/qemuxml2argvtest.c | 3 ++
16 files changed, 228 insertions(+)
create mode 100644
tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml
create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv.xml
create mode 12
tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml
create mode 100644
tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args
create mode 100644
tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml
create mode 100644
tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args
create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 8c1b6c3a09..b81c51728d 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -485,6 +485,7 @@
sev
+ s390-pv
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8c2f4fd227..3f55edeefd 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1401,6 +1401,7 @@ VIR_ENUM_IMPL(virDomainLaunchSecurity,
VIR_DOMAIN_LAUNCH_SECURITY_LAST,
"",
"sev",
+ "s390-pv",
);
static virClass *virDomainObjClass;
@@ -14793,6 +14794,8 @@ virDomainSecDefParseXML(xmlNodePtr lsecNode,
if (!sec->sev)
return NULL;
break;
+case VIR_DOMAIN_LAUNCH_SECURITY_PV:
+break;
case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
default:
@@ -26889,6 +26892,11 @@ virDomainSecDefFormat(virBuffer *buf, virDomainSecDef
*sec)
break;
}
+case VIR_DOMAIN_LAUNCH_SECURITY_PV:
+virBufferAsprintf(buf, "\n",
+ virDomainLaunchSecurityTypeToString(sec->sectype));
+break;
+
case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
break;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index fa7ab1895d..9d9acab50c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2645,6 +2645,7 @@ struct _virDomainKeyWrapDef {
typedef enum {
VIR_DOMAIN_LAUNCH_SECURITY_NONE,
VIR_DOMAIN_LAUNCH_SECURITY_SEV,
+VIR_DOMAIN_LAUNCH_SECURITY_PV,
VIR_DOMAIN_LAUNCH_SECURITY_LAST,
} virDomainLaunchSecurity;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6a1383f255..271a9b880c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6975,6 +6975,9 @@ qemuBuildMachineCommandLine(virCommand *cmd,
virBufferAddLit(, ",memory-encryption=sev0");
}
break;
+case VIR_DOMAIN_LAUNCH_SECURITY_PV:
+virBufferAddLit(, ",confidential-guest-support=pv0");
+break;
case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
break;
case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
@@ -9870,6 +9873,26 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand
*cmd,
}
+static int
+qemuBuildPVCommandLine(virDomainObj *vm, virCommand *cmd)
+{
+g_autoptr(virJSONValue) props = NULL;
+g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+qemuDomainObjPrivate *priv = vm->privateData;
+
+if (qemuMonitorCreateObjectProps(, "s390-pv-guest", "pv0",
+ NULL) < 0)
+return -1;
+
+if (qemuBuildObjectCommandlineFromJSON(, props, priv->qemuCaps) < 0)
+return -1;
+
+virCommandAddArg(cmd, "-object");
+virCommandAddArgBuffer(cmd, );
+return 0;
+}
+
+
static int
qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
virDomainSecDef *sec)
@@ -9881,6 +9904,9 @@ qemuBuildSecCommandLine(virDomainObj *vm, virCommand *cmd,
case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
return qemuBuildSEVCommandLine(vm, cmd, sec->sev);
[PATCH v2 0/4] Support for launchSecurity type s390-pv
This patch series introduces the launch security type s390-pv. Specifying s390-pv as launch security type in an s390 domain prepares for running the guest in protected virtualization secure mode, also known as IBM Secure Execution. diff to v1: - Rebased to current master - Added verification check for confidential-guest-support capability *** BLURB HERE *** Boris Fiuczynski (4): conf: refactor launch security to allow more types qemu: add s390-pv-guest capability conf: add s390-pv as launch security type docs: add s390-pv documentation docs/formatdomain.rst | 7 + docs/kbase/s390_protected_virt.rst| 55 +- docs/schemas/domaincommon.rng | 13 +- src/conf/domain_conf.c| 164 +++--- src/conf/domain_conf.h| 14 +- src/conf/virconftypes.h | 2 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_cgroup.c| 4 +- src/qemu/qemu_command.c | 69 +++- src/qemu/qemu_driver.c| 2 +- src/qemu/qemu_firmware.c | 8 +- src/qemu/qemu_namespace.c | 21 ++- src/qemu/qemu_process.c | 36 +++- src/qemu/qemu_validate.c | 31 +++- src/security/security_dac.c | 4 +- .../launch-security-s390-pv-ignore-policy.xml | 24 +++ .../launch-security-s390-pv.xml | 18 ++ .../launch-security-s390-pv-ignore-policy.xml | 1 + tests/genericxml2xmltest.c| 2 + .../qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 + ...ty-s390-pv-ignore-policy.s390x-latest.args | 35 .../launch-security-s390-pv-ignore-policy.xml | 33 .../launch-security-s390-pv.s390x-latest.args | 35 .../launch-security-s390-pv.xml | 30 ...urity-sev-missing-policy.x86_64-2.12.0.err | 1 + .../launch-security-sev-missing-policy.xml| 34 tests/qemuxml2argvtest.c | 4 + 28 files changed, 543 insertions(+), 108 deletions(-) create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml create mode 100644 tests/genericxml2xmlindata/launch-security-s390-pv.xml create mode 12 tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml create mode 100644 tests/qemuxml2argvdata/launch-security-sev-missing-policy.x86_64-2.12.0.err create mode 100644 tests/qemuxml2argvdata/launch-security-sev-missing-policy.xml -- 2.30.2
[PATCH v2 2/4] qemu: add s390-pv-guest capability
Add s390-pv-guest capability.
Signed-off-by: Boris Fiuczynski
Reviewed-by: Daniel Henrique Barboza
---
src/qemu/qemu_capabilities.c| 2 ++
src/qemu/qemu_capabilities.h| 1 +
tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 +
3 files changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 059d6badf2..25774ba1c1 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -636,6 +636,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 405 */
"confidential-guest-support",
"query-display-options",
+ "s390-pv-guest",
);
@@ -1354,6 +1355,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
{ "input-linux", QEMU_CAPS_INPUT_LINUX },
{ "virtio-gpu-gl-pci", QEMU_CAPS_VIRTIO_GPU_GL_PCI },
{ "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL },
+{ "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
};
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index b2878312ac..c79ace15bc 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -616,6 +616,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
/* 405 */
QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT, /* -machine
confidential-guest-support */
QEMU_CAPS_QUERY_DISPLAY_OPTIONS, /* 'query-display-options' qmp command
present */
+QEMU_CAPS_S390_PV_GUEST, /* -object s390-pv-guest,... */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
index 1a0e75570c..4b2df16ced 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml
@@ -168,6 +168,7 @@
+
600
0
39100242
--
2.30.2
[PATCH v2 4/4] docs: add s390-pv documentation
Add documentation for launch security type s390-pv.
Signed-off-by: Boris Fiuczynski
Reviewed-by: Daniel Henrique Barboza
---
docs/formatdomain.rst | 7
docs/kbase/s390_protected_virt.rst | 55 +-
2 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index c6dede053f..a1b028c4ad 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -8078,6 +8078,13 @@ Note: DEA/TDEA is synonymous with DES/TDES.
Launch Security
---
+Specifying in a s390 domain prepares
+the guest to run in protected virtualization secure mode, also known as
+IBM Secure Execution. For more required host and guest preparation steps, see
+`Protected Virtualization on s390 `__
+:since:`Since 7.5.0`
+
+
The contents of the element is used to provide
the guest owners input used for creating an encrypted VM using the AMD SEV
feature (Secure Encrypted Virtualization). SEV is an extension to the AMD-V
diff --git a/docs/kbase/s390_protected_virt.rst
b/docs/kbase/s390_protected_virt.rst
index 1718a556d4..66203568d9 100644
--- a/docs/kbase/s390_protected_virt.rst
+++ b/docs/kbase/s390_protected_virt.rst
@@ -127,10 +127,13 @@ Protected virtualization guests support I/O using virtio
devices.
As the virtio data structures of secure guests are not accessible
by the host, it is necessary to use shared memory ('bounce buffers').
-To enable virtio devices to use shared buffers, it is necessary
-to configure them with platform_iommu enabled. This can done by adding
-``iommu='on'`` to the driver element of a virtio device definition in the
-guest's XML, e.g.
+Since libvirt 7.5.0 the
+` `__
+element with type ``s390-pv`` should be used on protected virtualization
guests.
+Without ``launchSecurity`` you must enable all virtio devices to use shared
+buffers by configuring them with platform_iommu enabled.
+This can done by adding ``iommu='on'`` to the driver element of a virtio
+device definition in the guest's XML, e.g.
::
@@ -140,8 +143,10 @@ guest's XML, e.g.
-It is mandatory to define all virtio bus devices in this way to
-prevent the host from attempting to access protected memory.
+Unless you are using ``launchSecurity`` you must define all virtio bus
+devices in this way to prevent the host from attempting to access
+protected memory.
+
Ballooning will not work and is fenced by QEMU. It should be
disabled by specifying
@@ -158,8 +163,42 @@ allocated 2K entries. A commonly used value for swiotlb is
262144.
Example guest definition
-Minimal domain XML for a protected virtualization guest, essentially
-it's mostly about the ``iommu`` property
+Minimal domain XML for a protected virtualization guest with
+the ``launchSecurity`` element of type ``s390-pv``
+
+::
+
+
+ protected
+ 2048000
+ 2048000
+ 1
+
+ hvm
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+Example guest definition without launchSecurity
+===
+
+Minimal domain XML for a protected virtualization guest using the
+``iommu='on'`` setting for each virtio device.
::
--
2.30.2
[PATCH v2 1/4] conf: refactor launch security to allow more types
To allow other types of launch security the SEV type specific
parameters like e.g. policy need to be optional and be separated
from other new launch security types. A test is added to ensure
the previously required and now optional launch security policy
remains required when launch security type is SEV.
Signed-off-by: Boris Fiuczynski
Reviewed-by: Daniel Henrique Barboza
---
docs/schemas/domaincommon.rng | 12 +-
src/conf/domain_conf.c| 156 +++---
src/conf/domain_conf.h| 13 +-
src/conf/virconftypes.h | 2 +
src/qemu/qemu_cgroup.c| 4 +-
src/qemu/qemu_command.c | 43 -
src/qemu/qemu_driver.c| 2 +-
src/qemu/qemu_firmware.c | 8 +-
src/qemu/qemu_namespace.c | 20 ++-
src/qemu/qemu_process.c | 35 +++-
src/qemu/qemu_validate.c | 22 ++-
src/security/security_dac.c | 4 +-
...urity-sev-missing-policy.x86_64-2.12.0.err | 1 +
.../launch-security-sev-missing-policy.xml| 34
tests/qemuxml2argvtest.c | 1 +
15 files changed, 257 insertions(+), 100 deletions(-)
create mode 100644
tests/qemuxml2argvdata/launch-security-sev-missing-policy.x86_64-2.12.0.err
create mode 100644
tests/qemuxml2argvdata/launch-security-sev-missing-policy.xml
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 5ea14b6dbf..8c1b6c3a09 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -483,7 +483,9 @@
-sev
+
+ sev
+
@@ -496,9 +498,11 @@
-
-
-
+
+
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f65509d8ec..8c2f4fd227 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3490,8 +3490,7 @@ virDomainResctrlDefFree(virDomainResctrlDef *resctrl)
}
-static void
-virDomainSEVDefFree(virDomainSEVDef *def)
+void virDomainSEVDefFree(virDomainSEVDef *def)
{
if (!def)
return;
@@ -3502,6 +3501,17 @@ virDomainSEVDefFree(virDomainSEVDef *def)
g_free(def);
}
+void virDomainSecDefFree(virDomainSecDef *def)
+{
+if (!def)
+return;
+
+virDomainSEVDefFree(def->sev);
+
+g_free(def);
+}
+
+
static void
virDomainOSDefClear(virDomainOSDef *os)
{
@@ -3703,7 +3713,7 @@ void virDomainDefFree(virDomainDef *def)
if (def->namespaceData && def->ns.free)
(def->ns.free)(def->namespaceData);
-virDomainSEVDefFree(def->sev);
+virDomainSecDefFree(def->sec);
xmlFreeNode(def->metadata);
@@ -14719,72 +14729,80 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
xmlXPathContextPtr ctxt)
{
VIR_XPATH_NODE_AUTORESTORE(ctxt)
-virDomainSEVDef *def;
+g_autoptr(virDomainSEVDef) sev = g_new0(virDomainSEVDef, 1);
unsigned long policy;
-g_autofree char *type = NULL;
int rc = -1;
-def = g_new0(virDomainSEVDef, 1);
-
ctxt->node = sevNode;
-if (!(type = virXMLPropString(sevNode, "type"))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing launch security type"));
-goto error;
-}
-
-def->sectype = virDomainLaunchSecurityTypeFromString(type);
-switch ((virDomainLaunchSecurity) def->sectype) {
-case VIR_DOMAIN_LAUNCH_SECURITY_SEV:
-break;
-case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
-case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
-default:
-virReportError(VIR_ERR_XML_ERROR,
- _("unsupported launch security type '%s'"),
- type);
-goto error;
-}
-
if (virXPathULongHex("string(./policy)", ctxt, ) < 0) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("failed to get launch security policy"));
-goto error;
+ _("Failed to get launch security policy for "
+ "launch security type SEV"));
+return NULL;
}
/* the following attributes are platform dependent and if missing, we can
* autofill them from domain capabilities later
*/
-rc = virXPathUInt("string(./cbitpos)", ctxt, >cbitpos);
+rc = virXPathUInt("string(./cbitpos)", ctxt, >cbitpos);
if (rc == 0) {
-def->haveCbitpos = true;
+sev->haveCbitpos = true;
} else if (rc == -2) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("Invalid format for launch security cbitpos"));
-goto error;
+return NULL;
}
rc = virXPathUInt("string(./reducedPhysBits)", ctxt,
- >reduced_phys_bits);
+ >reduced_phys_bits);
[PATCH 4/4] virStorageBackendISCSIDirectFindPoolSources: Rework cleanup
virISCSIDirectScanTargets now returns a GStrv, so we can use automatic
cleanup for it and get rid of the cleanup section.
Signed-off-by: Peter Krempa
---
src/storage/storage_backend_iscsi_direct.c | 20 ++--
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c
b/src/storage/storage_backend_iscsi_direct.c
index 2073a6df38..b108b12f7b 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -485,8 +485,7 @@ virStorageBackendISCSIDirectFindPoolSources(const char
*srcSpec,
unsigned int flags)
{
size_t ntargets = 0;
-char **targets = NULL;
-char *ret = NULL;
+g_auto(GStrv) targets = NULL;
size_t i;
g_autoptr(virStoragePoolSourceList) list =
g_new0(virStoragePoolSourceList, 1);
g_autofree char *portal = NULL;
@@ -508,20 +507,20 @@ virStorageBackendISCSIDirectFindPoolSources(const char
*srcSpec,
if (source->nhost != 1) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Expected exactly 1 host for the storage pool"));
-goto cleanup;
+return NULL;
}
if (!source->initiator.iqn) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("missing initiator IQN"));
-goto cleanup;
+return NULL;
}
if (!(portal = virStorageBackendISCSIDirectPortal(source)))
-goto cleanup;
+return NULL;
if (virISCSIDirectScanTargets(source->initiator.iqn, portal, ,
) < 0)
-goto cleanup;
+return NULL;
list->sources = g_new0(virStoragePoolSource, ntargets);
@@ -541,14 +540,7 @@ virStorageBackendISCSIDirectFindPoolSources(const char
*srcSpec,
list->nsources++;
}
-if (!(ret = virStoragePoolSourceListFormat(list)))
-goto cleanup;
-
- cleanup:
-for (i = 0; i < ntargets; i++)
-VIR_FREE(targets[i]);
-VIR_FREE(targets);
-return ret;
+return virStoragePoolSourceListFormat(list);
}
static struct iscsi_context *
--
2.31.1
[PATCH 3/4] virISCSIDirectUpdateTargets: Rework to simplify cleanup and return GStrv
Count the elements in advance rather than using VIR_APPEND_ELEMENT and
ensure that there's a NULL terminator for the string list so it's GStrv
compatible.
Signed-off-by: Peter Krempa
---
src/storage/storage_backend_iscsi_direct.c | 29 --
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c
b/src/storage/storage_backend_iscsi_direct.c
index 263db835ae..2073a6df38 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -420,37 +420,30 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
size_t *ntargets,
char ***targets)
{
-int ret = -1;
struct iscsi_discovery_address *addr;
struct iscsi_discovery_address *tmp_addr;
-size_t tmp_ntargets = 0;
-char **tmp_targets = NULL;
+
+*ntargets = 0;
if (!(addr = iscsi_discovery_sync(iscsi))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to discover session: %s"),
iscsi_get_error(iscsi));
-return ret;
+return -1;
}
-for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
-g_autofree char *target = NULL;
-
-target = g_strdup(tmp_addr->target_name);
+for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
+(*ntargets)++;
-if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0)
-goto cleanup;
-}
+*targets = g_new0(char *, ntargets + 1);
+*ntargets = 0;
-*targets = g_steal_pointer(_targets);
-*ntargets = tmp_ntargets;
-tmp_ntargets = 0;
+for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
+*targets[(*ntargets)++] = g_strdup(tmp_addr->target_name);
-ret = 0;
- cleanup:
iscsi_free_discovery_data(iscsi, addr);
-virStringListFreeCount(tmp_targets, tmp_ntargets);
-return ret;
+
+return 0;
}
static int
--
2.31.1
[PATCH 2/4] virStorageBackendISCSIDirectFindPoolSources: Use allocated virStoragePoolSourceList
Using an allocated version together with copying the
host/initiator/device portions into it allows us to switch to automatic
clearing rather than open-coding it.
Signed-off-by: Peter Krempa
---
src/storage/storage_backend_iscsi_direct.c | 42 ++
1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c
b/src/storage/storage_backend_iscsi_direct.c
index e4a14c3fd6..263db835ae 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -495,23 +495,21 @@ virStorageBackendISCSIDirectFindPoolSources(const char
*srcSpec,
char **targets = NULL;
char *ret = NULL;
size_t i;
-virStoragePoolSourceList list = {
-.type = VIR_STORAGE_POOL_ISCSI_DIRECT,
-.nsources = 0,
-.sources = NULL
-};
+g_autoptr(virStoragePoolSourceList) list =
g_new0(virStoragePoolSourceList, 1);
g_autofree char *portal = NULL;
g_autoptr(virStoragePoolSource) source = NULL;
virCheckFlags(0, NULL);
+list->type = VIR_STORAGE_POOL_ISCSI_DIRECT;
+
if (!srcSpec) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("hostname must be specified for iscsi sources"));
return NULL;
}
-if (!(source = virStoragePoolDefParseSourceString(srcSpec, list.type)))
+if (!(source = virStoragePoolDefParseSourceString(srcSpec, list->type)))
return NULL;
if (source->nhost != 1) {
@@ -532,30 +530,28 @@ virStorageBackendISCSIDirectFindPoolSources(const char
*srcSpec,
if (virISCSIDirectScanTargets(source->initiator.iqn, portal, ,
) < 0)
goto cleanup;
-list.sources = g_new0(virStoragePoolSource, ntargets);
+list->sources = g_new0(virStoragePoolSource, ntargets);
for (i = 0; i < ntargets; i++) {
-list.sources[i].devices = g_new0(virStoragePoolSourceDevice, 1);
-list.sources[i].hosts = g_new0(virStoragePoolSourceHost, 1);
-list.sources[i].nhost = 1;
-list.sources[i].hosts[0] = source->hosts[0];
-list.sources[i].initiator = source->initiator;
-list.sources[i].ndevice = 1;
-list.sources[i].devices[0].path = targets[i];
-list.nsources++;
+list->sources[i].hosts = g_new0(virStoragePoolSourceHost, 1);
+list->sources[i].nhost = 1;
+list->sources[i].hosts[0].name = g_strdup(source->hosts[0].name);
+list->sources[i].hosts[0].port = source->hosts[0].port;
+
+virStorageSourceInitiatorCopy(>sources[i].initiator,
+ >initiator);
+
+list->sources[i].devices = g_new0(virStoragePoolSourceDevice, 1);
+list->sources[i].ndevice = 1;
+list->sources[i].devices[0].path = g_strdup(targets[i]);
+
+list->nsources++;
}
-if (!(ret = virStoragePoolSourceListFormat()))
+if (!(ret = virStoragePoolSourceListFormat(list)))
goto cleanup;
cleanup:
-if (list.sources) {
-for (i = 0; i < ntargets; i++) {
-VIR_FREE(list.sources[i].hosts);
-VIR_FREE(list.sources[i].devices);
-}
-VIR_FREE(list.sources);
-}
for (i = 0; i < ntargets; i++)
VIR_FREE(targets[i]);
VIR_FREE(targets);
--
2.31.1
[PATCH 1/4] conf: storage: Introduce virStoragePoolSourceListFree
Signed-off-by: Peter Krempa
---
src/conf/storage_conf.c | 16
src/conf/storage_conf.h | 5 +
src/libvirt_private.syms | 1 +
3 files changed, 22 insertions(+)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 0ecdb0969a..2aa9a3d8f9 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1803,3 +1803,19 @@ virStoragePoolSourceListFormat(virStoragePoolSourceList
*def)
return virBufferContentAndReset();
}
+
+
+void
+virStoragePoolSourceListFree(virStoragePoolSourceList *list)
+{
+size_t i;
+
+if (!list)
+return;
+
+for (i = 0; i < list->nsources; i++)
+virStoragePoolSourceClear(>sources[i]);
+
+g_free(list->sources);
+g_free(list);
+}
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 345026aa37..76efaac531 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -266,6 +266,11 @@ struct _virStoragePoolSourceList {
virStoragePoolSource *sources;
};
+void
+virStoragePoolSourceListFree(virStoragePoolSourceList *list);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStoragePoolSourceList,
virStoragePoolSourceListFree);
+
+
virStoragePoolDef *
virStoragePoolDefParseXML(xmlXPathContextPtr ctxt);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2efa787664..68e4b6aab8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1048,6 +1048,7 @@ virStoragePoolSourceClear;
virStoragePoolSourceDeviceClear;
virStoragePoolSourceFree;
virStoragePoolSourceListFormat;
+virStoragePoolSourceListFree;
virStoragePoolSourceListNewSource;
virStoragePoolTypeFromString;
virStoragePoolTypeToString;
--
2.31.1
[PATCH 0/4] storage_backend_iscsi_direct: Refactor string list use and cleanup
Peter Krempa (4): conf: storage: Introduce virStoragePoolSourceListFree virStorageBackendISCSIDirectFindPoolSources: Use allocated virStoragePoolSourceList virISCSIDirectUpdateTargets: Rework to simplify cleanup and return GStrv virStorageBackendISCSIDirectFindPoolSources: Rework cleanup src/conf/storage_conf.c| 16 src/conf/storage_conf.h| 5 ++ src/libvirt_private.syms | 1 + src/storage/storage_backend_iscsi_direct.c | 87 +- 4 files changed, 56 insertions(+), 53 deletions(-) -- 2.31.1
Re: [RFC PATCH 1/7] qemu: provide support to query the TDX capabilities
On Fri, Jun 18, 2021 at 04:50:46PM +0800, Zhenzhong Duan wrote:
> QEMU provides support for launching an encrypted VMs on Intel x86
> platform using Trust Domain Extension (TDX) feature. This patch adds
> support to query the TDX capabilities from the QEMU.
>
> Currently there is no elements in TDX capabilities except a placeholder.
>
> Signed-off-by: Chenyi Qiang
> Signed-off-by: Zhenzhong Duan
> ---
> src/conf/domain_capabilities.c | 8 +
> src/conf/domain_capabilities.h | 10 +++
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_capabilities.c | 30 +++
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_monitor.c| 8 +
> src/qemu/qemu_monitor.h| 3 ++
> src/qemu/qemu_monitor_json.c | 53 ++
> src/qemu/qemu_monitor_json.h | 3 ++
> 9 files changed, 117 insertions(+)
>
> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index cb90ae0176..31577095e9 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -76,6 +76,14 @@ virSEVCapabilitiesFree(virSEVCapability *cap)
> g_free(cap);
> }
>
> +void
> +virTDXCapabilitiesFree(virTDXCapability *cap)
> +{
> +if (!cap)
> +return;
> +
> +VIR_FREE(cap);
> +}
>
> static void
> virDomainCapsDispose(void *obj)
> diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
> index b6433b20c9..e099788da9 100644
> --- a/src/conf/domain_capabilities.h
> +++ b/src/conf/domain_capabilities.h
> @@ -173,6 +173,12 @@ struct _virSEVCapability {
> unsigned int reduced_phys_bits;
> };
>
> +typedef struct _virTDXCapability virTDXCapability;
> +struct _virTDXCapability {
> +/* no elements for Intel TDX for now, just put a placeholder */
> +uint64_t placeholder;
> +};
> +
There is no need to add unused code into libvirt especially if the
impact is only internal and it is not exposed to users. It can be added
in the future if needed. Because this patch series doesn't add any TDX
specific capability into this structure please drop all related code
to this placeholder.
Pavel
> typedef enum {
> VIR_DOMAIN_CAPS_FEATURE_IOTHREADS = 0,
> VIR_DOMAIN_CAPS_FEATURE_VMCOREINFO,
> @@ -254,3 +260,7 @@ void
> virSEVCapabilitiesFree(virSEVCapability *capabilities);
>
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSEVCapability, virSEVCapabilitiesFree);
> +
> +void virTDXCapabilitiesFree(virTDXCapability *capabilities);
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virTDXCapability, virTDXCapabilitiesFree);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2efa787664..8cbb60b577 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -218,6 +218,7 @@ virDomainCapsEnumSet;
> virDomainCapsFormat;
> virDomainCapsNew;
> virSEVCapabilitiesFree;
> +virTDXCapabilitiesFree;
>
>
> # conf/domain_conf.h
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 059d6badf2..a143e453f4 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -636,6 +636,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
>/* 405 */
>"confidential-guest-support",
>"query-display-options",
> + "tdx-guest",
> );
>
>
> @@ -716,6 +717,8 @@ struct _virQEMUCaps {
>
> virSEVCapability *sevCapabilities;
>
> +virTDXCapability *tdxCapabilities;
> +
> /* Capabilities which may differ depending on the accelerator. */
> virQEMUCapsAccel kvm;
> virQEMUCapsAccel tcg;
> @@ -1354,6 +1357,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[]
> = {
> { "input-linux", QEMU_CAPS_INPUT_LINUX },
> { "virtio-gpu-gl-pci", QEMU_CAPS_VIRTIO_GPU_GL_PCI },
> { "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL },
> +{ "tdx-guest", QEMU_CAPS_TDX_GUEST},
> };
>
>
> @@ -2027,6 +2031,7 @@ void virQEMUCapsDispose(void *obj)
> g_free(qemuCaps->gicCapabilities);
>
> virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
> +virTDXCapabilitiesFree(qemuCaps->tdxCapabilities);
>
> virQEMUCapsAccelClear(>kvm);
> virQEMUCapsAccelClear(>tcg);
> @@ -3354,6 +3359,29 @@ virQEMUCapsProbeQMPSEVCapabilities(virQEMUCaps
> *qemuCaps,
> return 0;
> }
>
> +static int
> +virQEMUCapsProbeQMPTDXCapabilities(virQEMUCaps *qemuCaps,
> + qemuMonitor *mon)
> +{
> +int rc = -1;
> +virTDXCapability *caps = NULL;
> +
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDX_GUEST))
> +return 0;
> +
> +if ((rc = qemuMonitorGetTDXCapabilities(mon, )) < 0)
> +return -1;
> +
> +/* TDX isn't actually supported */
> +if (rc == 0) {
> +virQEMUCapsClear(qemuCaps, QEMU_CAPS_TDX_GUEST);
> +return 0;
> +}
> +
> +virTDXCapabilitiesFree(qemuCaps->tdxCapabilities);
> +qemuCaps->tdxCapabilities = caps;
> +return 0;
> +}
>
> /*
> * Filter for features
Re: [RFC PATCH 5/7] qemu: add support to TDVF firmware loader
On Fri, Jun 18, 2021 at 04:50:50PM +0800, Zhenzhong Duan wrote:
> TDX guest need a specific firmware TDVF to bootup, add a new element
> in TrustDomain element for that purpose, like below:
>
>
>0x0001
>/path/to/TDVF-binary
>
Looking into QEMU patches and if I understand it correctly this loader
is supposed to be used instead of UEFI or BIOS? If that's true I don't
think it should be here as we already have XML bits to specify VM
loader.
We could use something like this:
/path/to/TDVF-binary
Currently supported types are:
- `rom` which is translated to
-bios /path/to/bios.bin
- `pflash` which is translated to
-drive file=/path/to/uefi.fd,if=pflash,format=raw,...
And we could add a new type called 'generic', 'device', 'binary' or
something else which would be translated to:
-device loader,file=/path/to/TDVF-binary,...
Pavel
> Qemu command line looks like:
>
> $QEMU ... \
> -device loader,file= /path/to/TDVF-binary,id=fd0
>
> Signed-off-by: Zhenzhong Duan
> ---
> docs/schemas/domaincommon.rng | 3 +++
> src/conf/domain_conf.c | 6 ++
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_command.c | 4
> tests/genericxml2xmlindata/trust-domain-tdx.xml | 1 +
> tests/qemuxml2argvdata/trust-domain-tdx.xml | 1 +
> 6 files changed, 16 insertions(+)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 2b39a01e84..b439012648 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -530,6 +530,9 @@
>
>
>
> +
> +
> +
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a51db088c1..0513d6d016 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3515,6 +3515,7 @@ virDomainTDXDefFree(virDomainTDXDef *def)
> if (!def)
> return;
>
> +g_free(def->loader);
> g_free(def);
> }
>
> @@ -14849,6 +14850,7 @@ virDomainTDXDefParseXML(xmlNodePtr tdxNode,
> }
>
> def->policy = policy;
> +def->loader = virXPathString("string(./loader)", ctxt);
>
> return def;
>
> @@ -26950,6 +26952,10 @@ virDomainTDXDefFormat(virBuffer *buf,
> virDomainTDXDef *tdx)
> virBufferAsprintf(buf, "\n");
> virBufferAdjustIndent(buf, 2);
> virBufferAsprintf(buf, "0x%04x\n", tdx->policy);
> +
> +if (tdx->loader)
> +virBufferEscapeString(buf, "%s\n", tdx->loader);
> +
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "\n");
> }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 7cb5061c8c..cabfc80b4b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2671,6 +2671,7 @@ typedef enum {
> struct _virDomainTDXDef {
> int sectype; /* enum virDomainTrustDomain */
> unsigned int policy; /* bit 0 set hint debug enabled, other bit reserved
> */
> +char *loader; /* patch for TDX TDVF firmware */
> };
>
> typedef enum {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1e14c95a49..891d795b02 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9885,6 +9885,10 @@ qemuBuildTDXCommandLine(virDomainObj *vm, virCommand
> *cmd,
>
> virCommandAddArg(cmd, "-object");
> virCommandAddArgBuffer(cmd, );
> +
> +virCommandAddArg(cmd, "-device");
> +virCommandAddArgFormat(cmd, "loader,id=fd0,file=%s", tdx->loader);
> +
> return 0;
> }
>
> diff --git a/tests/genericxml2xmlindata/trust-domain-tdx.xml
> b/tests/genericxml2xmlindata/trust-domain-tdx.xml
> index 7a56cf0e92..7422f0c06f 100644
> --- a/tests/genericxml2xmlindata/trust-domain-tdx.xml
> +++ b/tests/genericxml2xmlindata/trust-domain-tdx.xml
> @@ -16,6 +16,7 @@
>
>
> 0x0001
> +/path/to/TDVF-binary
>
>
>
> diff --git a/tests/qemuxml2argvdata/trust-domain-tdx.xml
> b/tests/qemuxml2argvdata/trust-domain-tdx.xml
> index e0f0b77866..1d8ad45c4c 100644
> --- a/tests/qemuxml2argvdata/trust-domain-tdx.xml
> +++ b/tests/qemuxml2argvdata/trust-domain-tdx.xml
> @@ -32,5 +32,6 @@
>
>
> 0x0001
> +/path/to/TDVF-binary
>
>
> --
> 2.25.1
>
signature.asc
Description: PGP signature
Re: [RFC PATCH 3/7] conf: introduce TrustDomain element in domain
On Fri, Jun 18, 2021 at 04:50:48PM +0800, Zhenzhong Duan wrote: > The TrustDomain element can be used to define the security model to > use when launching a domain. Only type 'tdx' is supported currently. > > When 'tdx' is used, the VM will launched with Intel TDX feature enabled. > TDX feature supports running encrypted VM (Trust Domain, TD) under the > control of KVM. A TD runs in a CPU model which protects the > confidentiality of its memory and its CPU state from other software > > There is a child element 'policy' in TrustDomain. In 'policy', bit 0 > is used to enable TDX debug, other bits are reserved currently. > > For example: > > >0x0001 > Any reason why you are adding a new element that basically copies exactly what is doing? In libvirt it will essentially use `confidential-guest-support` which is used for launchSecurity so there is no need to duplicate the code and the XML element. It could look like this: 0x0001 We would have to reorganize the documentation a little bit but otherwise there is nothing blocking us to have single element to specify different type of encrypted/confidential/secure/... VMs. We already have patches for similar feature for s390 which will also reuse this element and in the future any other CPU architecture can reuse it. Pavel > Signed-off-by: Zhenzhong Duan > --- > docs/schemas/domaincommon.rng | 16 > src/conf/domain_conf.c| 84 +++ > src/conf/domain_conf.h| 15 > src/conf/virconftypes.h | 2 + > src/qemu/qemu_validate.c | 8 ++ > .../genericxml2xmlindata/trust-domain-tdx.xml | 21 + > tests/genericxml2xmltest.c| 1 + > 7 files changed, 147 insertions(+) > create mode 100644 tests/genericxml2xmlindata/trust-domain-tdx.xml > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 5ea14b6dbf..2b39a01e84 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -89,6 +89,9 @@ > > > > + > + > + > > > > @@ -518,6 +521,19 @@ > > > > + > + > + > +tdx > + > + > + > + > + > + > + > + > + > 2.25.1 > signature.asc Description: PGP signature
[libvirt PATCH 3/3] util: fix typo
Signed-off-by: Ján Tomko --- src/util/virprocess.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 01d5d01d02..5fad0db63d 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -368,7 +368,7 @@ int virProcessKill(pid_t pid, int sig) * was killed forcibly, -1 if it is still alive, * or another error occurred. * - * Callers can proide an extra delay in seconds to + * Callers can provide an extra delay in seconds to * wait longer than the default. */ int -- 2.31.1
[libvirt PATCH 0/3] wait longer for virtiofsd to exit (virtio-fs epopee)
See patch 2/3. Ján Tomko (3): Introduce virPidFileForceCleanupPathDelay qemu: wait more for virtiofsd to exit util: fix typo src/libvirt_private.syms | 1 + src/qemu/qemu_virtiofs.c | 2 +- src/util/virpidfile.c| 16 +++- src/util/virpidfile.h| 2 ++ src/util/virprocess.c| 2 +- 5 files changed, 20 insertions(+), 3 deletions(-) -- 2.31.1
[libvirt PATCH 1/3] Introduce virPidFileForceCleanupPathDelay
Add a version of virPidFileForceCleanupPath with an extradelay
parameter for processes where the default timeout is not enough.
Signed-off-by: Ján Tomko
---
src/libvirt_private.syms | 1 +
src/util/virpidfile.c| 16 +++-
src/util/virpidfile.h| 2 ++
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2efa787664..dc25dd6ac0 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3027,6 +3027,7 @@ virPidFileConstructPath;
virPidFileDelete;
virPidFileDeletePath;
virPidFileForceCleanupPath;
+virPidFileForceCleanupPathDelay;
virPidFileRead;
virPidFileReadIfAlive;
virPidFileReadPath;
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index c6389c1869..6c3d869460 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -511,10 +511,14 @@ virPidFileConstructPath(bool privileged,
* called multiple times with the same path, be it in threads or
* processes. This function does not raise any errors.
*
+ * If @extradelay (in seconds) is specified, we wait for
+ * 15 + extradelay seconds more.
+ *
* Returns 0 if the pidfile was successfully cleaned up, -1 otherwise.
*/
int
-virPidFileForceCleanupPath(const char *path)
+virPidFileForceCleanupPathDelay(const char *path,
+unsigned int extradelay)
{
pid_t pid = 0;
int fd = -1;
@@ -532,6 +536,9 @@ virPidFileForceCleanupPath(const char *path)
/* Only kill the process if the pid is valid one. 0 means
* there is somebody else doing the same pidfile cleanup
* machinery. */
+if (extradelay &&
+virProcessKillPainfullyDelay(pid, false, extradelay) >= 0)
+pid = 0;
if (pid)
virProcessKillPainfully(pid, true);
@@ -544,3 +551,10 @@ virPidFileForceCleanupPath(const char *path)
return 0;
}
+
+
+int
+virPidFileForceCleanupPath(const char *path)
+{
+return virPidFileForceCleanupPathDelay(path, 0);
+}
diff --git a/src/util/virpidfile.h b/src/util/virpidfile.h
index 370a59892e..ef26377375 100644
--- a/src/util/virpidfile.h
+++ b/src/util/virpidfile.h
@@ -73,4 +73,6 @@ int virPidFileConstructPath(bool privileged,
const char *progname,
char **pidfile);
+int virPidFileForceCleanupPathDelay(const char *path,
+unsigned int extradelay)
ATTRIBUTE_NONNULL(1);
int virPidFileForceCleanupPath(const char *path) ATTRIBUTE_NONNULL(1);
--
2.31.1
[libvirt PATCH 2/3] qemu: wait more for virtiofsd to exit
In some cases, such as doing intense I/O on slow filesystems,
it can take virtiofsd as long as 42 seconds to exit.
Add a delay of extra 45 seconds before we forcefully kill it.
https://bugzilla.redhat.com/show_bug.cgi?id=1940276
Signed-off-by: Ján Tomko
---
src/qemu/qemu_virtiofs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index edaedf0304..3f99f0caa4 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -281,7 +281,7 @@ qemuVirtioFSStop(virQEMUDriver *driver G_GNUC_UNUSED,
if (!(pidfile = qemuVirtioFSCreatePidFilename(vm, fs->info.alias)))
goto cleanup;
-if (virPidFileForceCleanupPath(pidfile) < 0) {
+if (virPidFileForceCleanupPathDelay(pidfile, 30) < 0) {
VIR_WARN("Unable to kill virtiofsd process");
} else {
if (QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock)
--
2.31.1
Re: [RFC PATCH 6/7] qemu: force special features enabled for TDX guest
On Fri, Jun 18, 2021 at 16:50:51 +0800, Zhenzhong Duan wrote:
> TDX guest requires some special parameters in qemu command line.
> They are "pic=no,kernel_irqchip=split" without which guest fails to
> bootup.
>
> PMU has a big impact to the performance of TDX guest. So always
> disable PMU except it's forcely enabled.
>
> Signed-off-by: Zhenzhong Duan
> ---
> src/qemu/qemu_command.c | 6 +-
> src/qemu/qemu_validate.c | 6 ++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 891d795b02..bffa3fdf10 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6599,6 +6599,10 @@ qemuBuildCpuCommandLine(virCommand *cmd,
> virTristateSwitch pmu = def->features[VIR_DOMAIN_FEATURE_PMU];
> virBufferAsprintf(, ",pmu=%s",
>virTristateSwitchTypeToString(pmu));
> +} else if (!def->features[VIR_DOMAIN_FEATURE_PMU] && def->tdx) {
> +/* PMU lead to performance drop if TDX enabled, disable PMU by
> default */
> +virBufferAsprintf(, ",pmu=%s",
> +
> virTristateSwitchTypeToString(VIR_TRISTATE_SWITCH_OFF));
> }
This must be done in a way that will be visible in the XML rather than
silently hiding it in the command line generator code.
Also it feels very unjustified, it's bad for performance, but is it
guaranteed to always be like this?
>
> if (def->cpu && def->cpu->cache) {
[...]
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 2efd011cc0..3c3a00c7e8 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -202,6 +202,12 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
> return -1;
> }
> }
> +if (def->tdx && (!virQEMUCapsGet(qemuCaps,
> QEMU_CAPS_MACHINE_KERNEL_IRQCHIP)
> + || def->features[i] != VIR_DOMAIN_IOAPIC_QEMU))
> {
Our coding style usually has boolean operators at the end of the
previous line in multi-line statements.
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("TDX guest needs split kernel irqchip"));
> +return -1;
> +}
> break;
>
> case VIR_DOMAIN_FEATURE_HPT:
> --
> 2.25.1
>
Re: [RFC PATCH 5/7] qemu: add support to TDVF firmware loader
On Fri, Jun 18, 2021 at 16:50:50 +0800, Zhenzhong Duan wrote:
> TDX guest need a specific firmware TDVF to bootup, add a new element
> in TrustDomain element for that purpose, like below:
>
>
>0x0001
>/path/to/TDVF-binary
>
>
> Qemu command line looks like:
>
> $QEMU ... \
> -device loader,file= /path/to/TDVF-binary,id=fd0
>
> Signed-off-by: Zhenzhong Duan
> ---
> docs/schemas/domaincommon.rng | 3 +++
> src/conf/domain_conf.c | 6 ++
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_command.c | 4
> tests/genericxml2xmlindata/trust-domain-tdx.xml | 1 +
> tests/qemuxml2argvdata/trust-domain-tdx.xml | 1 +
> 6 files changed, 16 insertions(+)
[...]
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 7cb5061c8c..cabfc80b4b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2671,6 +2671,7 @@ typedef enum {
> struct _virDomainTDXDef {
> int sectype; /* enum virDomainTrustDomain */
> unsigned int policy; /* bit 0 set hint debug enabled, other bit reserved
> */
> +char *loader; /* patch for TDX TDVF firmware */
> };
>
> typedef enum {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1e14c95a49..891d795b02 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9885,6 +9885,10 @@ qemuBuildTDXCommandLine(virDomainObj *vm, virCommand
> *cmd,
>
> virCommandAddArg(cmd, "-object");
> virCommandAddArgBuffer(cmd, );
> +
> +virCommandAddArg(cmd, "-device");
> +virCommandAddArgFormat(cmd, "loader,id=fd0,file=%s", tdx->loader);
> +
> return 0;
> }
>
This again mixes config changes with qemu-specific changes.
Re: [RFC PATCH 3/7] conf: introduce TrustDomain element in domain
On Fri, Jun 18, 2021 at 16:50:48 +0800, Zhenzhong Duan wrote: > The TrustDomain element can be used to define the security model to > use when launching a domain. Only type 'tdx' is supported currently. > > When 'tdx' is used, the VM will launched with Intel TDX feature enabled. > TDX feature supports running encrypted VM (Trust Domain, TD) under the > control of KVM. A TD runs in a CPU model which protects the > confidentiality of its memory and its CPU state from other software > > There is a child element 'policy' in TrustDomain. In 'policy', bit 0 > is used to enable TDX debug, other bits are reserved currently. > > For example: > > >0x0001 > > > Signed-off-by: Zhenzhong Duan > --- > docs/schemas/domaincommon.rng | 16 > src/conf/domain_conf.c| 84 +++ > src/conf/domain_conf.h| 15 > src/conf/virconftypes.h | 2 + > src/qemu/qemu_validate.c | 8 ++ > .../genericxml2xmlindata/trust-domain-tdx.xml | 21 + > tests/genericxml2xmltest.c| 1 + > 7 files changed, 147 insertions(+) > create mode 100644 tests/genericxml2xmlindata/trust-domain-tdx.xml > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 5ea14b6dbf..2b39a01e84 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -89,6 +89,9 @@ > > > > + > + All our RNG schema entries ... > + > > > > @@ -518,6 +521,19 @@ > > > > + > + > + > +tdx > + > + > + > + > + > + > + > + > + >
Re: [RFC PATCH 0/7] LIBVIRT: X86: TDX support
On Fri, Jun 18, 2021 at 16:50:45 +0800, Zhenzhong Duan wrote: > * What's TDX? > TDX stands for Trust Domain Extensions which isolates VMs from > the virtual-machine manager (VMM)/hypervisor and any other software on > the platform. > > To support TDX, multiple software components, not only KVM but also QEMU, > guest Linux and virtual bios, need to be updated. For more details, please > check link[1], there are TDX spec links and public repository link at github > for each software component. > > This patchset is another software component to extend libvirt to support TDX, > with which one can start a VM from high level rather than running qemu > directly. > > > * The goal of this RFC patch > The purpose of this post is to get feedback early on high level design issue > of > libvirt enhancement for TDX. Referenced much on AMD SEV implemention at > link[2]. > > > * Patch organization > > - patch 1-2: Support query of TDX capabilities. > - patch 3-6: Add a new xml element 'TrustDomain' for TDX support. > - patch 7: Sure kvmSupportsSecureGuest cache updated. > > Using these patches we have succesfully booted and tested a guest both with > and > without TDX enabled. > > > [1] https://lkml.org/lkml/2020/11/16/1106 > [2] https://github.com/codomania/libvirt/commits/v9 Could you please also point to the relevant qemu patches? The first commit mentions 'query-tdx-capabilities' which is not in qemu upstream yet.
[PATCH libvirt v1] conf: verify for duplicate hostdevs
It is possible to define/edit(in shut off state) a domain XML with
same hostdev device repeated more than once, as shown below. This
behavior is not expected. So, this patch fixes it.
vser1:
[...]
[...]
[...]
$ virsh define vser1
Domain 'vser1' defined from vser1
Signed-off-by: Shalini Chellathurai Saroja
Reviewed-by: Bjoern Walk
Reviewed-by: Boris Fiuczynski
---
src/conf/domain_conf.c| 2 +-
src/conf/domain_conf.h| 2 +
src/conf/domain_validate.c| 21 ++
src/libvirt_private.syms | 1 +
.../hostdev-mdev-duplicate.err| 1 +
.../hostdev-mdev-duplicate.xml| 41 +++
.../hostdev-pci-duplicate.err | 1 +
.../hostdev-pci-duplicate.xml | 40 ++
.../hostdev-scsi-duplicate.err| 1 +
.../hostdev-scsi-duplicate.xml| 40 ++
.../hostdev-usb-duplicate.err | 1 +
.../hostdev-usb-duplicate.xml | 40 ++
tests/qemuxml2argvtest.c | 8
13 files changed, 198 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-duplicate.err
create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-duplicate.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-pci-duplicate.err
create mode 100644 tests/qemuxml2argvdata/hostdev-pci-duplicate.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-scsi-duplicate.err
create mode 100644 tests/qemuxml2argvdata/hostdev-scsi-duplicate.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-usb-duplicate.err
create mode 100644 tests/qemuxml2argvdata/hostdev-usb-duplicate.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f65509d8..5746f69e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15369,7 +15369,7 @@ virDomainHostdevMatchCaps(virDomainHostdevDef *a,
}
-static int
+int
virDomainHostdevMatch(virDomainHostdevDef *a,
virDomainHostdevDef *b)
{
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f706c498..4d9d499b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3590,6 +3590,8 @@ virDomainHostdevDef *
virDomainHostdevRemove(virDomainDef *def, size_t i);
int virDomainHostdevFind(virDomainDef *def, virDomainHostdevDef *match,
virDomainHostdevDef **found);
+int virDomainHostdevMatch(virDomainHostdevDef *a,
+ virDomainHostdevDef *b);
virDomainGraphicsListenDef *
virDomainGraphicsGetListen(virDomainGraphicsDef *def, size_t i);
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 2124d25d..df2ab473 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -1068,7 +1068,25 @@ virDomainDefDuplicateDiskInfoValidate(const virDomainDef
*def)
return 0;
}
+static int
+virDomainDefDuplicateHostdevInfoValidate(const virDomainDef *def)
+{
+size_t i;
+size_t j;
+for (i = 0; i < def->nhostdevs; i++) {
+for (j = i + 1; j < def->nhostdevs; j++) {
+if (virDomainHostdevMatch(def->hostdevs[i],
+ def->hostdevs[j])) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+_("Hostdev already exists in the domain configuration"));
+return -1;
+}
+}
+}
+
+return 0;
+}
/**
* virDomainDefDuplicateDriveAddressesValidate:
@@ -1529,6 +1547,9 @@ virDomainDefValidateInternal(const virDomainDef *def,
if (virDomainDefDuplicateDiskInfoValidate(def) < 0)
return -1;
+if (virDomainDefDuplicateHostdevInfoValidate(def) < 0)
+return -1;
+
if (virDomainDefDuplicateDriveAddressesValidate(def) < 0)
return -1;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2efa7876..f0d1b7b4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -457,6 +457,7 @@ virDomainHostdevDefFree;
virDomainHostdevDefNew;
virDomainHostdevFind;
virDomainHostdevInsert;
+virDomainHostdevMatch;
virDomainHostdevModeTypeToString;
virDomainHostdevRemove;
virDomainHostdevSubsysPCIBackendTypeToString;
diff --git a/tests/qemuxml2argvdata/hostdev-mdev-duplicate.err
b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.err
new file mode 100644
index ..590afd30
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.err
@@ -0,0 +1 @@
+XML error: Hostdev already exists in the domain configuration
diff --git a/tests/qemuxml2argvdata/hostdev-mdev-duplicate.xml
b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.xml
new file mode 100644
index ..1c5e3263
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-mdev-duplicate.xml
@@ -0,0 +1,41 @@
+
+ QEMUGuest2
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+
[RFC PATCH 6/7] qemu: force special features enabled for TDX guest
TDX guest requires some special parameters in qemu command line.
They are "pic=no,kernel_irqchip=split" without which guest fails to
bootup.
PMU has a big impact to the performance of TDX guest. So always
disable PMU except it's forcely enabled.
Signed-off-by: Zhenzhong Duan
---
src/qemu/qemu_command.c | 6 +-
src/qemu/qemu_validate.c | 6 ++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 891d795b02..bffa3fdf10 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6599,6 +6599,10 @@ qemuBuildCpuCommandLine(virCommand *cmd,
virTristateSwitch pmu = def->features[VIR_DOMAIN_FEATURE_PMU];
virBufferAsprintf(, ",pmu=%s",
virTristateSwitchTypeToString(pmu));
+} else if (!def->features[VIR_DOMAIN_FEATURE_PMU] && def->tdx) {
+/* PMU lead to performance drop if TDX enabled, disable PMU by default
*/
+virBufferAsprintf(, ",pmu=%s",
+
virTristateSwitchTypeToString(VIR_TRISTATE_SWITCH_OFF));
}
if (def->cpu && def->cpu->cache) {
@@ -6975,7 +6979,7 @@ qemuBuildMachineCommandLine(virCommand *cmd,
}
if (def->tdx)
-virBufferAddLit(, ",confidential-guest-support=tdx0,kvm-type=tdx");
+virBufferAddLit(,
",confidential-guest-support=tdx0,kvm-type=tdx,pic=no");
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
if (priv->pflash0)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 2efd011cc0..3c3a00c7e8 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -202,6 +202,12 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
return -1;
}
}
+if (def->tdx && (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_MACHINE_KERNEL_IRQCHIP)
+ || def->features[i] != VIR_DOMAIN_IOAPIC_QEMU)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("TDX guest needs split kernel irqchip"));
+return -1;
+}
break;
case VIR_DOMAIN_FEATURE_HPT:
--
2.25.1
[RFC PATCH 5/7] qemu: add support to TDVF firmware loader
TDX guest need a specific firmware TDVF to bootup, add a new element
in TrustDomain element for that purpose, like below:
0x0001
/path/to/TDVF-binary
Qemu command line looks like:
$QEMU ... \
-device loader,file= /path/to/TDVF-binary,id=fd0
Signed-off-by: Zhenzhong Duan
---
docs/schemas/domaincommon.rng | 3 +++
src/conf/domain_conf.c | 6 ++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_command.c | 4
tests/genericxml2xmlindata/trust-domain-tdx.xml | 1 +
tests/qemuxml2argvdata/trust-domain-tdx.xml | 1 +
6 files changed, 16 insertions(+)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 2b39a01e84..b439012648 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -530,6 +530,9 @@
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a51db088c1..0513d6d016 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3515,6 +3515,7 @@ virDomainTDXDefFree(virDomainTDXDef *def)
if (!def)
return;
+g_free(def->loader);
g_free(def);
}
@@ -14849,6 +14850,7 @@ virDomainTDXDefParseXML(xmlNodePtr tdxNode,
}
def->policy = policy;
+def->loader = virXPathString("string(./loader)", ctxt);
return def;
@@ -26950,6 +26952,10 @@ virDomainTDXDefFormat(virBuffer *buf, virDomainTDXDef
*tdx)
virBufferAsprintf(buf, "\n");
virBufferAdjustIndent(buf, 2);
virBufferAsprintf(buf, "0x%04x\n", tdx->policy);
+
+if (tdx->loader)
+virBufferEscapeString(buf, "%s\n", tdx->loader);
+
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "\n");
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 7cb5061c8c..cabfc80b4b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2671,6 +2671,7 @@ typedef enum {
struct _virDomainTDXDef {
int sectype; /* enum virDomainTrustDomain */
unsigned int policy; /* bit 0 set hint debug enabled, other bit reserved */
+char *loader; /* patch for TDX TDVF firmware */
};
typedef enum {
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1e14c95a49..891d795b02 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9885,6 +9885,10 @@ qemuBuildTDXCommandLine(virDomainObj *vm, virCommand
*cmd,
virCommandAddArg(cmd, "-object");
virCommandAddArgBuffer(cmd, );
+
+virCommandAddArg(cmd, "-device");
+virCommandAddArgFormat(cmd, "loader,id=fd0,file=%s", tdx->loader);
+
return 0;
}
diff --git a/tests/genericxml2xmlindata/trust-domain-tdx.xml
b/tests/genericxml2xmlindata/trust-domain-tdx.xml
index 7a56cf0e92..7422f0c06f 100644
--- a/tests/genericxml2xmlindata/trust-domain-tdx.xml
+++ b/tests/genericxml2xmlindata/trust-domain-tdx.xml
@@ -16,6 +16,7 @@
0x0001
+/path/to/TDVF-binary
diff --git a/tests/qemuxml2argvdata/trust-domain-tdx.xml
b/tests/qemuxml2argvdata/trust-domain-tdx.xml
index e0f0b77866..1d8ad45c4c 100644
--- a/tests/qemuxml2argvdata/trust-domain-tdx.xml
+++ b/tests/qemuxml2argvdata/trust-domain-tdx.xml
@@ -32,5 +32,6 @@
0x0001
+/path/to/TDVF-binary
--
2.25.1
[RFC PATCH 3/7] conf: introduce TrustDomain element in domain
The TrustDomain element can be used to define the security model to use when launching a domain. Only type 'tdx' is supported currently. When 'tdx' is used, the VM will launched with Intel TDX feature enabled. TDX feature supports running encrypted VM (Trust Domain, TD) under the control of KVM. A TD runs in a CPU model which protects the confidentiality of its memory and its CPU state from other software There is a child element 'policy' in TrustDomain. In 'policy', bit 0 is used to enable TDX debug, other bits are reserved currently. For example: 0x0001 Signed-off-by: Zhenzhong Duan --- docs/schemas/domaincommon.rng | 16 src/conf/domain_conf.c| 84 +++ src/conf/domain_conf.h| 15 src/conf/virconftypes.h | 2 + src/qemu/qemu_validate.c | 8 ++ .../genericxml2xmlindata/trust-domain-tdx.xml | 21 + tests/genericxml2xmltest.c| 1 + 7 files changed, 147 insertions(+) create mode 100644 tests/genericxml2xmlindata/trust-domain-tdx.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 5ea14b6dbf..2b39a01e84 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -89,6 +89,9 @@ + + + @@ -518,6 +521,19 @@ + + + +tdx + + + + + + + + +
[RFC PATCH 4/7] qemu: add support to launch TDX guest
QEMU will provides 'tdx-guest' object which is used to launch encrypted
VMs on Intel platform using TDX feature. The tag can be
used to launch a TDX guest. A typical TDX guest launch command line
looks like:
$QEMU ... \
-object tdx-guest,id=tdx0,debug=on \
-machine q35,
confidential-guest-support=tdx0,kvm-type=tdx,pic=no,kernel_irqchip=split
Signed-off-by: Zhenzhong Duan
---
src/qemu/qemu_command.c | 31 +++
.../.trust-domain-tdx.xml.swo | Bin 0 -> 12288 bytes
tests/qemuxml2argvdata/trust-domain-tdx.args | 32
tests/qemuxml2argvdata/trust-domain-tdx.xml | 36 ++
tests/qemuxml2argvtest.c | 3 ++
5 files changed, 102 insertions(+)
create mode 100644 tests/qemuxml2argvdata/.trust-domain-tdx.xml.swo
create mode 100644 tests/qemuxml2argvdata/trust-domain-tdx.args
create mode 100644 tests/qemuxml2argvdata/trust-domain-tdx.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ea513693f7..1e14c95a49 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6974,6 +6974,9 @@ qemuBuildMachineCommandLine(virCommand *cmd,
}
}
+if (def->tdx)
+virBufferAddLit(, ",confidential-guest-support=tdx0,kvm-type=tdx");
+
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
if (priv->pflash0)
virBufferAsprintf(, ",pflash0=%s", priv->pflash0->nodeformat);
@@ -9860,6 +9863,31 @@ qemuBuildSEVCommandLine(virDomainObj *vm, virCommand
*cmd,
return 0;
}
+static int
+qemuBuildTDXCommandLine(virDomainObj *vm, virCommand *cmd,
+virDomainTDXDef *tdx)
+{
+g_autoptr(virJSONValue) props = NULL;
+g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+qemuDomainObjPrivate *priv = vm->privateData;
+
+if (!tdx)
+return 0;
+
+VIR_DEBUG("policy=0x%x", tdx->policy);
+
+if (qemuMonitorCreateObjectProps(, "tdx-guest", "tdx0",
+ "B:debug", !!(tdx->policy & 1)) < 0)
+return -1;
+
+if (qemuBuildObjectCommandlineFromJSON(, props, priv->qemuCaps) < 0)
+return -1;
+
+virCommandAddArg(cmd, "-object");
+virCommandAddArgBuffer(cmd, );
+return 0;
+}
+
static int
qemuBuildVMCoreInfoCommandLine(virCommand *cmd,
const virDomainDef *def)
@@ -10562,6 +10590,9 @@ qemuBuildCommandLine(virQEMUDriver *driver,
if (qemuBuildSEVCommandLine(vm, cmd, def->sev) < 0)
return NULL;
+if (qemuBuildTDXCommandLine(vm, cmd, def->tdx) < 0)
+return NULL;
+
if (snapshot)
virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
diff --git a/tests/qemuxml2argvdata/.trust-domain-tdx.xml.swo
b/tests/qemuxml2argvdata/.trust-domain-tdx.xml.swo
new file mode 100644
index
..2835d4a14342651d239657fb1531318de3eff8e8
GIT binary patch
literal 12288
zcmeI2zmMER6vrpfAcgR=Ok<8>D?f+~!X5xRasc7k;p+lmDH)HSK<#Gt6jChv5^7+kop5J_CeA4M^x$ZO7?ei
z0@oXa+$f)X`sOEB$#-AALaZ@9g#YW3J#SM{Yt{Pn!_MoEYPCvlW&7EMiA=3JV?C>N
zwba(1^d0lcdB;9eO}}dD3AYcIl6%g)?VV4hX}E4vFIT%TdLbD;A^{}uoCL0s8<6RD^zpK|g>(P2YgEQVRL6RjKuwh+F)|cy|E?hon@77W8Zb^*3PRYUF}a#rZ)?=
zb%pbD?u-MJb^~Yt{wZMuHU{tg2wcN+1asd>kJ`qoFg8lr{7DQz^E=U=pGH
z1S~Ef?!6e+2)Q-~HN0ZOcMG64Tq;;t$NhqN!|vZcKHQzlf{g=^8j=Ta3AbV~qr+<9
zuyL`_Er9Aon(t<`Tr8V{1!Me3hPM_=(e+7JbD
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 219100
+ 219100
+ 1
+
+hvm
+
+
+
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-x86_64
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+0x0001
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 7fed871c9e..ac5dc6b40b 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -3456,6 +3456,9 @@ mymain(void)
DO_TEST_CAPS_VER("launch-security-sev", "2.12.0");
DO_TEST_CAPS_VER("launch-security-sev", "6.0.0");
DO_TEST_CAPS_VER("launch-security-sev-missing-platform-info", "2.12.0");
+DO_TEST("trust-domain-tdx",
+QEMU_CAPS_KVM,
+QEMU_CAPS_TDX_GUEST);
DO_TEST_CAPS_LATEST("vhost-user-fs-fd-memory");
DO_TEST_CAPS_LATEST("vhost-user-fs-hugepages");
--
2.25.1
[RFC PATCH 1/7] qemu: provide support to query the TDX capabilities
QEMU provides support for launching an encrypted VMs on Intel x86
platform using Trust Domain Extension (TDX) feature. This patch adds
support to query the TDX capabilities from the QEMU.
Currently there is no elements in TDX capabilities except a placeholder.
Signed-off-by: Chenyi Qiang
Signed-off-by: Zhenzhong Duan
---
src/conf/domain_capabilities.c | 8 +
src/conf/domain_capabilities.h | 10 +++
src/libvirt_private.syms | 1 +
src/qemu/qemu_capabilities.c | 30 +++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_monitor.c| 8 +
src/qemu/qemu_monitor.h| 3 ++
src/qemu/qemu_monitor_json.c | 53 ++
src/qemu/qemu_monitor_json.h | 3 ++
9 files changed, 117 insertions(+)
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index cb90ae0176..31577095e9 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -76,6 +76,14 @@ virSEVCapabilitiesFree(virSEVCapability *cap)
g_free(cap);
}
+void
+virTDXCapabilitiesFree(virTDXCapability *cap)
+{
+if (!cap)
+return;
+
+VIR_FREE(cap);
+}
static void
virDomainCapsDispose(void *obj)
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index b6433b20c9..e099788da9 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -173,6 +173,12 @@ struct _virSEVCapability {
unsigned int reduced_phys_bits;
};
+typedef struct _virTDXCapability virTDXCapability;
+struct _virTDXCapability {
+/* no elements for Intel TDX for now, just put a placeholder */
+uint64_t placeholder;
+};
+
typedef enum {
VIR_DOMAIN_CAPS_FEATURE_IOTHREADS = 0,
VIR_DOMAIN_CAPS_FEATURE_VMCOREINFO,
@@ -254,3 +260,7 @@ void
virSEVCapabilitiesFree(virSEVCapability *capabilities);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSEVCapability, virSEVCapabilitiesFree);
+
+void virTDXCapabilitiesFree(virTDXCapability *capabilities);
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virTDXCapability, virTDXCapabilitiesFree);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2efa787664..8cbb60b577 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -218,6 +218,7 @@ virDomainCapsEnumSet;
virDomainCapsFormat;
virDomainCapsNew;
virSEVCapabilitiesFree;
+virTDXCapabilitiesFree;
# conf/domain_conf.h
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 059d6badf2..a143e453f4 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -636,6 +636,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 405 */
"confidential-guest-support",
"query-display-options",
+ "tdx-guest",
);
@@ -716,6 +717,8 @@ struct _virQEMUCaps {
virSEVCapability *sevCapabilities;
+virTDXCapability *tdxCapabilities;
+
/* Capabilities which may differ depending on the accelerator. */
virQEMUCapsAccel kvm;
virQEMUCapsAccel tcg;
@@ -1354,6 +1357,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
{ "input-linux", QEMU_CAPS_INPUT_LINUX },
{ "virtio-gpu-gl-pci", QEMU_CAPS_VIRTIO_GPU_GL_PCI },
{ "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL },
+{ "tdx-guest", QEMU_CAPS_TDX_GUEST},
};
@@ -2027,6 +2031,7 @@ void virQEMUCapsDispose(void *obj)
g_free(qemuCaps->gicCapabilities);
virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
+virTDXCapabilitiesFree(qemuCaps->tdxCapabilities);
virQEMUCapsAccelClear(>kvm);
virQEMUCapsAccelClear(>tcg);
@@ -3354,6 +3359,29 @@ virQEMUCapsProbeQMPSEVCapabilities(virQEMUCaps *qemuCaps,
return 0;
}
+static int
+virQEMUCapsProbeQMPTDXCapabilities(virQEMUCaps *qemuCaps,
+ qemuMonitor *mon)
+{
+int rc = -1;
+virTDXCapability *caps = NULL;
+
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TDX_GUEST))
+return 0;
+
+if ((rc = qemuMonitorGetTDXCapabilities(mon, )) < 0)
+return -1;
+
+/* TDX isn't actually supported */
+if (rc == 0) {
+virQEMUCapsClear(qemuCaps, QEMU_CAPS_TDX_GUEST);
+return 0;
+}
+
+virTDXCapabilitiesFree(qemuCaps->tdxCapabilities);
+qemuCaps->tdxCapabilities = caps;
+return 0;
+}
/*
* Filter for features which should never be passed to QEMU. Either because
@@ -5316,6 +5344,8 @@ virQEMUCapsInitQMPMonitor(virQEMUCaps *qemuCaps,
return -1;
if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
return -1;
+if (virQEMUCapsProbeQMPTDXCapabilities(qemuCaps, mon) < 0)
+return -1;
virQEMUCapsInitProcessCaps(qemuCaps);
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index b2878312ac..a51bd9a256 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -616,6 +616,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
/* 405 */
[RFC PATCH 7/7] qemu: Check if INTEL Trust Domain Extention support is enabled
Implement trust domain check for INTEL TDX (Trust Domain eXtention)
in order to invalidate the qemu capabilities cache in case the
availability of the feature changed.
For INTEL TDX the verification is:
- checking if /sys/module/kvm_intel/parameters/tdx contains the
value 'Y': meaning TDX is enabled in the host kernel.
Signed-off-by: Zhenzhong Duan
---
src/qemu/qemu_capabilities.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 5e54d7e306..8f8802c121 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4767,6 +4767,24 @@ virQEMUCapsKVMSupportsSecureGuestAMD(void)
}
+/*
+ * Check whether INTEL Trust Domain Extention (x86) is enabled
+ */
+static bool
+virQEMUCapsKVMSupportsSecureGuestINTEL(void)
+{
+g_autofree char *modValue = NULL;
+
+if (virFileReadValueString(,
"/sys/module/kvm_intel/parameters/tdx") < 0)
+return false;
+
+if (modValue[0] != 'Y')
+return false;
+
+return true;
+}
+
+
/*
* Check whether the secure guest functionality is enabled.
* See the specific architecture function for details on the verifications
made.
@@ -4782,6 +4800,9 @@ virQEMUCapsKVMSupportsSecureGuest(void)
if (ARCH_IS_X86(arch))
return virQEMUCapsKVMSupportsSecureGuestAMD();
+if (ARCH_IS_X86(arch))
+return virQEMUCapsKVMSupportsSecureGuestINTEL();
+
return false;
}
--
2.25.1
[RFC PATCH 2/7] conf: expose TDX feature in domain capabilities
Extend hypervisor capabilities to include tdx feature. When available, hypervisor can launch an encrypted VM on Intel platform. Signed-off-by: Chenyi Qiang Signed-off-by: Zhenzhong Duan --- docs/formatdomaincaps.html.in| 16 docs/schemas/domaincaps.rng | 9 + src/conf/domain_capabilities.c | 14 ++ src/conf/domain_capabilities.h | 1 + src/qemu/qemu_capabilities.c | 12 tests/domaincapsdata/bhyve_basic.x86_64.xml | 1 + tests/domaincapsdata/bhyve_fbuf.x86_64.xml | 1 + tests/domaincapsdata/bhyve_uefi.x86_64.xml | 1 + tests/domaincapsdata/empty.xml | 1 + tests/domaincapsdata/libxl-xenfv.xml | 1 + tests/domaincapsdata/libxl-xenpv.xml | 1 + tests/domaincapsdata/qemu_2.11.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.11.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.11.0.s390x.xml | 1 + tests/domaincapsdata/qemu_2.11.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.12.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.12.0-tcg.x86_64.xml | 1 + .../domaincapsdata/qemu_2.12.0-virt.aarch64.xml | 1 + tests/domaincapsdata/qemu_2.12.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_2.12.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_2.12.0.s390x.xml | 1 + tests/domaincapsdata/qemu_2.12.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.4.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.4.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.4.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.5.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.5.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.5.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.6.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.6.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.6.0-virt.aarch64.xml | 1 + tests/domaincapsdata/qemu_2.6.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_2.6.0.ppc64.xml| 1 + tests/domaincapsdata/qemu_2.6.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.7.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.7.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.7.0.s390x.xml| 1 + tests/domaincapsdata/qemu_2.7.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.8.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.8.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.8.0.s390x.xml| 1 + tests/domaincapsdata/qemu_2.8.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.9.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.9.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.9.0.ppc64.xml| 1 + tests/domaincapsdata/qemu_2.9.0.s390x.xml| 1 + tests/domaincapsdata/qemu_2.9.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_3.0.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_3.0.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_3.0.0.ppc64.xml| 1 + tests/domaincapsdata/qemu_3.0.0.s390x.xml| 1 + tests/domaincapsdata/qemu_3.0.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_3.1.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_3.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_3.1.0.ppc64.xml| 1 + tests/domaincapsdata/qemu_3.1.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_4.0.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_4.0.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_4.0.0-virt.aarch64.xml | 1 + tests/domaincapsdata/qemu_4.0.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_4.0.0.ppc64.xml| 1 + tests/domaincapsdata/qemu_4.0.0.s390x.xml| 1 + tests/domaincapsdata/qemu_4.0.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_4.1.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_4.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_4.1.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_4.2.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_4.2.0-virt.aarch64.xml | 1 + tests/domaincapsdata/qemu_4.2.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_4.2.0.ppc64.xml| 1 + tests/domaincapsdata/qemu_4.2.0.s390x.xml| 1 + tests/domaincapsdata/qemu_4.2.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_5.0.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_5.0.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_5.0.0-virt.aarch64.xml | 1 + tests/domaincapsdata/qemu_5.0.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_5.0.0.ppc64.xml| 1 + tests/domaincapsdata/qemu_5.0.0.x86_64.xml | 1 + tests/domaincapsdata/qemu_5.1.0-q35.x86_64.xml | 1 + tests/domaincapsdata/qemu_5.1.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_5.1.0.sparc.xml| 1 + tests/domaincapsdata/qemu_5.1.0.x86_64.xml | 1 +
[RFC PATCH 0/7] LIBVIRT: X86: TDX support
* What's TDX? TDX stands for Trust Domain Extensions which isolates VMs from the virtual-machine manager (VMM)/hypervisor and any other software on the platform. To support TDX, multiple software components, not only KVM but also QEMU, guest Linux and virtual bios, need to be updated. For more details, please check link[1], there are TDX spec links and public repository link at github for each software component. This patchset is another software component to extend libvirt to support TDX, with which one can start a VM from high level rather than running qemu directly. * The goal of this RFC patch The purpose of this post is to get feedback early on high level design issue of libvirt enhancement for TDX. Referenced much on AMD SEV implemention at link[2]. * Patch organization - patch 1-2: Support query of TDX capabilities. - patch 3-6: Add a new xml element 'TrustDomain' for TDX support. - patch 7: Sure kvmSupportsSecureGuest cache updated. Using these patches we have succesfully booted and tested a guest both with and without TDX enabled. [1] https://lkml.org/lkml/2020/11/16/1106 [2] https://github.com/codomania/libvirt/commits/v9 Zhenzhong Duan (7): qemu: provide support to query the TDX capabilities conf: expose TDX feature in domain capabilities conf: introduce TrustDomain element in domain qemu: add support to launch TDX guest qemu: add support to TDVF firmware loader qemu: force special features enabled for TDX guest qemu: Check if INTEL Trust Domain Extention support is enabled docs/formatdomaincaps.html.in | 16 docs/schemas/domaincaps.rng | 9 ++ docs/schemas/domaincommon.rng | 19 src/conf/domain_capabilities.c| 22 + src/conf/domain_capabilities.h| 11 +++ src/conf/domain_conf.c| 90 ++ src/conf/domain_conf.h| 16 src/conf/virconftypes.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 63 src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 39 src/qemu/qemu_monitor.c | 8 ++ src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 53 +++ src/qemu/qemu_monitor_json.h | 3 + src/qemu/qemu_validate.c | 14 +++ tests/domaincapsdata/bhyve_basic.x86_64.xml | 1 + tests/domaincapsdata/bhyve_fbuf.x86_64.xml| 1 + tests/domaincapsdata/bhyve_uefi.x86_64.xml| 1 + tests/domaincapsdata/empty.xml| 1 + tests/domaincapsdata/libxl-xenfv.xml | 1 + tests/domaincapsdata/libxl-xenpv.xml | 1 + .../domaincapsdata/qemu_2.11.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_2.11.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.11.0.s390x.xml| 1 + tests/domaincapsdata/qemu_2.11.0.x86_64.xml | 1 + .../domaincapsdata/qemu_2.12.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_2.12.0-tcg.x86_64.xml | 1 + .../qemu_2.12.0-virt.aarch64.xml | 1 + tests/domaincapsdata/qemu_2.12.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_2.12.0.ppc64.xml| 1 + tests/domaincapsdata/qemu_2.12.0.s390x.xml| 1 + tests/domaincapsdata/qemu_2.12.0.x86_64.xml | 1 + .../domaincapsdata/qemu_2.4.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_2.4.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.4.0.x86_64.xml| 1 + .../domaincapsdata/qemu_2.5.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_2.5.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.5.0.x86_64.xml| 1 + .../domaincapsdata/qemu_2.6.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_2.6.0-tcg.x86_64.xml | 1 + .../qemu_2.6.0-virt.aarch64.xml | 1 + tests/domaincapsdata/qemu_2.6.0.aarch64.xml | 1 + tests/domaincapsdata/qemu_2.6.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_2.6.0.x86_64.xml| 1 + .../domaincapsdata/qemu_2.7.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_2.7.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.7.0.s390x.xml | 1 + tests/domaincapsdata/qemu_2.7.0.x86_64.xml| 1 + .../domaincapsdata/qemu_2.8.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_2.8.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.8.0.s390x.xml | 1 + tests/domaincapsdata/qemu_2.8.0.x86_64.xml| 1 + .../domaincapsdata/qemu_2.9.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_2.9.0-tcg.x86_64.xml | 1 + tests/domaincapsdata/qemu_2.9.0.ppc64.xml | 1 + tests/domaincapsdata/qemu_2.9.0.s390x.xml | 1 + tests/domaincapsdata/qemu_2.9.0.x86_64.xml| 1 + .../domaincapsdata/qemu_3.0.0-q35.x86_64.xml | 1 + .../domaincapsdata/qemu_3.0.0-tcg.x86_64.xml | 1 +
Plans for the next release
We are getting close to the next release of libvirt. To aim for the release on Jul 01 I suggest entering the freeze on Friday Jun 25 and tagging RC2 on Tuesday Jun 29. I hope this works for everyone. Jirka
Re: [PATCH] ch_domain: Add handler for virDomainDeviceDefPostParseCallback
On Wed, Jun 16, 2021 at 21:16:01 -0700, William Douglas wrote:
> Instead of trying to match devices passed in based on the monitor
> detecting the number of devices that were used in the domain
> definition, use the devicesPostParseCallback to evaluate if
> unsupported devices are used.
>
> This allows the compiler to detect when new device types are added
> that need to be checked.
>
> Signed-off-by: William Douglas
> ---
> src/ch/ch_domain.c | 121 +++
> src/ch/ch_monitor.c | 122
> 2 files changed, 121 insertions(+), 122 deletions(-)
>
> diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
> index f9a6f3f31d..02ca5ea06c 100644
> --- a/src/ch/ch_domain.c
> +++ b/src/ch/ch_domain.c
> @@ -197,7 +197,128 @@ virCHDomainDefPostParse(virDomainDef *def,
> return 0;
> }
>
> +static int
> +virCHDomainDeviceDefPostParse(virDomainDeviceDef *dev,
> + const virDomainDef *def G_GNUC_UNUSED,
> + unsigned int parseFlags G_GNUC_UNUSED,
> + void *opaque G_GNUC_UNUSED,
> + void *parseOpaque G_GNUC_UNUSED)
> +{
> +int ret = -1;
> +
> +switch ((virDomainDeviceType)dev->type) {
> +case VIR_DOMAIN_DEVICE_DISK:
> +ret = 0;
> +break;
> +case VIR_DOMAIN_DEVICE_LEASE:
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Cloud-Hypervisor doesn't support lease"));
> +break;
> +case VIR_DOMAIN_DEVICE_FS:
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Cloud-Hypervisor doesn't support fs"));
> +break;
> +case VIR_DOMAIN_DEVICE_NET:
> +ret = 0;
> +break;
> +case VIR_DOMAIN_DEVICE_INPUT:
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Cloud-Hypervisor doesn't support input"));
> +break;
> +case VIR_DOMAIN_DEVICE_SOUND:
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Cloud-Hypervisor doesn't support sound"));
> +break;
To further simplify the code you can also do the following (just an
example):
case VIR_DOMAIN_DEVICE_VIDEO:
case VIR_DOMAIN_DEVICE_HOSTDEV:
case VIR_DOMAIN_DEVICE_WATCHDOG:
case VIR_DOMAIN_DEVICE_CONTROLLER:
case VIR_DOMAIN_DEVICE_GRAPHICS:
case VIR_DOMAIN_DEVICE_HUB:
case VIR_DOMAIN_DEVICE_REDIRDEV:
case VIR_DOMAIN_DEVICE_SMARTCARD:
case VIR_DOMAIN_DEVICE_CHR:
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Cloud-Hypervisor doesn't support '%s' device"),
virDomainDeviceTypeToString(dev->type));
return -1;
Also note that in validation callbacks any invalid configuration should
direclty return -1 to short circuit the execution so that we don't need
to have a 'ret' variable. It makes the code easier to read.
Re: [PATCH] ch_domain: Add handler for virDomainDeviceDefPostParseCallback
On 6/18/21 2:46 AM, Douglas, William wrote:
> Ick sorry for the malformed mail...
>
> On 6/17/21 10:33 AM, Michal Prívozník wrote:
>> On 6/17/21 9:00 AM, Peter Krempa wrote:
>>> On Wed, Jun 16, 2021 at 21:16:01 -0700, William Douglas wrote:
Instead of trying to match devices passed in based on the monitor
detecting the number of devices that were used in the domain
definition, use the devicesPostParseCallback to evaluate if
unsupported devices are used.
This allows the compiler to detect when new device types are added
that need to be checked.
Signed-off-by: William Douglas
---
src/ch/ch_domain.c | 121 +++
src/ch/ch_monitor.c | 122
2 files changed, 121 insertions(+), 122 deletions(-)
>>>
>>> Note that putting stuff into the post-parse callback will result in the
>>> failure/rejection happening already at XML parsing time.
>>>
>>> I hope that's what you intended.
>>>
>>> Generally such a change would not be acceptable because strictening the
>>> parser means we would fail at loading already defined configuration XMLs
>>> e.g. at libvirtd restart.
>>>
>>> In this particular instance it's okay because the cloud hypervisor code
>>> was not yet released.
>>>
>>> Also note that the addition of the cloud hypervisor driver was not yet
>>> documented in NEWS.rst.
>>
>> However, I would argue that this should go into validation rather than
>> post parse. To me, XML parsing happens in these steps:
>>
>> 1) actual parsing and filling internal structures (e.g. using XPATHs,
>> xmlPropString, etc.)
>>
>> 2) post parse (filling in missing info, e.g. defaults)
>>
>> 3) validation (checking whether definition makes sense).
>>
>>
>> And this patch is performing validation. So I think we need to set
>> different member (taken from QEMU driver):
>>
>>
>> virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
>> .deviceValidateCallback = qemuValidateDomainDeviceDef,
>> }
>>
>
> I tried to follow advice from Daniel (on
> https://listman.redhat.com/archives/libvir-list/2021-June/msg00084.html).
> The goal I had in mind from his last comment was that the cloud-hypervisor
> driver would catch
> new devices due to missing items from the enum in the switch when they are
> added to libvirt.
>
> In this case we'd really only be adding support so it wouldn't be increasing
> strictness on newer
> versions. I'm assuming that XML definitions with devices libvirt doesn't know
> about would fail to
> load. Then if a new libvirt is loaded after restart with new hardware support
> (but cloud-hypervisor
> doesn't support it) existing configurations wouldn't be impacted.
>
> This is definitely validation but the validation call's type signature
> doesn't seem to align very
> closely to the use case I was trying to fill. Should I be going about this
> differently and I just
> misunderstood Daniel's advice?
>
No, Dan's point is correct and you are doing the right thing, almost :-)
What Peter and I are talking about is to put the check into different
phase of XML parsing.
Here is a snippet of stacktrace of XML parsing:
virDomainDefineXMLFlags
chDomainDefineXMLFlags
virDomainDefParseString
virDomainDefParse
virDomainDefParseNode
Now, this last function consists of those three steps I was talking
about earlier:
virDomainDef *
virDomainDefParseNode(...)
{
...
if (!(def = virDomainDefParseXML(xml, ctxt, xmlopt, flags)))
return NULL;
/* callback to fill driver specific domain aspects */
if (virDomainDefPostParse(def, flags, xmlopt, parseOpaque) < 0)
return NULL;
/* validate configuration */
if (virDomainDefValidate(def, flags, xmlopt, parseOpaque) < 0)
return NULL;
...
}
The devicesPostParseCallback will be called from virDomainDefPostParse()
and the deviceValidateCallback will be called from
virDomainDefValidate(). Thus moving the check from the former to the
latter does not make situation worse. I mean, the
virDomainDefParseNode() would still fail.
However, there is a strong reason why we want this kind of checks to
live in validation step. When libvirtd starts up it parses XMLs of
defined domains from /etc/libvirt/..., but it does so with @flags having
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE set. This means, that as we tighten
some checks in validation step, we don't fail to reload a defined domain
XML on libvirtd restart/upgrade.
Long story short, Dan's suggestion was correct from conceptual POV. But
slightly less so from placement POV.
Michal
Re: [PATCH] ch_domain: Add handler for virDomainDeviceDefPostParseCallback
On Fri, Jun 18, 2021 at 00:46:03 +, Douglas, William wrote:
> Ick sorry for the malformed mail...
>
> On 6/17/21 10:33 AM, Michal Prívozník wrote:
> > On 6/17/21 9:00 AM, Peter Krempa wrote:
> >> On Wed, Jun 16, 2021 at 21:16:01 -0700, William Douglas wrote:
> >>> Instead of trying to match devices passed in based on the monitor
> >>> detecting the number of devices that were used in the domain
> >>> definition, use the devicesPostParseCallback to evaluate if
> >>> unsupported devices are used.
> >>>
> >>> This allows the compiler to detect when new device types are added
> >>> that need to be checked.
> >>>
> >>> Signed-off-by: William Douglas
> >>> ---
> >>> src/ch/ch_domain.c | 121 +++
> >>> src/ch/ch_monitor.c | 122
> >>> 2 files changed, 121 insertions(+), 122 deletions(-)
> >>
> >> Note that putting stuff into the post-parse callback will result in the
> >> failure/rejection happening already at XML parsing time.
> >>
> >> I hope that's what you intended.
> >>
> >> Generally such a change would not be acceptable because strictening the
> >> parser means we would fail at loading already defined configuration XMLs
> >> e.g. at libvirtd restart.
> >>
> >> In this particular instance it's okay because the cloud hypervisor code
> >> was not yet released.
> >>
> >> Also note that the addition of the cloud hypervisor driver was not yet
> >> documented in NEWS.rst.
> >
> > However, I would argue that this should go into validation rather than
> > post parse. To me, XML parsing happens in these steps:
> >
> > 1) actual parsing and filling internal structures (e.g. using XPATHs,
> > xmlPropString, etc.)
> >
> > 2) post parse (filling in missing info, e.g. defaults)
> >
> > 3) validation (checking whether definition makes sense).
> >
> >
> > And this patch is performing validation. So I think we need to set
> > different member (taken from QEMU driver):
> >
> >
> > virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
> > .deviceValidateCallback = qemuValidateDomainDeviceDef,
> > }
> >
>
> I tried to follow advice from Daniel (on
> https://listman.redhat.com/archives/libvir-list/2021-June/msg00084.html).
> The goal I had in mind from his last comment was that the cloud-hypervisor
> driver would catch
> new devices due to missing items from the enum in the switch when they are
> added to libvirt.
Well that exact goal can be achieved by exactly the same way, even with
same code basically in the validation callback.
The semantic difference between the two is that:
- validation is done for any new XML
- validation is (should be) re-tried on startup of an instance with an
existing definition
- code in vaidation callback must not fill any defaults or change
anything
- post-parse is done with all XML parsing (even existing configs)
- post-parse may be retried in certain cases on startup but it's not the
norm
- post parse must never add any check which would fail with pre-existing
XML documents
You can move your code as-is as a validation callback. The benefit will
be that any further addition to it doesn't need to be as closely
scrutinized about breaking existing XML documents, while preserving both
the compile-time addition checks as you've wanted.
> In this case we'd really only be adding support so it wouldn't be increasing
> strictness on newer
> versions.
Well in fact yours is making the XML parser stricter, and even if you'd
have existing configs on devel versions they would vanish. Granted this
isn't making anythign stricter accross released versions.
A further note is that this decreases the likelyhood that further
addition making the parser stricter would land in the post-parse version
and go unnoticed in libvirt.
> I'm assuming that XML definitions with devices libvirt doesn't know about
> would fail to
> load.
Well, depending how you define them. They would fail in cases when the
user asks for a XML schema validation while defining them. In that case
if the XML contains anything not covered in the schema it's rejected.
On the other hand without validation any new things are silently
ignored.
> Then if a new libvirt is loaded after restart with new hardware support (but
> cloud-hypervisor
> doesn't support it) existing configurations wouldn't be impacted.
Yes that is true, any existing definition will not be impacted unless
aditional code is added to the post-parse callback.
> This is definitely validation but the validation call's type signature
> doesn't seem to align very
> closely to the use case I was trying to fill.
I don't think this is true:
static int
virCHDomainDeviceDefPostParse(virDomainDeviceDef *dev,
const virDomainDef *def G_GNUC_UNUSED,
unsigned int parseFlags G_GNUC_UNUSED,
void *opaque G_GNUC_UNUSED,
void *parseOpaque G_GNUC_UNUSED)
