[PATCH] spec: Drop numad usage on fedora 39+
numad is removed from Fedora 39. Upstream is dead
https://src.fedoraproject.org/rpms/numad/c/a6bb891e8447e3b2a4c63774da94ad0d9b4ee50a?branch=rawhide
https://pagure.io/releng/failed-composes/issue/4990#comment-857670
Signed-off-by: Cole Robinson
---
This powers vcpu and numatune placement='auto'. Anyone know if that's
being actively used in kubevirt, openstack, etc?
libvirt.spec.in | 4
1 file changed, 4 insertions(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 1f77cd90b7..5f267a086b 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -173,6 +173,10 @@
%define with_numad0%{!?_without_numad:1}
%endif
%endif
+%if 0%{?fedora} >= 39
+# numad is retired in fedora 39+, upstream is dead
+%define with_numad0
+%endif
%ifarch %{arches_dmidecode}
%define with_dmidecode 0%{!?_without_dmidecode:1}
--
2.40.1
Re: [libvirt PATCH v4 00/12] tools: provide virt-qemu-sev-validate for SEV(-ES) launch attestation
On 11/7/22 9:41 AM, Daniel P. Berrangé wrote: > The libvirt QEMU driver provides all the functionality required for > launching a guest on AMD SEV(-ES) platforms, with a configuration > that enables attestation of the launch measurement. The documentation > for how to actually perform an attestation is severely lacking and > not suitable for mere mortals to understand. IOW, someone trying to > implement attestation is in for a world of pain and suffering. > > This series doesn't fix the documentation problem, but it does > provide a reference implementation of a tool for performing > attestation of SEV(-ES) guests in the context of libvirt / KVM. > > There will be other tools and libraries that implement attestation > logic too, but this tool is likely somewhat unique in its usage of > libvirt. Now for a attestation to be trustworthy you don't want to > perform it on the hypervisor host, since the goal is to prove that > the hypervisor has not acted maliciously. None the less it is still > beneficial to have libvirt integration to some extent. > > When running this tool on a remote (trusted) host, it can connect > to the libvirt hypervisor and fetch the data provided by the > virDomainLaunchSecurityInfo API, which is safe to trust as the > key pieces are cryptographically measured. > > Attestation is a complex problem though and it is very easy to > screw up and feed the wrong information and then waste hours trying > to figure out what piece was wrong, to cause the hash digest to > change. For debugging such problems, you can thus tell the tool > to operate insecurely, by querying libvirt for almost all of the > configuration information required to determine the expected > measurement. By comparing these results,to the results obtained > in offline mode it helps narrow down where the mistake lies. > > So I view this tool as being useful in a number of ways: > > * Quality assurance engineers needing to test libvirt/QEMU/KVM >get a simple and reliable tool for automating tests with. > > * Users running simple libvirt deployments without any large >management stack, get a standalone tool for attestation >they can rely on. > > * Developers writing/integrating attestation support into >management stacks above libvirt, get a reference against >which they can debug their own tools. > > * Users wanting to demonstrate the core SEV/SEV-ES functionality >get a simple and reliable tool to illustrate the core concepts >involved. > > Since I didn't fancy writing such complex logic in C, this tool is > a python3 program. As such, we don't want to include it in the > main libvirt-client RPM, nor any other existing RPM. THus, this > series puts it in a new libvirt-client-qemu RPM which, through no > co-inicidence at all, is the same RPM I invented a few days ago to > hold the virt-qemu-qmp-proxy command. > > Note, people will have already seen an earlier version of this > tool I hacked up some months ago. This code is very significantly > changed since that earlier version, to make it more maintainable, > and simpler to use (especially for SEV-ES) but the general theme > is still the same. > > Changed in v4: > > - Fixed loading of initrd/cmdline from XML > - s/loader/firmware/ in some error messages > > Changed in v3: > > - Remove LUKS specific --disk-password and have generic >--inject-secret > - Fix handling of optional initrd/cmdline > - Require --kernel if --initrd or --cmdline are present > - Ensure VM is in paused state > > Changed in v2: > > - All the suggestions from Cole and Kashyap > Reviewed-by: Cole Robinson - Cole
Re: [libvirt PATCH v3 04/12] tools: support validating SEV direct kernel boot measurements
On 11/7/22 7:28 AM, Daniel P. Berrangé wrote:
> On Sun, Nov 06, 2022 at 04:03:15PM -0500, Cole Robinson wrote:
>> On 11/2/22 7:58 AM, Daniel P. Berrangé wrote:
>>> When doing direct kernel boot we need to include the kernel, initrd and
>>> cmdline in the measurement.
>>>
>>> Signed-off-by: Daniel P. Berrangé
>>> ---
>>> docs/manpages/virt-qemu-sev-validate.rst | 43 +
>>> tools/virt-qemu-sev-validate | 108 ++-
>>> 2 files changed, 150 insertions(+), 1 deletion(-)
>>>
>>
>> ...
>>
>>> +
>>> +class KernelTable(GUIDTable):
>>> +
>>> +TABLE_GUID = UUID('{9438d606-4f22-4cc9-b479-a793-d411fd21}').bytes_le
>>> +KERNEL_GUID = UUID('{4de79437-abd2-427f-b835-d5b1-72d2045b}').bytes_le
>>> +INITRD_GUID = UUID('{44baf731-3a2f-4bd7-9af1-41e2-9169781d}').bytes_le
>>> +CMDLINE_GUID = UUID('{97d02dd8-bd20-4c94-aa78-e771-4d36ab2a}').bytes_le
>>> +
>>> +def __init__(self):
>>> +super().__init__(guid=self.TABLE_GUID,
>>> + lenlen=2)
>>> +
>>> +self.kernel = None
>>> +self.initrd = sha256(bytes([])).digest()
>>> +self.cmdline = sha256(bytes([0])).digest()
>>> +
>>
>> This bit here caused a regression from v2. self.initrd and self.cmdline
>> should be initialized to None. Otherwise the code that triggers
>> load_kernel and load_initrd never runs.
>
> I'm not seeing any regression. The call to load_kernel/load_intrd
> is conditioned on args.initrd != None, not self.initrd != None.
Sorry, I should have been more clear. It's the load_initrd call
triggered from XML code path, not the cli --initrd code path.
- Cole
Re: [libvirt PATCH v3 04/12] tools: support validating SEV direct kernel boot measurements
On 11/2/22 7:58 AM, Daniel P. Berrangé wrote:
> When doing direct kernel boot we need to include the kernel, initrd and
> cmdline in the measurement.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> docs/manpages/virt-qemu-sev-validate.rst | 43 +
> tools/virt-qemu-sev-validate | 108 ++-
> 2 files changed, 150 insertions(+), 1 deletion(-)
>
...
> +
> +class KernelTable(GUIDTable):
> +
> +TABLE_GUID = UUID('{9438d606-4f22-4cc9-b479-a793-d411fd21}').bytes_le
> +KERNEL_GUID = UUID('{4de79437-abd2-427f-b835-d5b1-72d2045b}').bytes_le
> +INITRD_GUID = UUID('{44baf731-3a2f-4bd7-9af1-41e2-9169781d}').bytes_le
> +CMDLINE_GUID = UUID('{97d02dd8-bd20-4c94-aa78-e771-4d36ab2a}').bytes_le
> +
> +def __init__(self):
> +super().__init__(guid=self.TABLE_GUID,
> + lenlen=2)
> +
> +self.kernel = None
> +self.initrd = sha256(bytes([])).digest()
> +self.cmdline = sha256(bytes([0])).digest()
> +
This bit here caused a regression from v2. self.initrd and self.cmdline
should be initialized to None. Otherwise the code that triggers
load_kernel and load_initrd never runs.
- Cole
Re: [PATCH] qemu: Report sev measurement value and nonce explicitly
On 10/26/22 7:24 AM, Daniel P. Berrangé wrote: > On Tue, Oct 25, 2022 at 03:03:46PM -0400, Cole Robinson wrote: >> On 10/17/22 3:42 AM, Michal Prívozník wrote: >>> On 10/16/22 22:06, Cole Robinson wrote: >>>> The value returned by qemu's query-sev-launch-measure comes >>>> straight from the LAUNCH_MEASURE SEV firmware command. It's two >>>> values packed together: first 32 bytes is the launch measurement, >>>> last 16 bytes is the nonce. >>>> >>>> This combined value is really just an artifact of the return value >>>> of the firmware command, it has no direct usage. Users want the two >>>> individual values. But because qemu and libvirt do not separate them >>>> apart, every app that wants to process this value will have to do >>>> it manually. >>>> >>>> This performs the split for the user, and delivers the values in two >>>> new TYPED_PARAM fields: sev-measurement-value, sev-measurement-nonce >>>> >>>> Signed-off-by: Cole Robinson >>>> --- >>>> include/libvirt/libvirt-domain.h | 22 ++ >>>> src/qemu/qemu_driver.c | 23 +++ >>>> 2 files changed, 45 insertions(+) >>>> >>> >>> Reviewed-by: Michal Privoznik >>> >> >> Thanks, but thinking more, I'll propose this at the qemu layer and make >> sure it's acceptable there first. Otherwise long term tools will may >> still need to handle the sev-measurement format to cover both qemu and >> libvirt cases. > > I'm not entirely convinced we should split them apart at either > libvirt or QEMU level. > > I think I would tend to view CVM launch measurement data as being > an opaque blob from the POV of libvirt/QEMU. In the case of SEV/SEV-ES > it does represent a tuple of nonce+hash, but that encoding is an > artifact of the current firmware implementation. The firmware <-> > userspace API for SEV treats it as opaque, which means the firmware > has the freedom to change its contents at will. I expect this is > unlikely to happen in practice, but it is allowed for by the current > design, as we transmit the firmware major/minor version, alongside > the measurement. If we split them apart then it makes QMEU and > libvirt aware of the specific firmware implementation which is > undesirable. > > Overall I'd say the only 2 parties who should try to interpret the > measurement are the firmware and the attestation software client, > all the pieces inbetween should remain dumb transports. Ok sounds good, I'll leave it as is Thanks, Cole
[PATCH] tests: Fix libxlxml2domconfigtest with latest xen
shadow_memkb is populated from a libxl API call, and the value can
change. For example:
https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=2c992810854a15b41be920519ce83a4a328d5168
Mock libxl_get_required_shadow_memory to give consistent output
Signed-off-by: Cole Robinson
---
tests/libxlmock.c | 11 +++
tests/libxlxml2domconfigdata/basic-hvm.json | 2 +-
tests/libxlxml2domconfigdata/basic-pv.json| 2 +-
tests/libxlxml2domconfigdata/basic-pvh.json | 2 +-
tests/libxlxml2domconfigdata/cpu-shares-hvm.json | 2 +-
tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json | 2 +-
.../fullvirt-cpuid-legacy-nest.json | 2 +-
tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 2 +-
.../libxlxml2domconfigdata/max-eventchannels-hvm.json | 2 +-
tests/libxlxml2domconfigdata/max-gntframes-hvm.json | 2 +-
tests/libxlxml2domconfigdata/moredevs-hvm.json| 2 +-
tests/libxlxml2domconfigdata/multiple-ip.json | 2 +-
tests/libxlxml2domconfigdata/variable-clock-hvm.json | 2 +-
.../libxlxml2domconfigdata/vnuma-hvm-legacy-nest.json | 2 +-
tests/libxlxml2domconfigdata/vnuma-hvm.json | 2 +-
15 files changed, 25 insertions(+), 14 deletions(-)
diff --git a/tests/libxlmock.c b/tests/libxlmock.c
index 0e4bf7df52..4754597e5b 100644
--- a/tests/libxlmock.c
+++ b/tests/libxlmock.c
@@ -109,6 +109,17 @@ VIR_MOCK_STUB_RET_ARGS(bind,
const struct sockaddr *, addr,
socklen_t, addrlen)
+VIR_MOCK_IMPL_RET_ARGS(libxl_get_required_shadow_memory,
+ unsigned long,
+ unsigned long, maxmem_kb,
+ unsigned int, smp_cpus)
+{
+/* silence gcc warning about unused function */
+if (0)
+real_libxl_get_required_shadow_memory(maxmem_kb, smp_cpus);
+return 1234;
+}
+
VIR_MOCK_IMPL_RET_ARGS(__xstat, int,
int, ver,
const char *, path,
diff --git a/tests/libxlxml2domconfigdata/basic-hvm.json
b/tests/libxlxml2domconfigdata/basic-hvm.json
index 87f8cb7d8a..d30875420d 100644
--- a/tests/libxlxml2domconfigdata/basic-hvm.json
+++ b/tests/libxlxml2domconfigdata/basic-hvm.json
@@ -15,7 +15,7 @@
"max_memkb": 1048576,
"target_memkb": 1048576,
"video_memkb": 8192,
-"shadow_memkb": 12288,
+"shadow_memkb": 1234,
"device_model_version": "qemu_xen",
"device_model": "/bin/true",
"sched_params": {
diff --git a/tests/libxlxml2domconfigdata/basic-pv.json
b/tests/libxlxml2domconfigdata/basic-pv.json
index b71c3b0f49..32d188fabd 100644
--- a/tests/libxlxml2domconfigdata/basic-pv.json
+++ b/tests/libxlxml2domconfigdata/basic-pv.json
@@ -14,7 +14,7 @@
],
"max_memkb": 524288,
"target_memkb": 524288,
-"shadow_memkb": 8192,
+"shadow_memkb": 1234,
"sched_params": {
},
diff --git a/tests/libxlxml2domconfigdata/basic-pvh.json
b/tests/libxlxml2domconfigdata/basic-pvh.json
index 48365c9026..f51957aa85 100644
--- a/tests/libxlxml2domconfigdata/basic-pvh.json
+++ b/tests/libxlxml2domconfigdata/basic-pvh.json
@@ -14,7 +14,7 @@
],
"max_memkb": 524288,
"target_memkb": 524288,
-"shadow_memkb": 8192,
+"shadow_memkb": 1234,
"sched_params": {
},
diff --git a/tests/libxlxml2domconfigdata/cpu-shares-hvm.json
b/tests/libxlxml2domconfigdata/cpu-shares-hvm.json
index 2aa97e88c5..15105c83ad 100644
--- a/tests/libxlxml2domconfigdata/cpu-shares-hvm.json
+++ b/tests/libxlxml2domconfigdata/cpu-shares-hvm.json
@@ -15,7 +15,7 @@
"max_memkb": 1048576,
"target_memkb": 1048576,
"video_memkb": 8192,
-"shadow_memkb": 12288,
+"shadow_memkb": 1234,
"device_model_version": "qemu_xen",
"device_model": "/bin/true",
"sched_params": {
diff --git a/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
b/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
index a2d46797aa..26f5abefee 100644
--- a/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
+++ b/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
@@ -11,7 +11,7 @@
],
"max_memkb": 592896,
"target_memkb": 403456,
-"shadow_memkb": 5656,
+"shadow_memkb": 1234,
"sched_params": {
},
"apic": "True",
diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid-legacy-nest.json
b/tests/libxlxml2domconfigdata/fu
Re: [libvirt PATCH v2 09/12] tools: support generating SEV secret injection tables
On 10/19/22 6:17 AM, Daniel P. Berrangé wrote:
> It is possible to build OVMF for SEV with an embedded Grub that can
> fetch LUKS disk secrets. This adds support for injecting secrets in
> the required format.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> diff --git a/tools/virt-qemu-sev-validate b/tools/virt-qemu-sev-validate
> index 5ce5763d5b..2d15edb933 100755
> --- a/tools/virt-qemu-sev-validate
> +++ b/tools/virt-qemu-sev-validate
> @@ -36,16 +36,19 @@
>
> import abc
> import argparse
> -from base64 import b64decode
> +from base64 import b64decode, b64encode
> from hashlib import sha256
> import hmac
> import logging
> +import os
> import re
> import socket
> from struct import pack
> import sys
> import traceback
> from uuid import UUID
> +from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes
> +
>
> from lxml import etree
> import libvirt
> @@ -573,7 +576,26 @@ class KernelTable(GUIDTable):
> return entries
>
>
> -class ConfidentialVM(object):
> +class SecretsTable(GUIDTable):
> +
> +TABLE_GUID = UUID('{1e74f542-71dd-4d66-963e-ef4287ff173b}').bytes_le
> +DISK_PW_GUID = UUID('{736869e5-84f0-4973-92ec-06879ce3da0b}').bytes_le
> +
> +def __init__(self):
> +super().__init__(guid=self.TABLE_GUID,
> + lenlen=4)
> +self.disk_password = None
> +
> +def load_disk_password(self, path):
> +with open(path, 'rb') as fh:
> +self.disk_password = fh.read()
> +
> +def entries(self):
> +return self.build_entry(self.DISK_PW_GUID,
> +self.disk_password + bytes([0]), 4)
> +
This bytes([0]) NUL byte ends up in the efi_secret /sys path. Dropping
it doesn't seem to impact injecting the secret at all
FWIW once that's dropped, getting automatic luks unlock is really simple
with /etc/crypttab + kernel 5.19
sed -i -e "s| none |
/sys/kernel/security/secrets/coco/736869e5-84f0-4973-92ec-06879ce3da0b
|g" /etc/crypttab
dracut --force --add-drivers efi_secret
shutdown -r now
Thanks,
Cole
Re: [PATCH] qemu: Report sev measurement value and nonce explicitly
On 10/17/22 3:42 AM, Michal Prívozník wrote: > On 10/16/22 22:06, Cole Robinson wrote: >> The value returned by qemu's query-sev-launch-measure comes >> straight from the LAUNCH_MEASURE SEV firmware command. It's two >> values packed together: first 32 bytes is the launch measurement, >> last 16 bytes is the nonce. >> >> This combined value is really just an artifact of the return value >> of the firmware command, it has no direct usage. Users want the two >> individual values. But because qemu and libvirt do not separate them >> apart, every app that wants to process this value will have to do >> it manually. >> >> This performs the split for the user, and delivers the values in two >> new TYPED_PARAM fields: sev-measurement-value, sev-measurement-nonce >> >> Signed-off-by: Cole Robinson >> --- >> include/libvirt/libvirt-domain.h | 22 ++ >> src/qemu/qemu_driver.c | 23 +++ >> 2 files changed, 45 insertions(+) >> > > Reviewed-by: Michal Privoznik > Thanks, but thinking more, I'll propose this at the qemu layer and make sure it's acceptable there first. Otherwise long term tools will may still need to handle the sev-measurement format to cover both qemu and libvirt cases. - Cole
Re: [libvirt PATCH v2 00/12] tools: provide virt-qemu-sev-validate for SEV(-ES) launch attestation
On 10/19/22 6:17 AM, Daniel P. Berrangé wrote: > The libvirt QEMU driver provides all the functionality required for > launching a guest on AMD SEV(-ES) platforms, with a configuration > that enables attestation of the launch measurement. The documentation > for how to actually perform an attestation is severely lacking and > not suitable for mere mortals to understand. IOW, someone trying to > implement attestation is in for a world of pain and suffering. > > This series doesn't fix the documentation problem, but it does > provide a reference implementation of a tool for performing > attestation of SEV(-ES) guests in the context of libvirt / KVM. > > There will be other tools and libraries that implement attestation > logic too, but this tool is likely somewhat unique in its usage of > libvirt. Now for a attestation to be trustworthy you don't want to > perform it on the hypervisor host, since the goal is to prove that > the hypervisor has not acted maliciously. None the less it is still > beneficial to have libvirt integration to some extent. > > When running this tool on a remote (trusted) host, it can connect > to the libvirt hypervisor and fetch the data provided by the > virDomainLaunchSecurityInfo API, which is safe to trust as the > key pieces are cryptographically measured. > > Attestation is a complex problem though and it is very easy to > screw up and feed the wrong information and then waste hours trying > to figure out what piece was wrong, to cause the hash digest to > change. For debugging such problems, you can thus tell the tool > to operate insecurely, by querying libvirt for almost all of the > configuration information required to determine the expected > measurement. By comparing these results,to the results obtained > in offline mode it helps narrow down where the mistake lies. > > So I view this tool as being useful in a number of ways: > > * Quality assurance engineers needing to test libvirt/QEMU/KVM >get a simple and reliable tool for automating tests with. > > * Users running simple libvirt deployments without any large >management stack, get a standalone tool for attestation >they can rely on. > > * Developers writing/integrating attestation support into >management stacks above libvirt, get a reference against >which they can debug their own tools. > > * Users wanting to demonstrate the core SEV/SEV-ES functionality >get a simple and reliable tool to illustrate the core concepts >involved. > > Since I didn't fancy writing such complex logic in C, this tool is > a python3 program. As such, we don't want to include it in the > main libvirt-client RPM, nor any other existing RPM. THus, this > series puts it in a new libvirt-client-qemu RPM which, through no > co-inicidence at all, is the same RPM I invented a few days ago to > hold the virt-qemu-qmp-proxy command. > > Note, people will have already seen an earlier version of this > tool I hacked up some months ago. This code is very significantly > changed since that earlier version, to make it more maintainable, > and simpler to use (especially for SEV-ES) but the general theme > is still the same. > > Changed in v2: > > - All the suggestions from Cole and Kashyap > > Daniel P. Berrangé (12): > build-aux: only forbid gethostname in C files > tools: support validating SEV firmware boot measurements > tools: load guest config from libvirt > tools: support validating SEV direct kernel boot measurements > tools: load direct kernel config from libvirt > tools: support validating SEV-ES initial vCPU state measurements > tools: support automatically constructing SEV-ES vCPU state > tools: load CPU count and CPU SKU from libvirt > tools: support generating SEV secret injection tables > docs/kbase: describe attestation for SEV guests > scripts: add systemtap script for capturing SEV-ES VMSA > docs/manpages: add checklist of problems for SEV attestation > > build-aux/syntax-check.mk|1 + > docs/kbase/launch_security_sev.rst | 105 ++ > docs/manpages/meson.build|1 + > docs/manpages/virt-qemu-sev-validate.rst | 647 +++ > examples/systemtap/amd-sev-es-vmsa.stp | 48 + > libvirt.spec.in |2 + > tools/meson.build|5 + > tools/virt-qemu-sev-validate | 1292 ++++++ > 8 files changed, 2101 insertions(+) > create mode 100644 docs/manpages/virt-qemu-sev-validate.rst > create mode 100644 examples/systemtap/amd-sev-es-vmsa.stp > create mode 100755 tools/virt-qemu-sev-validate > Reviewed-by: Cole Robinson - Cole
Re: [libvirt PATCH 00/12] tools: provide virt-qemu-sev-validate for SEV(-ES) launch attestation
On 10/18/22 5:22 AM, Daniel P. Berrangé wrote: > On Sun, Oct 16, 2022 at 03:06:17PM -0400, Cole Robinson wrote: >> On 10/7/22 7:42 AM, Daniel P. Berrangé wrote: >>> The libvirt QEMU driver provides all the functionality required for >>> launching a guest on AMD SEV(-ES) platforms, with a configuration >>> that enables attestation of the launch measurement. The documentation >>> for how to actually perform an attestation is severely lacking and >>> not suitable for mere mortals to understand. IOW, someone trying to >>> implement attestation is in for a world of pain and suffering. >>> >>> This series doesn't fix the documentation problem, but it does >>> provide a reference implementation of a tool for performing >>> attestation of SEV(-ES) guests in the context of libvirt / KVM. >>> >>> There will be other tools and libraries that implement attestation >>> logic too, but this tool is likely somewhat unique in its usage of >>> libvirt. Now for a attestation to be trustworthy you don't want to >>> perform it on the hypervisor host, since the goal is to prove that >>> the hypervisor has not acted maliciously. None the less it is still >>> beneficial to have libvirt integration to some extent. >>> >>> When running this tool on a remote (trusted) host, it can connect >>> to the libvirt hypervisor and fetch the data provided by the >>> virDomainLaunchSecurityInfo API, which is safe to trust as the >>> key pieces are cryptographically measured. >>> >>> Attestation is a complex problem though and it is very easy to >>> screw up and feed the wrong information and then waste hours trying >>> to figure out what piece was wrong, to cause the hash digest to >>> change. For debugging such problems, you can thus tell the tool >>> to operate insecurely, by querying libvirt for almost all of the >>> configuration information required to determine the expected >>> measurement. By comparing these results,to the results obtained >>> in offline mode it helps narrow down where the mistake lies. >>> >>> So I view this tool as being useful in a number of ways: >>> >>> * Quality assurance engineers needing to test libvirt/QEMU/KVM >>>get a simple and reliable tool for automating tests with. >>> >>> * Users running simple libvirt deployments without any large >>>management stack, get a standalone tool for attestation >>>they can rely on. >>> >>> * Developers writing/integrating attestation support into >>>management stacks above libvirt, get a reference against >>>which they can debug their own tools. >>> >>> * Users wanting to demonstrate the core SEV/SEV-ES functionality >>>get a simple and reliable tool to illustrate the core concepts >>>involved. >>> >>> Since I didn't fancy writing such complex logic in C, this tool is >>> a python3 program. As such, we don't want to include it in the >>> main libvirt-client RPM, nor any other existing RPM. THus, this >>> series puts it in a new libvirt-client-qemu RPM which, through no >>> co-inicidence at all, is the same RPM I invented a few days ago to >>> hold the virt-qemu-qmp-proxy command. >>> >>> Note, people will have already seen an earlier version of this >>> tool I hacked up some months ago. This code is very significantly >>> changed since that earlier version, to make it more maintainable, >>> and simpler to use (especially for SEV-ES) but the general theme >>> is still the same. >>> >>> Daniel P. Berrangé (12): >>> build-aux: only forbid gethostname in C files >>> tools: support validating SEV firmware boot measurements >>> tools: load guest config from libvirt >>> tools: support validating SEV direct kernel boot measurements >>> tools: load direct kernel config from libvirt >>> tools: support validating SEV-ES initial vCPU state measurements >>> tools: support automatically constructing SEV-ES vCPU state >>> tools: load CPU count and CPU SKU from libvirt >>> tools: support generating SEV secret injection tables >>> docs/kbase: describe attestation for SEV guests >>> scripts: add systemtap script for capturing SEV-ES VMSA >>> docs/manpages: add checklist of problems for SEV attestation >> >> >> After the fix I mentioned on patch 7, the script worked for my sev, >> sev-es, sev-es + kernel setup, with vmsa passed in and vmsa generated. >> >> Attached is some pylint output, nothing really serious: > > We've not run pylint on libvirt tree historically, but I wonder > if we should introduce it to check all our build scripts too. Pylint needs a lot of hand holding. Every distro upgrade you'll hit new issues, want to exclude more checks etc. I never looked into how projects use it for CI gating, seems kinda exhausting unless you only enable specific checks. - Cole
Re: [libvirt PATCH 02/12] tools: support validating SEV firmware boot measurements
On 10/20/22 8:11 AM, Cole Robinson wrote:
> On 10/18/22 5:15 AM, Daniel P. Berrangé wrote:
>> On Sun, Oct 16, 2022 at 02:54:47PM -0400, Cole Robinson wrote:
>>> On 10/7/22 7:42 AM, Daniel P. Berrangé wrote:
>>>> The virt-qemu-sev-validate program will compare a reported SEV/SEV-ES
>>>> domain launch measurement, to a computed launch measurement. This
>>>> determines whether the domain has been tampered with during launch.
>>>>
>>>> This initial implementation requires all inputs to be provided
>>>> explicitly, and as such can run completely offline, without any
>>>> connection to libvirt.
>>>>
>>>> The tool is placed in the libvirt-client-qemu sub-RPM since it is
>>>> specific to the QEMU driver.
>>>>
>>>> Signed-off-by: Daniel P. Berrangé
>>>
>>>> +try:
>>>> +check_usage(args)
>>>> +
>>>> +attest(args)
>>>> +
>>>> +sys.exit(0)
>>>> +except AttestationFailedException as e:
>>>> +if not args.quiet:
>>>> +print("ERROR: %s" % e, file=sys.stderr)
>>>> +sys.exit(1)
>>>> +except UnsupportedUsageException as e:
>>>> +if not args.quiet:
>>>> +print("ERROR: %s" % e, file=sys.stderr)
>>>> +sys.exit(2)
>>>> +except Exception as e:
>>>> +if args.debug:
>>>> +traceback.print_tb(e.__traceback__)
>>>> +if not args.quiet:
>>>> +print("ERROR: %s" % e, file=sys.stderr)
>>>> +sys.exit(3)
>>>
>>> This only tracebacks on --debug for an unexpected error. I think it's
>>> more useful to have --debug always print backtrace. It helped me
>>> debugging usage of the script
>>
>> Ok, I can do that.
>>
>> Do you recall what sort of problems required you to be looking at
>> the debug output ? Wondering if there's anything we can do to make
>> it more foolproof for less knowledgable users ?
>>
>
> I was running the script from git, but against an older running libvirtd
> which did not support the cpu XML, and the error didn't call
> that out specifically. I thought about suggesting an explicit error for
> that case but I think it's unlikely to happen in the real world.
>
Hmm I see now that I did actually suggest this elsewhere :P
- Cole
Re: [libvirt PATCH 02/12] tools: support validating SEV firmware boot measurements
On 10/18/22 5:15 AM, Daniel P. Berrangé wrote:
> On Sun, Oct 16, 2022 at 02:54:47PM -0400, Cole Robinson wrote:
>> On 10/7/22 7:42 AM, Daniel P. Berrangé wrote:
>>> The virt-qemu-sev-validate program will compare a reported SEV/SEV-ES
>>> domain launch measurement, to a computed launch measurement. This
>>> determines whether the domain has been tampered with during launch.
>>>
>>> This initial implementation requires all inputs to be provided
>>> explicitly, and as such can run completely offline, without any
>>> connection to libvirt.
>>>
>>> The tool is placed in the libvirt-client-qemu sub-RPM since it is
>>> specific to the QEMU driver.
>>>
>>> Signed-off-by: Daniel P. Berrangé
>>
>>> +try:
>>> +check_usage(args)
>>> +
>>> +attest(args)
>>> +
>>> +sys.exit(0)
>>> +except AttestationFailedException as e:
>>> +if not args.quiet:
>>> +print("ERROR: %s" % e, file=sys.stderr)
>>> +sys.exit(1)
>>> +except UnsupportedUsageException as e:
>>> +if not args.quiet:
>>> +print("ERROR: %s" % e, file=sys.stderr)
>>> +sys.exit(2)
>>> +except Exception as e:
>>> +if args.debug:
>>> +traceback.print_tb(e.__traceback__)
>>> +if not args.quiet:
>>> +print("ERROR: %s" % e, file=sys.stderr)
>>> +sys.exit(3)
>>
>> This only tracebacks on --debug for an unexpected error. I think it's
>> more useful to have --debug always print backtrace. It helped me
>> debugging usage of the script
>
> Ok, I can do that.
>
> Do you recall what sort of problems required you to be looking at
> the debug output ? Wondering if there's anything we can do to make
> it more foolproof for less knowledgable users ?
>
I was running the script from git, but against an older running libvirtd
which did not support the cpu XML, and the error didn't call
that out specifically. I thought about suggesting an explicit error for
that case but I think it's unlikely to happen in the real world.
- Cole
[PATCH 0/3] test: Fix regression parsing nested XML
This adds a couple extensions to the example/ testdriver XML, and fixes a regression introduced in b3e33a0ef7e Cole Robinson (3): examples: testdriver: Add xmlns runstate example examples: testdriver: Add a nested inline example test: Fix parsing nested XML examples/xml/test/testnodeinline.xml | 25 - src/test/test_driver.c | 2 +- 2 files changed, 25 insertions(+), 2 deletions(-) -- 2.37.3
[PATCH 3/3] test: Fix parsing nested XML
Reproducer:
./build/tools/virsh \
--connect test:///`pwd`/examples/xml/test/testnodeinline.xml \
vol-list default-pool
Fixes: b3e33a0ef7e62169175280c647aa9ac361bd554d
Signed-off-by: Cole Robinson
---
src/test/test_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 8675f8ad07..67c70de11d 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1152,7 +1152,7 @@ testOpenVolumesForPool(const char *file,
g_autofree xmlNodePtr *nodes = NULL;
g_autoptr(virStorageVolDef) volDef = NULL;
-num = virXPathNodeSet("/pool/volume", ctxt, );
+num = virXPathNodeSet("./volume", ctxt, );
if (num < 0)
return -1;
--
2.37.3
[PATCH 2/3] examples: testdriver: Add a nested inline example
Signed-off-by: Cole Robinson --- examples/xml/test/testnodeinline.xml | 22 ++ 1 file changed, 22 insertions(+) diff --git a/examples/xml/test/testnodeinline.xml b/examples/xml/test/testnodeinline.xml index 90805f025a..4657ecadd2 100644 --- a/examples/xml/test/testnodeinline.xml +++ b/examples/xml/test/testnodeinline.xml @@ -177,6 +177,28 @@ + + default-pool + 35bb2ad9-388a-cdfe-461a-b8907f6e53fe + 107374182400 + 0 + 107374182400 + +/default-pool + + 0700 + 10736 + 10736 + + + + +default-vol +100 +5 + + + 6000 -- 2.37.3
[PATCH 1/3] examples: testdriver: Add xmlns runstate example
The testdriver has xmlns support for overriding object default state. demo it by pausing a VM Signed-off-by: Cole Robinson --- examples/xml/test/testnodeinline.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/examples/xml/test/testnodeinline.xml b/examples/xml/test/testnodeinline.xml index 9165d9302d..90805f025a 100644 --- a/examples/xml/test/testnodeinline.xml +++ b/examples/xml/test/testnodeinline.xml @@ -86,7 +86,8 @@ - + +3 fc5 08721f993d1d4aec96eb97803297bb36 -- 2.37.3
[PATCH] qemu: Report sev measurement value and nonce explicitly
The value returned by qemu's query-sev-launch-measure comes
straight from the LAUNCH_MEASURE SEV firmware command. It's two
values packed together: first 32 bytes is the launch measurement,
last 16 bytes is the nonce.
This combined value is really just an artifact of the return value
of the firmware command, it has no direct usage. Users want the two
individual values. But because qemu and libvirt do not separate them
apart, every app that wants to process this value will have to do
it manually.
This performs the split for the user, and delivers the values in two
new TYPED_PARAM fields: sev-measurement-value, sev-measurement-nonce
Signed-off-by: Cole Robinson
---
include/libvirt/libvirt-domain.h | 22 ++
src/qemu/qemu_driver.c | 23 +++
2 files changed, 45 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 8357aea797..55723ba150 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -6317,6 +6317,28 @@ int virDomainSetLifecycleAction(virDomainPtr domain,
*/
# define VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS
"sev-secret-set-address"
+/**
+ * VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT_VALUE:
+ *
+ * Macro represents the measurement value of the SEV guest,
+ * extracted from the compound VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT
value,
+ * as VIR_TYPED_PARAM_STRING.
+ *
+ * Since: 8.9.0
+ */
+# define VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT_VALUE
"sev-measurement-value"
+
+/**
+ * VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT_NONCE:
+ *
+ * Macro represents the measurement nonce of the SEV guest,
+ * extracted from the compound VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT
value,
+ * as VIR_TYPED_PARAM_STRING.
+ *
+ * Since: 8.9.0
+ */
+# define VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT_NONCE
"sev-measurement-nonce"
+
int virDomainGetLaunchSecurityInfo(virDomainPtr domain,
virTypedParameterPtr *params,
int *nparams,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 40d23b5723..590e8f3fab 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19951,10 +19951,14 @@ qemuDomainGetSEVInfo(virDomainObj *vm,
int ret = -1;
int rv;
g_autofree char *tmp = NULL;
+g_autofree char *measurement = NULL;
+g_autofree char *measurement_val = NULL;
+g_autofree char *nonce = NULL;
unsigned int apiMajor = 0;
unsigned int apiMinor = 0;
unsigned int buildID = 0;
unsigned int policy = 0;
+size_t measurement_size = 0;
int maxpar = 0;
virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
@@ -19982,6 +19986,17 @@ qemuDomainGetSEVInfo(virDomainObj *vm,
if (rv < 0)
goto endjob;
+measurement = (char *) g_base64_decode(tmp, _size);
+if (measurement_size != 48) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected SEV measurement size %zu, expected 48"),
+ measurement_size);
+goto endjob;
+}
+
+measurement_val = g_base64_encode((unsigned char *) measurement, 32);
+nonce = g_base64_encode((unsigned char *) measurement + 32, 16);
+
if (virTypedParamsAddString(params, nparams, ,
VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT,
tmp) < 0)
@@ -20002,6 +20017,14 @@ qemuDomainGetSEVInfo(virDomainObj *vm,
VIR_DOMAIN_LAUNCH_SECURITY_SEV_POLICY,
policy) < 0)
goto endjob;
+if (virTypedParamsAddString(params, nparams, ,
+
VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT_VALUE,
+measurement_val) < 0)
+goto endjob;
+if (virTypedParamsAddString(params, nparams, ,
+
VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT_NONCE,
+nonce) < 0)
+goto endjob;
ret = 0;
--
2.37.3
Re: [libvirt PATCH 12/12] docs/manpages: add checklist of problems for SEV attestation
On 10/7/22 7:43 AM, Daniel P. Berrangé wrote: > Despite efforts to make the virt-qemu-sev-validate tool friendly, it is > a certainty that almost everyone who tries it will hit false negative > results, getting a failure despite the VM being trustworthy. > > Diagnosing these problems is no easy matter, especially for those not > familiar with SEV/SEV-ES in general. This extra docs text attempts to > set out a checklist of items to look at to identify what went wrong. > > Signed-off-by: Daniel P. Berrangé > --- > docs/manpages/virt-qemu-sev-validate.rst | 112 +++ > 1 file changed, 112 insertions(+) > > diff --git a/docs/manpages/virt-qemu-sev-validate.rst > b/docs/manpages/virt-qemu-sev-validate.rst > index 7542bea9aa..e0c18f2d20 100644 > --- a/docs/manpages/virt-qemu-sev-validate.rst > +++ b/docs/manpages/virt-qemu-sev-validate.rst > @@ -437,6 +437,118 @@ inject a disk password on success: > --domain fedora34x86_64 \ > --disk-password passwd.txt > > +COMMON MISTAKES CHECKLIST > += > + > +The complexity of configuring a guest and validating its boot measurement > +means it is very likely to see the failure:: > + > + ERROR: Measurement does not match, VM is not trustworthy > + > +This error message assumes the worst, but in most cases will failure will be > +a result of either mis-configuring the guest, or passing the wrong > information > +when trying to validate it. The following information is a guide for what > +items to check in order to stand the best chance of diagnosing the problem > + > +* Check the VM configuration for the DH certificate and session > + blob in the libvirt guest XML. > + > + The content for these fields should be in base64 format, which is > + what ``sevctl session`` generates. Other tools may generate the files > + in binary format, so ensure it has been correctly converted to base64. > + > +* Check the VM configuration policy value matches the session blob > + > + The value in libvirt guest XML has to match the value > + passed to the ``sevctl session`` command. > + FWIW In this case, qemu will explicitly error. From 7.0.0-6.fc36: -accel kvm: sev_launch_start: LAUNCH_START ret=1 fw_error=11 'Bad measurement' I think it's worth putting some subset of that qemu error string at the top of this section too. If users hit it, going through the checklist here may solve their issue. For example, If you're flailing around with sevctl like I have, some of the sub commands will invalidate all your previous generated session/dhCert blobs, and subsequent VM boots will fail as above. `sevctl reset` and/or `sevctl rotate`. That's probably obscure enough to not need documenting, but if the first item here leads to re-running sevctl session then you'll fix your problem :) Thanks, Cole
Re: [libvirt PATCH 08/12] tools: load CPU count and CPU SKU from libvirt
On 10/7/22 7:43 AM, Daniel P. Berrangé wrote:
> When validating a SEV-ES guest, we need to know the CPU count and VMSA
> state. We can get the CPU count directly from libvirt's guest info. The
> VMSA state can be constructed automatically if we query the CPU SKU from
> host capabilities XML. Neither of these is secure, however, so this
> behaviour is restricted.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> docs/manpages/virt-qemu-sev-validate.rst | 4
> tools/virt-qemu-sev-validate.py | 23 +++
> 2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/docs/manpages/virt-qemu-sev-validate.rst
> b/docs/manpages/virt-qemu-sev-validate.rst
> index 7ba7323e13..fcc13d68c8 100644
> --- a/docs/manpages/virt-qemu-sev-validate.rst
> +++ b/docs/manpages/virt-qemu-sev-validate.rst
> @@ -356,7 +356,6 @@ Validate the measurement of a SEV-ES SMP guest booting
> from disk:
>
> # virt-dom-sev-validate \
> --insecure \
> - --num-cpus 2 \
> --vmsa-cpu0 vmsa0.bin \
> --vmsa-cpu1 vmsa1.bin \
> --tk this-guest-tk.bin \
> @@ -369,9 +368,6 @@ automatically constructed VMSA:
>
> # virt-dom-sev-validate \
> --insecure \
> - --cpu-family 23 \
> - --cpu-model 49 \
> - --cpu-stepping 0 \
> --tk this-guest-tk.bin \
> --domain fedora34x86_64
>
> diff --git a/tools/virt-qemu-sev-validate.py b/tools/virt-qemu-sev-validate.py
> index 2505aff07f..5da1353e60 100755
> --- a/tools/virt-qemu-sev-validate.py
> +++ b/tools/virt-qemu-sev-validate.py
> @@ -869,6 +869,14 @@ class LibvirtConfidentialVM(ConfidentialVM):
> if self.policy is None:
> self.policy = sevinfo["sev-policy"]
>
> +if self.is_sev_es() and self.num_cpus is None:
> +if secure:
> +raise InsecureUsageException(
> +"Using CPU count from guest is not secure")
> +
> +info = self.dom.info()
> +self.num_cpus = info[3]
> +
> if self.firmware is None:
> if remote:
> raise UnsupportedUsageException(
> @@ -914,6 +922,21 @@ class LibvirtConfidentialVM(ConfidentialVM):
> "Using cmdline string from XML is not secure")
> self.kernel_table.load_cmdline(cmdlinenodes[0].text)
>
> +capsxml = self.conn.getCapabilities()
> +capsdoc = etree.fromstring(capsxml)
> +
> +if self.is_sev_es() and self.vmsa_cpu0 is None:
> +if secure:
> +raise InsecureUsageException(
> +"Using CPU SKU from capabilities is not secure")
> +
> +sig = capsdoc.xpath("/capabilities/host/cpu/signature")
> +if len(sig) == 1:
If this is missing, I'd make it fatal, libvirtd isn't new enough. Could
happen if talking to a remote machine (or testing the script while f36
fedora libvirtd is running, which I did :) ) . It's going to fail later
anyways.
- Cole
> +cpu_family = int(sig[0].get("family"))
> +cpu_model = int(sig[0].get("model"))
> +cpu_stepping = int(sig[0].get("stepping"))
> +self.build_vmsas(cpu_family, cpu_model, cpu_stepping)
> +
>
> def parse_command_line():
> parser = argparse.ArgumentParser(
Re: [libvirt PATCH 00/12] tools: provide virt-qemu-sev-validate for SEV(-ES) launch attestation
On 10/7/22 7:42 AM, Daniel P. Berrangé wrote: > The libvirt QEMU driver provides all the functionality required for > launching a guest on AMD SEV(-ES) platforms, with a configuration > that enables attestation of the launch measurement. The documentation > for how to actually perform an attestation is severely lacking and > not suitable for mere mortals to understand. IOW, someone trying to > implement attestation is in for a world of pain and suffering. > > This series doesn't fix the documentation problem, but it does > provide a reference implementation of a tool for performing > attestation of SEV(-ES) guests in the context of libvirt / KVM. > > There will be other tools and libraries that implement attestation > logic too, but this tool is likely somewhat unique in its usage of > libvirt. Now for a attestation to be trustworthy you don't want to > perform it on the hypervisor host, since the goal is to prove that > the hypervisor has not acted maliciously. None the less it is still > beneficial to have libvirt integration to some extent. > > When running this tool on a remote (trusted) host, it can connect > to the libvirt hypervisor and fetch the data provided by the > virDomainLaunchSecurityInfo API, which is safe to trust as the > key pieces are cryptographically measured. > > Attestation is a complex problem though and it is very easy to > screw up and feed the wrong information and then waste hours trying > to figure out what piece was wrong, to cause the hash digest to > change. For debugging such problems, you can thus tell the tool > to operate insecurely, by querying libvirt for almost all of the > configuration information required to determine the expected > measurement. By comparing these results,to the results obtained > in offline mode it helps narrow down where the mistake lies. > > So I view this tool as being useful in a number of ways: > > * Quality assurance engineers needing to test libvirt/QEMU/KVM >get a simple and reliable tool for automating tests with. > > * Users running simple libvirt deployments without any large >management stack, get a standalone tool for attestation >they can rely on. > > * Developers writing/integrating attestation support into >management stacks above libvirt, get a reference against >which they can debug their own tools. > > * Users wanting to demonstrate the core SEV/SEV-ES functionality >get a simple and reliable tool to illustrate the core concepts >involved. > > Since I didn't fancy writing such complex logic in C, this tool is > a python3 program. As such, we don't want to include it in the > main libvirt-client RPM, nor any other existing RPM. THus, this > series puts it in a new libvirt-client-qemu RPM which, through no > co-inicidence at all, is the same RPM I invented a few days ago to > hold the virt-qemu-qmp-proxy command. > > Note, people will have already seen an earlier version of this > tool I hacked up some months ago. This code is very significantly > changed since that earlier version, to make it more maintainable, > and simpler to use (especially for SEV-ES) but the general theme > is still the same. > > Daniel P. Berrangé (12): > build-aux: only forbid gethostname in C files > tools: support validating SEV firmware boot measurements > tools: load guest config from libvirt > tools: support validating SEV direct kernel boot measurements > tools: load direct kernel config from libvirt > tools: support validating SEV-ES initial vCPU state measurements > tools: support automatically constructing SEV-ES vCPU state > tools: load CPU count and CPU SKU from libvirt > tools: support generating SEV secret injection tables > docs/kbase: describe attestation for SEV guests > scripts: add systemtap script for capturing SEV-ES VMSA > docs/manpages: add checklist of problems for SEV attestation After the fix I mentioned on patch 7, the script worked for my sev, sev-es, sev-es + kernel setup, with vmsa passed in and vmsa generated. Attached is some pylint output, nothing really serious: - Cole * Module virt-qemu-sev-validate tools/virt-qemu-sev-validate.py:88:35: W0622: Redefining built-in 'format' (redefined-builtin) tools/virt-qemu-sev-validate.py:101:41: W0622: Redefining built-in 'format' (redefined-builtin) tools/virt-qemu-sev-validate.py:151:16: C0200: Consider using enumerate instead of iterating with range and len (consider-using-enumerate) tools/virt-qemu-sev-validate.py:351:8: W0201: Attribute 'cr0' defined outside __init__ (attribute-defined-outside-init) tools/virt-qemu-sev-validate.py:352:8: W0201: Attribute 'rip' defined outside __init__ (attribute-defined-outside-init) tools/virt-qemu-sev-validate.py:354:8: W0201: Attribute 'cs_selector' defined outside __init__ (attribute-defined-outside-init) tools/virt-qemu-sev-validate.py:355:8: W0201: Attribute 'cs_base' defined outside __init__ (attribute-defined-outside-init)
Re: [libvirt PATCH 07/12] tools: support automatically constructing SEV-ES vCPU state
On 10/7/22 7:43 AM, Daniel P. Berrangé wrote:
> The VMSA files contain the expected CPU register state for the VM. Their
> content varies based on a few pieces of the stack
>
> - AMD CPU architectural initial state
> - KVM hypervisor VM CPU initialization
> - QEMU userspace VM CPU initialization
> - AMD CPU SKU (family/model/stepping)
>
> The first three pieces of information we can obtain through code
> inspection. The last piece of information we can take on the command
> line. This allows a user to validate a SEV-ES guest merely by providing
> the CPU SKU information, using --cpu-family, --cpu-model,
> --cpu-stepping. This avoids the need to obtain or construct VMSA files
> directly.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> docs/manpages/virt-qemu-sev-validate.rst | 45 +++
> tools/virt-qemu-sev-validate.py | 475 +++
> 2 files changed, 520 insertions(+)
>
> diff --git a/docs/manpages/virt-qemu-sev-validate.rst
> b/docs/manpages/virt-qemu-sev-validate.rst
> index 24bca98d28..7ba7323e13 100644
> --- a/docs/manpages/virt-qemu-sev-validate.rst
> +++ b/docs/manpages/virt-qemu-sev-validate.rst
> @@ -243,6 +243,24 @@ Validate the measurement of a SEV-ES SMP guest booting
> from disk:
> --build-id 13 \
> --policy 7
>
> +Validate the measurement of a SEV-ES SMP guest booting from disk, with
> +automatically constructed VMSA:
> +
> +::
> +
> + # virt-dom-sev-validate \
> + --firmware OVMF.sev.fd \
> + --num-cpus 2 \
> + --cpu-family 23 \
> + --cpu-model 49 \
> + --cpu-stepping 0 \
> + --tk this-guest-tk.bin \
> + --measurement
> Zs2pf19ubFSafpZ2WKkwquXvACx9Wt/BV+eJwQ/taO8jhyIj/F8swFrybR1fZ2ID \
> + --api-major 0 \
> + --api-minor 24 \
> + --build-id 13 \
> + --policy 7
> +
> Fetch from remote libvirt
> -
>
> @@ -289,6 +307,20 @@ Validate the measurement of a SEV-ES SMP guest booting
> from disk:
> --tk this-guest-tk.bin \
> --domain fedora34x86_64
>
> +Validate the measurement of a SEV-ES SMP guest booting from disk, with
> +automatically constructed VMSA:
> +
> +::
> +
> + # virt-dom-sev-validate \
> + --connect qemu+ssh://r...@some.remote.host/system \
> + --firmware OVMF.sev.fd \
> + --cpu-family 23 \
> + --cpu-model 49 \
> + --cpu-stepping 0 \
> + --tk this-guest-tk.bin \
> + --domain fedora34x86_64
> +
> Fetch from local libvirt
>
>
> @@ -330,6 +362,19 @@ Validate the measurement of a SEV-ES SMP guest booting
> from disk:
> --tk this-guest-tk.bin \
> --domain fedora34x86_64
>
> +Validate the measurement of a SEV-ES SMP guest booting from disk, with
> +automatically constructed VMSA:
> +
> +::
> +
> + # virt-dom-sev-validate \
> + --insecure \
> + --cpu-family 23 \
> + --cpu-model 49 \
> + --cpu-stepping 0 \
> + --tk this-guest-tk.bin \
> + --domain fedora34x86_64
> +
> EXIT STATUS
> ===
>
> diff --git a/tools/virt-qemu-sev-validate.py b/tools/virt-qemu-sev-validate.py
> index ea5be80129..2505aff07f 100755
> --- a/tools/virt-qemu-sev-validate.py
> +++ b/tools/virt-qemu-sev-validate.py
> @@ -44,6 +44,7 @@ import logging
> from lxml import etree
> import re
> import socket
> +from struct import pack
> import sys
> import traceback
> from uuid import UUID
> @@ -71,6 +72,435 @@ class InvalidStateException(Exception):
> pass
>
>
> +class Field(object):
> +U8 = 0
> +U16 = 2
> +U32 = 4
> +U64 = 8
> +
> +SCALAR = 0
> +BITMASK = 1
> +ARRAY = 2
> +
> +def __init__(self, name, size, format, value, order):
> +self.name = name
> +self.size = size
> +self.value = value
> +self.format = format
> +self.order = order
> +
> +
> +class Struct(object):
> +def __init__(self, size):
> +self._fields = {}
> +self.size = size
> +
> +def register_field(self, name, size, format=Field.SCALAR, defvalue=0):
> +self._fields[name] = Field(name, size, format,
> + defvalue, len(self.fields))
> +
> +@property
> +def fields(self):
> +return sorted(self._fields.values(), key=lambda f: f.order)
> +
> +def __getattr__(self, name):
> +return self._fields[name]
> +
> +def __setattr__(self, name, value):
> +if name in ["_fields", "size"]:
> +super().__setattr__(name, value)
> +else:
> +self._fields[name].value = value
> +
> +def binary_format(self):
> +fmt = ["<"]
> +datalen = 0
> +for field in self.fields:
> +if field.size == Field.U8:
> +if field.format == Field.ARRAY:
> +datalen += len(field.value)
> +fmt += ["%dB" % len(field.value)]
> +else:
> +datalen += 1
> +
Re: [libvirt PATCH 02/12] tools: support validating SEV firmware boot measurements
On 10/7/22 7:42 AM, Daniel P. Berrangé wrote:
> The virt-qemu-sev-validate program will compare a reported SEV/SEV-ES
> domain launch measurement, to a computed launch measurement. This
> determines whether the domain has been tampered with during launch.
>
> This initial implementation requires all inputs to be provided
> explicitly, and as such can run completely offline, without any
> connection to libvirt.
>
> The tool is placed in the libvirt-client-qemu sub-RPM since it is
> specific to the QEMU driver.
>
> Signed-off-by: Daniel P. Berrangé
> +try:
> +check_usage(args)
> +
> +attest(args)
> +
> +sys.exit(0)
> +except AttestationFailedException as e:
> +if not args.quiet:
> +print("ERROR: %s" % e, file=sys.stderr)
> +sys.exit(1)
> +except UnsupportedUsageException as e:
> +if not args.quiet:
> +print("ERROR: %s" % e, file=sys.stderr)
> +sys.exit(2)
> +except Exception as e:
> +if args.debug:
> +traceback.print_tb(e.__traceback__)
> +if not args.quiet:
> +print("ERROR: %s" % e, file=sys.stderr)
> +sys.exit(3)
This only tracebacks on --debug for an unexpected error. I think it's
more useful to have --debug always print backtrace. It helped me
debugging usage of the script
Thanks,
Cole
[PATCH] spec: change gettext requires to gettext-runtime for F37
From: Jens Petersen
See https://fedoraproject.org/wiki/Changes/GettextRuntimeSubpackage
Signed-off-by: Cole Robinson
---
libvirt.spec.in | 4
1 file changed, 4 insertions(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 5408aa3154..d946a8da48 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -471,7 +471,11 @@ Requires: dbus
# For uid creation during pre
Requires(pre): shadow-utils
# Needed by /usr/libexec/libvirt-guests.sh script.
+%if 0%{?fedora} >= 37
+Requires: gettext-runtime
+%else
Requires: gettext
+%endif
# Ensure smooth upgrades
Obsoletes: libvirt-admin < 7.3.0
--
2.37.3
Re: [PATCH 2/2] virfile: Fix build with glibc 2.36
On 8/2/22 3:12 AM, Erik Skultety wrote:
> On Mon, Aug 01, 2022 at 03:59:15PM -0400, Cole Robinson wrote:
>> With glibc 2.36, sys/mount.h and linux/mount.h conflict:
>> https://sourceware.org/glibc/wiki/Release/2.36#Usage_of_.3Clinux.2Fmount.h.3E_and_.3Csys.2Fmount.h.3E
>>
>> virfile.c imports sys/mount.h and linux/fs.h, which pulls in
>> linux/mount.h.
>>
>> Manually define the constants we need from linux/fs.h, like was
>> done in llvm:
>>
>> https://reviews.llvm.org/rGb379129c4beb3f26223288627a1291739f33af02
>>
>> Signed-off-by: Cole Robinson
>> ---
>> src/util/virfile.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/util/virfile.c b/src/util/virfile.c
>> index 99da058db3..65d6d2a701 100644
>> --- a/src/util/virfile.c
>> +++ b/src/util/virfile.c
>> @@ -71,7 +71,9 @@
>> # endif
>> # include
>> # include
>> -# include
>
> The commit message does explain the issue, but I'd still appreciate if there
> was a short commentary explaining this explicit constant definition here as
> well.
>
Done.
>> +# define FS_IOC_GETFLAGS _IOR('f', 1, long)
>> +# define FS_IOC_SETFLAGS _IOR('f', 2, long)
>
> ^this one has to be defined as FS_IOC_SETFLAGS _IOW('f', 2, long)
>
Darn, nice catch!
Here's the CI pipeline:
https://gitlab.com/crobinso/libvirt/-/pipelines/602982400
It passed, so I pushed with your suggested changes.
Thanks,
Cole
[PATCH 2/2] virfile: Fix build with glibc 2.36
With glibc 2.36, sys/mount.h and linux/mount.h conflict:
https://sourceware.org/glibc/wiki/Release/2.36#Usage_of_.3Clinux.2Fmount.h.3E_and_.3Csys.2Fmount.h.3E
virfile.c imports sys/mount.h and linux/fs.h, which pulls in
linux/mount.h.
Manually define the constants we need from linux/fs.h, like was
done in llvm:
https://reviews.llvm.org/rGb379129c4beb3f26223288627a1291739f33af02
Signed-off-by: Cole Robinson
---
src/util/virfile.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 99da058db3..65d6d2a701 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -71,7 +71,9 @@
# endif
# include
# include
-# include
+# define FS_IOC_GETFLAGS _IOR('f', 1, long)
+# define FS_IOC_SETFLAGS _IOR('f', 2, long)
+# define FS_NOCOW_FL 0x0080
#endif
#if WITH_LIBATTR
--
2.36.1
[PATCH 0/2] Fix build with glibc 2.36
With glibc 2.36, sys/mount.h and linux/mount.h conflict: https://sourceware.org/glibc/wiki/Release/2.36#Usage_of_.3Clinux.2Fmount.h.3E_and_.3Csys.2Fmount.h.3E Easiest way to run this through CI? Submit MR to libvirt gitlab? Cole Robinson (2): lxc: containter: fix build with glibc 2.36 virfile: Fix build with glibc 2.36 src/lxc/lxc_container.c | 3 --- src/util/virfile.c | 4 +++- 2 files changed, 3 insertions(+), 4 deletions(-) -- 2.36.1
[PATCH 1/2] lxc: containter: fix build with glibc 2.36
With glibc 2.36, sys/mount.h and linux/mount.h conflict: https://sourceware.org/glibc/wiki/Release/2.36#Usage_of_.3Clinux.2Fmount.h.3E_and_.3Csys.2Fmount.h.3E lxc_container.c imports sys/mount.h and linux/fs.h, which pulls in linux/mount.h. linux/fs.h isn't required here though. glibc sys/mount.h has had MS_MOVE since 2.12 in 2010 Signed-off-by: Cole Robinson --- src/lxc/lxc_container.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index b5278831da..a5401c2186 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -33,9 +33,6 @@ /* Yes, we want linux private one, for _syscall2() macro */ #include -/* For MS_MOVE */ -#include - #if WITH_CAPNG # include #endif -- 2.36.1
Re: [PATCH] libxl: Fix domain startup failure error reporting
On 6/21/22 3:55 AM, Michal Prívozník wrote:
> On 6/17/22 23:29, Cole Robinson wrote:
>> When domain startup fails, domain cleanup calls
>> libxlNetworkUnwindDevices, which calls virGetConnectNetwork, which
>> is a top level API entry point, which resets the initial saved error,
>> leading to clients seeing:
>>
>> error: An error occurred, but the cause is unknown
>>
>> This preserves the error from before virGetConnectNetwork is called.
>>
>> Signed-off-by: Cole Robinson
>> ---
>> src/libxl/libxl_domain.c | 7 ++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>> index 17b347de4e..bda110e9e6 100644
>> --- a/src/libxl/libxl_domain.c
>> +++ b/src/libxl/libxl_domain.c
>> @@ -830,12 +830,17 @@ libxlNetworkUnwindDevices(virDomainDef *def)
>> /* cleanup actual device */
>> virDomainNetRemoveHostdev(def, net);
>> if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
>> -g_autoptr(virConnect) conn = virGetConnectNetwork();
>> +g_autoptr(virConnect) conn = NULL;
>> +virErrorPtr save_err;
>> +
>> +virErrorPreserveLast(_err);
>> +conn = virGetConnectNetwork();
>>
>> if (conn)
>> virDomainNetReleaseActualDevice(conn, def, net);
>> else
>> VIR_WARN("Unable to release network device '%s'",
>> NULLSTR(net->ifname));
>> +virErrorRestore(_err);
>> }
>> }
>> }
>
> This fixes this particular function. I wonder whether we should mimic
> what QEMU driver does and wrap whole qemuProcessShutdown(), I mean
> libxlDomainCleanup() in virErrorPreserveLast(). Something like this:
>
> diff --git i/src/libxl/libxl_domain.c w/src/libxl/libxl_domain.c
> index bda110e9e6..8e8ddd284a 100644
> --- i/src/libxl/libxl_domain.c
> +++ w/src/libxl/libxl_domain.c
> @@ -908,10 +908,13 @@ libxlDomainCleanup(libxlDriverPrivate *driver,
> virHostdevManager *hostdev_mgr = driver->hostdevMgr;
> unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI;
> size_t i;
> +virErrorPtr save_err;
>
> VIR_DEBUG("Cleaning up domain with id '%d' and name '%s'",
>vm->def->id, vm->def->name);
>
> +virErrorPreserveLast(_err);
> +
> hostdev_flags |= VIR_HOSTDEV_SP_USB;
>
> /* Call hook with stopped operation. Ignore error and continue with
> cleanup */
> @@ -984,6 +987,7 @@ libxlDomainCleanup(libxlDriverPrivate *driver,
> VIR_HOOK_SUBOP_END, NULL));
>
> virDomainObjRemoveTransientDef(vm);
> +virErrorRestore(_err);
> }
>
> /*
> @@ -1245,6 +1249,7 @@ libxlDomainStartPrepare(libxlDriverPrivate *driver,
> {
> virHostdevManager *hostdev_mgr = driver->hostdevMgr;
> unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI | VIR_HOSTDEV_SP_USB;
> +virErrorPtr save_err;
>
> if (virDomainObjSetDefTransient(driver->xmlopt, vm, NULL) < 0)
> return -1;
> @@ -1272,10 +1277,12 @@ libxlDomainStartPrepare(libxlDriverPrivate *driver,
> return 0;
>
> error:
> +virErrorPreserveLast(_err);
> libxlNetworkUnwindDevices(vm->def);
> virHostdevReAttachDomainDevices(hostdev_mgr, LIBXL_DRIVER_INTERNAL_NAME,
> vm->def, hostdev_flags);
> virDomainObjRemoveTransientDef(vm);
> +virErrorRestore(_err);
> return -1;
> }
>
>
> If this works, replace your patch with this diff, apply my:
>
> Reviewed-by: Michal Privoznik
Thanks, I made that change and pushed now
- Cole
Re: [PATCH 4/4] qemu: validate: use domcaps for tpm validation
On 6/21/22 4:11 AM, Michal Prívozník wrote:
> On 6/18/22 20:32, Cole Robinson wrote:
>> Replace tpm->type and tpm->model qemuCaps validation with the
>> similar logic in domcaps.
>>
>> Signed-off-by: Cole Robinson
>> ---
>> src/qemu/qemu_validate.c | 71 ++--
>> 1 file changed, 17 insertions(+), 54 deletions(-)
>>
>> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
>> index db47fcaa9c..39210ba65b 100644
>> --- a/src/qemu/qemu_validate.c
>> +++ b/src/qemu/qemu_validate.c
>> @@ -4750,7 +4750,7 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
>> const virDomainDef *def,
>> virQEMUCaps *qemuCaps)
>> {
>> -virQEMUCapsFlags flag;
>> +virDomainCapsDeviceTPM tpmCaps = { 0 };
>>
>> switch (tpm->version) {
>> case VIR_DOMAIN_TPM_VERSION_1_2:
>> @@ -4781,57 +4781,28 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
>> break;
>> }
>>
>> -switch (tpm->type) {
>> -case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
>> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_PASSTHROUGH))
>> -goto no_support;
>> -break;
>> -
>> -case VIR_DOMAIN_TPM_TYPE_EMULATOR:
>> -if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_EMULATOR))
>> -goto no_support;
>> +virQEMUCapsFillDomainDeviceTPMCaps(qemuCaps, );
>>
>> -break;
>> -case VIR_DOMAIN_TPM_TYPE_LAST:
>> -break;
>> +if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.backendModel, tpm->type)) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("The QEMU executable %s does not support TPM "
>> + "backend type %s"),
>> + def->emulator,
>> + virDomainTPMBackendTypeToString(tpm->type));
>
> Whoa, very nice idea! And looking around the file I can see it used
> already. How could this slipped by me? I mean, the more I think about it
> the more possibilities for code deduplication I see. And on the flip
> side - we would be motivated to keep domcaps on the bleeding edge.
>
Heh, it's your code :)
https://listman.redhat.com/archives/libvir-list/2020-November/211844.html
But yes I agree. domcaps can be a detriment to apps if it's not
reliable, and duplicating the qemuCaps checking is always going to lead
to issues like this. It would be nice if we could normalize adding
domcaps coverage for basic qemuCaps validation cases like this.
Thanks,
Cole
Re: [PATCH 1/4] qemu: validate: Drop tpm-tis arch validation
On 6/18/22 2:32 PM, Cole Robinson wrote: > Checking against qemu capabilities should be enough here > > Resolves: https://gitlab.com/libvirt/libvirt/-/issues/321 > Should be https://gitlab.com/libvirt/libvirt/-/issues/329 I've fixed locally
[PATCH 3/4] tests: mock swtpm initialization for all qemu tests
Don't restrict this to domcaps testing only, we will soon
need it for qemu command line validation
Signed-off-by: Cole Robinson
---
tests/domaincapstest.c | 7 ---
tests/testutilsqemu.c | 8
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index da5c629fd4..3b8216a8f6 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -22,7 +22,6 @@
#include "domain_capabilities.h"
#include "virfilewrapper.h"
#include "configmake.h"
-#include "virtpm.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -129,12 +128,6 @@ fillQemuCaps(virDomainCaps *domCaps,
}
-/* Enough to tell capabilities code that swtpm is usable */
-bool virTPMHasSwtpm(void)
-{
-return true;
-}
-
#endif /* WITH_QEMU */
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index d26caba5fb..6dabbaf36a 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -11,6 +11,7 @@
# include "qemu/qemu_capspriv.h"
# include "virstring.h"
# include "virfilecache.h"
+# include "virtpm.h"
# include
# include
@@ -138,6 +139,13 @@ virFindFileInPath(const char *file)
}
+/* Enough to tell capabilities code that swtpm is usable */
+bool virTPMHasSwtpm(void)
+{
+return true;
+}
+
+
virCapsHostNUMA *
virCapabilitiesHostNUMANewHost(void)
{
--
2.36.1
[PATCH 4/4] qemu: validate: use domcaps for tpm validation
Replace tpm->type and tpm->model qemuCaps validation with the
similar logic in domcaps.
Signed-off-by: Cole Robinson
---
src/qemu/qemu_validate.c | 71 ++--
1 file changed, 17 insertions(+), 54 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index db47fcaa9c..39210ba65b 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4750,7 +4750,7 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
const virDomainDef *def,
virQEMUCaps *qemuCaps)
{
-virQEMUCapsFlags flag;
+virDomainCapsDeviceTPM tpmCaps = { 0 };
switch (tpm->version) {
case VIR_DOMAIN_TPM_VERSION_1_2:
@@ -4781,57 +4781,28 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
break;
}
-switch (tpm->type) {
-case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_PASSTHROUGH))
-goto no_support;
-break;
-
-case VIR_DOMAIN_TPM_TYPE_EMULATOR:
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_EMULATOR))
-goto no_support;
+virQEMUCapsFillDomainDeviceTPMCaps(qemuCaps, );
-break;
-case VIR_DOMAIN_TPM_TYPE_LAST:
-break;
+if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.backendModel, tpm->type)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The QEMU executable %s does not support TPM "
+ "backend type %s"),
+ def->emulator,
+ virDomainTPMBackendTypeToString(tpm->type));
+return -1;
}
-switch (tpm->model) {
-case VIR_DOMAIN_TPM_MODEL_TIS:
-flag = QEMU_CAPS_DEVICE_TPM_TIS;
-break;
-case VIR_DOMAIN_TPM_MODEL_CRB:
-flag = QEMU_CAPS_DEVICE_TPM_CRB;
-break;
-case VIR_DOMAIN_TPM_MODEL_SPAPR:
-flag = QEMU_CAPS_DEVICE_TPM_SPAPR;
-break;
-case VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY:
-if (!ARCH_IS_PPC64(def->os.arch)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("TPM Proxy model %s is only available for "
- "PPC64 guests"),
- virDomainTPMModelTypeToString(tpm->model));
-return -1;
-}
-
-/* TPM Proxy devices have 'passthrough' backend */
-if (tpm->type != VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("TPM Proxy model %s requires "
- "'Passthrough' backend"),
-virDomainTPMModelTypeToString(tpm->model));
-}
-
-flag = QEMU_CAPS_DEVICE_SPAPR_TPM_PROXY;
-break;
-case VIR_DOMAIN_TPM_MODEL_LAST:
-default:
-virReportEnumRangeError(virDomainTPMModel, tpm->model);
+if (ARCH_IS_PPC64(def->os.arch) &&
+tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY &&
+tpm->type != VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("TPM Proxy model %s requires "
+ "'Passthrough' backend"),
+virDomainTPMModelTypeToString(tpm->model));
return -1;
}
-if (!virQEMUCapsGet(qemuCaps, flag)) {
+if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.model, tpm->model)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("The QEMU executable %s does not support TPM "
"model %s"),
@@ -4841,14 +4812,6 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
}
return 0;
-
- no_support:
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("The QEMU executable %s does not support TPM "
- "backend type %s"),
- def->emulator,
- virDomainTPMBackendTypeToString(tpm->type));
-return -1;
}
--
2.36.1
[PATCH 2/4] qemu: command: Use correct tpm device for all non-x86
The qemu `tpm-tis` device is an ISA device, so only really applicable
to x86 archs. For all non-x86 archs we should use `tpm-tis-device`
This fixes tpm-tis usage on armv7l and riscv
Signed-off-by: Cole Robinson
---
src/qemu/qemu_command.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 57334ab246..b307d3139c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9731,7 +9731,7 @@ qemuBuildTPMDevCmd(virCommand *cmd,
const char *model = virDomainTPMModelTypeToString(tpm->model);
g_autofree char *tpmdev = g_strdup_printf("tpm-%s", tpm->info.alias);
-if (tpm->model == VIR_DOMAIN_TPM_MODEL_TIS && def->os.arch ==
VIR_ARCH_AARCH64)
+if (tpm->model == VIR_DOMAIN_TPM_MODEL_TIS && !ARCH_IS_X86(def->os.arch))
model = "tpm-tis-device";
if (virJSONValueObjectAdd(,
--
2.36.1
[PATCH 0/4] qemu: Fix tpm-tis for armv7l and riscv
This fixes tpm-tis usage for armv7l and riscv arches, and then switches qemu tpm validation to use domcaps as the source of truth Cole Robinson (4): qemu: validate: Drop tpm-tis arch validation qemu: command: Use correct tpm device for all non-x86 tests: mock swtpm initialization for all qemu tests qemu: validate: use domcaps for tpm validation src/qemu/qemu_command.c | 2 +- src/qemu/qemu_validate.c | 77 +--- tests/domaincapstest.c | 7 tests/testutilsqemu.c| 8 + 4 files changed, 26 insertions(+), 68 deletions(-) -- 2.36.1
[PATCH 1/4] qemu: validate: Drop tpm-tis arch validation
Checking against qemu capabilities should be enough here
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/321
Signed-off-by: Cole Robinson
---
src/qemu/qemu_validate.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index f8aa83c1cb..db47fcaa9c 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4798,12 +4798,6 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
switch (tpm->model) {
case VIR_DOMAIN_TPM_MODEL_TIS:
-if (!ARCH_IS_X86(def->os.arch) && (def->os.arch != VIR_ARCH_AARCH64)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("TPM model '%s' is only available for x86 and
aarch64 guests"),
- virDomainTPMModelTypeToString(tpm->model));
-return -1;
-}
flag = QEMU_CAPS_DEVICE_TPM_TIS;
break;
case VIR_DOMAIN_TPM_MODEL_CRB:
--
2.36.1
[PATCH] libxl: Fix domain startup failure error reporting
When domain startup fails, domain cleanup calls
libxlNetworkUnwindDevices, which calls virGetConnectNetwork, which
is a top level API entry point, which resets the initial saved error,
leading to clients seeing:
error: An error occurred, but the cause is unknown
This preserves the error from before virGetConnectNetwork is called.
Signed-off-by: Cole Robinson
---
src/libxl/libxl_domain.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 17b347de4e..bda110e9e6 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -830,12 +830,17 @@ libxlNetworkUnwindDevices(virDomainDef *def)
/* cleanup actual device */
virDomainNetRemoveHostdev(def, net);
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
-g_autoptr(virConnect) conn = virGetConnectNetwork();
+g_autoptr(virConnect) conn = NULL;
+virErrorPtr save_err;
+
+virErrorPreserveLast(_err);
+conn = virGetConnectNetwork();
if (conn)
virDomainNetReleaseActualDevice(conn, def, net);
else
VIR_WARN("Unable to release network device '%s'",
NULLSTR(net->ifname));
+virErrorRestore(_err);
}
}
}
--
2.36.1
[PATCH v2] conf: cpu: Add
Internally we already collect x86 host family + model + stepping
numeric values. This exposed them in capabilities CPU output.
Example:
$ sudo virsh capabilities | grep -A1 -B1 signature
Users need to know these values to calculate an expected.
SEV-ES/SEV-SNP launch measurement.
Signed-off-by: Cole Robinson
---
v2:
sigXXX naming was kept, since there's a conflicting 'model'
member in the struct already.
Addressed Jirka's review comments
src/conf/cpu_conf.c | 32 +++
src/conf/cpu_conf.h | 3 ++
src/conf/schemas/cputypes.rng | 13
src/cpu/cpu_x86.c | 3 ++
.../x86_64-cpuid-A10-5800K-host.xml | 1 +
.../x86_64-cpuid-Atom-D510-host.xml | 1 +
.../x86_64-cpuid-Atom-N450-host.xml | 1 +
.../x86_64-cpuid-Atom-P5362-host.xml | 1 +
.../x86_64-cpuid-Cooperlake-host.xml | 1 +
.../x86_64-cpuid-Core-i5-2500-host.xml| 1 +
.../x86_64-cpuid-Core-i5-2540M-host.xml | 1 +
.../x86_64-cpuid-Core-i5-4670T-host.xml | 1 +
.../x86_64-cpuid-Core-i5-650-host.xml | 1 +
.../x86_64-cpuid-Core-i5-6600-host.xml| 1 +
.../x86_64-cpuid-Core-i7-2600-host.xml| 1 +
...86_64-cpuid-Core-i7-2600-xsaveopt-host.xml | 1 +
.../x86_64-cpuid-Core-i7-3520M-host.xml | 1 +
.../x86_64-cpuid-Core-i7-3740QM-host.xml | 1 +
.../x86_64-cpuid-Core-i7-3770-host.xml| 1 +
.../x86_64-cpuid-Core-i7-4510U-host.xml | 1 +
.../x86_64-cpuid-Core-i7-4600U-host.xml | 1 +
.../x86_64-cpuid-Core-i7-5600U-arat-host.xml | 1 +
.../x86_64-cpuid-Core-i7-5600U-host.xml | 1 +
.../x86_64-cpuid-Core-i7-5600U-ibrs-host.xml | 1 +
.../x86_64-cpuid-Core-i7-7600U-host.xml | 1 +
.../x86_64-cpuid-Core-i7-7700-host.xml| 1 +
.../x86_64-cpuid-Core-i7-8550U-host.xml | 1 +
.../x86_64-cpuid-Core-i7-8700-host.xml| 1 +
.../x86_64-cpuid-Core2-E6850-host.xml | 1 +
.../x86_64-cpuid-Core2-Q9500-host.xml | 1 +
.../x86_64-cpuid-EPYC-7502-32-Core-host.xml | 1 +
.../x86_64-cpuid-EPYC-7601-32-Core-host.xml | 1 +
...6_64-cpuid-EPYC-7601-32-Core-ibpb-host.xml | 1 +
.../cputestdata/x86_64-cpuid-FX-8150-host.xml | 1 +
...6_64-cpuid-Hygon-C86-7185-32-core-host.xml | 1 +
.../x86_64-cpuid-Ice-Lake-Server-host.xml | 1 +
.../x86_64-cpuid-Opteron-1352-host.xml| 1 +
.../x86_64-cpuid-Opteron-2350-host.xml| 1 +
.../x86_64-cpuid-Opteron-6234-host.xml| 1 +
.../x86_64-cpuid-Opteron-6282-host.xml| 1 +
.../x86_64-cpuid-Pentium-P6100-host.xml | 1 +
.../x86_64-cpuid-Phenom-B95-host.xml | 1 +
...64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml | 1 +
...86_64-cpuid-Ryzen-9-3900X-12-Core-host.xml | 1 +
.../x86_64-cpuid-Xeon-5110-host.xml | 1 +
.../x86_64-cpuid-Xeon-E3-1225-v5-host.xml | 1 +
.../x86_64-cpuid-Xeon-E3-1245-v5-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2609-v3-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2623-v4-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2630-v3-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2630-v4-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2650-host.xml| 1 +
.../x86_64-cpuid-Xeon-E5-2650-v3-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2650-v4-host.xml | 1 +
.../x86_64-cpuid-Xeon-E7-4820-host.xml| 1 +
.../x86_64-cpuid-Xeon-E7-4830-host.xml| 1 +
.../x86_64-cpuid-Xeon-E7-8890-v3-host.xml | 1 +
.../x86_64-cpuid-Xeon-E7540-host.xml | 1 +
.../x86_64-cpuid-Xeon-Gold-5115-host.xml | 1 +
.../x86_64-cpuid-Xeon-Gold-6130-host.xml | 1 +
.../x86_64-cpuid-Xeon-Gold-6148-host.xml | 1 +
.../x86_64-cpuid-Xeon-Platinum-8268-host.xml | 1 +
.../x86_64-cpuid-Xeon-Platinum-9242-host.xml | 1 +
.../x86_64-cpuid-Xeon-W3520-host.xml | 1 +
.../x86_64-cpuid-Xeon-X5460-host.xml | 1 +
65 files changed, 112 insertions(+)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 2d447da7c3..8774a625ab 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -245,6 +245,9 @@ virCPUDefCopyWithoutModel(const virCPUDef *cpu)
copy->threads = cpu->threads;
copy->arch = cpu->arch;
copy->migratable = cpu->migratable;
+copy->sigFamily = cpu->sigFamily;
+copy->sigModel = cpu->sigModel;
+copy->sigStepping = cpu->sigStepping;
if (cpu->cache) {
copy->cache = g_new0(virCPUCacheDef, 1);
@@ -421,6 +424,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
if (def->type == VIR_CPU_TYPE_HOST) {
g_autofree char *arch = virXPathString("string(./arch[1])", ctxt);
xmlNodePtr counter_node = NULL;
+xmlNodePtr signature_node = NULL;
if (!arch) {
virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -441,6 +445,26 @@
Re: [PATCH] spec: Xen arches have changed on Fedora 36+
On 6/13/22 9:56 AM, Erik Skultety wrote: > On Mon, Jun 13, 2022 at 09:16:09AM -0400, Cole Robinson wrote: >> On 6/13/22 3:03 AM, Erik Skultety wrote: >>> On Sat, Jun 11, 2022 at 04:18:37PM -0400, Cole Robinson wrote: >>>> Latest fedora 36+ xen builds have dropped i686 and armv7hl builds. >>>> >>>> Signed-off-by: Cole Robinson >>>> --- >>> Reviewed-by: Erik Skultety >>> >> >> Pushed, but I neglected to run the test suite before submitting, and >> spec indentation was wrong :/ I pushed a trivial fix >> >> commit aabace2aa53b4e792f94a442fe03d3c596350494 >> Author: Cole Robinson >> Date: Mon Jun 13 09:09:35 2022 -0400 >> >> spec: Fix indentation >> >> - Cole >> > > Weird, I ran the rpmbuild with your patch in the morning hoping that any > syntax > problems would be revealed and it finished just fine. > Yeah rpm build is fine, I verified that before submitting. But I forgot about the syntax check for prettyifying the spec indentation. - Cole
Re: [PATCH] spec: Xen arches have changed on Fedora 36+
On 6/13/22 3:03 AM, Erik Skultety wrote: > On Sat, Jun 11, 2022 at 04:18:37PM -0400, Cole Robinson wrote: >> Latest fedora 36+ xen builds have dropped i686 and armv7hl builds. >> >> Signed-off-by: Cole Robinson >> --- > Reviewed-by: Erik Skultety > Pushed, but I neglected to run the test suite before submitting, and spec indentation was wrong :/ I pushed a trivial fix commit aabace2aa53b4e792f94a442fe03d3c596350494 Author: Cole Robinson Date: Mon Jun 13 09:09:35 2022 -0400 spec: Fix indentation - Cole
Re: [PATCH] docs: kbase/launch_security_sev: QEMU 6.0+ sets iommu=on for us
On 6/13/22 2:56 AM, Erik Skultety wrote: > On Sat, Jun 11, 2022 at 12:44:54PM -0400, Cole Robinson wrote: >> Signed-off-by: Cole Robinson >> --- >> docs/kbase/launch_security_sev.rst | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/docs/kbase/launch_security_sev.rst >> b/docs/kbase/launch_security_sev.rst >> index 3ebb01ad80..9f6330a1ca 100644 >> --- a/docs/kbase/launch_security_sev.rst >> +++ b/docs/kbase/launch_security_sev.rst >> @@ -295,6 +295,8 @@ In order to make virtio devices work, we need to use >> inside the given device XML element in order >> to enable DMA API in the virtio driver. >> >> +QEMU 6.0 and later will `set this by default >> <https://gitlab.com/qemu-project/qemu/-/commit/9f88a7a3df>`__. For earlier >> QEMU versions, you will need to explicitly enable this in the device XML: > > Do we need to link the specific commit in the kbase article? I think simply > saying (in the same paragrap) > > "... enable DMA API in the virtio driver. Starting with QEMU 6.0.0 QEMU will > set this for us by default. For earlier versions though, you will need to > explicitly enable this in the device XML as follows:" > > Reviewed-by: Erik Skultety Thanks, I pushed with your suggested wording - Cole
[PATCH] spec: Xen arches have changed on Fedora 36+
Latest fedora 36+ xen builds have dropped i686 and armv7hl builds.
Signed-off-by: Cole Robinson
---
libvirt.spec.in | 3 +++
1 file changed, 3 insertions(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 59d2f96709..c4ea02fc8e 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -21,6 +21,9 @@
%define arches_systemtap_64bit %{arches_64bit}
%define arches_dmidecode%{arches_x86}
%define arches_xen %{arches_x86} aarch64
+%if 0%{?fedora} >= 36
+%define arches_xen x86_64 aarch64
+%endif
%define arches_vbox %{arches_x86}
%define arches_ceph %{arches_64bit}
%define arches_zfs %{arches_x86} %{power64} %{arm}
--
2.36.1
[PATCH] conf: cpu: Add
Internally we already collect x86 host family + model + stepping
numeric values. This exposed them in capabilities CPU output.
Example:
$ sudo virsh capabilities | grep -A1 -B1 signature
Users need to know these values to calculate an expected.
SEV-ES/SEV-SNP launch measurement.
Signed-off-by: Cole Robinson
---
src/conf/cpu_conf.c | 36 +++
src/conf/cpu_conf.h | 3 ++
src/conf/schemas/cputypes.rng | 13 +++
src/cpu/cpu_x86.c | 3 ++
.../x86_64-cpuid-A10-5800K-host.xml | 1 +
.../x86_64-cpuid-Atom-D510-host.xml | 1 +
.../x86_64-cpuid-Atom-N450-host.xml | 1 +
.../x86_64-cpuid-Atom-P5362-host.xml | 1 +
.../x86_64-cpuid-Cooperlake-host.xml | 1 +
.../x86_64-cpuid-Core-i5-2500-host.xml| 1 +
.../x86_64-cpuid-Core-i5-2540M-host.xml | 1 +
.../x86_64-cpuid-Core-i5-4670T-host.xml | 1 +
.../x86_64-cpuid-Core-i5-650-host.xml | 1 +
.../x86_64-cpuid-Core-i5-6600-host.xml| 1 +
.../x86_64-cpuid-Core-i7-2600-host.xml| 1 +
...86_64-cpuid-Core-i7-2600-xsaveopt-host.xml | 1 +
.../x86_64-cpuid-Core-i7-3520M-host.xml | 1 +
.../x86_64-cpuid-Core-i7-3740QM-host.xml | 1 +
.../x86_64-cpuid-Core-i7-3770-host.xml| 1 +
.../x86_64-cpuid-Core-i7-4510U-host.xml | 1 +
.../x86_64-cpuid-Core-i7-4600U-host.xml | 1 +
.../x86_64-cpuid-Core-i7-5600U-arat-host.xml | 1 +
.../x86_64-cpuid-Core-i7-5600U-host.xml | 1 +
.../x86_64-cpuid-Core-i7-5600U-ibrs-host.xml | 1 +
.../x86_64-cpuid-Core-i7-7600U-host.xml | 1 +
.../x86_64-cpuid-Core-i7-7700-host.xml| 1 +
.../x86_64-cpuid-Core-i7-8550U-host.xml | 1 +
.../x86_64-cpuid-Core-i7-8700-host.xml| 1 +
.../x86_64-cpuid-Core2-E6850-host.xml | 1 +
.../x86_64-cpuid-Core2-Q9500-host.xml | 1 +
.../x86_64-cpuid-EPYC-7601-32-Core-host.xml | 1 +
...6_64-cpuid-EPYC-7601-32-Core-ibpb-host.xml | 1 +
.../cputestdata/x86_64-cpuid-FX-8150-host.xml | 1 +
.../x86_64-cpuid-Opteron-1352-host.xml| 1 +
.../x86_64-cpuid-Opteron-2350-host.xml| 1 +
.../x86_64-cpuid-Opteron-6234-host.xml| 1 +
.../x86_64-cpuid-Opteron-6282-host.xml| 1 +
.../x86_64-cpuid-Pentium-P6100-host.xml | 1 +
.../x86_64-cpuid-Phenom-B95-host.xml | 1 +
...64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml | 1 +
.../x86_64-cpuid-Xeon-5110-host.xml | 1 +
.../x86_64-cpuid-Xeon-E3-1225-v5-host.xml | 1 +
.../x86_64-cpuid-Xeon-E3-1245-v5-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2609-v3-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2623-v4-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2630-v3-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2630-v4-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2650-host.xml| 1 +
.../x86_64-cpuid-Xeon-E5-2650-v3-host.xml | 1 +
.../x86_64-cpuid-Xeon-E5-2650-v4-host.xml | 1 +
.../x86_64-cpuid-Xeon-E7-4820-host.xml| 1 +
.../x86_64-cpuid-Xeon-E7-4830-host.xml| 1 +
.../x86_64-cpuid-Xeon-E7-8890-v3-host.xml | 1 +
.../x86_64-cpuid-Xeon-E7540-host.xml | 1 +
.../x86_64-cpuid-Xeon-Gold-5115-host.xml | 1 +
.../x86_64-cpuid-Xeon-Gold-6130-host.xml | 1 +
.../x86_64-cpuid-Xeon-Gold-6148-host.xml | 1 +
.../x86_64-cpuid-Xeon-Platinum-8268-host.xml | 1 +
.../x86_64-cpuid-Xeon-Platinum-9242-host.xml | 1 +
.../x86_64-cpuid-Xeon-W3520-host.xml | 1 +
.../x86_64-cpuid-Xeon-X5460-host.xml | 1 +
61 files changed, 112 insertions(+)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 2d447da7c3..1a09e27dca 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -159,6 +159,9 @@ virCPUDefCopyModelFilter(virCPUDef *dst,
dst->vendor = g_strdup(src->vendor);
dst->vendor_id = g_strdup(src->vendor_id);
dst->microcodeVersion = src->microcodeVersion;
+dst->sigFamily = src->sigFamily;
+dst->sigModel = src->sigModel;
+dst->sigStepping = src->sigStepping;
dst->nfeatures_max = src->nfeatures;
dst->nfeatures = 0;
@@ -210,6 +213,9 @@ virCPUDefStealModel(virCPUDef *dst,
dst->model = g_steal_pointer(>model);
dst->features = g_steal_pointer(>features);
dst->microcodeVersion = src->microcodeVersion;
+dst->sigFamily = src->sigFamily;
+dst->sigModel = src->sigModel;
+dst->sigStepping = src->sigStepping;
dst->nfeatures_max = src->nfeatures_max;
src->nfeatures_max = 0;
dst->nfeatures = src->nfeatures;
@@ -421,6 +427,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
if (def->type == VIR_CPU_TYPE_HOST) {
g_autofree char *arch = virXPathString("string(./arch[1])", ctxt);
xmlNodePtr counter_node = NULL;
+
[PATCH] docs: kbase/launch_security_sev: QEMU 6.0+ sets iommu=on for us
Signed-off-by: Cole Robinson --- docs/kbase/launch_security_sev.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/kbase/launch_security_sev.rst b/docs/kbase/launch_security_sev.rst index 3ebb01ad80..9f6330a1ca 100644 --- a/docs/kbase/launch_security_sev.rst +++ b/docs/kbase/launch_security_sev.rst @@ -295,6 +295,8 @@ In order to make virtio devices work, we need to use inside the given device XML element in order to enable DMA API in the virtio driver. +QEMU 6.0 and later will `set this by default <https://gitlab.com/qemu-project/qemu/-/commit/9f88a7a3df>`__. For earlier QEMU versions, you will need to explicitly enable this in the device XML: + :: # virsh edit -- 2.36.1
Re: [RFC PATCH v1 0/5] Add virDomainGetSevAttestationReport API
On 4/14/22 2:46 PM, Tyler Fanelli wrote:
> On 4/11/22 10:57 AM, Cole Robinson wrote:
>> On 3/23/22 3:36 PM, Tyler Fanelli wrote:
>>> This an RFC discussing a new API, virDomainGetSevAttestationReport
>>> (along with a
>>> virsh command "domgetsevreport"), with initial QEMU support via the
>>> "query-sev-attestation-report" QAPI mechanism.
>>> "query-sev-attestation-report" is
>>> supplied a base64-encoded 16 byte "mnonce" string as input, with a
>>> purpose of
>>> being embedded into the attestation report to provide protection.
>>>
>>> My main point of concern is the design/communication of the
>>> virTypedParameterPtr
>>> exchanged between the client and libvirtd and how they interact
>>> together, as I
>>> have seen no other API follow the method I used. Namely, the same
>>> virTypedParameterPtr is used for both input _AND_ output. The same
>>> virTypedParameterPtr containing the original mnonce string inputted
>>> to the API is
>>> also used to contain the attestation report upon being returned from
>>> the API.
>>>
>>> This contrasts with much of the APIs I've noticed, which use a
>>> virTypedParameterPtr for either input or output, but not both.
>>>
>>> This patch is not final, as I still would like some human-readable
>>> outputting
>>> and storage of the attestation report.
>>>
>>> Looking for thoughts on the design of this API, as well as suggested
>>> improvements.
>> The proposed API name contains Sev, when all the existing domain APIs
>> have generic names: virDomainGetLaunchSecurityInfo,
>> virDomainSetLaunchSecurityState.
>>
>> I was thinking it would be nice to avoid that Sev specific bit. So I
>> looked at upcoming SNP and TDX qemu patches to see if they add any qmp
>> commands that take input and return a lot of output. Then maybe it would
>> make sense to name this something like
>> virDomainGetLaunchSecurityInfoWithParams which we could use more in the
>> future.
>>
>> qemu SNP patches: https://github.com/AMDESE/qemu/tree/snp-v3
>> - Only extend existing query-sev and query-sev-capabilities
>>
>> qemu TDX patches: https://github.com/intel/qemu-tdx
>> - Adds query-tdx and query-tdx-capabilities, both with no input
>>
>> So, that doesn't help.
>
> What about adding the attestation report to
> virDomainGetLaunchSecurityInfo? If that route is taken, there would
> probably need to be some extra logic added to decipher getting launch
> security info on SEV vs. SGX/TDX, right?
>
The problem is virDomainGetLaunchSecurityInfo doesn't have any way to
pass in the nonce, so we can't reuse that API as is.
>>
>>
>> After that, my question is, what query-sev-attestation-report adds. The
>> kernel commit explains what it is:
>> https://github.com/torvalds/linux/commit/2c07ded06427dd3339278487a1413d5e478f05f9
>>
>>
>>> The SEV FW version >= 0.23 added a new command that can be used
>>> to query
>>> the attestation report containing the SHA-256 digest of the
>>> guest memory
>>> encrypted through the KVM_SEV_LAUNCH_UPDATE_{DATA, VMSA}
>>> commands and
>>> sign the report with the Platform Endorsement Key (PEK).
>>> See the SEV FW API spec section 6.8 for more details.
>>> Note there already exist a command (KVM_SEV_LAUNCH_MEASURE)
>>> that can be
>>> used to get the SHA-256 digest. The main difference between the
>>> KVM_SEV_LAUNCH_MEASURE and KVM_SEV_ATTESTATION_REPORT is that
>>> the latter
>>> can be called while the guest is running and the measurement
>>> value is
>>> signed with PEK.
>> So it is LAUNCH_MEASURE, available at VM runtime, signed with an extra
>> key.
>>
And with a nonce passed in, I forgot to mention that. That's another bit
I'm not sure what it adds over the traditional measurement
>> qemu caches the LAUNCH_MEASURE value at VM startup and lets us query it
>> with qmp any time, so I don't think runtime access matters.
>
> I'm surprised by this. I was under the impression that LAUNCH_MEASURE
> always had to be called when a VM is paused.
>
>>
>> Maybe the extra key signing is a security fix or something. I haven't
>> figured it out.
>
> Signing with the PEK also allows a user to verify the root of trust
> between the owner and the platform.
>
But I don't understand what that means in practice. I figured key
management via
Re: [RFC PATCH v1 0/5] Add virDomainGetSevAttestationReport API
On 3/23/22 3:36 PM, Tyler Fanelli wrote:
> This an RFC discussing a new API, virDomainGetSevAttestationReport (along
> with a
> virsh command "domgetsevreport"), with initial QEMU support via the
> "query-sev-attestation-report" QAPI mechanism. "query-sev-attestation-report"
> is
> supplied a base64-encoded 16 byte "mnonce" string as input, with a purpose of
> being embedded into the attestation report to provide protection.
>
> My main point of concern is the design/communication of the
> virTypedParameterPtr
> exchanged between the client and libvirtd and how they interact together, as I
> have seen no other API follow the method I used. Namely, the same
> virTypedParameterPtr is used for both input _AND_ output. The same
> virTypedParameterPtr containing the original mnonce string inputted to the
> API is
> also used to contain the attestation report upon being returned from the API.
>
> This contrasts with much of the APIs I've noticed, which use a
> virTypedParameterPtr for either input or output, but not both.
>
> This patch is not final, as I still would like some human-readable outputting
> and storage of the attestation report.
>
> Looking for thoughts on the design of this API, as well as suggested
> improvements.
The proposed API name contains Sev, when all the existing domain APIs
have generic names: virDomainGetLaunchSecurityInfo,
virDomainSetLaunchSecurityState.
I was thinking it would be nice to avoid that Sev specific bit. So I
looked at upcoming SNP and TDX qemu patches to see if they add any qmp
commands that take input and return a lot of output. Then maybe it would
make sense to name this something like
virDomainGetLaunchSecurityInfoWithParams which we could use more in the
future.
qemu SNP patches: https://github.com/AMDESE/qemu/tree/snp-v3
- Only extend existing query-sev and query-sev-capabilities
qemu TDX patches: https://github.com/intel/qemu-tdx
- Adds query-tdx and query-tdx-capabilities, both with no input
So, that doesn't help.
After that, my question is, what query-sev-attestation-report adds. The
kernel commit explains what it is:
https://github.com/torvalds/linux/commit/2c07ded06427dd3339278487a1413d5e478f05f9
> The SEV FW version >= 0.23 added a new command that can be used to query
> the attestation report containing the SHA-256 digest of the guest memory
> encrypted through the KVM_SEV_LAUNCH_UPDATE_{DATA, VMSA} commands and
> sign the report with the Platform Endorsement Key (PEK).
>
> See the SEV FW API spec section 6.8 for more details.
>
> Note there already exist a command (KVM_SEV_LAUNCH_MEASURE) that can be
> used to get the SHA-256 digest. The main difference between the
> KVM_SEV_LAUNCH_MEASURE and KVM_SEV_ATTESTATION_REPORT is that the latter
> can be called while the guest is running and the measurement value is
> signed with PEK.
So it is LAUNCH_MEASURE, available at VM runtime, signed with an extra key.
qemu caches the LAUNCH_MEASURE value at VM startup and lets us query it
with qmp any time, so I don't think runtime access matters.
Maybe the extra key signing is a security fix or something. I haven't
figured it out.
But as is it's not clear what this buys us over the launch measurement
we already report with virDomainGetLaunchSecurityInfo
If we figure out what the point of this is, IMO we can more easily
reason about whether it makes sense to add a Sev specific libvirt API,
and whether we need virTypedParams for both input and output. For
example if the API really is specific to this one and only KVM ioctl/QMP
command, we could hardcode the parameters and skip the virTypedParams
question entirely.
CCing danpb for his thoughts
- Cole
Re: [libvirt PATCH 00/10] Cleanup and test more firmware handling scenarios
On 2/16/22 8:17 AM, Michal Prívozník wrote: > On 2/15/22 19:54, Daniel P. Berrangé wrote: >> There are a mind bending number of possible ways to configure the >> firmware with/without NVRAM. Only a small portion are tested and >> many error scenarios are silently ignored. >> >> This series attempts to get coverage of every possible XML config >> scenario and report explicit errors in all invalid configs. >> >> There is an open question on patch 4. Essentially the use of NVRAM >> combined with writable executable feels like an accidental feature >> in libvirt that hasn't really been thought through. I'd like to >> better define expectations here but there are several possible >> strategies and I'm undecided which is best. >> >> Daniel P. Berrangé (10): >> qemu: fix bad indentation for qemuDomainNVRAMPathFormat >> tests: add explicit test case for pflash loader lacking path >> tests: add test case for NVRAM with template >> conf: validate NVRAM template usage with R/W loader binary >> tests: don't permit NVRAM path when using firmware auto-select >> qemu: inline code for filling in per-VM NVRAM path >> conf: rename struct field for NVRAM template >> conf: switch nvram parsing to use XML node / property helpers >> conf: move nvram parsing into virDomainLoaderDefParseXML >> conf: stop ignoring / with firmware auto-select >> >> > > Reviewed-by: Michal Privoznik > > Michal > I don't see the last 3 patches in git. Daniel was that intentional? Thanks, Cole
ANNOUNCE: virt-manager 4.0.0 released
I'm happy to announce the release of virt-manager 4.0.0! The release can be downloaded from: http://virt-manager.org/download/ Some notable defaults changes: * virt-install: missing --os-variant/--osinfo is now a hard error in most cases. If you weren't specifying a value, or getting one from install media detection, you were probably getting crappy defaults and didn't realize it. If you hit this case you will see a big descriptive error hopefully guiding you to an easy solution. You can see the error and some more details in this email: https://listman.redhat.com/archives/virt-tools-list/2022-February/msg00021.html * For qemu x86 we now use mode=host-passthrough as the CPU default instead of mode=host-model * We now use video model virtio-gpu/virtio-vga in many cases where we previously used qxl, following the suggestions here: https://www.kraxel.org/blog/2019/09/display-devices-in-qemu/ * We now add an TPM emulated device when the VM will use UEFI * qemu x86 q35 VMs will be created with extra pcie-root-ports to facilitate device hotplug. * We set disk discard=unmap when we know the disk storage sparse, or when storage is a block device. Some other notable changes: - Add 'Enable shared memory' UI checkbox (Lin Ma) - add UI preference to default to UEFI for new VMs (Charles Arnold) - Add virtiofs filesystem driver UI option - Fill in all --cputune, --cpu, --shmem, --input, and --boot suboptions (Hugues Fafard) - virt-* mdev improvements (Shalini Chellathurai Saroja) - bhyve improvments (Roman Bogorodskiy) - Revive network portgroup UI Notes for distro maintainers: * We replaced usage of isoinfo with xorisso * We now depend on python setuptools for build + install * We added an explicit runtime requirement to pygobject >= 3.31.3. This is from June 2014 so probably not relevant for modern distros. Thanks to everyone who has contributed to this release through testing, bug reporting, submitting patches, and otherwise sending in feedback! Thanks, Cole
Re: [PATCH 0/3] Unbreak MIPS Malta
On 2/2/22 4:09 AM, Michal Prívozník wrote: > On 2/1/22 15:33, Lubomir Rintel wrote: >> My day started like this: >> >> # virt-install --connect qemu:///system --arch mips --machine malta >> --memory 256 --disk none --import >> Using default --name vm-mips >> >> Starting install... >> ERRORXML error: No PCI buses available >> >> Needless to say, it ended up completely ruined. >> >> Chained to this message are the patches I've created in an attempt to >> remedy the highly unfortunate situation, with hope that they'll be >> treated with warmth, understanding and perhaps even applied to the >> libvirt tree. >> Hi Lubo, after these patches was the VM usefully usable, did this simply start up? There is effectively no qemu-system-mips* mips coverage in virt-install or libvirt unit test suites, we should add some if we don't want this to regress. If the XML virt-install generates for you is useful, we could drop it into libvirt's qemuxml2argvtest Thanks, Cole
[PATCH] lxc: controller: Fix container launch on cgroup v1
With cgroup v1 I'm seeing LXC container startup failures: $ sudo virt-install --connect lxc:/// --name test-container --memory 128 --boot init=/bin/sh Starting install... ERRORerror from service: GDBus.Error:org.freedesktop.machine1.NoMachineForPID: PID 2145047 does not belong to any known machine libvirt 7.0.0 works but 7.1.0+ does not. The root error seems to predate that, showing up in syslog, but commit 9c1693eff made it fatal: commit 9c1693eff427661616ce1bd2795688f87288a412 Author: Pavel Hrdina Date: Fri Feb 5 16:17:35 2021 +0100 vircgroup: use DBus call to systemd for some APIs The error comes from virSystemdGetMachineByPID. The PID that shows up in the above error message does not match the leader PID as reported by machinectl. This change fixes the error. Things seem to continue to work with cgroupsv2 after this change. https://gitlab.com/libvirt/libvirt/-/issues/182 Signed-off-by: Cole Robinson --- This is from the thread in August, posted as non RFC now src/lxc/lxc_controller.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 8953e0c904..444f728af4 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -865,12 +865,12 @@ static int virLXCControllerSetupCgroupLimits(virLXCController *ctrl) nodeset = virDomainNumatuneGetNodeset(ctrl->def->numa, auto_nodeset, -1); if (!(ctrl->cgroup = virLXCCgroupCreate(ctrl->def, -ctrl->initpid, +getpid(), ctrl->nnicindexes, ctrl->nicindexes))) goto cleanup; -if (virCgroupAddMachineProcess(ctrl->cgroup, getpid()) < 0) +if (virCgroupAddMachineProcess(ctrl->cgroup, ctrl->initpid) < 0) goto cleanup; /* Add all qemu-nbd tasks to the cgroup */ -- 2.31.1
lxc container startup error and RFC patch
Hi all, I'm seeing LXC container startup failures. This is with libvirt git, fedora 34 host with systemd-248.6-1.fc34.x86_64 (I didn't confirm with other versions). Reproducer: sudo virt-install --connect lxc:/// --name test-container --memory 128 --boot init=/bin/sh Starting install... ERRORerror from service: GDBus.Error:org.freedesktop.machine1.NoMachineForPID: PID 2145047 does not belong to any known machine libvirt 7.0.0 works but 7.1.0+ does not. The root error seems to predate that, showing up in syslog, but commit 9c1693eff made it fatal: commit 9c1693eff427661616ce1bd2795688f87288a412 Author: Pavel Hrdina Date: Fri Feb 5 16:17:35 2021 +0100 vircgroup: use DBus call to systemd for some APIs The error comes from virSystemdGetMachineByPID. The PID that shows up in the above error message does not match the leader PID as reported by machinectl. This change fixes the error but I don't know if it's correct or if it has other implications: diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 066e013ed4..54ecb1316b 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -866,12 +866,12 @@ static int virLXCControllerSetupCgroupLimits(virLXCController *ctrl) nodeset = virDomainNumatuneGetNodeset(ctrl->def->numa, auto_nodeset, -1); if (!(ctrl->cgroup = virLXCCgroupCreate(ctrl->def, -ctrl->initpid, +getpid(), ctrl->nnicindexes, ctrl->nicindexes))) goto cleanup; -if (virCgroupAddMachineProcess(ctrl->cgroup, getpid()) < 0) +if (virCgroupAddMachineProcess(ctrl->cgroup, ctrl->initpid) < 0) goto cleanup; /* Add all qemu-nbd tasks to the cgroup */ Maybe something else isn't working elsewhere. Clearly we try to add both pids to the systemd machine, but virSystemdGetMachineByPID is not working to match the non-leader pid, which is the one that the LXC driver knows about. Thoughts? Can anyone else reproduce? Thanks, Cole
Re: [PATCH 1/2] virSetUIDGIDWithCaps: Check for capng_apply() retval properly
On 7/22/21 11:29 AM, Michal Privoznik wrote:
> After all capabilities were set (except for CAP_SETGID,
> CAP_SETUID and CAP_SETPCAP) and after UID:GID was changed we drop
> the last aforementioned capabilities (we couldn't drop them
> before because we needed UID:GID and capabilities change).
> Therefore, there's final capng_apply() call. However, it's return
> value is not checked for properly. It's typical problem of:
>
> var = func() < 0
>
> Signed-off-by: Michal Privoznik
> ---
> src/util/virutil.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index ed3d57662b..aba0aea0ff 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1261,7 +1261,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t
> *groups, int ngroups,
> if (need_setpcap)
> capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED,
> CAP_SETPCAP);
>
> -if (((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0)) {
> +if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot apply process capabilities %d"), capng_ret);
> return -1;
>
Does this have any functional impact?
before: if (((a = b()) < c))
after: if ((a = b()) < c)
Looks like a paren was dropped off outside, which shouldn't make a
difference. So IMO amend the commit message and push as trivial.
- Cole
Re: [PATCH 2/2] virSetUIDGIDWithCaps: Set bounding capabilities only with CAP_SETPCAP
On 7/22/21 11:29 AM, Michal Privoznik wrote: > In one of my previous patches I've tried to postpone dropping > CAP_SETPCAP until the very end because it's needed for > capng_apply(). What I did not realize back then was that we might > not have the capability to begin with. Because of unknown reasons > capng_apply() pollutes logs only for CAPNG_SELECT_BOUNDS and not > for CAPNG_SELECT_CAPS. > > Reproducer is really simple: run libvirtd as a regular user. > During its initialization, libvirtd will spawn some binaries > (dnsmasq, qemu-*, etc.) and while doing so it will try to drop > capabilities. > > Anyway, let's call capng_apply(CAPNG_SELECT_BOUNDS) only if we > have the CAP_SETPCAP (which is tracked in need_setpcap variable). > > Fixes: 438b50dda8a863fdc988e9ab612f097cc1626e8a > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1924218 > Signed-off-by: Michal Privoznik > --- > src/util/virutil.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/util/virutil.c b/src/util/virutil.c > index aba0aea0ff..00cd56e2b2 100644 > --- a/src/util/virutil.c > +++ b/src/util/virutil.c > @@ -1250,7 +1250,8 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t > *groups, int ngroups, > * do this if we failed to get the capability above, so ignore the > * return value. > */ > -capng_apply(CAPNG_SELECT_BOUNDS); > +if (!need_setpcap) > +capng_apply(CAPNG_SELECT_BOUNDS); > > /* Drop the caps that allow setuid/gid (unless they were requested) */ > if (need_setgid) > Reviewed-by: Cole Robinson - Cole - Cole
[PATCH] qemu_vhost_user: don't raise error for unknown features
Similar to what was done for qemu_firmware.c in 61d95a1073, don't
report an error for unknown vhost-user features, just log it and
correctly continue on
Signed-off-by: Cole Robinson
---
src/qemu/qemu_vhost_user.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_vhost_user.c b/src/qemu/qemu_vhost_user.c
index bc8e7ad898..75cc718c09 100644
--- a/src/qemu/qemu_vhost_user.c
+++ b/src/qemu/qemu_vhost_user.c
@@ -280,6 +280,7 @@ qemuVhostUserGPUFillCapabilities(qemuVhostUser *vu,
qemuVhostUserGPU *gpu = >capabilities.gpu;
virJSONValue *featuresJSON;
size_t nfeatures;
+size_t nparsed = 0;
size_t i;
g_autoptr(qemuVhostUserGPUFeature) features = NULL;
@@ -299,17 +300,16 @@ qemuVhostUserGPUFillCapabilities(qemuVhostUser *vu,
int tmp;
if ((tmp = qemuVhostUserGPUFeatureTypeFromString(tmpStr)) <= 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unknown feature %s"),
- tmpStr);
+VIR_DEBUG("ignoring unknown QEMU vhost-user feature '%s'", tmpStr);
continue;
}
-features[i] = tmp;
+features[nparsed] = tmp;
+nparsed++;
}
gpu->features = g_steal_pointer();
-gpu->nfeatures = nfeatures;
+gpu->nfeatures = nparsed;
return 0;
}
--
2.31.1
Re: [PATCH] qemu: don't reject virtiofs for qemu:///session
On 3/29/21 6:52 AM, Ján Tomko wrote: > On a Friday in 2021, Cole Robinson wrote: >> Currently libvirt rejects attempts to use virtiofs with >> qemu:///session. Indeed virtiofs does not have a chance of working >> with typical session usage, because virtiofsd needs multiple linux >> capabilities to perform its job. The only way to do this with out >> of the box qemu packaging is to run virtiofsd as root, so libvirtd >> must run as root, so qemu:///system is required. >> >> But it's possible that a custom environment could setuid or set >> file capabilities on the virtiofsd binary. In this case qemu:///session >> would work fine. This may be an option for kubevirt to help them >> transition to using qemu:///session everywhere >> >> Drop the two pieces that block virtiofs for qemu:///session. Attempts >> to use virtiofs for stock qemu:///session will fail at qemu startup, >> though it's a bit opaque: >> >> error: Failed to start domain 'f32' >> error: internal error: qemu unexpectedly closed the monitor: >> 2021-03-26T16:26:12.459293Z qemu-system-x86_64: -device >> vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0: >> Failed to write msg. Wrote -1 instead of 12. >> 2021-03-26T16:26:12.459317Z qemu-system-x86_64: -device >> vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0: >> vhost_dev_init failed: Operation not permitted >> > > That is not a friendly error message for regular users. > > Some alternatives come to mind: > * XML element telling libvirt to ignore the check/do not set the UID. > The downside is, that we usually do this via: > > and the seclabel code makes my head hurt. > * Introduce a QEMU capability for this, that kubevirt could set via > > https://libvirt.org/drvqemu.html#xmlnsfeatures > * Introduce the capability to 50-qemu-virtiofsd.json > * Wait until the lockdown eases up and I finally post the patches > for externally launched virtiofsd > https://bugzilla.redhat.com/show_bug.cgi?id=1855789 > I don't have much of an opinion. The latter will be useful for CNV someday but it will take some rearchitechting on their part >> Signed-off-by: Cole Robinson >> --- >> The SetUID/SetGID bits don't seem to be necessary for qemu:///system >> usage AFAICT, but it's a bit tough to decode virSetUIDGIDWithCaps. > > I *think* the only difference without the two virCommandSet?ID calls > is that we error out if UID/GID 0 can't be set. But yes it's tough > to read. > I just tested with the qemu_validate.c removal only, so SetUID still in place. The error: error: Failed to start domain 'f32' error: internal error: process exited while connecting to monitor: 2021-03-29T19:11:41.217583Z qemu-system-x86_64: -chardev socket,id=chr-vu-fs0,path=/home/crobinso/.config/libvirt/qemu/lib/domain-3-f32/fs0-fs.sock: Failed to connect to '/home/crobinso/.config/libvirt/qemu/lib/domain-3-f32/fs0-fs.sock': Connection refused virtiofsd log has: libvirt: error : internal error: cannot apply process capabilities -5 So seems like libvirt error reporting here needs some tweaks regardless. Maybe if catching virtiofsd startup error is improved it will improve the first error case as well. - Cole
Re: [PATCH] qemu: virtiofs: support
On 3/26/21 11:53 AM, Peter Krempa wrote: > On Fri, Mar 26, 2021 at 11:37:48 -0400, Cole Robinson wrote: >> Add a new XML element >> >> >> >> >> >> >> >> Which maps to `virtiofsd -o sandbox=chroot|namespace`, which was added >> in qemu 5.2.0: >> >> https://git.qemu.org/?p=qemu.git;a=commit;h=06844584b62a43384642f7243b0fc01c9fff0fc7 >> >> Signed-off-by: Cole Robinson >> --- >> docs/formatdomain.rst | 4 >> docs/schemas/domaincommon.rng | 12 ++ >> src/conf/domain_conf.c| 23 +++ >> src/conf/domain_conf.h| 10 >> src/libvirt_private.syms | 1 + >> src/qemu/qemu_virtiofs.c | 2 ++ >> .../vhost-user-fs-fd-memory.xml | 1 + >> 7 files changed, 53 insertions(+) > > Please split the commit as it's usual for libvirt patches. > Okay, fixed in v2. I addressed the docs and validation piece in v2 too > Also a test case modifying any of the .args files in qemuxml2argv test > is missing. > This option affects the virtiofsd command line only, so it won't be reflected in .args files Thanks, Cole
[PATCH v2 2/2] qemu: virtiofs: support
This maps to `virtiofsd -o sandbox=chroot|namespace`, which was added
in qemu 5.2.0:
https://git.qemu.org/?p=qemu.git;a=commit;h=06844584b62a43384642f7243b0fc01c9fff0fc7
Signed-off-by: Cole Robinson
---
src/qemu/qemu_validate.c | 7 +++
src/qemu/qemu_virtiofs.c | 2 ++
2 files changed, 9 insertions(+)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 6043f974ce..b272ab0087 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4081,6 +4081,13 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
}
}
+if (fs->fsdriver != VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS &&
+fs->sandbox != VIR_DOMAIN_FS_SANDBOX_MODE_DEFAULT) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("sandbox can only be used with driver=virtiofs"));
+return -1;
+}
+
switch ((virDomainFSDriverType) fs->fsdriver) {
case VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT:
case VIR_DOMAIN_FS_DRIVER_TYPE_PATH:
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index 2e239cad66..988b757d6f 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -131,6 +131,8 @@ qemuVirtioFSBuildCommandLine(virQEMUDriverConfigPtr cfg,
virQEMUBuildBufferEscapeComma(, fs->src->path);
if (fs->cache)
virBufferAsprintf(, ",cache=%s",
virDomainFSCacheModeTypeToString(fs->cache));
+if (fs->sandbox)
+virBufferAsprintf(, ",sandbox=%s",
virDomainFSSandboxModeTypeToString(fs->sandbox));
if (fs->xattr == VIR_TRISTATE_SWITCH_ON)
virBufferAddLit(, ",xattr");
--
2.30.2
[PATCH v2 1/2] conf: Introduce for
This adds a new XML element
This will be used by qemu virtiofs
Signed-off-by: Cole Robinson
---
docs/formatdomain.rst | 6 +
docs/schemas/domaincommon.rng | 12 ++
src/conf/domain_conf.c| 23 +++
src/conf/domain_conf.h| 10
src/libvirt_private.syms | 1 +
.../vhost-user-fs-fd-memory.xml | 1 +
6 files changed, 53 insertions(+)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 9392c80113..42217a4005 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3234,6 +3234,7 @@ A directory on the host that can be accessed directly
from the guest.
+
@@ -3358,6 +3359,11 @@ A directory on the host that can be accessed directly
from the guest.
``cache`` element, possible ``mode`` values being ``none`` and ``always``.
Locking can be controlled via the ``lock`` element - attributes ``posix``
and
``flock`` both accepting values ``on`` or ``off``. ( :since:`Since 6.2.0` )
+ The sandboxing method used by virtiofsd can be configured with the
``sandbox``
+ element, possible ``mode`` values being ``namespace`` and
+ ``chroot``, see the
+ `virtiofsd documentation
<https://qemu.readthedocs.io/en/latest/tools/virtiofsd.html>`__
+ for more details. ( :since:`Since 7.2.0` )
``source``
The resource on the host that is being accessed in the guest. The ``name``
attribute must be used with ``type='template'``, and the ``dir`` attribute
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 1dbfc68f18..6404ebf210 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2960,6 +2960,18 @@
+
+
+
+
+
+ namespace
+ chroot
+
+
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b0eba9f7bd..70a900ee25 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -538,6 +538,13 @@ VIR_ENUM_IMPL(virDomainFSCacheMode,
"always",
);
+VIR_ENUM_IMPL(virDomainFSSandboxMode,
+ VIR_DOMAIN_FS_SANDBOX_MODE_LAST,
+ "default",
+ "namespace",
+ "chroot",
+);
+
VIR_ENUM_IMPL(virDomainNet,
VIR_DOMAIN_NET_TYPE_LAST,
@@ -10373,6 +10380,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
g_autofree char *binary = virXPathString("string(./binary/@path)",
ctxt);
g_autofree char *xattr = virXPathString("string(./binary/@xattr)",
ctxt);
g_autofree char *cache =
virXPathString("string(./binary/cache/@mode)", ctxt);
+g_autofree char *sandbox =
virXPathString("string(./binary/sandbox/@mode)", ctxt);
g_autofree char *posix_lock =
virXPathString("string(./binary/lock/@posix)", ctxt);
g_autofree char *flock =
virXPathString("string(./binary/lock/@flock)", ctxt);
int val;
@@ -10406,6 +10414,16 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
def->cache = val;
}
+if (sandbox) {
+if ((val = virDomainFSSandboxModeTypeFromString(sandbox)) <= 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("cannot parse sandbox mode '%s' for
virtiofs"),
+ sandbox);
+goto error;
+}
+def->sandbox = val;
+}
+
if (posix_lock) {
if ((val = virTristateSwitchTypeFromString(posix_lock)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -25483,6 +25501,11 @@ virDomainFSDefFormat(virBufferPtr buf,
virDomainFSCacheModeTypeToString(def->cache));
}
+if (def->sandbox != VIR_DOMAIN_FS_SANDBOX_MODE_DEFAULT) {
+virBufferAsprintf(, "\n",
+
virDomainFSSandboxModeTypeToString(def->sandbox));
+}
+
if (def->posix_lock != VIR_TRISTATE_SWITCH_ABSENT) {
virBufferAsprintf(, " posix='%s'",
virTristateSwitchTypeToString(def->posix_lock));
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0b8895bbdf..d77b04847b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -846,6 +846,14 @@ typedef enum {
VIR_DOMAIN_FS_CACHE_MODE_LAST
} virDomainFSCacheMode;
+typedef enum {
+VIR_DOMAIN_FS_SANDBOX_MODE_DEFAULT = 0,
+VIR_DOMAIN_FS_SANDBOX_MODE_NAMESPACE,
+VIR_DOM
[PATCH v2 0/2] qemu: virtiofs: support
Add support for virtiofsd -o sandbox=chroot|namespace v2: Add link to virtiofsd docs in libvirt docs validate the new field for qemu break up the patch Cole Robinson (2): conf: Introduce for qemu: virtiofs: support docs/formatdomain.rst | 6 + docs/schemas/domaincommon.rng | 12 ++ src/conf/domain_conf.c| 23 +++ src/conf/domain_conf.h| 10 src/libvirt_private.syms | 1 + src/qemu/qemu_validate.c | 7 ++ src/qemu/qemu_virtiofs.c | 2 ++ .../vhost-user-fs-fd-memory.xml | 1 + 8 files changed, 62 insertions(+) -- 2.30.2
[PATCH] qemu: don't reject virtiofs for qemu:///session
Currently libvirt rejects attempts to use virtiofs with
qemu:///session. Indeed virtiofs does not have a chance of working
with typical session usage, because virtiofsd needs multiple linux
capabilities to perform its job. The only way to do this with out
of the box qemu packaging is to run virtiofsd as root, so libvirtd
must run as root, so qemu:///system is required.
But it's possible that a custom environment could setuid or set
file capabilities on the virtiofsd binary. In this case qemu:///session
would work fine. This may be an option for kubevirt to help them
transition to using qemu:///session everywhere
Drop the two pieces that block virtiofs for qemu:///session. Attempts
to use virtiofs for stock qemu:///session will fail at qemu startup,
though it's a bit opaque:
error: Failed to start domain 'f32'
error: internal error: qemu unexpectedly closed the monitor:
2021-03-26T16:26:12.459293Z qemu-system-x86_64: -device
vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0:
Failed to write msg. Wrote -1 instead of 12.
2021-03-26T16:26:12.459317Z qemu-system-x86_64: -device
vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0:
vhost_dev_init failed: Operation not permitted
Signed-off-by: Cole Robinson
---
The SetUID/SetGID bits don't seem to be necessary for qemu:///system
usage AFAICT, but it's a bit tough to decode virSetUIDGIDWithCaps.
virtiofsd is meticulous about managing its capability set though
src/qemu/qemu_validate.c | 7 +--
src/qemu/qemu_virtiofs.c | 4
2 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 6043f974ce..d4079f6b67 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4053,7 +4053,7 @@ qemuValidateDomainDeviceDefGraphics(const
virDomainGraphicsDef *graphics,
static int
qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
const virDomainDef *def,
- virQEMUDriverPtr driver,
+ virQEMUDriverPtr driver G_GNUC_UNUSED,
virQEMUCapsPtr qemuCaps)
{
if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
@@ -4107,11 +4107,6 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
_("virtiofs does not yet support read-only mode"));
return -1;
}
-if (!driver->privileged) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("virtiofs is not yet supported in session mode"));
-return -1;
-}
if (fs->accessmode != VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("virtiofs only supports passthrough accessmode"));
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index 2e239cad66..0bb4e3c0d1 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -214,10 +214,6 @@ qemuVirtioFSStart(virLogManagerPtr logManager,
if (!(cmd = qemuVirtioFSBuildCommandLine(cfg, fs, )))
goto cleanup;
-/* so far only running as root is supported */
-virCommandSetUID(cmd, 0);
-virCommandSetGID(cmd, 0);
-
virCommandSetPidFile(cmd, pidfile);
virCommandSetOutputFD(cmd, );
virCommandSetErrorFD(cmd, );
--
2.30.2
[PATCH] qemu: virtiofs: support
Add a new XML element
Which maps to `virtiofsd -o sandbox=chroot|namespace`, which was added
in qemu 5.2.0:
https://git.qemu.org/?p=qemu.git;a=commit;h=06844584b62a43384642f7243b0fc01c9fff0fc7
Signed-off-by: Cole Robinson
---
docs/formatdomain.rst | 4
docs/schemas/domaincommon.rng | 12 ++
src/conf/domain_conf.c| 23 +++
src/conf/domain_conf.h| 10
src/libvirt_private.syms | 1 +
src/qemu/qemu_virtiofs.c | 2 ++
.../vhost-user-fs-fd-memory.xml | 1 +
7 files changed, 53 insertions(+)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 9392c80113..9dda39dbcb 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3234,6 +3234,7 @@ A directory on the host that can be accessed directly
from the guest.
+
@@ -3358,6 +3359,9 @@ A directory on the host that can be accessed directly
from the guest.
``cache`` element, possible ``mode`` values being ``none`` and ``always``.
Locking can be controlled via the ``lock`` element - attributes ``posix``
and
``flock`` both accepting values ``on`` or ``off``. ( :since:`Since 6.2.0` )
+ The sandboxing method used by virtiofsd can be configured with the
``sandbox``
+ element, possible ``mode`` values being ``namespace`` and
+ ``chroot``. ( :since:`Since 7.2.0` )
``source``
The resource on the host that is being accessed in the guest. The ``name``
attribute must be used with ``type='template'``, and the ``dir`` attribute
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 1dbfc68f18..6404ebf210 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2960,6 +2960,18 @@
+
+
+
+
+
+ namespace
+ chroot
+
+
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b0eba9f7bd..70a900ee25 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -538,6 +538,13 @@ VIR_ENUM_IMPL(virDomainFSCacheMode,
"always",
);
+VIR_ENUM_IMPL(virDomainFSSandboxMode,
+ VIR_DOMAIN_FS_SANDBOX_MODE_LAST,
+ "default",
+ "namespace",
+ "chroot",
+);
+
VIR_ENUM_IMPL(virDomainNet,
VIR_DOMAIN_NET_TYPE_LAST,
@@ -10373,6 +10380,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
g_autofree char *binary = virXPathString("string(./binary/@path)",
ctxt);
g_autofree char *xattr = virXPathString("string(./binary/@xattr)",
ctxt);
g_autofree char *cache =
virXPathString("string(./binary/cache/@mode)", ctxt);
+g_autofree char *sandbox =
virXPathString("string(./binary/sandbox/@mode)", ctxt);
g_autofree char *posix_lock =
virXPathString("string(./binary/lock/@posix)", ctxt);
g_autofree char *flock =
virXPathString("string(./binary/lock/@flock)", ctxt);
int val;
@@ -10406,6 +10414,16 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
def->cache = val;
}
+if (sandbox) {
+if ((val = virDomainFSSandboxModeTypeFromString(sandbox)) <= 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("cannot parse sandbox mode '%s' for
virtiofs"),
+ sandbox);
+goto error;
+}
+def->sandbox = val;
+}
+
if (posix_lock) {
if ((val = virTristateSwitchTypeFromString(posix_lock)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -25483,6 +25501,11 @@ virDomainFSDefFormat(virBufferPtr buf,
virDomainFSCacheModeTypeToString(def->cache));
}
+if (def->sandbox != VIR_DOMAIN_FS_SANDBOX_MODE_DEFAULT) {
+virBufferAsprintf(, "\n",
+
virDomainFSSandboxModeTypeToString(def->sandbox));
+}
+
if (def->posix_lock != VIR_TRISTATE_SWITCH_ABSENT) {
virBufferAsprintf(, " posix='%s'",
virTristateSwitchTypeToString(def->posix_lock));
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0b8895bbdf..d77b04847b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -846,6 +846,14 @@ typedef enum {
VIR_DOMAIN_FS_CACHE_MODE_LAST
} virDomainFSCacheMode;
+typedef enum {
+VIR_DOMAIN_FS_SANDBOX_MODE_DEFAULT = 0,
+VIR_DOMAIN_F
[PATCH] hyperv: Fix 32bit compilation
Example:
../src/hyperv/hyperv_driver.c:3007:54: error: format ‘%lu’ expects argument of
type ‘long unsigned int’, but argument 7 has type ‘size_t’ {aka ‘unsigned int’}
[-Werror=format=]
3007 | virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not attach
serial port %lu"), i);
Signed-off-by: Cole Robinson
---
Pushed
src/hyperv/hyperv_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 701456cdb3..e4f537bd12 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -3004,7 +3004,7 @@ hypervDomainDefineXML(virConnectPtr conn, const char *xml)
/* Attach serials */
for (i = 0; i < def->nserials; i++) {
if (hypervDomainAttachSerial(domain, def->serials[i]) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not attach serial
port %lu"), i);
+virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not attach serial
port %zu"), i);
goto error;
}
}
@@ -3012,7 +3012,7 @@ hypervDomainDefineXML(virConnectPtr conn, const char *xml)
/* Attach networks */
for (i = 0; i < def->nnets; i++) {
if (hypervDomainAttachSyntheticEthernetAdapter(domain, def->nets[i],
hostname) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not attach network
%lu"), i);
+virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not attach network
%zu"), i);
goto error;
}
}
--
2.29.2
Re: [PATCH] spec: Increase meson test timeout 10x
On 1/25/21 5:54 AM, Michal Privoznik wrote:
> On 1/21/21 10:55 PM, Cole Robinson wrote:
>> Tests time out when building in slow environments, like emulated
>> s390x in Fedora copr. Bump up the test timeout
>>
>> Signed-off-by: Cole Robinson
>> ---
>> libvirt.spec.in | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index 0a8b0ebad4..1731aa8bd9 100644
>> --- a/libvirt.spec.in
>> +++ b/libvirt.spec.in
>> @@ -1288,7 +1288,9 @@ mv
>> $RPM_BUILD_ROOT%{_datadir}/systemtap/tapset/libvirt_qemu_probes.stp \
>> %endif
>> %check
>> -VIR_TEST_DEBUG=1 %meson_test --no-suite syntax-check
>> +# Building on slow archs, like emulated s390x in Fedora copr, requires
>> +# raising the test timeout
>> +VIR_TEST_DEBUG=1 %meson_test --no-suite syntax-check
>> --timeout-multiplier 10
>> %post libs
>> %if 0%{?rhel} == 7
>>
>
> Looks like we have consensus here. In the past, we've increased the
> timeout for qemuxml2argvtest:
>
> https://gitlab.com/libvirt/libvirt/-/commit/94146c9d2ba
>
> Would it make sense finally admit that not everybody is running top
> shelf CPU and increase the timeout even for 'ninja test'? I mean, set
> timeout to a bigger value in tests/meson.build and drop special casing
> of one test?
>
+1 from me. In my case the slow test was virschematest
- Cole
[PATCH] docs: formatdomain: Fix poll-control XML example
Fixes: 3fc4412c6f5 Signed-off-by: Cole Robinson --- Pushed as trivial docs/formatdomain.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index af540391db..c738078b90 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1766,7 +1766,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off. - + -- 2.29.2
[PATCH] spec: Increase meson test timeout 10x
Tests time out when building in slow environments, like emulated
s390x in Fedora copr. Bump up the test timeout
Signed-off-by: Cole Robinson
---
libvirt.spec.in | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 0a8b0ebad4..1731aa8bd9 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1288,7 +1288,9 @@ mv
$RPM_BUILD_ROOT%{_datadir}/systemtap/tapset/libvirt_qemu_probes.stp \
%endif
%check
-VIR_TEST_DEBUG=1 %meson_test --no-suite syntax-check
+# Building on slow archs, like emulated s390x in Fedora copr, requires
+# raising the test timeout
+VIR_TEST_DEBUG=1 %meson_test --no-suite syntax-check --timeout-multiplier 10
%post libs
%if 0%{?rhel} == 7
--
2.29.2
Re: [PATCH 0/5] Deduplicate some of validation code
On 11/17/20 6:28 AM, Michal Privoznik wrote: > These stemmed from Cole's comment on my patches: > > https://www.redhat.com/archives/libvir-list/2020-November/msg00888.html > > The approach might not be that obvious at first because domCaps do > qemuCaps to device model translation and during validation we want > the exact opposite - we have parsed device model and want to check > whether qemuCaps has corresponding capability. > > But what we can do, is let domCaps code fill a bitmap of supported > device models and then check if the bit that corresponds to parsed > device model is set. > > In this series I'm fixing RNG and video models, which were checked > this way until very recently (until I touched the code). And also > I'm introducing graphics check (which is new) because that one > looked the most sane from virQEMUCapsFillDomainCaps(). The rest > there not that much. > > Michal Prívozník (5): > domain_capabilities: Introduce VIR_DOMAIN_CAPS_ENUM_IS_SET > qemu_validate: Deduplicate code for video model check > qemu_validate: Deduplicate code for RNG model check > domcaps: Report egl-headless graphics type > qemu_validate: Deduplicate code for graphics type check Reviewed-by: Cole Robinson Thanks for taking care of this - Cole
Re: [PATCH 4/5] qemu: Don't cache domCaps in virQEMUDriverGetDomainCapabilities()
On 11/17/20 2:50 AM, Michal Privoznik wrote: > On 11/16/20 8:40 PM, Cole Robinson wrote: >> On 11/16/20 2:43 AM, Michal Privoznik wrote: >>> Currently, whenever a domain capabilities is needed (fortunately, >>> after cleanup done by previous commits it is now only in >>> virConnectGetDomainCapabilities()), the object is stored in a >>> cache. But there is no invalidation mechanism for the cache >>> (except the implicit one - the cache is part of qemuCaps and thus >>> share its lifetime, but that is not enough). Therefore, if >>> something changes - for instance new firmware files are >>> installed, or old are removed these changes are not reflected in >>> the virConnectGetDomainCapabilities() output. >>> >>> Originally, the caching was there because domCaps were used >>> during device XML validation and they were used a lot from our >>> test suite. But this is no longer the case. And therefore, we >>> don't need the cache and can construct fresh domCaps on each >>> virConnectGetDomainCapabilities() call. >>> >> >> Maybe the caching is not needed or was never needed. I added it as a >> premature optimization to facilitate sharing validation data between >> qemu and domcaps code. There's more info here: >> >> https://www.redhat.com/archives/libvir-list/2019-April/msg00450.html >> >> I was trying to establish a pattern where there was only one source of >> truth for things like 'this qemu supports VGA video'. The linked mail >> spells it out. >> >> My fear is that if domcapabilities data is duplicating what is done at >> qemu validation time, it's much more likely that domcapabilities content >> will bitrot. It's not hard to imagine a scenario where qemu validation >> gets tighter or more nuanced based on changing qemu capabilities, but >> domcapabilities data is not updated to match, and now domcapabilities is >> advertising a feature that qemu_validation.c immediately rejects. If >> that feature is say a video model that an app like virt-install or >> virt-manager is programmatically setting by default, now those tools are >> generating invalid configurations that libvirt claims qemu supports. >> Inaccurate domcapabilities can be worse than if nothing was reported >> at all.\ > > Yes, I was thinking about this too when writing these patches. And this > can happen if our validation code doesn't match domCaps building code. > So what if, for instance, instead of building whole domCaps, I'd export > virQEMUCapsFillDomainDeviceVideoCaps() and call it from > qemuValidateDomainDeviceDefVideo()? And then used ENUM_VALUE_MISSING() > macro to check if the model is supported? This way we will "force" all > those nuances to be implemented in domCaps building code. > Yes that sounds good to me. Thanks! - Cole
Re: [PATCH 4/5] qemu: Don't cache domCaps in virQEMUDriverGetDomainCapabilities()
On 11/16/20 2:43 AM, Michal Privoznik wrote: > Currently, whenever a domain capabilities is needed (fortunately, > after cleanup done by previous commits it is now only in > virConnectGetDomainCapabilities()), the object is stored in a > cache. But there is no invalidation mechanism for the cache > (except the implicit one - the cache is part of qemuCaps and thus > share its lifetime, but that is not enough). Therefore, if > something changes - for instance new firmware files are > installed, or old are removed these changes are not reflected in > the virConnectGetDomainCapabilities() output. > > Originally, the caching was there because domCaps were used > during device XML validation and they were used a lot from our > test suite. But this is no longer the case. And therefore, we > don't need the cache and can construct fresh domCaps on each > virConnectGetDomainCapabilities() call. > Maybe the caching is not needed or was never needed. I added it as a premature optimization to facilitate sharing validation data between qemu and domcaps code. There's more info here: https://www.redhat.com/archives/libvir-list/2019-April/msg00450.html I was trying to establish a pattern where there was only one source of truth for things like 'this qemu supports VGA video'. The linked mail spells it out. My fear is that if domcapabilities data is duplicating what is done at qemu validation time, it's much more likely that domcapabilities content will bitrot. It's not hard to imagine a scenario where qemu validation gets tighter or more nuanced based on changing qemu capabilities, but domcapabilities data is not updated to match, and now domcapabilities is advertising a feature that qemu_validation.c immediately rejects. If that feature is say a video model that an app like virt-install or virt-manager is programmatically setting by default, now those tools are generating invalid configurations that libvirt claims qemu supports. Inaccurate domcapabilities can be worse than if nothing was reported at all. - Cole
[PATCH v2] qemu: migration: don't open storage driver too early
If storage migration is requested, and the destination storage does
not exist on the remote host, qemu's migration support will call
into the libvirt storage driver to precreate the destination storage.
The storage driver virConnectPtr is opened too early though, adding
an unnecessary dependency on the storage driver for several cases
that don't require it. This currently requires kubevirt to install
the storage driver even though they aren't actually using it.
Push the virGetConnectStorage calls to right before the cases they are
actually needed.
Signed-off-by: Cole Robinson
---
v2:
Only open the connection once per VM via
qemuMigrationDstPrecreateStorage
src/qemu/qemu_migration.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 2000c86640..4e959abebf 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -169,7 +169,7 @@ qemuMigrationSrcRestoreDomainState(virQEMUDriverPtr driver,
virDomainObjPtr vm)
static int
-qemuMigrationDstPrecreateDisk(virConnectPtr conn,
+qemuMigrationDstPrecreateDisk(virConnectPtr *conn,
virDomainDiskDefPtr disk,
unsigned long long capacity)
{
@@ -204,7 +204,12 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
*volName = '\0';
volName++;
-if (!(pool = virStoragePoolLookupByTargetPath(conn, basePath)))
+if (!*conn) {
+if (!(*conn = virGetConnectStorage()))
+goto cleanup;
+}
+
+if (!(pool = virStoragePoolLookupByTargetPath(*conn, basePath)))
goto cleanup;
format = virStorageFileFormatTypeToString(disk->src->format);
if (disk->src->format == VIR_STORAGE_FILE_QCOW2)
@@ -212,7 +217,12 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
break;
case VIR_STORAGE_TYPE_VOLUME:
-if (!(pool = virStoragePoolLookupByName(conn,
disk->src->srcpool->pool)))
+if (!*conn) {
+if (!(*conn = virGetConnectStorage()))
+goto cleanup;
+}
+
+if (!(pool = virStoragePoolLookupByName(*conn,
disk->src->srcpool->pool)))
goto cleanup;
format = virStorageFileFormatTypeToString(disk->src->format);
volName = disk->src->srcpool->volume;
@@ -304,14 +314,11 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
{
int ret = -1;
size_t i = 0;
-virConnectPtr conn;
+virConnectPtr conn = NULL;
if (!nbd || !nbd->ndisks)
return 0;
-if (!(conn = virGetConnectStorage()))
-return -1;
-
for (i = 0; i < nbd->ndisks; i++) {
virDomainDiskDefPtr disk;
const char *diskSrcPath;
@@ -349,7 +356,8 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath));
-if (qemuMigrationDstPrecreateDisk(conn, disk, nbd->disks[i].capacity)
< 0)
+if (qemuMigrationDstPrecreateDisk(,
+ disk, nbd->disks[i].capacity) < 0)
goto cleanup;
}
--
2.28.0
Re: [PATCH] qemu: migration: don't open storage driver too early
On 10/12/20 5:05 AM, Daniel P. Berrangé wrote: > On Mon, Oct 12, 2020 at 10:55:52AM +0200, Michal Privoznik wrote: >> On 10/12/20 10:50 AM, Daniel P. Berrangé wrote: >>> On Sun, Oct 11, 2020 at 07:44:36PM -0400, Cole Robinson wrote: >>>> If storage migration is requested, and the destination storage does >>>> not exist on the remote host, qemu's migration support will call >>>> into the libvirt storage driver to precreate the destination storage. >>>> >>>> The storage driver virConnectPtr is opened too early though, adding >>>> an unnecessary dependency on the storage driver for several cases >>>> that don't require it. This currently requires kubevirt to install >>>> the storage driver even though they aren't actually using it. >>>> >>>> Push the virGetConnectStorage calls to right before the cases they are >>>> actually needed. >>> >>> This pushes the connection open attempts inside a loop body. So if the >>> VM has multiple disks, then we're going to be repeatedly opening and >>> closing the connection which is not desirable. >>> >>> I think we need a global connection across all disks, which is lazy >>> opened on first access. >> >> Doesn't virGetConnectStorage() cache the connection? > > Yes & no. > > You can call virSetConnectStorage(conn) to populate the cache with a > pre-opened connection. If you haven't done that, then a new connection > will be opened every time. The ltter is what this code will be doing. > What is the use case for not caching the connection by default? virSetConnect* has very few users, but virGetConnect* is sprinkled in many places, including multiple uses in generic code in domain_conf.c. Can this be done as part of virGetConnect*? - Cole
[PATCH] qemu: migration: don't open storage driver too early
If storage migration is requested, and the destination storage does
not exist on the remote host, qemu's migration support will call
into the libvirt storage driver to precreate the destination storage.
The storage driver virConnectPtr is opened too early though, adding
an unnecessary dependency on the storage driver for several cases
that don't require it. This currently requires kubevirt to install
the storage driver even though they aren't actually using it.
Push the virGetConnectStorage calls to right before the cases they are
actually needed.
Signed-off-by: Cole Robinson
---
src/qemu/qemu_migration.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 2000c86640..99a6b41483 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -169,8 +169,7 @@ qemuMigrationSrcRestoreDomainState(virQEMUDriverPtr driver,
virDomainObjPtr vm)
static int
-qemuMigrationDstPrecreateDisk(virConnectPtr conn,
- virDomainDiskDefPtr disk,
+qemuMigrationDstPrecreateDisk(virDomainDiskDefPtr disk,
unsigned long long capacity)
{
int ret = -1;
@@ -181,6 +180,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
const char *format = NULL;
unsigned int flags = 0;
+virConnectPtr conn = NULL;
VIR_DEBUG("Precreate disk type=%s",
virStorageTypeToString(disk->src->type));
@@ -204,6 +204,9 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
*volName = '\0';
volName++;
+if (!(conn = virGetConnectStorage()))
+goto cleanup;
+
if (!(pool = virStoragePoolLookupByTargetPath(conn, basePath)))
goto cleanup;
format = virStorageFileFormatTypeToString(disk->src->format);
@@ -212,6 +215,9 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
break;
case VIR_STORAGE_TYPE_VOLUME:
+if (!(conn = virGetConnectStorage()))
+goto cleanup;
+
if (!(pool = virStoragePoolLookupByName(conn,
disk->src->srcpool->pool)))
goto cleanup;
format = virStorageFileFormatTypeToString(disk->src->format);
@@ -270,6 +276,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
VIR_FREE(volStr);
virObjectUnref(vol);
virObjectUnref(pool);
+virObjectUnref(conn);
return ret;
}
@@ -304,13 +311,10 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
{
int ret = -1;
size_t i = 0;
-virConnectPtr conn;
if (!nbd || !nbd->ndisks)
return 0;
-if (!(conn = virGetConnectStorage()))
-return -1;
for (i = 0; i < nbd->ndisks; i++) {
virDomainDiskDefPtr disk;
@@ -349,13 +353,12 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath));
-if (qemuMigrationDstPrecreateDisk(conn, disk, nbd->disks[i].capacity)
< 0)
+if (qemuMigrationDstPrecreateDisk(disk, nbd->disks[i].capacity) < 0)
goto cleanup;
}
ret = 0;
cleanup:
-virObjectUnref(conn);
return ret;
}
--
2.28.0
[PATCH] spec: Add cpu.rng to %files
Fixes: 51v5d325240c645ea6c1a0902c695cf299410b1f90c
Signed-off-by: Cole Robinson
---
Pushed as trivial
libvirt.spec.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 933e12f4d2..52f30be096 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1868,6 +1868,7 @@ exit 0
%{_datadir}/libvirt/schemas/basictypes.rng
%{_datadir}/libvirt/schemas/capability.rng
+%{_datadir}/libvirt/schemas/cpu.rng
%{_datadir}/libvirt/schemas/cputypes.rng
%{_datadir}/libvirt/schemas/domain.rng
%{_datadir}/libvirt/schemas/domainbackup.rng
--
2.28.0
[PATCH] qemu: Taint cpu host-passthrough only after migration
>From a discussion last year[1], Dan recommended libvirt drop the tain
flag for cpu host-passthrough, unless the VM has been migrated.
This repurposes the existing host-cpu taint flag to do just that.
[1]: https://www.redhat.com/archives/virt-tools-list/2019-February/msg00041.html
https://bugzilla.redhat.com/show_bug.cgi?id=1673098
Signed-off-by: Cole Robinson
---
src/conf/domain_conf.h | 2 +-
src/qemu/qemu_domain.c | 7 +--
src/qemu/qemu_domain.h | 3 ++-
src/qemu/qemu_process.c | 2 +-
4 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9a44315519..450686dfb5 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2736,7 +2736,7 @@ typedef enum {
VIR_DOMAIN_TAINT_SHELL_SCRIPTS,/* Network configuration using opaque
shell scripts */
VIR_DOMAIN_TAINT_DISK_PROBING, /* Relying on potentially unsafe disk
format probing */
VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH, /* Externally launched guest domain */
-VIR_DOMAIN_TAINT_HOST_CPU, /* Host CPU passthrough in use */
+VIR_DOMAIN_TAINT_HOST_CPU, /* Host CPU passthrough in use after
migration */
VIR_DOMAIN_TAINT_HOOK, /* Domain (possibly) changed via hook
script */
VIR_DOMAIN_TAINT_CDROM_PASSTHROUGH,/* CDROM passthrough */
VIR_DOMAIN_TAINT_CUSTOM_DTB, /* Custom device tree blob was
specified */
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0331fd55e0..ed4bdbd7fb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6115,7 +6115,8 @@ void qemuDomainObjTaint(virQEMUDriverPtr driver,
void qemuDomainObjCheckTaint(virQEMUDriverPtr driver,
virDomainObjPtr obj,
- qemuDomainLogContextPtr logCtxt)
+ qemuDomainLogContextPtr logCtxt,
+ bool incomingMigration)
{
size_t i;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
@@ -6144,7 +6145,9 @@ void qemuDomainObjCheckTaint(virQEMUDriverPtr driver,
VIR_DOMAIN_TAINT_CUSTOM_HYPERVISOR_FEATURE,
logCtxt);
}
-if (obj->def->cpu && obj->def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH)
+if (obj->def->cpu &&
+obj->def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH &&
+incomingMigration)
qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_HOST_CPU, logCtxt);
for (i = 0; i < obj->def->ndisks; i++)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index ec776ced72..9bf32e16c9 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -567,7 +567,8 @@ void qemuDomainObjTaint(virQEMUDriverPtr driver,
void qemuDomainObjCheckTaint(virQEMUDriverPtr driver,
virDomainObjPtr obj,
- qemuDomainLogContextPtr logCtxt);
+ qemuDomainLogContextPtr logCtxt,
+ bool incomingMigration);
void qemuDomainObjCheckDiskTaint(virQEMUDriverPtr driver,
virDomainObjPtr obj,
virDomainDiskDefPtr disk,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9122069cc9..57d764014a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6856,7 +6856,7 @@ qemuProcessLaunch(virConnectPtr conn,
qemuLogOperation(vm, "starting up", cmd, logCtxt);
-qemuDomainObjCheckTaint(driver, vm, logCtxt);
+qemuDomainObjCheckTaint(driver, vm, logCtxt, incoming != NULL);
qemuDomainLogContextMarkPosition(logCtxt);
--
2.28.0
[PATCH] tests: cover disk, interface
There is present no XML test coverage for this.
Add genericxml parse + formatting coverage.
Signed-off-by: Cole Robinson
---
.../device-backenddomain.xml | 30 +++
.../device-backenddomain.xml | 1 +
tests/genericxml2xmltest.c| 1 +
3 files changed, 32 insertions(+)
create mode 100644 tests/genericxml2xmlindata/device-backenddomain.xml
create mode 12 tests/genericxml2xmloutdata/device-backenddomain.xml
diff --git a/tests/genericxml2xmlindata/device-backenddomain.xml
b/tests/genericxml2xmlindata/device-backenddomain.xml
new file mode 100644
index 00..8e89c7fec3
--- /dev/null
+++ b/tests/genericxml2xmlindata/device-backenddomain.xml
@@ -0,0 +1,30 @@
+
+ foo
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 219136
+ 219136
+ 1
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/genericxml2xmloutdata/device-backenddomain.xml
b/tests/genericxml2xmloutdata/device-backenddomain.xml
new file mode 12
index 00..f19471e3b5
--- /dev/null
+++ b/tests/genericxml2xmloutdata/device-backenddomain.xml
@@ -0,0 +1 @@
+../genericxml2xmlindata/device-backenddomain.xml
\ No newline at end of file
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index 102abfdec2..5110bfba86 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -233,6 +233,7 @@ mymain(void)
DO_TEST("launch-security-sev");
DO_TEST_DIFFERENT("cputune");
+DO_TEST("device-backenddomain");
#define DO_TEST_BACKUP_FULL(name, intrnl) \
do { \
--
2.28.0
[PATCH 4/4] docs: formatdomain: add spicevmc example
spicevmc is the most common usage. This adds an XML example for it. Signed-off-by: Cole Robinson --- docs/formatdomain.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 8fc08c5fd2..9316dab9cc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -4066,6 +4066,7 @@ after 0.9.5 (KVM only)` : ... + -- 2.28.0
[PATCH 3/4] docs: formatdomain: fix net downscript 'since'
It was added in 6.4.0, not 5.1.0 Signed-off-by: Cole Robinson --- docs/formatdomain.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 37c65add4d..8fc08c5fd2 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -,7 +,7 @@ After creating/opening the tap device, an optional shell script (given in the ``path`` attribute of the ``
[PATCH 1/4] docs: formatdomain: remove doubled filesystem
libvirt doesn't reject this but only one element takes effect. Drop the instance that is already referenced in the previous example Signed-off-by: Cole Robinson --- docs/formatdomain.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index f3cf9e1fb3..d75a91bbf4 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3064,7 +3064,6 @@ A directory on the host that can be accessed directly from the guest. - -- 2.28.0
[PATCH 2/4] docs: formatdomain: fix incorrect 'Vsock' heading indent
Currently it is visually at the same indent as . This fixes it to be grouped it with Signed-off-by: Cole Robinson --- docs/formatdomain.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index d75a91bbf4..37c65add4d 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -7286,7 +7286,7 @@ Example: :anchor:`` Vsock -- +~ A vsock host/guest interface. The ``model`` attribute defaults to ``virtio``. :since:`Since 5.2.0` ``model`` can also be 'virtio-transitional' and -- 2.28.0
[PATCH 0/4] docs: formatdomain: misc fixes
A collection of formatdomain fixes that I've noticed over the past few months Cole Robinson (4): docs: formatdomain: remove doubled filesystem docs: formatdomain: fix incorrect 'Vsock' heading indent docs: formatdomain: fix net downscript 'since' docs: formatdomain: add spicevmc example docs/formatdomain.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.28.0
Re: [libvirt PATCH] tests: fix incorrect free of GVariant in our GLib mock functions
On 10/2/20 6:36 AM, Pavel Hrdina wrote: > GLib implementation of g_dbus_connection_call_sync() calls > g_variant_ref_sink() on the passed @parameters to make sure they have > proper reference. If the original reference is floating the > g_dbus_connection_call_sync() consumes it, but if it's normal reference > it will just add another one. > > Our mock functions were only freeing the @parameters which is incorrect > and doesn't reflect how the real implementation works. > > Reported-by: Cole Robinson > Signed-off-by: Pavel Hrdina I added this to Fedora and the build passed. Thanks! - Cole
virsystemdtest segfault on ppc64
I'm seeing failures building libvirt 6.8.0 rpm on fedora 32, 33, and rawhide. virsystemdtest is segfaulting on ppc64. https://kojipkgs.fedoraproject.org//work/tasks/5494/52595494/build.log The output from the failed tests: 74/160 libvirt / virsystemdtest FAIL 0.44s (killed by signal 11 SIGSEGV) --- command --- 17:14:09 abs_srcdir='/builddir/build/BUILD/libvirt-6.8.0/tests' abs_top_srcdir='/builddir/build/BUILD/libvirt-6.8.0' VIR_TEST_EXPENSIVE='1' LIBVIRT_AUTOSTART='0' abs_top_builddir='/builddir/build/BUILD/libvirt-6.8.0/ppc64le-redhat-linux-gnu' LC_ALL='C' abs_builddir='/builddir/build/BUILD/libvirt-6.8.0/ppc64le-redhat-linux-gnu/tests' /builddir/build/BUILD/libvirt-6.8.0/ppc64le-redhat-linux-gnu/tests/virsystemdtest --- stderr --- TEST: virsystemdtest (process:3397099): GLib-CRITICAL **: 17:14:09.137: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.137: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.137: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.137: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.137: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.137: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.137: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.137: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.138: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.138: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.138: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . (process:3397099): GLib-CRITICAL **: 17:14:09.138: g_atomic_ref_count_dec: assertion 'g_atomic_int_get (arc) > 0' failed . --- Full log written to /builddir/build/BUILD/libvirt-6.8.0/ppc64le-redhat-linux-gnu/meson-logs/testlog.txt RPM build errors: error: Bad exit status from /var/tmp/rpm-tmp.Yh8pF5 (%check) Bad exit status from /var/tmp/rpm-tmp.Yh8pF5 (%check) Child return code was: 1 Thanks, Cole
Re: [libvirt PATCH] tests: fix license blurb in virsh-undefine
On 8/4/20 2:28 PM, Ján Tomko wrote: > Assume commit 0466ff28f2 used case-insensitive replace s/OUT/EXP/ > by mistake and this file is still licensed under GPLv2.0+ > > Undo the change. > > Signed-off-by: Ján Tomko > FIxes: 0466ff28f23f4c430906efd5859f87672cf08782 > Cc: Cole Robinson > Cc: Eric Blake > Cc: Pino Toscano > --- > tests/virsh-undefine | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/virsh-undefine b/tests/virsh-undefine > index 4a9f68dd39..998d4d3268 100755 > --- a/tests/virsh-undefine > +++ b/tests/virsh-undefine > @@ -9,7 +9,7 @@ > # (at your option) any later version. > > # This program is distributed in the hope that it will be useful, > -# but WITHEXP ANY WARRANTY; without even the implied warranty of > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > # GNU General Public License for more details. > > Reviewed-by: Cole Robinson - Cole
Re: Release of libvirt-6.5.0
On 7/3/20 3:56 AM, Daniel Veillard wrote: > Half a day late, but I pushed the 6.5.0 release out, it is as usual > available as a signed tarball and source rpms from the server: > > https://libvirt.org/sources/ > > I also tagged and pushed the 6.5.0 python bindings that one can find at > > https://libvirt.org/sources/python/ Hmm I'm not seeing 6.5.0 libvirt-python release there... Thanks, Cole
Re: Release of libvirt-6.1.0
I see the 6.1.0 commit in libvirt-python, but it seems the release .tar.gz wasn't uploaded: https://libvirt.org/sources/python/ Thanks, Cole On 3/4/20 4:40 AM, Daniel Veillard wrote: > It's out, one day late, though I did the build and pushed the tag > in git yesterday, but I had forgotten to push the commit, thanks > Boris for raising this to me. So it's now available as signed tarball > and rpm source package at the usual place: > >https://libvirt.org/sources/ >
Re: [libvirt PATCH] daemon: set default memlock limit for systemd service
On 2/26/20 10:07 AM, Pavel Hrdina wrote: > The default memlock limit is 64k which is not enough to start a single > VM. The requirements for one VM are 12k, 8k for eBPF map and 4k for eBPF > program, however, it fails to create eBPF map and program with 64k limit. > By testing I figured out that the minimal limit is 80k to start a single > VM with functional eBPF and if I add 12k I can start another one. > > This leads into following calculation: > > 80k as memlock limit worked to start a VM with eBPF which means there > is 68k of lock memory that I was not able to figure out what was using > it. So to get a number for 4096 VMs: > > 68 + 12 * 4096 = 49220 > > If we round it up we will get 49M of memory lock limit to support 4096 > VMs with default map size which can hold 64 entries for devices. > > This should be good enough as a sane default and users can change it if > the need to. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1807090 > > Signed-off-by: Pavel Hrdina > --- > src/remote/libvirtd.service.in | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in > index 9c8c54a2ef..8a3ace5bdb 100644 > --- a/src/remote/libvirtd.service.in > +++ b/src/remote/libvirtd.service.in > @@ -40,6 +40,11 @@ LimitNOFILE=8192 > # A conservative default of 8 tasks per guest results in a TasksMax of > # 32k to support 4096 guests. > TasksMax=32768 > +# With cgroups v2 there is no devices controller anymore, we have to use > +# eBPF to control access to devices. In order to do that we create a eBPF > +# hash MAP which locked memory. The default map size for 64 devices together > +# with program takes 12k per guest which results in 49M to support 4096 > guests. > +LimitMEMLOCK=49M > > [Install] > WantedBy=multi-user.target > I guess we will want this for virtqemud and virtlxcd as well. Any ideas if the root issue affects qemu:///session ? Thanks, Cole - Cole
Re: [PATCH v1 01/14] vircgroup: add virCgroupSetupBlkioTune()
On 2/11/20 4:21 PM, Daniel Henrique Barboza wrote: > > > On 2/11/20 11:35 AM, Cole Robinson wrote: >> On 2/11/20 7:28 AM, Daniel Henrique Barboza wrote: >>> > > [...] > >> >> I asked about this case in December, danpb suggested creating a new >> src/hypervisor/ directory for containing shared driver code like this: >> >> https://www.redhat.com/archives/libvir-list/2019-December/msg00817.html > > Thanks, I'm going for it. > > Do you have any suggestions for the file names? I see that you mentioned > "domain_cgroup" in that thread, thus I'm thinking about creating a new > src/hypervisor/domain_cgroup.c for the shared code between lxc_cgroup.c and > qemu_cgroup.c. > > I'm also considering a src/hypervisor/domain_driver.c for the driver common > code between lxc_driver.c and qemu_driver.c that I ended up finding in this > work. > Those both sound reasonable to me. Thanks, Cole
Re: [PATCH v1 01/14] vircgroup: add virCgroupSetupBlkioTune()
On 2/11/20 7:28 AM, Daniel Henrique Barboza wrote: > > > On 2/10/20 10:39 PM, Ján Tomko wrote: >> On Mon, Feb 10, 2020 at 07:05:07PM -0300, Daniel Henrique Barboza wrote: >>> This new util function puts the duplicated code from >>> qemu_cgroup.c:qemuSetupBlkioCgroup() and >>> lxc_cgroup.c:virLXCCgroupSetupBlkioTune() in a single >>> function to be used in both places. >>> >>> Signed-off-by: Daniel Henrique Barboza >>> --- >>> src/libvirt_private.syms | 1 + >>> src/lxc/lxc_cgroup.c | 49 +-- >>> src/qemu/qemu_cgroup.c | 47 +- >>> src/util/vircgroup.c | 55 >>> src/util/vircgroup.h | 3 +++ >>> 5 files changed, 61 insertions(+), 94 deletions(-) >>> >>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c >>> index 0680ff7c24..0d83e2094f 100644 >>> --- a/src/util/vircgroup.c >>> +++ b/src/util/vircgroup.c >>> @@ -35,6 +35,7 @@ >>> #define LIBVIRT_VIRCGROUPPRIV_H_ALLOW >>> #include "vircgrouppriv.h" >>> >>> +#include "conf/domain_conf.h" >>> #include "virutil.h" >>> #include "viralloc.h" >>> #include "vircgroupbackend.h" >>> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h >>> index 15263f534a..d2d7e7ab51 100644 >>> --- a/src/util/vircgroup.h >>> +++ b/src/util/vircgroup.h >>> @@ -24,6 +24,7 @@ >>> #include "virutil.h" >>> #include "virbitmap.h" >>> #include "virenum.h" >>> +#include "conf/virconftypes.h" >>> >> >> src/util should be below src/conf and not including stuff from it >> (even though we do have some debt there at the moment) >> >> The last time this came up: >> https://www.redhat.com/archives/libvir-list/2018-June/msg00381.html >> >> And there should've been a syntax-check rule yelling about this, >> but I see the out-of-srcdir build broke it: >> https://www.redhat.com/archives/libvir-list/2020-February/msg00442.html > > > Just saw that you fixed this check on newest master. syntax-check is not > pleased with these patches anymore. > > I'll send a v2. I asked about this case in December, danpb suggested creating a new src/hypervisor/ directory for containing shared driver code like this: https://www.redhat.com/archives/libvir-list/2019-December/msg00817.html - Cole
glib crash via eventtest.c
I attempted to review some patches on Friday and started hitting occasional crashes via eventtest.c. Long story short it's a glib bug: https://gitlab.gnome.org/GNOME/glib/merge_requests/1358 It's a ref counting issue caused when g_source_remove (virEventRemoveHandle) is called from one thread, while the main loop is in a particular state in a different thread. The way vireventglib is implemented means every user initiated g_source_remove is likely called from a different thread so we risk hitting this. Not sure how likely it is in realworld usage, vireventtest.c is pretty pathologic in this area. We could change vireventglib.c to do the final source_unref from the idle callback which would avoid the problem Thanks, Cole
Re: [libvirt] [PATCH v2 1/1] qemu: hide details of fake reboot
On 12/19/19 5:50 AM, Nikolay Shirokovskiy wrote: > If we use fake reboot then domain goes thru running->shutdown->running > state changes with shutdown state only for short period of time. At > least this is implementation details leaking into API. And also there is > one real case when this is not convinient. I'm doing a backup with the > help of temporary block snapshot (with the help of qemu's API which is > used in the newly created libvirt's backup API). If guest is shutdowned > I want to continue to backup so I don't kill the process and domain is > in shutdown state. Later when backup is finished I want to destroy qemu > process. So I check if it is in shutdowned state and destroy it if it > is. Now if instead of shutdown domain got fake reboot then I can destroy > process in the middle of fake reboot process. > > After shutdown event we also get stop event and now as domain state is > running it will be transitioned to paused state and back to running > later. Though this is not critical for the described case I guess it is > better not to leak these details to user too. So let's leave domain in > running state on stop event if fake reboot is in process. > > Reconnection code handles this patch without modification. It detects > that qemu is not running due to shutdown and then calls > qemuProcessShutdownOrReboot > which reboots as fake reboot flag is set. > > Signed-off-by: Nikolay Shirokovskiy Reviewed-by: Cole Robinson - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/3] Storage: Use rc hold intermediate function return values.
On 12/22/19 8:15 PM, Yi Li wrote: > most libvirt code uses 'int rc' to hold intermediate > function return values. consistent with the rest of libvirt. > > Signed-off-by: Yi Li > --- > src/storage/storage_backend_rbd.c | 202 > ++ > 1 file changed, 96 insertions(+), 106 deletions(-) > patch #2 had a hard tab in it, which made syntax-check fail. I fixed it and pushed this series. Please remember to check 'make check' and 'make syntax-check' pass on every patch before sending Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [go-xml PATCH 1/2] Fix virtio-s390 address lookup
On 12/23/19 5:02 AM, Daniel P. Berrangé wrote: > On Fri, Dec 20, 2019 at 05:22:25PM -0500, Cole Robinson wrote: >> It was comparing against spapr-vio >> >> Signed-off-by: Cole Robinson >> --- >> All this virtio-s390 stuff is kinda bogus though, because it is >> never actually output in libvirt XML. > > Hmm, I wonder what I was thinking when i added that. I guess we can > just delete it all. > There's an argument for removing virtio-s390 from libvirt entirely (not the XML bit, but the qemu implementation) Unclear if any supported distros even support it anymore, it's been removed from qemu for a few years IIRC (non-ccw virtio machine type) - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: Fix daemon crash on lookup storagepool by targetpath
On 12/20/19 7:33 PM, Yi Li wrote: > Causing a crash when storagePoolLookupByTargetPath beacuse of > Some types of storage pool have no target elements. > Use STREQ_NULLABLE instead of STREQ > Avoids segfaults when using NULL arguments. > > Core was generated by `/usr/sbin/libvirtd'. > Program terminated with signal 11, Segmentation fault. > (gdb) bt > 0 0x9e951388 in strcmp () from /lib64/libc.so.6 > 1 0x92103e9c in storagePoolLookupByTargetPathCallback ( > obj=0x7009aab0, opaque=0x801058b0) at > storage/storage_driver.c:1649 > 2 0x9f2c52a4 in virStoragePoolObjListSearchCb ( > payload=0x801058b0, name=, opaque=) > at conf/virstorageobj.c:476 > 3 0x9f1f2f7c in virHashSearch (ctable=0x800f4f60, > iter=iter@entry=0x9f2c5278 , > data=data@entry=0x95af7488, name=name@entry=0x0) at util/virhash.c:696 > 4 0x9f2c64f0 in virStoragePoolObjListSearch (pools=0x800f2ce0, > searcher=searcher@entry=0x92103e68 > , > opaque=) at conf/virstorageobj.c:505 > 5 0x92101f54 in storagePoolLookupByTargetPath (conn=0x5c0009f0, > path=0x7009a850 "/vms/images") at storage/storage_driver.c:1672 > > Signed-off-by: Yi Li Reviewed-by: Cole Robinson and pushed now - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Add support for vhost-user-scsi-pci/vhost-user-blk-pci
FYI there's some more recent discussion over here: https://www.redhat.com/archives/libvir-list/2019-December/msg00817.html There isn't any objections to using for vhost-user-blk, so maybe that's a good place to start. Thanks, Cole On 10/15/19 7:34 AM, Li Feng wrote: > Cole Robinson 于2019年10月15日周二 上午1:48写道： >> >> On 10/14/19 3:12 AM, Li Feng wrote: >>> Hi Cole & Michal, >>> >>> I'm sorry for my late response, I just end my journey today. >>> Thank your response, your suggestion is very helpful to me. >>> >>> I have added Michal in this mail, Michal helps me review my initial >>> patchset. >>> (https://www.spinics.net/linux/fedora/libvir/msg191339.html) >>> >> >> Whoops I missed that posting, I didn't realize you had sent patches! >> >>> All concern about this feature is the XML design. >>> My original XML design exposes more details of Qemu. >>> >>> >>> >>> >>> >>> >>> >>> >>> As Cole's suggestion, the better design with all vhost-user-scsi/blk >>> features would like this: >>> >>> vhost-user-blk: >>> >>> >>> >>> >>> >>> >>> >>> >> function='0x0'/> >>> >>> >>> vhost-user-scsi: >>> >>> >>> >>> >>> >>> >>> >>> >>> >> >> I think my SCSI idea is wrong, sorry. vhost-user-scsi is for passing a >> scsi host adapter to the VM, correct? If so, then it's not really a >> , and so using the existing vhost-scsi support in is >> probably better. could possible be used for vhost-user-blk as well >> >> Can you provide some examples of full qemu command lines using >> vhost-user-blk and vhost-user-scsi? Just linking to examples else where >> is fine, but I'm wondering if there's more context > > You could check the vhost-user-scsi/blk examples from SPDK pages: > https://spdk.io/doc/vhost.html > >> >> Internally we already have an abstraction for vhost-scsi: >> >> >> >> >> >> >> The obvious extension would be >> >> >> > path='/path/to/vhost-user-scsi.sock' mode='client'/> >> >> >> Internally implementing this will be weird. The parameters are >> only dictated by the hostdev type= field, but in this case they would be >> dictated by the field, and we would want to reuse the >> internal chardev abstraction. >> >> vhost-user-blk could be implemented similarly, but with type='storage' >> which is the way we pass through block devices to LXC guests, but it >> isn't currently supported in the qemu driver. >> >> I dunno. Maybe Michal or danpb can provide guidance >> > @Michal, @danpb, could you give some guidance? >> >>> Conclusion: >>> 1. Add new type(vhostuser) for disk label; >>> 2. Add queue sub-label for disk to support multiqueue(>> num='4'/>) or reusing the driver label >>> (>> Qemu support multiqueue like this: >>> -device vhost-user-scsi-pci,id=scsi0,chardev=spdk_vhost_scsi0,num_queues=4 >>> -device vhost-user-blk-pci,chardev=spdk_vhost_blk0,num-queues=4 >>> >> >> num-queues is already supported by libvirt for both and >> with , so whether we use or you won't >> need to add any new XML here. > Got it. > >> >>> Another question: >>> When qemu is connecting to a vhost-user-scsi controller[1], there may >>> exist multiple LUNs under one target, >>> then one disklabel() will represent multiple SCSI LUNs, >>> the 'dev' property() will be ignored, right? >>> In other words, for vhost-user-scsi disk, it more likes a controller, >>> maybe the controller label is suitable. >>> >> >> Yes you are right, and this was my understanding. But then its not >> really a in libvirt's sense because we can't attach >> emulated devices to it, so it's more a fit for the existing >> vhost-user support. Unfortunately it's not really a clean fit anywhere, >> there will have to be some kind of compromise. And I'm not sure if >> or is right for vhost-user-blk, but hopefully others >> have more clear opinions. > > I'm also confused about it. > Thanks for your reply. > > Thanks, > Feng Li > >> >> Thanks, >> Cole >> >>> I look forward to he
Re: [libvirt] libvirt API/design questions
On 12/12/19 9:57 AM, Ján Tomko wrote: > On Wed, Dec 11, 2019 at 09:45:07AM -0500, Cole Robinson wrote: >> * Splitting domain_conf up. There's 30k lines in domain_conf.c and 3500 >> in domain_conf.h, it would be nice to unwind it a bit. Getting some sign >> off on this ahead of time is critical IMO so pieces can be posted and >> committed quickly because they will obviously go out of date fast. > > Oh yes, so fast I did not even keep the branch for this failed attempt: > https://www.redhat.com/archives/libvir-list/2019-July/msg01257.html > Ah I must have missed that posting before. If after the break you want to coordinate, I'm happy to do quick review of any domain_conf split so it can go in quickly Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt API/design questions
On 12/12/19 6:13 AM, Daniel P. Berrangé wrote:
> On Wed, Dec 11, 2019 at 09:45:07AM -0500, Cole Robinson wrote:
>> There's some pre-existing libvirt design questions I would like some
>> feedback on, and one new one
>>
>>
>> * `virsh blockresize` doesn't prevent shrink by default, couple users
>> have blown their foot off and there's attempts at patches.
>> https://www.redhat.com/archives/libvir-list/2019-October/msg00258.html
>> https://www.redhat.com/archives/libvir-list/2019-November/msg00843.html
>>
>> Do we implement this as a protection in virsh, or change the API
>> behavior and require a new API flag to allow shrinking? Or some other idea?
>
> Clearly we should have had protection when first implementing this.
> We can't change the default API behaviour now as that would break
> existing valid usage.
>
> I think it is reasonable to change virsh though to try to add
> protection in some way.
>
>> * vhost-user-scsi and vhost-user-blk XML schema
>> https://www.redhat.com/archives/libvirt-users/2019-October/msg00018.html
>>
>> Last proposal was using for this, which revisiting it now
>> makes more sense than , because it vhost-user-X doesn't seem to
>> use qemu's block layer at all. So vhost-user-scsi would be:
>>
>>
>> > path='/path/to/vhost-user-scsi.sock' mode='client'/>
>>
>>
>> vhost-user-blk maybe requires a new type='block' ? Otherwise it's
>> unclear how to differentiate between the two. Regardless it would be
>> nice to give the original patch author guidance here, they seemed
>> motivated to get this working
>
> This is a really tricky question in general. It is basically saying
> whether we consider to refer to the guest visible device
> type or the QEMU visible backend type.
>
> Originally these were always the same thing, but offloading to
> vhostuser has blurred the boundaries. I think in non-QEMU hypervisors
> where you don't have a split of frontend in the config, you'd
> just have disks no matter what.
>
> With network we've continued to use with vhost-user.
>
> This makes me biased towards , at least for vhost-user-blk.
>
Okay, makes sense to me.
> I presume that with vhost-user-scsi we're exposing the entire
> SCSI controller, so we'd need a . As you show
> above we do have use of already for scsi_host
> but that's for something that's known to the kernel. We
> can reasonably argue that vhost-uuser-scsi is not the same
> as scsi_host as its still emulated.
>
> I think we should bear in mind that using vhost-user-blk/scsi
> does *not* imply that QEMU's block layer is not there. The
> backend vhost-user process could be implemented in QEMU
> codebase & thus have a QMP monitor and full QEMU block backend
> featureset. This isn't the case today, but it is at least
> conceivable for the future. This would bias towards
> at least for vhostuser-blk. vhost-user-scsi is more difficult
> still.
>
The downside of using for the scsi case is that it will
complicate libvirt SCSI address handling. Really in practical terms, a
libvirt controller is something you can assign other device
onto. With vhost-user-scsi that won't be the case at first. So if we use
or similar it's going to
make things a headache internally and possibly mess up bad app
assumptions. I guess it could be type='vhostuserscsi' too, to signify
it's entirely different.
So maybe is the simpler path forward in that case even though
it's not a clean conceptual fit their either.
>> * Splitting domain_conf up. There's 30k lines in domain_conf.c and 3500
>> in domain_conf.h, it would be nice to unwind it a bit. Getting some sign
>> off on this ahead of time is critical IMO so pieces can be posted and
>> committed quickly because they will obviously go out of date fast. My
>> thoughts:
>>
>> - domain_def.[ch]: DomainDefXXX and enum definitions.
>> - probably New and Free functions too
>> - domain_parse.[ch]: XML parsing
>> - domain_format.[ch]: XML formatting
>> - domain_validate.[ch]: validate and postparse handling
>> - domain_util.[ch]: everything else, or keep it in domain_conf?
>
> FWIW, if we're introducing new files, I'd like to see is move
> to the new naming based on object type.
>
> ievirdomaindef.[ch], virdomainobj.[ch]
>
> if virdomaindef.[ch] are too big, then virdomaindef{validate,util}.[ch]
> etc. I'd keep the base file name as purely the XML parse/format code,
> and move any helper / addon logic out.
>
Makes sense, though I think it would be helpful and an easy first step
to move the structs and enums to their own file. So maybe:
virdomaindef.[ch]: structs, enums, maybe New and Free
Re: [libvirt] virsh blockresize syntax is inconsistent with vol-resize and somewhat dangerous
On 11/18/19 3:16 PM, Cole Robinson wrote: > On 10/25/19 4:28 AM, Patrik Martinsson wrote: >> Hi Tim, >> >> I recently stumbled on the same thing, accidentally shrinking a blockdevice. >> >> I have written a patch for virsh that will force the user to append a >> '--force' flag if shrinking is desired. >> >> The behavior is somewhat still inconsistent with the vol-resize >> command, however a bigger rewrite is needed to make both commands >> operate exactly the same, which I don't know if actually needed. >> >> Previous discussion can be found below, >> - https://www.redhat.com/archives/libvir-list/2019-October/msg00258.html >> - https://www.redhat.com/archives/libvir-list/2019-October/msg01437.html >> >> Best regards, >> Patrik >> >> >> On Thu, Oct 24, 2019 at 6:04 PM Tim Small wrote: >>> >>> Hello, >>> >>> virsh has two commands which can be used to resize block devices - >>> "blockresize" for volumes in use by and active guest, and "vol-resize" >>> for volumes which are not in use. >>> >>> The vol-resize syntax allows to specify the size as a delta (increase or >>> decrease vs. the current size), and also refuses to shrink a volume >>> unless the "--shrink" argument is also passed. >>> >>> Most other tools which can be used for block device resizing (outside of >>> libvirt) also have similar "--shrink" argument requirements when >>> reducing the size of an existing block device. e.g. ceph requires >>> "--allow-shrink" when using the "rbd resize" command. >>> >>> The lack of such a safety device makes "blockresize" a foot-gun (which I >>> recently found to great effect when I typoed the domain name to another >>> valid domain). >>> >>> It seems I am not alone in making this error e.g. >>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=902171 >>> >>> One possible solution would be to make a new command e.g. "domblkresize" >>> or perhaps "live-resize", which implement the "--shrink" and "--delta" >>> behaviour to make it consistent with "vol-resize" syntax, and mark the >>> "blockresize" command as deprecated in the documentation and help (so >>> that existing automation which depends on the current behaviour doesn't >>> break). >>> >>> Any thoughts? Should I open this as an RFE? >>> > > Considering there's been multiple people hitting it, I think it's > something we should fix in libvirt. Just need buy in from other devs. To > summarize: > > 'virsh blockresize' will online resize an image path for a running VM. > It does this with the qemu block_resize monitor command via the > virDomainBlockResize API. The API doesn't provide any protection against > shrinking the disk image though, which I presume is both the less common > intention of the operation, and much less often safe to do for a running > VM. And a user typo can mean data loss > > virsh vol-resize, which is storage API virStorageVolResize, is for > offline image resizing, mostly using qemu-img. It has had a SHRINK API > flag from the outset, rejecting requests to reduce the image size unless > the flag is passed. Seems like a safe pattern to follow. > > Can we change existing blockresize behavior? I think it's reasonable; > we've added flags to other APIs that are required to restore old > behavior, UNDEFINE_NVRAM for one example. > I brought this question up in this thread: https://www.redhat.com/archives/libvir-list/2019-December/msg00817.html danpb suggested making this a protection that lives in virsh only. So, change blockresize to reject shrinking, but add a --shrink option to override that behavior, and all the code lives in tools/ so the old API behavior is preserved. You can CC me on a patch and I'll review it (but I'll be offline until January) Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 3/5] qemu_process.c: use g_autoptr()
On 12/20/19 4:16 PM, Daniel Henrique Barboza wrote:
> Change all feasible pointers to use g_autoptr().
>
> Signed-off-by: Daniel Henrique Barboza
> ---
> src/qemu/qemu_process.c | 123 ++--
> 1 file changed, 41 insertions(+), 82 deletions(-)
> @@ -4340,7 +4314,7 @@ qemuProcessFetchCPUDefinitions(virQEMUDriverPtr driver,
> virDomainCapsCPUModelsPtr *cpuModels)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> -virDomainCapsCPUModelsPtr models = NULL;
> +g_autoptr(virDomainCapsCPUModels) models = NULL;
> int rc;
>
> if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> @@ -4355,7 +4329,6 @@ qemuProcessFetchCPUDefinitions(virQEMUDriverPtr driver,
> return 0;
>
> error:
> -virObjectUnref(models);
> return -1;
> }
The 'error:' label here tipped me that something might be off. 'models'
is only supposed to be freed on error, but this frees it
unconditionally. I adapted this function to use g_steal_pointer as well,
and pushed this series with that addition
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [go-xml PATCH 2/2] Support domain address type='unassigned'
Signed-off-by: Cole Robinson
---
This should fix the libvirt-go-xml CI
domain.go | 23 +++
1 file changed, 23 insertions(+)
diff --git a/domain.go b/domain.go
index 7552c4b..c290003 100644
--- a/domain.go
+++ b/domain.go
@@ -887,6 +887,9 @@ type DomainAddressCCID struct {
type DomainAddressVirtioS390 struct {
}
+type DomainAddressUnassigned struct {
+}
+
type DomainAddress struct {
PCI *DomainAddressPCI
Drive*DomainAddressDrive
@@ -899,6 +902,7 @@ type DomainAddress struct {
VirtioMMIO *DomainAddressVirtioMMIO
ISA *DomainAddressISA
DIMM *DomainAddressDIMM
+ Unassigned *DomainAddressUnassigned
}
type DomainChardevLog struct {
@@ -4801,6 +4805,12 @@ func (a *DomainAddressVirtioS390) MarshalXML(e
*xml.Encoder, start xml.StartElem
return nil
}
+func (a *DomainAddressUnassigned) MarshalXML(e *xml.Encoder, start
xml.StartElement) error {
+ e.EncodeToken(start)
+ e.EncodeToken(start.End())
+ return nil
+}
+
func (a *DomainAddress) MarshalXML(e *xml.Encoder, start xml.StartElement)
error {
if a.USB != nil {
start.Attr = append(start.Attr, xml.Attr{
@@ -4857,6 +4867,11 @@ func (a *DomainAddress) MarshalXML(e *xml.Encoder, start
xml.StartElement) error
xml.Name{Local: "type"}, "virtio-s390",
})
return e.EncodeElement(a.VirtioS390, start)
+ } else if a.Unassigned != nil {
+ start.Attr = append(start.Attr, xml.Attr{
+ xml.Name{Local: "type"}, "unassigned",
+ })
+ return e.EncodeElement(a.Unassigned, start)
} else {
return nil
}
@@ -5102,6 +5117,11 @@ func (a *DomainAddressVirtioS390) UnmarshalXML(d
*xml.Decoder, start xml.StartEl
return nil
}
+func (a *DomainAddressUnassigned) UnmarshalXML(d *xml.Decoder, start
xml.StartElement) error {
+ d.Skip()
+ return nil
+}
+
func (a *DomainAddress) UnmarshalXML(d *xml.Decoder, start xml.StartElement)
error {
var typ string
for _, attr := range start.Attr {
@@ -5148,6 +5168,9 @@ func (a *DomainAddress) UnmarshalXML(d *xml.Decoder,
start xml.StartElement) err
} else if typ == "virtio-s390" {
a.VirtioS390 = {}
return d.DecodeElement(a.VirtioS390, )
+ } else if typ == "unassigned" {
+ a.Unassigned = {}
+ return d.DecodeElement(a.Unassigned, )
}
return nil
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [go-xml PATCH 0/2] Fix libvirt-go-xml test CI
Patch 1 is a nearby 'fix' Patch 2 fixes the CI for the new address type='unassigned' Cole Robinson (2): Fix virtio-s390 address lookup Support domain address type='unassigned' domain.go | 25 - 1 file changed, 24 insertions(+), 1 deletion(-) -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [go-xml PATCH 1/2] Fix virtio-s390 address lookup
It was comparing against spapr-vio
Signed-off-by: Cole Robinson
---
All this virtio-s390 stuff is kinda bogus though, because it is
never actually output in libvirt XML.
domain.go | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/domain.go b/domain.go
index 8c4d7a4..7552c4b 100644
--- a/domain.go
+++ b/domain.go
@@ -5145,7 +5145,7 @@ func (a *DomainAddress) UnmarshalXML(d *xml.Decoder,
start xml.StartElement) err
} else if typ == "ccid" {
a.CCID = {}
return d.DecodeElement(a.CCID, )
- } else if typ == "spapr-vio" {
+ } else if typ == "virtio-s390" {
a.VirtioS390 = {}
return d.DecodeElement(a.VirtioS390, )
}
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 0/4] qemu_process: Glib sponsored cleanup
On 12/20/19 3:54 PM, Michal Prívozník wrote: > On 12/20/19 7:57 PM, Cole Robinson wrote: >> On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote: >>> This series got buried a few months ago. Let's go onward unto >>> the 20s with no one left behind. >>> >>> changes from version 3 [1]: >>> - rebased to master at commit 330b556829 >>> - removed former patch 4. The 'g_strdup_printf' change was >>> made along the road >>> - new patch 4: I am sending this one in separate to patch 3 >>> because this unneeded label was already in the code, being >>> unrelated to the changes of this series. The maintainer is >>> welcome to squash it into patch 3. >>> >>> [1] https://www.redhat.com/archives/libvir-list/2019-October/msg01080.html >>> >>> Daniel Henrique Barboza (4): >>> qemu_process.c: use g_autofree >>> qemu_process.c: use g_autoptr() >>> qemu_process.c: remove cleanup labels after g_auto*() changes >>> qemu_process.c: remove 'cleanup' label from >>> qemuProcessCreatePretendCmd() >>> >>> src/qemu/qemu_process.c | 429 +++- >>> 1 file changed, 157 insertions(+), 272 deletions(-) >>> >> >> Patch 3 and 4 look fine, some comments on the first two. >> >> I can't really decide what is the best way to approach cleanups like >> this. Should patches be split by function, and do all cleanups in one >> pass, or do one type of cleanup, but over a larger surface per file? >> Like you have done here. >> >> The first method is more tedious, but it's easier for reviewers, and >> good patches can go in first while patches with issues can be kicked out >> for v2. But it could be thousands of patches judging by the 3000+ >> cleanup labels we have in the code base, which sounds extreme. >> >> I think in general you will find the list more receptive to series of >> small per function patches though. Optimizing for ease of review will >> always give things a better chance of getting committed in my >> experience. This is just recommendation for future series though, I'll >> review this one as is > > I think the sooner we get this over with the better (i.e. less rebase > conflicts). It's like tearing off a patch (bandage?) - it hurts less if > you do it all at once. > Yes I agree. Dropping the ALLOC < 0 bits shouldn't be done incrementally. It should be a largely mechanical change, everything under the 'if' is already dead code. - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
