[libvirt] [PATCH v2 4/6] tpm: Label the external swtpm with SELinux labels
In this patch we label the swtpm process with SELinux labels. We give it the same label as the QEMU process has. We label its state directory and files as well. The file and process labels now look as follows: Directory: /var/lib/libvirt/swtpm [root@localhost swtpm]# ls -lZ total 4 rwx--. 2 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 4096 Apr 5 16:46 testvm [root@localhost testvm]# ls -lZ total 8 -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 3648 Apr 5 16:46 tpm-00.permall The log in /var/log/swtpm/libvirt/qemu is labeled as follows: -rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 2237 Apr 5 16:46 vtpm.log [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep ctrl | grep -v grep system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0 0.0 28172 3892 ? Ss 16:57 0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0 0.0 3096704 48500 ? Sl 16:57 3:28 /bin/qemu-system-x86_64 [..] Signed-off-by: Stefan Berger --- src/libvirt_private.syms| 1 + src/qemu/qemu_extdevice.c | 22 - src/security/security_driver.h | 4 +++ src/security/security_manager.c | 18 +++ src/security/security_manager.h | 3 ++ src/security/security_selinux.c | 68 + src/security/security_stack.c | 19 7 files changed, 134 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 935ffcc..af9163f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1334,6 +1334,7 @@ virSecurityManagerSetProcessLabel; virSecurityManagerSetSavedStateLabel; virSecurityManagerSetSocketLabel; virSecurityManagerSetTapFDLabel; +virSecurityManagerSetTPMLabels; virSecurityManagerStackAddNested; virSecurityManagerTransactionAbort; virSecurityManagerTransactionCommit; diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index be3df7c..ee327ca 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -141,12 +141,32 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, virCommandSetErrorBuffer(cmd, &errbuf); -if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { +if (virSecurityManagerSetTPMLabels(driver->securityManager, + def) < 0) +goto error; + +if (virSecurityManagerSetChildProcessLabel(driver->securityManager, + def, cmd) < 0) +goto error; + +if (virSecurityManagerPreFork(driver->securityManager) < 0) +goto error; + +/* make sure we run this with the appropriate user */ +virCommandSetUID(cmd, cfg->swtpm_user); +virCommandSetGID(cmd, cfg->swtpm_user); + +ret = virCommandRun(cmd, &exitstatus); + +virSecurityManagerPostFork(driver->securityManager); + +if (ret < 0 || exitstatus != 0) { VIR_ERROR("Could not start 'swtpm'. exitstatus: %d\n" "stderr: %s\n", exitstatus, errbuf); virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not start 'swtpm'. exitstatus: %d, " "error: %s"), exitstatus, errbuf); +ret = -1; goto error; } diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 95e7c4d..4aa415f 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -149,6 +149,8 @@ typedef int (*virSecurityDomainRestoreChardevLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainChrSourceDefPtr dev_source, bool chardevStdioLogd); +typedef int (*virSecurityDomainSetTPMLabels) (virSecurityManagerPtr mgr, + virDomainDefPtr def); struct _virSecurityDriver { @@ -213,6 +215,8 @@ struct _virSecurityDriver { virSecurityDomainSetChardevLabel domainSetSecurityChardevLabel; virSecurityDomainRestoreChardevLabel domainRestoreSecurityChardevLabel; + +virSecurityDomainSetTPMLabels domainSetSecurityTPMLabels; }; virSecurityDriverPtr virSecurityDriverLookup(const char *name, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index fdeea4d..0547daa 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1,3 +1,4 @@ + /* * security_manager.c: Internal security manager API * @@ -1207,3 +1208,20 @@ virSecurityManagerRestoreChardevLabel(virSecurityManagerPt
[libvirt] [PATCH v2 2/6] tpm: Add support for external swtpm TPM emulator
This patch adds support for an external swtpm TPM emulator. The XML for this type of TPM looks as follows: The XML will currently only start a TPM 1.2. Upon first start, libvirt will run `swtpm_setup`, which will simulate the manufacturing of a TPM and create certificates for it and write them into NVRAM locations of the emulated TPM. After that libvirt starts the swtpm TPM emulator using the `swtpm` executable. Once the VM terminates, libvirt uses the swtpm_ioctl executable to gracefully shut down the `swtpm` in case it is still running (QEMU did not send shutdown) or clean up the socket file. The above mentioned executables must be found in the PATH. The executables can either be run as root or started as root and switch to the tss user. The requirement for the tss user comes through 'tcsd', which is used for the simulation of the manufacturing. Which user is used can be configured through qemu.conf. By default 'tss' is used. The swtpm writes out state into files. The state is kept in /var/lib/libvirt/swtpm: [root@localhost libvirt]# ls -lZ | grep swtpm drwx--x--x. 7 root root unconfined_u:object_r:virt_var_lib_t:s0 4096 Apr 5 16:22 swtpm The directory /var/lib/libvirt/swtpm maintains per-TPM state directories. [root@localhost swtpm]# ls -lZ total 4 drwx--. 2 tss tss system_u:object_r:virt_var_lib_t:s0 4096 Apr 5 16:46 testvm [root@localhost testvm]# ls -lZ total 4 drwx--. 2 tss tss system_u:object_r:virt_var_lib_t:s0 4096 Apr 10 21:34 tpm1.2 [root@localhost tpm1.2]# ls -lZ total 8 -rw-r--r--. 1 tss tss system_u:object_r:virt_var_lib_t:s0 3648 Apr 5 16:46 tpm-00.permall The directory /var/run/libvirt/qemu/swtpm/domain-1-testvm hosts the swtpm.sock that QEMU uses to communicate with the swtpm: root@localhost domain-1-testvm]# ls -lZ total 0 srw---. 1 qemu qemu system_u:object_r:svirt_image_t:s0:c597,c632 0 Apr 6 10:24 swtpm.sock The logfile for the swtpm is in /var/log/swtpm/libvirt/qemu: [root@localhost-3 qemu]# ls -lZ total 4 -rw---. 1 tss tss unconfined_u:object_r:var_log_t:s0 2199 Apr 6 14:01 testvm-swtpm.log The processes are labeled as follows: [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep socket | grep -v grep system_u:system_r:virtd_t:s0-s0:c0.c1023 tss 18697 0.0 0.0 28172 3892 ? Ss 16:46 0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log [root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep system_u:system_r:svirt_t:s0:c413,c430 qemu 18702 2.5 0.0 3036052 48676 ? Sl 16:46 0:08 /bin/qemu-system-x86_64 [...] Signed-off-by: Stefan Berger --- docs/formatdomain.html.in | 30 +++ docs/schemas/domaincommon.rng | 5 + src/conf/domain_audit.c| 2 + src/conf/domain_conf.c | 49 +++- src/conf/domain_conf.h | 7 + src/libvirt_private.syms | 7 + src/qemu/Makefile.inc.am | 2 + src/qemu/libvirtd_qemu.aug | 3 + src/qemu/qemu.conf | 7 + src/qemu/qemu_capabilities.c | 5 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_cgroup.c | 1 + src/qemu/qemu_command.c| 52 +++- src/qemu/qemu_conf.c | 35 ++- src/qemu/qemu_conf.h | 5 + src/qemu/qemu_domain.c | 4 + src/qemu/qemu_driver.c | 7 + src/qemu/qemu_extdevice.c | 264 src/qemu/qemu_extdevice.h | 44 src/qemu/qemu_process.c| 12 + src/qemu/test_libvirtd_qemu.aug.in | 1 + src/security/security_dac.c| 6 + src/security/security_selinux.c| 7 + src/util/virfile.c | 60 + src/util/virfile.h | 2 + src/util/virtpm.c | 493 - src/util/virtpm.h | 25 +- 27 files changed, 1121 insertions(+), 15 deletions(-) create mode 100644 src/qemu/qemu_extdevice.c create mode 100644 src/qemu/qemu_extdevice.h diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 16fc7db..bd6fedc 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7621,6 +7621,26 @@ qemu-kvm -net nic,model=? /dev/null ... + + + The emulator device type gives access to a TPM emulator providing + TPM functionlity for each VM. QEMU talks to it over a UnixIO socket. With + the emulator device type each guest gets its own private TPM. + 'emulator' since 4.x.y + + + Example: usage of the TPM Emulator + + + ... ++ + ... + model @@ -7653,6 +7673,16+ ++ +
[libvirt] [PATCH v2 3/6] tpm: Add test cases for external swtpm TPM emulator
This patch adds extensions to existing test cases and specific test cases for the tpm-emulator. Signed-off-by: Stefan Berger --- tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemuxml2argvdata/tpm-emulator.args | 24 +++ tests/qemuxml2argvdata/tpm-emulator.xml| 30 +++ tests/qemuxml2argvtest.c | 15 ++ tests/qemuxml2xmloutdata/tpm-emulator.xml | 34 ++ tests/qemuxml2xmltest.c| 1 + 10 files changed, 109 insertions(+) create mode 100644 tests/qemuxml2argvdata/tpm-emulator.args create mode 100644 tests/qemuxml2argvdata/tpm-emulator.xml create mode 100644 tests/qemuxml2xmloutdata/tpm-emulator.xml diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml index 70a35ef..376f58a 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -150,6 +150,7 @@ + 2011000 0 342058 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml index ff48293..069e0ae 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -187,6 +187,7 @@ + 2011090 0 342346 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index ee7fb9e..46d2463 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -185,6 +185,7 @@ + 2011090 0 419215 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index b5b6b5b..36ffd75 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -150,6 +150,7 @@ + 2011090 0 0 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 39ee4f4..b2f06b3 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -226,6 +226,7 @@ + 2011090 0 390060 diff --git a/tests/qemuxml2argvdata/tpm-emulator.args b/tests/qemuxml2argvdata/tpm-emulator.args new file mode 100644 index 000..9418c74 --- /dev/null +++ b/tests/qemuxml2argvdata/tpm-emulator.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name TPM-VM \ +-S \ +-M pc-0.12 \ +-m 2048 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 11d7cd22-da89-3094-6212-079a48a309a1 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-TPM-VM/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-boot c \ +-usb \ +-tpmdev emulator,id=tpm-tpm0,chardev=chrtpm \ +-chardev socket,id=chrtpm,path=/dev/test \ +-device tpm-tis,tpmdev=tpm-tpm0,id=tpm0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/tpm-emulator.xml b/tests/qemuxml2argvdata/tpm-emulator.xml new file mode 100644 index 000..2f4e777 --- /dev/null +++ b/tests/qemuxml2argvdata/tpm-emulator.xml @@ -0,0 +1,30 @@ + + TPM-VM + 11d7cd22-da89-3094-6212-079a48a309a1 + 2097152 + 512288 + 1 + +hvm + + + + + + + + destroy + restart + destroy + +/usr/bin/qemu-system-x86_64 + + + + + + + + + + diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 2992197..06dca97 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -497,6 +497,19 @@ testCompareXMLToArgv(const void *data) } } +if (vm->def->tpm) { + switch (vm->def->tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + if (VIR_STRDUP(vm->def->tpm->data.emulator.source.data.file.path, + "/dev/test") < 0) + goto cleanup; + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } +} + if (!(cmd = qemuProcessCreatePretendCmd(&driver, vm, migrateURI, (flags & FLAG_FIPS), false, VIR_QEMU_PROCESS_START_COLD))) { @@ -2139,6 +2152,8 @@ mymain(void) QEMU_CAPS_DEVICE_TPM_CRB); DO_TEST_PARSE_ERROR("tpm-no-backend-invalid", QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS); +
[libvirt] [PATCH v2 6/6] tpm: Add swtpm to emulator cgroup
Add the external swtpm to the emulator cgroup so that upper limits of CPU usage can be enforced on the emulated TPM. To enable this we need to have the swtpm write its process id (pid) into a file. We then read it from the file to configure the emulator cgroup. The PID file is created in /var/run/libvirt/qemu/swtpm: [root@localhost swtpm]# ls -lZ /var/run/libvirt/qemu/swtpm/ total 4 -rw-r--r--. 1 tss tss system_u:object_r:qemu_var_run_t:s0 5 Apr 10 12:26 testvm-swtpm.pid srw-rw. 1 qemu qemu system_u:object_r:svirt_image_t:s0:c597,c632 0 Apr 10 12:26 testvm-swtpm.sock The swtpm command line now looks as follows: root@localhost testvm]# ps auxZ | grep swtpm | grep socket | grep -v grep system_u:system_r:virtd_t:s0:c597,c632 tss 18697 0.0 0.0 28172 3892 ? Ss 16:46 0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log --pid file=/var/run/libvirt/qemu/swtpm/testvm-swtpm.pid Signed-off-by: Stefan Berger --- src/conf/domain_conf.c| 1 + src/conf/domain_conf.h| 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c| 53 +++ src/qemu/qemu_cgroup.h| 1 + src/qemu/qemu_extdevice.c | 19 + src/qemu/qemu_process.c | 4 src/util/vircgroup.c | 42 + src/util/vircgroup.h | 1 + src/util/virtpm.c | 33 + 10 files changed, 156 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0bbb547..e19f7dc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2620,6 +2620,7 @@ void virDomainTPMDefFree(virDomainTPMDefPtr def) VIR_FREE(def->data.emulator.source.data.nix.path); VIR_FREE(def->data.emulator.storagepath); VIR_FREE(def->data.emulator.logfile); +VIR_FREE(def->data.emulator.pidfile); break; case VIR_DOMAIN_TPM_TYPE_LAST: break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 80f599c..34bd4a2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1309,6 +1309,7 @@ struct _virDomainTPMDef { virDomainChrSourceDef source; char *storagepath; char *logfile; +char *pidfile; } emulator; } data; }; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index af9163f..00cb294 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1467,6 +1467,7 @@ virBufferVasprintf; # util/vircgroup.h virCgroupAddMachineTask; +virCgroupAddProc; virCgroupAddTask; virCgroupAddTaskController; virCgroupAllowAllDevices; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index bd4859c..859ed55 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -37,6 +37,7 @@ #include "virtypedparam.h" #include "virnuma.h" #include "virsystemd.h" +#include "virpidfile.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -1106,6 +1107,58 @@ qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, int +qemuSetupCgroupForExtDevices(virDomainObjPtr vm) +{ +qemuDomainObjPrivatePtr priv = vm->privateData; +virDomainTPMDefPtr tpm = vm->def->tpm; +virCgroupPtr cgroup_temp = NULL; +pid_t pid; +int ret = -1; + +if (priv->cgroup == NULL) +return 0; /* Not supported, so claim success */ + +/* + * If CPU cgroup controller is not initialized here, then we need + * neither period nor quota settings. And if CPUSET controller is + * not initialized either, then there's nothing to do anyway. + */ +if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && +!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) +return 0; + +if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, + false, &cgroup_temp) < 0) +goto cleanup; + +if (tpm) { +switch (tpm->type) { +case VIR_DOMAIN_TPM_TYPE_EMULATOR: +if (virPidFileReadPath(tpm->data.emulator.pidfile, &pid) < 0) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not read swtpm's pidfile %s"), + tpm->data.emulator.pidfile); +goto cleanup; +} +if (virCgroupAddProc(cgroup_temp, pid) < 0) +goto cleanup; +break; +case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: +case VIR_DOMAIN_TPM_TYPE_LAST: +break; +} +} + +ret = 0; + +cleanup: +virCgroupFree(&cgroup_temp); + +return ret; +} + + +int qemuSetupGlobalCpuCgroup(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 3b8ff60..478bf7e 100
[libvirt] [PATCH v2 5/6] tpm: Add support for choosing emulation of a TPM 2
This patch extends the TPM's device XML with TPM 2 support. This only works for the emulator type backend and looks as follows: Once the version of a TPM has been chosen it cannot be changed anymore unless one removes the TPM device first and then reads it. However, one looses all the secrets stored inside or tied to the emulated TPM by doing this. Signed-off-by: Stefan Berger --- docs/formatdomain.html.in | 17 +- docs/schemas/domaincommon.rng | 13 src/conf/domain_conf.c | 20 +- src/conf/domain_conf.h | 6 ++ src/util/virtpm.c | 84 +++--- tests/qemuxml2argvdata/tpm-emulator-tpm2.args | 24 tests/qemuxml2argvdata/tpm-emulator-tpm2.xml | 30 + tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmloutdata/tpm-emulator-tpm2.xml | 34 +++ 9 files changed, 221 insertions(+), 9 deletions(-) create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2.args create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2.xml create mode 100644 tests/qemuxml2xmloutdata/tpm-emulator-tpm2.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bd6fedc..e5463a0 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7635,7 +7635,7 @@ qemu-kvm -net nic,model=? /dev/null ...@@ -7684,6 +7684,21 @@ qemu-kvm -net nic,model=? /dev/null + tpmversion + + + The tpmversion attribute indicates the version + of the TPM. By default a TPM 1.2 is created. This attribute + only works with the emulator backend. The following + versions are supported: + + + '1.2' : creates a TPM 1.2 + '2.0' or '2' : creates a TPM 2 + +Note that once a certain version of a TPM has been created for +a guest, the version must not be changed anymore. + NVRAM device diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index d628444..77328bd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4140,6 +4140,19 @@ + + + + + +1.2 +2 +2.0 + + + + + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b5f1c3f..0bbb547 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12552,7 +12552,7 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, * or like this: * * - * + * * */ static virDomainTPMDefPtr @@ -12565,6 +12565,7 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, char *path = NULL; char *model = NULL; char *backend = NULL; +char *tpmversion = NULL; virDomainTPMDefPtr def; xmlNodePtr save = ctxt->node; xmlNodePtr *backends = NULL; @@ -12611,6 +12612,20 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } +tpmversion = virXMLPropString(backends[0], "tpmversion"); +if (!tpmversion || STREQ(tpmversion, "1.2")) { +def->tpmversion = VIR_DOMAIN_TPM_VERSION_1_2; +/* only TIS available for emulator */ +if (def->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) +def->model = VIR_DOMAIN_TPM_MODEL_TIS; +} else if (STREQ(tpmversion, "2.0") || STREQ(tpmversion, "2")) { +def->tpmversion = VIR_DOMAIN_TPM_VERSION_2; +} else { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported TPM version '%s'"), + tpmversion); +} + switch (def->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: path = virXPathString("string(./backend/device/@path)", ctxt); @@ -12635,6 +12650,7 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(model); VIR_FREE(backend); VIR_FREE(backends); +VIR_FREE(tpmversion); ctxt->node = save; return def; @@ -24798,6 +24814,8 @@ virDomainTPMDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "type)); +if (def->tpmversion == VIR_DOMAIN_TPM_VERSION_2) +virBufferAddLit(buf, " tpmversion='2'"); virBufferAdjustIndent(buf, 2); switch (def->type) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f632184..80f599c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1289,12 +1289,18 @@ typedef enum { VIR_DOMAIN_TPM_TYPE_LAST } virDomainTPMBackendType; +typedef enum { +VIR_DOMAIN_TPM_VERSION_1_2, +VIR_DOMAIN_TPM_VERSION_2, +} virDomainTPMVersion; + # - +
[libvirt] [PATCH v2 0/6] Add support for TPM emulator and CRB interface
This series of patches add support for the new TPM CRB interface in QEMU that will become available with QEMU 2.12. The rest of the patches add support for the TPM emulator backend that is available in QEMU and based on swtpm + libtpms. It allows to attach a TPM 1.2 or 2 to a QEMU VM. sVirt labels are used for labeling the swtpm process, its UnixIO socket, and log file with the same label that the QEMU process gets. Besides that swtpm is added to the emulator cgroup. The device XML can be changed from a TPM 1.2 to a TPM 2 and back to a TPM 1.2. The device state is not removed during those changes but only when the domain is undefined. I must admit that the first swtpm support patch is quite big. So far I only broke out the test cases into a subsequent patch. Regards, Stefan v1->v2: - reorganized directories where files are written to - all directories and files are chown'ed before swtpm is started - much refactoring Stefan Berger (6): tpm: Enable TPM CRB interface tpm: Add support for external swtpm TPM emulator tpm: Add test cases for external swtpm TPM emulator tpm: Label the external swtpm with SELinux labels tpm: Add support for choosing emulation of a TPM 2 tpm: Add swtpm to emulator cgroup docs/formatdomain.html.in | 47 ++ docs/schemas/domaincommon.rng | 23 +- src/conf/domain_audit.c| 2 + src/conf/domain_conf.c | 73 ++- src/conf/domain_conf.h | 15 + src/libvirt_private.syms | 9 + src/qemu/Makefile.inc.am | 2 + src/qemu/libvirtd_qemu.aug | 3 + src/qemu/qemu.conf | 7 + src/qemu/qemu_capabilities.c | 10 + src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_cgroup.c | 54 ++ src/qemu/qemu_cgroup.h | 1 + src/qemu/qemu_command.c| 52 +- src/qemu/qemu_conf.c | 35 +- src/qemu/qemu_conf.h | 5 + src/qemu/qemu_domain.c | 4 + src/qemu/qemu_driver.c | 7 + src/qemu/qemu_extdevice.c | 303 +++ src/qemu/qemu_extdevice.h | 44 ++ src/qemu/qemu_process.c| 16 + src/qemu/test_libvirtd_qemu.aug.in | 1 + src/security/security_dac.c| 6 + src/security/security_driver.h | 4 + src/security/security_manager.c| 18 + src/security/security_manager.h| 3 + src/security/security_selinux.c| 75 +++ src/security/security_stack.c | 19 + src/util/vircgroup.c | 42 ++ src/util/vircgroup.h | 1 + src/util/virfile.c | 60 +++ src/util/virfile.h | 2 + src/util/virtpm.c | 596 - src/util/virtpm.h | 25 +- tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 2 + tests/qemuxml2argvdata/tpm-emulator-tpm2.args | 24 + tests/qemuxml2argvdata/tpm-emulator-tpm2.xml | 30 ++ tests/qemuxml2argvdata/tpm-emulator.args | 24 + tests/qemuxml2argvdata/tpm-emulator.xml| 30 ++ tests/qemuxml2argvdata/tpm-passthrough-crb.args| 24 + tests/qemuxml2argvdata/tpm-passthrough-crb.xml | 32 ++ tests/qemuxml2argvtest.c | 20 + tests/qemuxml2xmloutdata/tpm-emulator-tpm2.xml | 34 ++ tests/qemuxml2xmloutdata/tpm-emulator.xml | 34 ++ tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml | 36 ++ tests/qemuxml2xmltest.c| 1 + 50 files changed, 1842 insertions(+), 19 deletions(-) create mode 100644 src/qemu/qemu_extdevice.c create mode 100644 src/qemu/qemu_extdevice.h create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2.args create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2.xml create mode 100644 tests/qemuxml2argvdata/tpm-emulator.args create mode 100644 tests/qemuxml2argvdata/tpm-emulator.xml create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.args create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.xml create mode 100644 tests/qemuxml2xmloutdata/tpm-emulator-tpm2.xml create mode 100644 tests/qemuxml2xmloutdata/tpm-emulator.xml create mode 100644 tests/qemuxml2xmloutda
[libvirt] [PATCH v2 1/6] tpm: Enable TPM CRB interface
Enable the TPM CRB interface added in QEMU 2.12. the TPM CRB interface is a simpler interface than the TPM TIS and is only available for TPM 2. Signed-off-by: Stefan Berger --- docs/formatdomain.html.in | 2 ++ docs/schemas/domaincommon.rng | 5 +++- src/conf/domain_conf.c| 5 ++-- src/conf/domain_conf.h| 1 + src/qemu/qemu_capabilities.c | 5 src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemuxml2argvdata/tpm-passthrough-crb.args | 24 +++ tests/qemuxml2argvdata/tpm-passthrough-crb.xml| 32 tests/qemuxml2argvtest.c | 3 ++ tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml | 36 +++ 11 files changed, 111 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.args create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.xml create mode 100644 tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 08dc74b..16fc7db 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7628,6 +7628,8 @@ qemu-kvm -net nic,model=? /dev/null The model attribute specifies what device model QEMU provides to the guest. If no model name is provided, tpm-tis will automatically be chosen. + Another available choice is the tpm-crb, which + should only be used when the backend is a TPM 2. backend diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8165e69..be5c628 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4112,7 +4112,10 @@ - tpm-tis + +tpm-tis +tpm-crb + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae7c0d9..232174a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -858,7 +858,8 @@ VIR_ENUM_IMPL(virDomainRNGBackend, "egd"); VIR_ENUM_IMPL(virDomainTPMModel, VIR_DOMAIN_TPM_MODEL_LAST, - "tpm-tis") + "tpm-tis", + "tpm-crb") VIR_ENUM_IMPL(virDomainTPMBackend, VIR_DOMAIN_TPM_TYPE_LAST, "passthrough") @@ -12549,8 +12550,6 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown TPM frontend model '%s'"), model); goto error; -} else { -def->model = VIR_DOMAIN_TPM_MODEL_TIS; } ctxt->node = node; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 61379e5..1724340 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1277,6 +1277,7 @@ struct _virDomainHubDef { typedef enum { VIR_DOMAIN_TPM_MODEL_TIS, +VIR_DOMAIN_TPM_MODEL_CRB, VIR_DOMAIN_TPM_MODEL_LAST } virDomainTPMModel; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e54dde6..0952663 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -466,6 +466,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 285 */ "virtio-mouse-ccw", "virtio-tablet-ccw", + "tpm-crb", ); @@ -3104,6 +3105,10 @@ const struct tpmTypeToCaps virQEMUCapsTPMModelsToCaps[] = { .type = VIR_DOMAIN_TPM_MODEL_TIS, .caps = QEMU_CAPS_DEVICE_TPM_TIS, }, +{ +.type = VIR_DOMAIN_TPM_MODEL_CRB, +.caps = QEMU_CAPS_DEVICE_TPM_CRB, +}, }; static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3f3c29f..604525a 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -450,6 +450,7 @@ typedef enum { /* 285 */ QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW, /* -device virtio-mouse-ccw */ QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */ +QEMU_CAPS_DEVICE_TPM_CRB, /* -device tpm-crb */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 334296e..39ee4f4 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -225,6 +225,7 @@ + 2011090 0 390060 diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.args b/tests/qemuxml2argvdata/tpm-passthrough-crb.args new file mode 100644 index 000..ae052b4 --- /dev/null +++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name T
Re: [libvirt] [PATCH v3] qemu: add virQEMUBuildBufferEscapeComma in qemu_command.c
On 04/02/2018 04:17 PM, Sukrit Bhatnagar wrote: > This patch adds virQEMUBuildBufferEscapeComma to properly > escape commas in user provided data fields for qemu command > line processing. > > Signed-off-by: Sukrit Bhatnagar > --- > > Thank you for the helpful feedback and apologies for the delay. Well we all get busy and delayed by other things! It's been a week since you posted and well, I know I'm behind doing reviews! Nice on the using the --- for your adjustments and the issue you discovered. What happened to the changes from your previous version? They don't seem to be included in this patch and they weren't pushed upstream. This patch is all new changes. The "next" step is to attempt to generate patches that make incremental progress towards the end goal. Not all your changes need to go in one pile especially if something is separable - there's a methodology one develops over time. Changes don't need to be "so separated" that you get very large series, but you can create smaller patches, altering single API's/helpers and adjusting anything called by them to handle the changes. Some times it's a changed result and other times it's doing nothing because the patch fixes something. Again, it's one of those over time things. In this case, almost every function could have had it's own patch. That way a reviewer can ACK/Reviewed-by and push part of the series while perhaps asking for respins on other parts. It also allows for a NACK of a specific area. Much easier to change and review smaller things too! Since this is a GSOC type activity I won't "do" the work for you, but I will help "guide" you to the answer. First things first - hopefully you haven't lost your original patch. It's easily recreateable at least. We will start easy/slow... Using that patch as a starting point, create a series of 5 patches patch 1: Changes for qemuBuildRomStr patch 2: Changes for qemuBuildDriveDevStr patch 3: Changes for qemuBuildFSStr and qemuBuildFSDevStr patch 4: Changes for qemuBuildGraphicsVNCCommandLine patch 5: Changes for qemuBuildDomainLoaderCommandLine Hold onto the changes for qemuBuildHostNetStr, qemuBuildChrChardevFileStr, qemuBuildChrChardevStr, and qemuBuildGraphicsSPICECommandLine as they'll be combined with separated patches from this patch. Post the above 5 - I've included patch 1 & 2 for you as an attachment to this reply so you can see the format... It should be fairly easy to copy from there and post as a v4. Once you've done that - work through the rest of this using similar context - although a few of these will be a bit larger and more complicated (especially the first one for build network drive. > > Changes in v3: > virQEMUBuildBufferEscapeComma was applied to: > - src->hosts->socket in qemuBuildNetworkDriveURI > - src->path, src->configFile in qemuBuildNetworkDriveStr > - disk->blkdeviotune.group_name in qemuBuildDiskThrottling > - net->data.socket.address, net->data.socket.localaddr in > qemuBuildHostNetStr > - dev->data.file.path in qemuBuildChrChardevStr > - graphics->data.spice.rendernode in > qemuBuildGraphicsSPICECommandLine > - smartcard->data.cert.file[i], smartcard->data.cert.database in > qemuBuildSmartcardCommandLine > > Changes in v2: > virQEMUBuildBufferEscapeComma was applied to: > - info->romfile in qemuBuildRomStr > - disk->vendor, disk->product in qemuBuildDriveDevStr > - fs->src->path in qemuBuildFSStr > - fs->dst in qemuBuildFSDevStr > - connect= in qemuBuildHostNetStr > - fileval handling in qemuBuildChrChardevStr > - TYPE_DEV, TYPE_PIPE handling in qemuBuildChrChardevStr > - cfg->vncTLSx509certdir in qemuBuildGraphicsVNCCommandLine > - cfg->spiceTLSx509certdir in qemuBuildGraphicsSPICECommandLine > - loader->path, loader->nvram usage in > qemuBuildDomainLoaderCommandLine > > Link to v2: > https://www.redhat.com/archives/libvir-list/2018-March/msg00965.html > > > When I tried to change src->path in qemuBuildNetworkDriveStr > for this portion > > 961 } else if (src->nhosts == 1) { > 962 if (virAsprintf(&ret, "sheepdog:%s:%u:%s", > 963 src->hosts->name, src->hosts->port, > 964 src->path) < 0) > 965 goto cleanup; > 966 } else { > > make check reported the following error. > > 141) QEMU XML-2-ARGV disk-drive-network-sheepdog ... > In > '/home/skrtbhtngr/libvirt/tests/qemuxml2argvdata/disk-drive-network-sheepdog.args': > Offset 0 > Expect [LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test > QEMU_AUDIO_DRV=none /usr/bin/qemu-system-i686 -name QEMUGuest1 -S -M pc -m > 214 -smp 1,sockets=1,cores=1,threads=1 -uuid > c7a5fdbd-edaf-9455-926a-d65c16db1809 -nographic -nodefaults -chardev > socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait > -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb > -drive file=/dev/HostVG/QEMU,,Guest1,format=r
Re: [libvirt] [PATCH V2 2/8] tests: move sexpr2xml tests to WITH_LIBXL
On 04/09/2018 03:10 PM, Jim Fehlig wrote: On 04/09/2018 08:32 AM, Daniel P. Berrangé wrote: On Fri, Apr 06, 2018 at 02:44:54PM -0600, Jim Fehlig wrote: In preparation of removing the legacy Xen driver, move the sexpr2xml tests from WITH_XEN to WITH_LIBXL. Even though the legacy driver will be removed, we'll want to maintain the ability to convert sexpr to XML. Requires fixing up the tests to account for different behavior of Xen vs libxl post parse functions. There is some test file fallout due to differences in handling of default values between xend and libxl. diff --git a/tests/sexpr2xmldata/sexpr2xml-boot-grub.xml b/tests/sexpr2xmldata/sexpr2xml-boot-grub.xml index b9a8716b2..4b9f535fc 100644 --- a/tests/sexpr2xmldata/sexpr2xml-boot-grub.xml +++ b/tests/sexpr2xmldata/sexpr2xml-boot-grub.xml @@ -15,7 +15,7 @@ destroy - + @@ -24,6 +24,5 @@ - For this I wonder if a better solution is actually to make the libxl driver fill in the memballoon device. IIUC, you can't actually turn it off, so all libxl guests will have this regardless. Yes, good point. So we should just need to add the element a post-parse callback, and check for unsupported "model" value when starting the guest I'll send a follow up along these lines. In the meantime I've pushed this series. Good riddance xend... Sorry, I should have put a bit more thought into this approach since it caused a lot of unnecessary test file churn. Regardless, the follow-up has been sent https://www.redhat.com/archives/libvir-list/2018-April/msg00753.html Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] libxl: add support for memballoon device
All Xen PV and HVM with PV driver support a memory balloon device, which cannot be disabled through the toolstack. Model the device in the libxl driver, similar to the recently removed xend-based driver. Signed-off-by: Jim Fehlig --- Apologies for the large amount of test file churn... src/libxl/libxl_conf.c | 26 ++ src/libxl/libxl_domain.c | 10 + tests/sexpr2xmldata/sexpr2xml-boot-grub.xml| 1 + tests/sexpr2xmldata/sexpr2xml-bridge-ipaddr.xml| 1 + tests/sexpr2xmldata/sexpr2xml-curmem.xml | 1 + .../sexpr2xml-disk-block-shareable.xml | 1 + tests/sexpr2xmldata/sexpr2xml-disk-block.xml | 1 + .../sexpr2xml-disk-drv-blktap-qcow.xml | 1 + .../sexpr2xml-disk-drv-blktap-raw.xml | 1 + .../sexpr2xml-disk-drv-blktap2-raw.xml | 1 + tests/sexpr2xmldata/sexpr2xml-disk-file.xml| 1 + tests/sexpr2xmldata/sexpr2xml-fv-autoport.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-empty-kernel.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.xml| 1 + tests/sexpr2xmldata/sexpr2xml-fv-force-nohpet.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml| 1 + tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-net-netfront.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-parallel-tcp.xml | 1 + .../sexpr2xml-fv-serial-dev-2-ports.xml| 1 + .../sexpr2xml-fv-serial-dev-2nd-port.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-serial-file.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-serial-null.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.xml| 1 + tests/sexpr2xmldata/sexpr2xml-fv-serial-stdio.xml | 1 + .../sexpr2xml-fv-serial-tcp-telnet.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp.xml| 1 + tests/sexpr2xmldata/sexpr2xml-fv-serial-udp.xml| 1 + tests/sexpr2xmldata/sexpr2xml-fv-serial-unix.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-sound-all.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-sound.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-usbmouse.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-usbtablet.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-utc.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-v2.xml| 1 + tests/sexpr2xmldata/sexpr2xml-fv.xml | 1 + tests/sexpr2xmldata/sexpr2xml-net-bridged.xml | 1 + tests/sexpr2xmldata/sexpr2xml-net-e1000.xml| 1 + tests/sexpr2xmldata/sexpr2xml-net-routed.xml | 1 + tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml | 1 + tests/sexpr2xmldata/sexpr2xml-pci-devs.xml | 1 + .../sexpr2xml-pv-bootloader-cmdline.xml| 1 + tests/sexpr2xmldata/sexpr2xml-pv-bootloader.xml| 1 + tests/sexpr2xmldata/sexpr2xml-pv-localtime.xml | 1 + tests/sexpr2xmldata/sexpr2xml-pv-vcpus.xml | 1 + .../sexpr2xml-pv-vfb-new-vncdisplay.xml| 1 + tests/sexpr2xmldata/sexpr2xml-pv-vfb-new.xml | 1 + .../sexpr2xmldata/sexpr2xml-pv-vfb-type-crash.xml | 1 + tests/sexpr2xmldata/sexpr2xml-pv.xml | 1 + tests/sexpr2xmldata/sexpr2xml-vif-rate.xml | 1 + tests/xlconfigdata/test-channel-pty.xml| 1 + tests/xlconfigdata/test-channel-unix.xml | 1 + .../test-disk-positional-parms-full.xml| 1 + .../test-disk-positional-parms-partial.xml | 1 + tests/xlconfigdata/test-disk-qed.xml | 1 + ...est-fullvirt-direct-kernel-boot-bogus-extra.xml | 1 + .../test-fullvirt-direct-kernel-boot-extra.xml | 1 + .../test-fullvirt-direct-kernel-boot.xml | 1 + tests/xlconfigdata/test-fullvirt-hpet-timer.xml| 1 + tests/xlconfigdata/test-fullvirt-multi-timer.xml | 1 + tests/xlconfigdata/test-fullvirt-multiserial.xml | 1 + tests/xlconfigdata/test-fullvirt-multiusb.xml | 1 + .../test-fullvirt-nestedhvm-disabled.xml | 1 + tests/xlconfigdata/test-fullvirt-nestedhvm.xml | 1 + tests/xlconfigdata/test-fullvirt-nohap.xml | 1 + tests/xlconfigdata/test-fullvirt-ovmf.xml | 1 + tests/xlconfigdata/test-fullvirt-tsc-timer.xml | 1 + tests/xlconfigdata/test-fullvirt-vnuma.xml | 1 + tests/xlconfigdata/test-new-disk.xml | 1 + .../test-paravirt-cmdline-bogus-extra-root.xml | 1 + .../test-paravirt-cmdline-extra-root.xml | 1 + tests/xlconfigdata/test-paravirt-cmdline.xml | 1 + tests/xlconfigdata/test-paravirt-maxvcpus.xml | 1 + tests/xlconfigdata/test-rbd-multihost-noauth.xml | 1 + tests/xlconfigdata/test-spice-features.xml | 1 + tests/xlconfigdata/test-spice.xml | 1 + tests/xlconfigdata/test-usb.xml| 1 + tests/xlconfigdata/test-usbctrl.xml| 1 + tests/xlconfigda
Re: [libvirt] [PATCH v6.1 2/9] libxl: pass driver config to libxlMakeDomBuildInfo
On 04/06/2018 06:54 PM, Marek Marczykowski-Górecki wrote: On Wed, Mar 28, 2018 at 01:42:47PM -0600, Jim Fehlig wrote: On 03/27/2018 05:55 PM, Marek Marczykowski-Górecki wrote: diff --git a/tests/virmocklibxl.c b/tests/virmocklibxl.c index 747f9f8..28281b6 100644 --- a/tests/virmocklibxl.c +++ b/tests/virmocklibxl.c @@ -27,6 +27,7 @@ # include # include # include +# include # include # include @@ -48,6 +49,24 @@ VIR_MOCK_IMPL_RET_ARGS(xc_interface_open, } +VIR_MOCK_IMPL_RET_ARGS(libxl_get_version_info, + const libxl_version_info*, + libxl_ctx *, ctx) +{ +static libxl_version_info info; + +memset(&info, 0, sizeof(info)); + +return &info; +/* silence gcc warning */ +return real_libxl_get_version_info(ctx); +} + +VIR_MOCK_STUB_RET_ARGS(libxl_get_free_memory, + int, 0, + libxl_ctx *, ctx, + uint32_t *, memkb); + This doesn't compile with Xen >= 4.8 In file included from virmocklibxl.c:26:0: virmocklibxl.c:66:24: error: conflicting types for 'libxl_get_free_memory' VIR_MOCK_STUB_RET_ARGS(libxl_get_free_memory, ^ virmock.h:182:13: note: in definition of macro 'VIR_MOCK_STUB_RET_ARGS' rettype name(VIR_MOCK_ARGTYPENAMES_UNUSED(__VA_ARGS__)) \ ^~~~ In file included from virmocklibxl.c:29:0: /usr/include/libxl.h:1570:5: note: previous declaration of 'libxl_get_free_memory' was here int libxl_get_free_memory(libxl_ctx *ctx, uint64_t *memkb); ^ Using the uint32_t variant works in the libxl driver since we have -DLIBXL_API_VERSION=0x040400 in LIBXL_CFLAGS. I worked around the compilation failure with LIBXL_HAVE_MEMKB_64BITS, I can't reproduce this problem, either with 4.8 or 4.10. Even more, if I add alternative mock with uint64_t, under #if LIBXL_HAVE_MEMKB_64BITS, I get compile failure, because of conflicting types (with libxl_get_free_memory_0x040700)... Can you confirm it's really a problem, not some mismatching header versions on your side? Perhaps I've also made a mistake rebasing some of these patches. Can you pretty please rebase against current master and repost a V7 (adding all the R-B)? If you say it passes 'make check' on 4.5 and 4.10, I'll chase down any problems on my side. Thanks! Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH jenkins-ci] Run "make check" for osinfo-db
Signed-off-by: Daniel P. Berrangé --- projects/osinfo-db.yaml | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/projects/osinfo-db.yaml b/projects/osinfo-db.yaml index 0dd73b5..7f83722 100644 --- a/projects/osinfo-db.yaml +++ b/projects/osinfo-db.yaml @@ -17,8 +17,12 @@ command: | $MAKE -j{smp} all $MAKE install OSINFO_DB_TARGET="--system" - - generic-rpm-job: + - generic-check-job: parent_jobs: 'osinfo-db-master-build' + command: | +$MAKE -j{smp} check + - generic-rpm-job: + parent_jobs: 'osinfo-db-master-check' machines: - libvirt-centos-7 - libvirt-fedora-26 -- 2.14.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 08/14] qemu: Generate cmd line at startup
This is the easier part. All we need to do here is put -object pr-manager-helper,id=$alias,path=$socketPath and then just reference the object in -drive file.pr-manager=$alias. Signed-off-by: Michal Privoznik --- src/qemu/qemu_command.c| 94 ++ src/qemu/qemu_command.h| 3 + .../disk-virtio-scsi-reservations.args | 35 tests/qemuxml2argvtest.c | 4 + 4 files changed, 136 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 514c3ab2ef..81f6025207 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src) } +static void +qemuBuildDriveSourcePR(virBufferPtr buf, + virStorageSourcePtr src) +{ +qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); +qemuDomainDiskPRDPtr prd = srcPriv->prd; + +if (!prd || !prd->alias) +return; + +virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias); +} + + static int qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, virQEMUCapsPtr qemuCaps, @@ -1591,6 +1605,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, if (disk->src->debug) virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel); + +qemuBuildDriveSourcePR(buf, disk->src); } else { if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops))) goto cleanup; @@ -9872,6 +9888,81 @@ qemuBuildPanicCommandLine(virCommandPtr cmd, } +/** + * qemuBuildPRManagerInfoProps: + * @prd: disk PR runtime info + * @propsret: JSON properties to return + * + * Build the JSON properties for the pr-manager object. + * + * Returns: 0 on success (@propsret is NULL if no properties are needed), + * -1 on failure (with error message set). + */ +int +qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd, +virJSONValuePtr *propsret) +{ +*propsret = NULL; + +if (!prd || !prd->alias) +return 0; + +if (virJSONValueObjectCreate(propsret, + "s:path", prd->path, + NULL) < 0) +return -1; + +return 0; +} + + +static int +qemuBuildMasterPRCommandLine(virCommandPtr cmd, + const virDomainDef *def) +{ +size_t i; +bool managedAdded = false; +virJSONValuePtr props = NULL; +char *tmp = NULL; +int ret = -1; + +for (i = 0; i < def->ndisks; i++) { +const virDomainDiskDef *disk = def->disks[i]; +qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); +qemuDomainDiskPRDPtr prd = srcPriv->prd; + + +if (virStoragePRDefIsManaged(disk->src->pr)) { +if (managedAdded) +continue; + +managedAdded = true; +} + +if (qemuBuildPRManagerInfoProps(prd, &props) < 0) +goto cleanup; + +if (!props) +continue; + +if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper", + prd->alias, + props))) +goto cleanup; +virJSONValueFree(props); +props = NULL; + +virCommandAddArgList(cmd, "-object", tmp, NULL); +VIR_FREE(tmp); +} + +ret = 0; + cleanup: +virJSONValueFree(props); +return ret; +} + + /** * qemuBuildCommandLineValidate: * @@ -10024,6 +10115,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0) goto error; +if (qemuBuildMasterPRCommandLine(cmd, def) < 0) +goto error; + if (enableFips) virCommandAddArg(cmd, "-enable-fips"); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 31c9da673c..32f3697181 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -54,6 +54,9 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, size_t *nnicindexes, int **nicindexes); +/* Generate the object properties for pr-manager */ +int qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd, +virJSONValuePtr *propsret); /* Generate the object properties for a secret */ int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args new file mode 100644 index 00..195e83a048 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args @@ -0,0 +1,35 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=tes
[libvirt] [PATCH v4 14/14] qemu: Detect pr-manager-helper capability
Signed-off-by: Michal Privoznik --- src/qemu/qemu_capabilities.c | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + 6 files changed, 6 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index efd468692a..4a3afa7668 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1105,6 +1105,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "virtio-mouse-ccw", QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW }, { "virtio-tablet-ccw", QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW }, { "pcie-pci-bridge", QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE }, +{ "pr-manager-helper", QEMU_CAPS_PR_MANAGER_HELPER }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = { diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml index cbd645ae93..4e6e77a57f 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -151,6 +151,7 @@ + 2011000 0 342058 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml index 66629ff5bc..d31ad0d13f 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -189,6 +189,7 @@ + 2011090 0 342346 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index 1122d6408b..db455227f7 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -186,6 +186,7 @@ + 2011090 0 419215 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 191b1e0e37..19a7b97967 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -151,6 +151,7 @@ + 2011090 0 0 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 4ed2e1ea96..0105c99687 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -227,6 +227,7 @@ + 2011090 0 390060 -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 11/14] qemu: Start PR daemon on disk hotplug
When plugging a disk into domain we need to start qemu-pr-helper process iff this is the first disk with PR enabled for the domain. Otherwise the helper is already running (or not needed). Signed-off-by: Michal Privoznik --- src/qemu/qemu_hotplug.c | 51 + src/qemu/qemu_process.c | 4 ++-- src/qemu/qemu_process.h | 5 + 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f0d549de38..468153c79c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -348,6 +348,49 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } +/** + * qemuDomainMaybeStartPRDaemon: + * @vm: domain object + * @disk: disk to hotplug + * + * Checks if it's needed to start qemu-pr-helper and starts it. + * + * Returns: 0 if qemu-pr-helper is not needed + * 1 if it is needed and was started + * -1 otherwise. + */ +static int +qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ +size_t i; +qemuDomainStorageSourcePrivatePtr srcPriv; + +if (!virStoragePRDefIsManaged(disk->src->pr)) { +/* @disk itself does not require qemu-pr-helper. */ +return 0; +} + +for (i = 0; i < vm->def->ndisks; i++) { +const virDomainDiskDef *domainDisk = vm->def->disks[i]; + +if (virStoragePRDefIsManaged(domainDisk->src->pr)) { +/* qemu-pr-helper should be already started because + * another disk in domain requires it. */ +return 0; +} +} + +/* @disk requires qemu-pr-helper but none is running. + * Start it now. */ +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); +if (qemuProcessStartPRDaemon(vm, srcPriv->prd) < 0) +return -1; + +return 1; +} + + /** * qemuDomainAttachDiskGeneric: * @@ -368,6 +411,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, bool driveAdded = false; bool secobjAdded = false; bool encobjAdded = false; +bool prdStarted = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virJSONValuePtr secobjProps = NULL; virJSONValuePtr encobjProps = NULL; @@ -384,6 +428,11 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto error; +if ((rv = qemuDomainMaybeStartPRDaemon(vm, disk)) < 0) +goto cleanup; +else if (rv > 0) +prdStarted = true; + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); if (srcPriv) { secinfo = srcPriv->secinfo; @@ -481,6 +530,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, error: qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src); ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true)); +if (prdStarted) +qemuProcessKillPRDaemon(vm); goto cleanup; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 982c989a0a..11aaeabb38 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2571,7 +2571,7 @@ qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm, } -static void +void qemuProcessKillPRDaemon(virDomainObjPtr vm) { virErrorPtr orig_err; @@ -2629,7 +2629,7 @@ qemuProcessStartPRDaemonHook(void *opaque) } -static int +int qemuProcessStartPRDaemon(virDomainObjPtr vm, qemuDomainDiskPRDPtr prd) { diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2741115673..8df5832e23 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -203,4 +203,9 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob); +int qemuProcessStartPRDaemon(virDomainObjPtr vm, + qemuDomainDiskPRDPtr prd); + +void qemuProcessKillPRDaemon(virDomainObjPtr vm); + #endif /* __QEMU_PROCESS_H__ */ -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 13/14] qemu_hotplug: Hotunplug of reservations
If we are the last one to use pr-manager object we need to remove it and also kill the qemu-pr-helper process. Signed-off-by: Michal Privoznik --- src/qemu/qemu_hotplug.c | 38 ++ 1 file changed, 38 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 43bb910ed6..98e1bf7082 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3894,6 +3894,34 @@ static bool qemuIsMultiFunctionDevice(virDomainDefPtr def, } +static qemuDomainDiskPRDPtr +qemuDomainDiskNeedRemovePR(virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ +qemuDomainStorageSourcePrivatePtr srcPriv; +size_t i; + +if (!virStoragePRDefIsManaged(disk->src->pr)) +return NULL; + +for (i = 0; i < vm->def->ndisks; i++) { +const virDomainDiskDef *domainDisk = vm->def->disks[i]; + +if (domainDisk == disk) +continue; + +if (virStoragePRDefIsManaged(domainDisk->src->pr)) +break; +} + +if (i != vm->def->ndisks) +return NULL; + +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); +return srcPriv->prd; +} + + static int qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3907,6 +3935,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, char *drivestr; char *objAlias = NULL; char *encAlias = NULL; +qemuDomainDiskPRDPtr prd = NULL; VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); @@ -3944,6 +3973,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } } +prd = qemuDomainDiskNeedRemovePR(vm, disk); + qemuDomainObjEnterMonitor(driver, vm); qemuMonitorDriveDel(priv->mon, drivestr); @@ -3959,6 +3990,10 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, ignore_value(qemuMonitorDelObject(priv->mon, encAlias)); VIR_FREE(encAlias); +/* If it fails, then so be it - it was a best shot */ +if (prd) +ignore_value(qemuMonitorDelObject(priv->mon, prd->alias)); + if (disk->src->haveTLS) ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias)); @@ -3977,6 +4012,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } } +if (prd) +qemuProcessKillPRDaemon(vm); + qemuDomainReleaseDeviceAddress(vm, &disk->info, src); if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0) -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 12/14] qemu_hotplug: Hotplug of reservations
When attaching a disk that requires pr-manager we might need to plug the pr-manager object too. Signed-off-by: Michal Privoznik --- src/qemu/qemu_hotplug.c | 41 + 1 file changed, 41 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 468153c79c..43bb910ed6 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -391,6 +391,29 @@ qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm, } +static int +qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm, + qemuDomainDiskPRDPtr prd, + virJSONValuePtr *propsret) +{ +size_t i; + +*propsret = NULL; + +for (i = 0; i < vm->def->ndisks; i++) { +const virDomainDiskDef *domainDisk = vm->def->disks[i]; + +if (virStoragePRDefIsManaged(domainDisk->src->pr)) { +/* qemu-pr-helper should be already started because + * another disk in domain requires it. */ +return 0; +} +} + +return qemuBuildPRManagerInfoProps(prd, propsret); +} + + /** * qemuDomainAttachDiskGeneric: * @@ -411,13 +434,16 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, bool driveAdded = false; bool secobjAdded = false; bool encobjAdded = false; +bool prmgrAdded = false; bool prdStarted = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virJSONValuePtr secobjProps = NULL; virJSONValuePtr encobjProps = NULL; +virJSONValuePtr prmgrProps = NULL; qemuDomainStorageSourcePrivatePtr srcPriv; qemuDomainSecretInfoPtr secinfo = NULL; qemuDomainSecretInfoPtr encinfo = NULL; +qemuDomainDiskPRDPtr prd = NULL; if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -437,6 +463,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (srcPriv) { secinfo = srcPriv->secinfo; encinfo = srcPriv->encinfo; +prd = srcPriv->prd; } if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { @@ -447,6 +474,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) < 0) goto error; +if (qemuMaybeBuildPRManagerInfoProps(vm, prd, &prmgrProps) < 0) +goto error; + if (disk->src->haveTLS && qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src, disk->info.alias) < 0) @@ -484,6 +514,15 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, encobjAdded = true; } +if (prmgrProps) { +rv = qemuMonitorAddObject(priv->mon, "pr-manager-helper", prd->alias, + prmgrProps); +prmgrProps = NULL; /* qemuMonitorAddObject consumes */ +if (rv < 0) +goto exit_monitor; +prmgrAdded = true; +} + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto exit_monitor; driveAdded = true; @@ -521,6 +560,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); if (encobjAdded) ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); +if (prmgrAdded) +ignore_value(qemuMonitorDelObject(priv->mon, prd->alias)); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -2; virErrorRestore(&orig_err); -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 05/14] qemu: Store pr runtime data in status XML
Now that we generate pr-manager alias and socket path store them in status XML so that they are preserved across daemon restarts. Signed-off-by: Michal Privoznik --- src/qemu/qemu_domain.c | 64 ++ 1 file changed, 64 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 361a80be84..0856f04406 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1961,13 +1961,74 @@ qemuDomainObjPrivateFree(void *data) } +static int +qemuStorageSourcePrivateDataParsePR(xmlXPathContextPtr ctxt, +virStorageSourcePtr src) +{ +qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); +qemuDomainDiskPRDPtr prd = NULL; +int rc; +int ret = -1; + +if ((rc = virXPathBoolean("boolean(./prd)", ctxt)) == 0) { +return 0; +} else if (rc < 0) { +return ret; +} + +if (VIR_ALLOC(prd) < 0) +return ret; + +if (!(prd->alias = virXPathString("string(./prd/alias)", ctxt)) || +!(prd->path = virXPathString("string(./prd/path)", ctxt))) { +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed ")); +goto cleanup; +} + +VIR_STEAL_PTR(srcPriv->prd, prd); +ret = 0; + cleanup: +qemuDomainDiskPRDFree(prd); +return ret; +} + + +static int +qemuStorageSourcePrivateDataFormatPR(virStorageSourcePtr src, + virBufferPtr buf) +{ +qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); +qemuDomainDiskPRDPtr prd; + +if (!srcPriv || !srcPriv->prd) +return 0; + +prd = srcPriv->prd; + +virBufferAddLit(buf, "\n"); +virBufferAdjustIndent(buf, 2); +virBufferAsprintf(buf, "%s\n", prd->alias); +virBufferEscapeString(buf, "%s\n", prd->path); +virBufferAdjustIndent(buf, -2); +virBufferAddLit(buf, "\n"); +return 0; +} + + static int qemuStorageSourcePrivateDataParse(xmlXPathContextPtr ctxt, virStorageSourcePtr src) { +if (!(src->privateData = qemuDomainStorageSourcePrivateNew())) +return -1; + if (virStorageSourcePrivateDataParseRelPath(ctxt, src) < 0) return -1; +if (qemuStorageSourcePrivateDataParsePR(ctxt, src) < 0) +return -1; + return 0; } @@ -1979,6 +2040,9 @@ qemuStorageSourcePrivateDataFormat(virStorageSourcePtr src, if (virStorageSourcePrivateDataFormatRelPath(src, buf) < 0) return -1; +if (qemuStorageSourcePrivateDataFormatPR(src, buf) < 0) +return -1; + return 0; } -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 09/14] qemu: Introduce pr_helper to qemu.conf
Just like we allow users overriding path to bridge-helper detected at compile time we can allow them to override path to qemu-pr-helper. Signed-off-by: Michal Privoznik --- m4/virt-driver-qemu.m4 | 5 + src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 4 src/qemu/qemu_conf.c | 7 ++- src/qemu/qemu_conf.h | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 18 insertions(+), 1 deletion(-) diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4 index b9bafdab90..80e1d3ad46 100644 --- a/m4/virt-driver-qemu.m4 +++ b/m4/virt-driver-qemu.m4 @@ -57,6 +57,11 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [ [/usr/libexec:/usr/lib/qemu:/usr/lib]) AC_DEFINE_UNQUOTED([QEMU_BRIDGE_HELPER], ["$QEMU_BRIDGE_HELPER"], [QEMU bridge helper]) + AC_PATH_PROG([QEMU_PR_HELPER], [qemu-pr-helper], + [/usr/bin/qemu-pr-helper], + [/usr/bin:/usr/libexec]) + AC_DEFINE_UNQUOTED([QEMU_PR_HELPER], ["$QEMU_PR_HELPER"], + [QEMU PR helper]) ]) AC_DEFUN([LIBVIRT_DRIVER_RESULT_QEMU], [ diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index c19bf3a43a..2dc16e91fd 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -86,6 +86,7 @@ module Libvirtd_qemu = let process_entry = str_entry "hugetlbfs_mount" | bool_entry "clear_emulator_capabilities" | str_entry "bridge_helper" + | str_entry "pr_helper" | bool_entry "set_process_name" | int_entry "max_processes" | int_entry "max_files" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 07eab7efff..30fdd54e2c 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -775,3 +775,7 @@ # This directory is used for memoryBacking source if configured as file. # NOTE: big files will be stored here #memory_backing_dir = "/var/lib/libvirt/qemu/ram" + +# Path to the SCSI persistent reservations helper. This helper is +# used whenever are enabled for SCSI disks. +#pr_helper = "/usr/libexec/qemu-pr-helper" diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 36cf3a281c..8c69dbe75c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -307,7 +307,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) goto error; } -if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0) +if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0 || +VIR_STRDUP(cfg->prHelperName, QEMU_PR_HELPER) < 0) goto error; cfg->clearEmulatorCapabilities = true; @@ -392,6 +393,7 @@ static void virQEMUDriverConfigDispose(void *obj) } VIR_FREE(cfg->hugetlbfs); VIR_FREE(cfg->bridgeHelperName); +VIR_FREE(cfg->prHelperName); VIR_FREE(cfg->saveImageFormat); VIR_FREE(cfg->dumpImageFormat); @@ -759,6 +761,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueString(conf, "bridge_helper", &cfg->bridgeHelperName) < 0) goto cleanup; +if (virConfGetValueString(conf, "pr_helper", &cfg->prHelperName) < 0) +goto cleanup; + if (virConfGetValueBool(conf, "mac_filter", &cfg->macFilter) < 0) goto cleanup; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index e1ad5463f3..7a63780c48 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -153,6 +153,7 @@ struct _virQEMUDriverConfig { size_t nhugetlbfs; char *bridgeHelperName; +char *prHelperName; bool macFilter; diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 688e5b9fda..c0efae47bd 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -100,3 +100,4 @@ module Test_libvirtd_qemu = { "1" = "mount" } } { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" } +{ "pr_helper" = "/usr/libexec/qemu-pr-helper" } -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 04/14] qemu: Generate alias and socket path for pr-helper
While we're not generating the command line just yet (look for the next commits), we can generate the alias for pr-manager. A domain can have up to one managed pr-manager (in which case socket path is decided by libvirt and pr-helper is spawned by libvirt too), but up to ndisks of unmanaged ones (one per disk basically). In case of the former we can generate a simple alias and be sure it'll not conflict. But in case of the latter to avoid any conflicts let's generate alias that's based on something unique - like disk target. Signed-off-by: Michal Privoznik --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_alias.c | 11 ++ src/qemu/qemu_alias.h | 2 ++ src/qemu/qemu_domain.c| 87 +-- src/qemu/qemu_domain.h| 10 ++ src/util/virstoragefile.c | 15 src/util/virstoragefile.h | 2 ++ 7 files changed, 126 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a376e3bb0d..5b7b5514a2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2799,7 +2799,9 @@ virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStoragePRDefFormat; virStoragePRDefFree; +virStoragePRDefIsEnabled; virStoragePRDefIsEqual; +virStoragePRDefIsManaged; virStoragePRDefParseXML; virStorageSourceBackingStoreClear; virStorageSourceClear; diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 95d1e0370a..6db3da27a8 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -773,3 +773,14 @@ qemuAliasChardevFromDevAlias(const char *devAlias) return ret; } + + +char * +qemuDomainGetManagedPRAlias(void) +{ +char *alias; + +ignore_value(VIR_STRDUP(alias, "pr-helper0")); + +return alias; +} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index 8c744138ce..91e0a9c893 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -92,4 +92,6 @@ char *qemuAliasTLSObjFromSrcAlias(const char *srcAlias) char *qemuAliasChardevFromDevAlias(const char *devAlias) ATTRIBUTE_NONNULL(1); +char * qemuDomainGetManagedPRAlias(void); + #endif /* __QEMU_ALIAS_H__*/ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5a7b5f8417..361a80be84 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -982,6 +982,18 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo) } +static void +qemuDomainDiskPRDFree(qemuDomainDiskPRDPtr prd) +{ +if (!prd) +return; + +VIR_FREE(prd->alias); +VIR_FREE(prd->path); +VIR_FREE(prd); +} + + static virClassPtr qemuDomainDiskPrivateClass; static void qemuDomainDiskPrivateDispose(void *obj); @@ -1062,6 +1074,7 @@ qemuDomainStorageSourcePrivateDispose(void *obj) qemuDomainSecretInfoFree(&priv->secinfo); qemuDomainSecretInfoFree(&priv->encinfo); +qemuDomainDiskPRDFree(priv->prd); } @@ -1473,9 +1486,6 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivatePtr priv, if (!hasAuth && !hasEnc) return 0; -if (!(src->privateData = qemuDomainStorageSourcePrivateNew())) -return -1; - srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); if (hasAuth) { @@ -11925,11 +11935,79 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr disk) } +static int +qemuDomainPrepareDiskPRD(qemuDomainObjPrivatePtr priv, + virDomainDiskDefPtr disk) +{ +qemuDomainStorageSourcePrivatePtr srcPriv; +virStoragePRDefPtr prd = disk->src->pr; +char *prAlias = NULL; +char *prPath = NULL; +int ret = -1; + +if (!virStoragePRDefIsEnabled(prd)) +return 0; + +if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reservations not supported with this QEMU binary")); +return ret; +} + +if (!virStorageSourceIsLocalStorage(disk->src)) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reservations supported only for local storage")); +return ret; +} + +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); + +/* Managed PR means there's one pr-manager object per domain + * and the pr-helper process is spawned and managed by + * libvirt. + * If PR is unmanaged there's one pr-manager object per disk + * (in general each disk can have its own pr-manager) and + * it's user's responsibility to start the pr-helper process. + */ +if (virStoragePRDefIsManaged(prd)) { +/* Generate PR helper socket path & alias that are same + * for each disk in the domain. */ + +if (!(prAlias = qemuDomainGetManagedPRAlias())) +return ret; + +if (virAsprintf(&prPath, "%s/pr-helper0.sock", priv->libDir) < 0) +goto cleanup; + +} else { +if (virAsprintf(&prAlias, "pr-helper-%s", disk->info.alias) < 0) +return ret; + +
[libvirt] [PATCH v4 10/14] qemu: Start PR daemon on domain startup
Before we exec() qemu we have to spawn pr-helper processes for all managed reservations (well, technically there can only one). The only caveat there is that we should place the process into the same namespace and cgroup as qemu (so that it shares the same view of the system). But we can do that only after we've forked. That means calling the setup function between fork() and exec(). Signed-off-by: Michal Privoznik --- src/qemu/qemu_process.c | 224 1 file changed, 224 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f02114c693..982c989a0a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2556,6 +2556,223 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver, ret = 0; cleanup: virObjectUnref(caps); + +return ret; +} + + +static char * +qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm, +const char *prdAlias) +{ +qemuDomainObjPrivatePtr priv = vm->privateData; + +return virPidFileBuildPath(priv->libDir, prdAlias); +} + + +static void +qemuProcessKillPRDaemon(virDomainObjPtr vm) +{ +virErrorPtr orig_err; +char *prAlias; +char *prPidfile; + +if (!(prAlias = qemuDomainGetManagedPRAlias())) { +VIR_WARN("Unable to get default PR manager alias"); +return; +} + +if (!(prPidfile = qemuProcessBuildPRHelperPidfilePath(vm, prAlias))) { +VIR_WARN("Unable to construct pr-helper pidfile path"); +VIR_FREE(prAlias); +return; +} +VIR_FREE(prAlias); + +virErrorPreserveLast(&orig_err); +if (virPidFileForceCleanupPath(prPidfile) < 0) { +VIR_WARN("Unable to kill pr-helper process"); +} else if (unlink(prPidfile) < 0 && + errno != ENOENT) { +virReportSystemError(errno, + _("Unable to remove stale pidfile %s"), + prPidfile); +} +virErrorRestore(&orig_err); + +VIR_FREE(prPidfile); +} + + +static int +qemuProcessStartPRDaemonHook(void *opaque) +{ +virDomainObjPtr vm = opaque; +size_t i, nfds = 0; +int *fds = NULL; +int ret = -1; + +if (virProcessGetNamespaces(vm->pid, &nfds, &fds) < 0) +return ret; + +if (nfds > 0 && +virProcessSetNamespaces(nfds, fds) < 0) +goto cleanup; + +ret = 0; + cleanup: +for (i = 0; i < nfds; i++) +VIR_FORCE_CLOSE(fds[i]); +VIR_FREE(fds); +return ret; +} + + +static int +qemuProcessStartPRDaemon(virDomainObjPtr vm, + qemuDomainDiskPRDPtr prd) +{ +qemuDomainObjPrivatePtr priv = vm->privateData; +virQEMUDriverPtr driver = priv->driver; +virQEMUDriverConfigPtr cfg; +int errfd = -1; +char *pidfile = NULL; +int pidfd = -1; +pid_t cpid = -1; +virCommandPtr cmd = NULL; +virTimeBackOffVar timebackoff; +const unsigned long long timeout = 50; /* ms */ +int ret = -1; + +cfg = virQEMUDriverGetConfig(driver); + +if (!virFileIsExecutable(cfg->prHelperName)) { +virReportSystemError(errno, _("'%s' is not a suitable pr helper"), + cfg->prHelperName); +goto cleanup; +} + +if (!(pidfile = qemuProcessBuildPRHelperPidfilePath(vm, prd->alias))) +goto cleanup; + +/* Just try to acquire. Dummy pid will be replaced later */ +if ((pidfd = virPidFileAcquirePath(pidfile, false, -1)) < 0) +goto cleanup; + +/* Remove stale socket */ +if (unlink(prd->path) < 0 && +errno != ENOENT) { +virReportSystemError(errno, + _("Unable to remove stale socket path: %s"), + prd->path); +goto cleanup; +} + +if (!(cmd = virCommandNewArgList(cfg->prHelperName, + "-k", prd->path, + "-f", pidfile, + NULL))) +goto cleanup; + +virCommandDaemonize(cmd); +/* We want our virCommand to write child PID into the pidfile + * so that we can read it even before exec(). */ +virCommandSetPidFile(cmd, pidfile); +virCommandSetErrorFD(cmd, &errfd); + +/* Place the process into the same namespace and cgroup as + * qemu (so that it shares the same view of the system). */ +virCommandSetPreExecHook(cmd, qemuProcessStartPRDaemonHook, vm); + +if (virCommandRun(cmd, NULL) < 0) +goto cleanup; + +if (virPidFileReadPath(pidfile, &cpid) < 0) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("pr helper %s didn't show up"), prd->alias); +goto cleanup; +} + +if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0) +goto cleanup; +while (virTimeBackOffWait(&timebackoff)) { +char errbuf[1024] = { 0 }; + +if (virFileExists(prd->path)) +break; + +if (virProcessKill(cpid, 0) == 0)
[libvirt] [PATCH v4 03/14] qemu: Introduce pr-manager-helper capability
The capability tracks if qemu has pr-manager-helper object. At this time don't actually detect if qemu has the capability. Not just yet. Only after the code is written the feature will be enabled. Signed-off-by: Michal Privoznik --- src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_capabilities.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 91b7aa31ec..efd468692a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -468,6 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-tablet-ccw", "qcow2-luks", "pcie-pci-bridge", + "pr-manager-helper", ); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index bec28cae92..f0948e3016 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -452,6 +452,7 @@ typedef enum { QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */ QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */ QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */ +QEMU_CAPS_PR_MANAGER_HELPER, /* -object pr-manager-helper */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 00/14] Basic implementation of persistent reservations
v4 of: https://www.redhat.com/archives/libvir-list/2018-March/msg00745.html diff to v3: - Peter's review worked in Michal Privoznik (14): virstoragefile: Introduce virStoragePRDef qemuDomainDiskChangeSupported: Deny changing reservations qemu: Introduce pr-manager-helper capability qemu: Generate alias and socket path for pr-helper qemu: Store pr runtime data in status XML qemu_ns: Allow /dev/mapper/control for PR qemu_cgroup: Allow /dev/mapper/control for PR qemu: Generate cmd line at startup qemu: Introduce pr_helper to qemu.conf qemu: Start PR daemon on domain startup qemu: Start PR daemon on disk hotplug qemu_hotplug: Hotplug of reservations qemu_hotplug: Hotunplug of reservations qemu: Detect pr-manager-helper capability docs/formatdomain.html.in | 25 ++- docs/schemas/domaincommon.rng | 34 +--- docs/schemas/storagecommon.rng | 50 + m4/virt-driver-qemu.m4 | 5 + src/conf/domain_conf.c | 38 src/libvirt_private.syms | 6 + src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 4 + src/qemu/qemu_alias.c | 11 + src/qemu/qemu_alias.h | 2 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_cgroup.c | 33 ++- src/qemu/qemu_command.c| 94 + src/qemu/qemu_command.h| 3 + src/qemu/qemu_conf.c | 7 +- src/qemu/qemu_conf.h | 1 + src/qemu/qemu_domain.c | 173 +++- src/qemu/qemu_domain.h | 10 + src/qemu/qemu_hotplug.c| 130 src/qemu/qemu_process.c| 224 + src/qemu/qemu_process.h| 5 + src/qemu/test_libvirtd_qemu.aug.in | 1 + src/util/virdevmapper.c| 8 +- src/util/virstoragefile.c | 164 +++ src/util/virstoragefile.h | 18 ++ tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + .../disk-virtio-scsi-reservations.args | 35 .../disk-virtio-scsi-reservations.xml | 49 + tests/qemuxml2argvtest.c | 4 + .../disk-virtio-scsi-reservations.xml | 1 + tests/qemuxml2xmltest.c| 2 + 36 files changed, 1107 insertions(+), 39 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml create mode 12 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 01/14] virstoragefile: Introduce virStoragePRDef
This is a definition that holds information on SCSI persistent reservation settings. The XML part looks like this: If @managed is set to 'yes' then the is not parsed. This design was agreed on here: https://www.redhat.com/archives/libvir-list/2017-November/msg01005.html Signed-off-by: Michal Privoznik --- docs/formatdomain.html.in | 25 +++- docs/schemas/domaincommon.rng | 34 +- docs/schemas/storagecommon.rng | 50 src/conf/domain_conf.c | 38 ++ src/libvirt_private.syms | 3 + src/util/virstoragefile.c | 131 + src/util/virstoragefile.h | 14 +++ .../disk-virtio-scsi-reservations.xml | 49 .../disk-virtio-scsi-reservations.xml | 1 + tests/qemuxml2xmltest.c| 2 + 10 files changed, 316 insertions(+), 31 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml create mode 12 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 5e99884dc5..c20e98b4ef 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2563,7 +2563,10 @@@@ -2926,6 +2929,26 @@ Storage Encryption page for more information. + reservations + Since libvirt 4.1.0, the +reservations can be a sub-element of the +source element for storage sources (QEMU driver only). +If present (and enabled) it enabled persistent reservations for +SCSI based disks. The element has one mandatory attribute +enabled with accepted values yes and +no. If the feature is enabled, then there's another +mandatory attribute managed (accepted values are the +same as for enabled) that enables or disables libvirt +spawning any helper process (should one be needed). However, if +libvirt is not enabled spawning helper process (i.e. hypervisor +should just use one which is already running), path to its socket +must be provided in child element source, which +currently accepts only the following attributes: type +with one value unix, path with path the +socket, and finally mode which accepts either +server or client and specifies the role +of hypervisor. + diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4cab55f05d..93084887fb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1530,6 +1530,9 @@ + + + @@ -2434,18 +2437,6 @@ - - - - - - - - - - - - - +
[libvirt] [PATCH v4 07/14] qemu_cgroup: Allow /dev/mapper/control for PR
Just like in previous commit, qemu-pr-helper might want to open /dev/mapper/control under certain circumstances. Therefore we have to allow it in cgroups. Signed-off-by: Michal Privoznik --- src/qemu/qemu_cgroup.c | 33 ++--- src/util/virdevmapper.c | 8 +++- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index d88eb7881f..546a4c8e63 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -114,6 +114,8 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm, } +#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control" + static int qemuSetupImageCgroupInternal(virDomainObjPtr vm, virStorageSourcePtr src, @@ -125,6 +127,10 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm, return 0; } +if (virStoragePRDefIsManaged(src->pr) && +qemuSetupImagePathCgroup(vm, DEVICE_MAPPER_CONTROL_PATH, false) < 0) +return -1; + return qemuSetupImagePathCgroup(vm, src->path, src->readonly || forceReadonly); } @@ -142,9 +148,8 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src) { qemuDomainObjPrivatePtr priv = vm->privateData; -int perms = VIR_CGROUP_DEVICE_READ | -VIR_CGROUP_DEVICE_WRITE | -VIR_CGROUP_DEVICE_MKNOD; +int perms = VIR_CGROUP_DEVICE_RWM; +size_t i; int ret; if (!virCgroupHasController(priv->cgroup, @@ -157,6 +162,28 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, return 0; } +for (i = 0; i < vm->def->ndisks; i++) { +virStorageSourcePtr diskSrc = vm->def->disks[i]->src; + +if (src == diskSrc) +continue; + +if (virStoragePRDefIsManaged(diskSrc->pr)) +break; +} + +if (i == vm->def->ndisks) { +VIR_DEBUG("Disabling device mapper control"); +ret = virCgroupDenyDevicePath(priv->cgroup, + DEVICE_MAPPER_CONTROL_PATH, perms, true); +virDomainAuditCgroupPath(vm, priv->cgroup, "deny", + DEVICE_MAPPER_CONTROL_PATH, + virCgroupGetDevicePermsString(perms), ret); +if (ret < 0) +return ret; +} + + VIR_DEBUG("Deny path %s", src->path); ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index d2c25af003..ef4b1e480a 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -101,8 +101,14 @@ virDevMapperGetTargetsImpl(const char *path, dm_task_no_open_count(dmt); -if (!dm_task_run(dmt)) +if (!dm_task_run(dmt)) { +if (errno == ENXIO) { +/* In some cases devmapper realizes this late device + * is not managed by it. */ +ret = 0; +} goto cleanup; +} if (!dm_task_get_info(dmt, &info)) goto cleanup; -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 06/14] qemu_ns: Allow /dev/mapper/control for PR
If qemu-pr-helper is compiled with multipath support the first thing it does is opening /dev/mapper/control. Since we're going to be running it inside qemu namespace we need to create it there. Unfortunately, we don't know if it was compiled with or without multipath so we have to create it anyway. BTW: This might be the ugliest piece of code I've ever written but @devMapperControl really needs to be type of char * otherwise some crazy check in VIR_APPEND_ELEMENT fails. Signed-off-by: Michal Privoznik --- src/qemu/qemu_domain.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0856f04406..6fe4eb57e1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -108,6 +108,7 @@ VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST, #define PROC_MOUNTS "/proc/mounts" #define DEVPREFIX "/dev/" #define DEV_VFIO "/dev/vfio/vfio" +#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control" struct _qemuDomainLogContext { @@ -10269,6 +10270,11 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, goto cleanup; } +/* qemu-pr-helper might require access to /dev/mapper/control. */ +if (virStoragePRDefIsEnabled(disk->src->pr) && +qemuDomainCreateDevice(DEVICE_MAPPER_CONTROL_PATH, data, true) < 0) +goto cleanup; + ret = 0; cleanup: VIR_FREE(dst); @@ -11281,6 +11287,9 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, const char **paths = NULL; size_t npaths = 0; int ret = -1; +/* This is very nasty but we need it to work around some + * stupid checks in VIR_APPEND_ELEMENT macro. */ +char *devMapperControl = (char *) DEVICE_MAPPER_CONTROL_PATH; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -11296,6 +11305,11 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, goto cleanup; } +/* qemu-pr-helper might require access to /dev/mapper/control. */ +if (virStoragePRDefIsEnabled(src->pr) && +VIR_APPEND_ELEMENT_COPY(paths, npaths, devMapperControl) < 0) +goto cleanup; + if (qemuDomainNamespaceMknodPaths(vm, paths, npaths) < 0) return -1; -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 02/14] qemuDomainDiskChangeSupported: Deny changing reservations
Couple of reasons for that: a) there's no monitor command to change path where the pr-helper connects to, or b) there's no monitor command to introduce a new pr-helper for a disk that already exists. Signed-off-by: Michal Privoznik --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c| 8 src/util/virstoragefile.c | 18 ++ src/util/virstoragefile.h | 2 ++ 4 files changed, 29 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b39b694c60..a376e3bb0d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2799,6 +2799,7 @@ virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStoragePRDefFormat; virStoragePRDefFree; +virStoragePRDefIsEqual; virStoragePRDefParseXML; virStorageSourceBackingStoreClear; virStorageSourceClear; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 100304fd05..5a7b5f8417 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7820,6 +7820,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(src->readonly, "readonly", true); CHECK_EQ(src->shared, "shared", true); +if (!virStoragePRDefIsEqual(disk->src->pr, +orig_disk->src->pr)) { +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "reservations"); +return false; +} + #undef CHECK_EQ return true; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 9917837513..b017024b2f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2022,6 +2022,24 @@ virStoragePRDefFormat(virBufferPtr buf, } +bool +virStoragePRDefIsEqual(virStoragePRDefPtr a, + virStoragePRDefPtr b) +{ +if (!a && !b) +return true; + +if (!a || !b) +return false; + +if (a->enabled != b->enabled || +a->managed != b->managed || +STRNEQ_NULLABLE(a->path, b->path)) +return false; + +return true; +} + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b705ab1f92..77853eefe8 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -383,6 +383,8 @@ void virStoragePRDefFree(virStoragePRDefPtr prd); virStoragePRDefPtr virStoragePRDefParseXML(xmlXPathContextPtr ctxt); void virStoragePRDefFormat(virBufferPtr buf, virStoragePRDefPtr prd); +bool virStoragePRDefIsEqual(virStoragePRDefPtr a, +virStoragePRDefPtr b); virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 4/4] qemu: deny privilege elevation and spawn in seccomp
If QEMU uses a seccomp blacklist (since 2.11), -sandbox on no longer tries to whitelist all the calls, but uses sets of blacklists: default (always blacklisted with -sandbox on) obsolete (defaults to deny) elevateprivileges (setuid & co, default: allow) spawn (fork & execve, default: allow) resourcecontrol (setaffinity, setscheduler, default: allow) If these are supported, default to sandbox with all four categories blacklisted. https://bugzilla.redhat.com/show_bug.cgi?id=1492597 Signed-off-by: Ján Tomko --- src/qemu/qemu.conf | 7 +++--- src/qemu/qemu_command.c | 10 + tests/qemuxml2argvdata/minimal-sandbox.args | 29 tests/qemuxml2argvdata/minimal-sandbox.xml | 34 + tests/qemuxml2argvtest.c| 11 ++ 5 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/minimal-sandbox.args create mode 100644 tests/qemuxml2argvdata/minimal-sandbox.xml diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 07eab7eff..740129cf5 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -669,9 +669,10 @@ -# Use seccomp syscall whitelisting in QEMU. -# 1 = on, 0 = off, -1 = use QEMU default -# Defaults to -1. +# Use seccomp syscall sandbox in QEMU. +# 1 = on, 0 = off, -1 = use the default +# For QEMUs using a whitelist, the default (-1) is off. +# For QEMUs using a blacklist, the default (-1) is on. # #seccomp_sandbox = 1 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ba279e640..fa5906d0b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9987,6 +9987,16 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, return 0; } +/* Use blacklist by default if supported */ +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_BLACKLIST)) { +virCommandAddArgList(cmd, "-sandbox", + "on,obsolete=deny,elevateprivileges=deny," + "spawn=deny,resourcecontrol=deny", + NULL); +return 0; +} + +/* Seccomp whitelist is opt-in */ if (cfg->seccompSandbox > 0) virCommandAddArgList(cmd, "-sandbox", "on", NULL); diff --git a/tests/qemuxml2argvdata/minimal-sandbox.args b/tests/qemuxml2argvdata/minimal-sandbox.args new file mode 100644 index 0..c9d71fe8f --- /dev/null +++ b/tests/qemuxml2argvdata/minimal-sandbox.args @@ -0,0 +1,29 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-S \ +-machine pc,accel=tcg,usb=off,dump-guest-core=off \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny diff --git a/tests/qemuxml2argvdata/minimal-sandbox.xml b/tests/qemuxml2argvdata/minimal-sandbox.xml new file mode 100644 index 0..9ef92f8fe --- /dev/null +++ b/tests/qemuxml2argvdata/minimal-sandbox.xml @@ -0,0 +1,34 @@ + + QEMUGuest1 + c7a5fdbd-edaf-9455-926a-d65c16db1809 + A description of the test machine. + + A test of qemu's minimal configuration. + This test also tests the description and title elements. + + 219100 + 219100 + 1 + +hvm + + + + destroy + restart + destroy + +/usr/bin/qemu-system-i686 + + + + + + + + + + + + + diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 9a22fe5f4..cf69135e8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -729,6 +729,17 @@ mymain(void) unsetenv("SDL_AUDIODRIVER"); DO_TEST("minimal", NONE); +DO_TEST("minimal-sandbox", +QEMU_CAPS_MACHINE_OPT, +QEMU_CAPS_MONITOR_JSON, +QEMU_CAPS_NO_USER_CONFIG, +QEMU_CAPS_RTC, +QEMU_CAPS_NO_SHUTDOWN, +QEMU_CAPS_DUMP_GUEST_CORE, +QEMU_CAPS_DISPLAY, +QEMU_CAPS_MACHINE_USB_OPT, +QEMU_CAPS_SECCOMP_SANDBOX, +QEMU_CAPS_SECCOMP_BLACKLIST); DO_TEST_PARSE_ERROR("minimal-no-memory", NONE); DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP); DO_TEST("machine-aliases1", NONE); -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 2/4] Introduce qemuBuildSeccompSandboxCommandLine
Move the building of -sandbox command line into a separate function. Signed-off-by: Ján Tomko --- src/qemu/qemu_command.c | 30 +- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 514c3ab2e..dfeba54ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9969,6 +9969,26 @@ qemuBuildCommandLineValidate(virQEMUDriverPtr driver, } +static int +qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, + virQEMUDriverConfigPtr cfg, + virQEMUCapsPtr qemuCaps) +{ +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) { +if (cfg->seccompSandbox == 0) +virCommandAddArgList(cmd, "-sandbox", "off", NULL); +else if (cfg->seccompSandbox > 0) +virCommandAddArgList(cmd, "-sandbox", "on", NULL); +} else if (cfg->seccompSandbox > 0) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("QEMU does not support seccomp sandboxes")); +return -1; +} +return 0; + +} + + /* * Constructs a argv suitable for launching qemu with config defined * for a given virtual machine. @@ -10206,16 +10226,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, ? qemucmd->env_value[i] : ""); } -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) { -if (cfg->seccompSandbox == 0) -virCommandAddArgList(cmd, "-sandbox", "off", NULL); -else if (cfg->seccompSandbox > 0) -virCommandAddArgList(cmd, "-sandbox", "on", NULL); -} else if (cfg->seccompSandbox > 0) { -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("QEMU does not support seccomp sandboxes")); +if (qemuBuildSeccompSandboxCommandLine(cmd, cfg, qemuCaps) < 0) goto error; -} if (qemuBuildPanicCommandLine(cmd, def, qemuCaps) < 0) goto error; -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 3/4] Refactor qemuBuildSeccompSandboxCommandLine
Exit early if possible to simplify the logic. Signed-off-by: Ján Tomko --- src/qemu/qemu_command.c | 18 -- 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dfeba54ee..ba279e640 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9974,16 +9974,22 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, virQEMUDriverConfigPtr cfg, virQEMUCapsPtr qemuCaps) { -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) { -if (cfg->seccompSandbox == 0) -virCommandAddArgList(cmd, "-sandbox", "off", NULL); -else if (cfg->seccompSandbox > 0) -virCommandAddArgList(cmd, "-sandbox", "on", NULL); -} else if (cfg->seccompSandbox > 0) { +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX) && +cfg->seccompSandbox > 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("QEMU does not support seccomp sandboxes")); return -1; } + +if (cfg->seccompSandbox == 0) { +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX)) +virCommandAddArgList(cmd, "-sandbox", "off", NULL); +return 0; +} + +if (cfg->seccompSandbox > 0) +virCommandAddArgList(cmd, "-sandbox", "on", NULL); + return 0; } -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 0/4] qemu: enable sandbox whitelist by default
v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01965.html https://bugzilla.redhat.com/show_bug.cgi?id=1492597 v2: * also deny resource control * split out and refactor the command line building * be explicit about denying the obsolete syscalls Ján Tomko (4): Introduce QEMU_CAPS_SECCOMP_BLACKLIST Introduce qemuBuildSeccompSandboxCommandLine Refactor qemuBuildSeccompSandboxCommandLine qemu: deny privilege elevation and spawn in seccomp src/qemu/qemu.conf | 7 ++-- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c| 46 +- tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemuxml2argvdata/minimal-sandbox.args| 29 ++ tests/qemuxml2argvdata/minimal-sandbox.xml | 34 tests/qemuxml2argvtest.c | 11 ++ 12 files changed, 123 insertions(+), 12 deletions(-) create mode 100644 tests/qemuxml2argvdata/minimal-sandbox.args create mode 100644 tests/qemuxml2argvdata/minimal-sandbox.xml -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 1/4] Introduce QEMU_CAPS_SECCOMP_BLACKLIST
QEMU commit 1bd6152 changed the default behavior from whitelist to blacklist and introduced a few sets of system calls. Use the 'elevateprivileges' parameter of -sandbox as a witness of this change. https://bugzilla.redhat.com/show_bug.cgi?id=1492597 Signed-off-by: Ján Tomko --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + 7 files changed, 8 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 35905e993..729e29e20 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -468,6 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-tablet-ccw", "qcow2-luks", "pcie-pci-bridge", + "seccomp-blacklist", ); @@ -3214,6 +3215,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "machine", "loadparm", QEMU_CAPS_LOADPARM }, { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS }, { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT }, +{ "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST }, }; static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index bec28cae9..d88102f34 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -452,6 +452,7 @@ typedef enum { QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */ QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */ QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */ +QEMU_CAPS_SECCOMP_BLACKLIST, /* -sandbox.elevateprivileges */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml index cbd645ae9..3861666e5 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -151,6 +151,7 @@ + 2011000 0 342058 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml index 66629ff5b..39238a9b6 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -189,6 +189,7 @@ + 2011090 0 342346 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index 1122d6408..6bf293b9d 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -186,6 +186,7 @@ + 2011090 0 419215 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 191b1e0e3..b77aec9c9 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -151,6 +151,7 @@ + 2011090 0 0 diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 4ed2e1ea9..1bb825c9b 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -227,6 +227,7 @@ + 2011090 0 390060 -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v2 04/44] Require QEMU 1.5.0
On Mon, Apr 09, 2018 at 05:54:17PM +0200, Andrea Bolognani wrote: On Thu, 2018-04-05 at 14:22 +0200, Ján Tomko wrote: According to the policy described on https://libvirt.org/platforms.html the QEMU versions in the oldest relevant releses are: Empty line here. Possibly indent the distros with two spaces. SLES 12: 2.0.0 RHEL 7: 1.5.3 Ubuntu 14.04: 2.0.0 Set the minimum to 1.5.0 and drop support for RHEL 6. This lets us drop the -help parsing code and assume lots of capabilities. Except we already dropped the -help parsing code in the previous commit, and we haven't started assuming capabilities yet :) So I would use This will let us assume lots of capabilities. here. diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 0be39b76dd..f427cfdeaa 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3755,6 +3755,9 @@ virQEMUCapsProbeQMPSchemaCapabilities(virQEMUCapsPtr qemuCaps, return 0; } +#define QEMU_MIN_MAJOR 1 +#define QEMU_MIN_MINOR 5 +#define QEMU_MIN_MICRO 0 int virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, @@ -3785,9 +3788,12 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, VIR_DEBUG("Got version %d.%d.%d (%s)", major, minor, micro, NULLSTR(package)); -if (major < 1 || (major == 1 && minor < 2)) { -VIR_DEBUG("Not new enough for QMP capabilities detection"); -ret = 0; +if (major < QEMU_MIN_MAJOR || +(major == QEMU_MIN_MAJOR && minor < QEMU_MIN_MINOR)) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("QEMU version >= %d.%d.%d is required, but %d.%d.%d found"), + QEMU_MIN_MAJOR, QEMU_MIN_MINOR, QEMU_MIN_MICRO, + major, minor, micro); goto cleanup; } I think it would make more sense for the check and the error message to be converted in the previous commit, where you raise the minimum QEMU version to 1.2.0, so that this commit will end up only changing QEMU_MIN_MINOR to 5 and dropping "ret = 0" (along with the expected test suite churn, of course). With the comments addressed, Reviewed-by: Andrea Bolognani Thanks, I have pushed the first four patches (this time including your review feedback) I will deal with usedQMP separately. Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH v2 00/11] Some code move to have functions sorted
On Mon, Apr 09, 2018 at 07:05:32PM +0200, Katerina Koukiou wrote: > Better do it now before the projects grows more. > > Katerina Koukiou (11): > APIs should appear in alphabetical order: Move Active property > APIs should appear in alphabetical order: Move Autostart property > APIs should appear in alphabetical order: Move Id property > APIs should appear in alphabetical order: Move UUID property > APIs should appear in alphabetical order: Move Create method > APIs should appear in alphabetical order: Move Destroy method > APIs should appear in alphabetical order: Move GetStats method > APIs should appear in alphabetical order: Move Resume method > APIs should appear in alphabetical order: Move Undefine method > APIs should appear in alphabetical order: Move Shutdown method > APIs should appear in alphabetical order: Move ListDomains method Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 12:20, Daniel P. Berrangé wrote: > On Sat, Apr 07, 2018 at 02:01:17AM +0200, Laszlo Ersek wrote: >> +{ 'struct' : 'SystemFirmware', >> + 'data' : { 'executable' : 'FirmwareFile', >> + 'type' : 'SystemFirmwareType', >> + 'targets': [ 'str' ], >> + 'sysfw-map' : 'FirmwareMapping', >> + '*nvram-slots' : [ 'NVRAMSlot' ], >> + '*supports-uefi-secure-boot' : 'bool', >> + '*supports-amd-sev' : 'bool', >> + '*supports-acpi-s3' : 'bool', >> + '*supports-acpi-s4' : 'bool' } } > > Elsewhere in the thread I mentioned that I think we should try to use a > union approach to isolate which information is relevant to "flash" loader > format and which is relevant to "memory" and "kernel". To try to illustrate > what I mean by that I've knocked up an alternative structure. I also > incorporated the points about features & target/machine types. I've left > out the read/write/etc fields, but they could be put back in at the > relevant position I think this looks very nice; with the addition of - "requires-smm" to "SystemFirmwareFeature": > { 'enum' : 'SystemFirmwareFeature', > 'data': ['acpi-s3', 'acpi-s5', 'secure-boot', 'amd-sev' ]} - and another feature flag (perhaps in SystemFirmwareFeature, perhaps in SystemFirmwareBinaryFlashVars) for the cmdline option "-global driver=cfi.pflash01,property=secure,value=on", this could be called a day as far as SeaBIOS and OVMF are concerned. Thanks Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 11:55, Daniel P. Berrangé wrote: > On Tue, Apr 10, 2018 at 11:51:31AM +0200, Gerd Hoffmann wrote: Hmm, I'm wondering whenever it is useful to model things this way. It's not like you can actually configure things for -bios seabios.rom or -kernel uboot.elf. Only pflash allows to actually configure things, and there are not that many useful combinations. The code needs Read+Execute. Allowing Write could be useful in theory, to allow the guest doing firmware updates. But I think nobody actually does that, so in practice it is fixed. The varstore can have different permissions, but it's only two useful combinations. Either allow access unconditionally, or allow access in secure contect (aka smm) only. >>> >>> (I hope I understand your point right:) >>> >>> I'm also fine if we simply define a fixed (but extensible) set of >>> mapping methods, basically a new enum type, that simply tells libvirtd >>> what this firmware *is*. IOW, directly reference a mapping method we >>> know libvirt implements, rather than give vague hints. >>> >>> This could repurpose SystemFirmwareType, but it should become more >>> detailed. I'm thinking like: >>> - ovmf: split files without requiring SMM >>> - ovmf_smm: split files with SMM requirement >>> - seabios: exactly that >>> - ... other things others suggest. >> >> I wouldn't name them by firmware, that is misleading. Basically we have >> three cases: >> >> (1) single firmware image (seabios, OVMF.fd, ...). >> (2) split firmware image (OVMF_{CODE,VARS}.fd), where vars can be >> writable unconditinally. >> (3) split firmware image, where access to vars should be restricted >> to smm mode. >> >> (2) + (3) requires pflash. (1) works with both pflash and -bios. > > A big chunk of the data in the schema looks specific to the pflash > case, but this is not expressed except in the docs. Most of the time > with QAPI when we have data that is only relevant in certain types, > we use a discriminated union to describe it. It feels like a unioon > approach could be better suited to this I used a discriminated union specifically for pflash options in RFCv0, which I didn't post. I felt that it wasn't flexible enough. :) Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/9] Be consistent with vir*Obj*Remove* APIs
On 04/10/2018 04:47 AM, Marc Hartmayer wrote: > On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan > wrote: >> v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html >> >> NB: This can wait until 4.2.0 is release, but I figured I'd post this >> now just to put it on the radar and of course in hopes that someone >> will look during the idle moment or two before the release. >> >> Changes since v1: >> >> Short story: Rework the processing of the code >> >> Longer story: >> >> In his review Erik noted that there's a "fire dance" when processing >> the vir*Obj*Remove APIs of requiring a locked object upon entry, then >> adding a reference to that object, unlocking the object, locking the >> list to which it is contained, and then relocking the object. >> >> So it took some time to think about it and during one lengthy meeting >> today I had the aha moment that the *Remove API's could take the same >> key (e.g., uuid or name) used to Add or Find the object and use it for >> the Remove API. This allows the Remove API to not require a locked (and >> reffed) object upon entry and perform the lock dance, remove the object, >> and return just just a reffed object forcing the caller to know to Unref >> object. >> >> Instead, let's simplify things. The *Remove API can take the key, Find >> the object in the list, remove it from the hash tables, and dispose of >> the object. In essence the antecedent to the Add or AssignDef API's >> taking a def, creating an object, and adding it the object to the hash >> table(s). If there are two *Remove threads competing, one will win and >> perform the removal, while the other will call *Remove, but won't find >> the object in the hash table, and just return none the wiser. >> >> And yes, I think this can also work for the Domain code, but it's going >> to take a few patch series to get there as that code is not consistent >> between consumers. >> >> John Ferlan (9): >> secret: Rework LoadAllConfigs >> secret: Alter virSecretObjListRemove processing >> interface: Alter virInterfaceObjListRemove processing >> nodedev: Alter virNodeDeviceObjListRemove processing >> conf: Clean up virStoragePoolObjLoad error processing >> storage: Clean up storagePoolCreateXML error processing >> test: Clean up testStoragePoolCreateXML error processing >> conf: Move virStoragePoolObjRemove closer to AssignDef >> storagepool: Alter virStoragePoolObjRemove processing > > Side note: > Wouldn’t is be useful to refactor all the vir*ObjList* things as they’re > looking quite similar? Not sure if it’s easily feasible in all places. > Well - that was the point of what I started last year, but there hasn't been any general agreement or acceptance of patches for that. My changes made use of objects and more generic naming to unify things; however, they weren't acceptable with (IIRC and in my words) the preference being more specific naming using switch/case statements and shim API's. The last full posting (a v5) of what I had done is here: https://www.redhat.com/archives/libvir-list/2017-August/msg00659.html If you feel so inclined you can follow the history of comments through v4: https://www.redhat.com/archives/libvir-list/2017-August/msg00537.html and v3: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html w/ review comments starting here: https://www.redhat.com/archives/libvir-list/2017-July/msg01032.html Maybe once the domain code is modified to be more common (in process now) and if nwfilter ever could gain acceptance, something could be done. Still I have my doubts it'll happen especially since nwfilter patches just cannot get any sort of agreement, last try here: https://www.redhat.com/archives/libvir-list/2018-February/msg00325.html John > […snip] > > -- > Beste Grüße / Kind regards >Marc Hartmayer > > IBM Deutschland Research & Development GmbH > Vorsitzende des Aufsichtsrats: Martina Koederitz > Geschäftsführung: Dirk Wittkopp > Sitz der Gesellschaft: Böblingen > Registergericht: Amtsgericht Stuttgart, HRB 243294 > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 11:34, Daniel P. Berrangé wrote: > On Tue, Apr 10, 2018 at 11:16:01AM +0200, Laszlo Ersek wrote: >> On 04/10/18 08:27, Gerd Hoffmann wrote: >>> Hi, >>> - I considered adding wildcards (say, blacklist "all" i440fx machtypes, present and future, for SMM-requiring OVMF builds), but then you get into version sorting and similar mess. I considered fnmatch() -- basically simple ? and * wildcards -- but that's not expressive enough. >>> >>> I'd suggest whitelist with wildcards. So the smm builds would get >>> "pc-q35-*". >>> >>> libvirt knows about aliases, so it should be able to handle the "q35" >>> shortcut like "pc-q35-${latest}". >>> >>> Or do you see another issue? >> >> Well, one issue I see is version sorting; I should say "Q35 but no >> earlier than 2.4", and lexicographically, "2.11" sorts before "2.4". >> >> Anyway (also asking for Thomas's input here): if we run with your idea >> to refer to exact mapping methods / firmware *implementation* types that >> we know libvirt implements / supports as a "white box", do we still deem >> machine type identification necessary? Because, libvirt already knows >> (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just >> have to make a *reference* to that knowledge in the JSON file. > > BTW, that's not quite correct - when libvirt handles the "smm" arg it > checks if machine type == q35, and QEMU version >= 2.4. > > It is *not* checking the version of the machine type. ie it will happily > use smm with pc-q35-2.0, as long as QEMU version is 2.4. Perhaps this is > not quite right, (it's not) > but we don't try to parse the version number out of the > machine type, because we can't assume a specific format for the machine > type version part. eg version can be "2.4", or it can be "rhel-7.0.0" > or something else again on Ubuntu. Indeed, that's exactly why I'm troubled about expressing a "minimum" machine type version. > > IMHO it would be valid to just keep life simple and only record the base > machine type name that can use the firmware ie "pc", "q35", and ignore > the fact that in some cases the firmware might require a specific version > of the machine type. Esp. with regard to SMM, there have been quite big jumps in usability / stability across Q35 machtype versions. But, if it works for you, it works for me. (I double-checked Thomas's recent example about U-Boot, and he mentioned the "ppce500" and "sam460ex" machine types, not machine type versions.) Thanks, Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 11:32, Thomas Huth wrote: > On 10.04.2018 11:22, Laszlo Ersek wrote: >> On 04/10/18 09:33, Thomas Huth wrote: > [...] >>> Alternatively, what about providing some kind of "alias" or "nickname" >>> setting here, too? So the EDK2 builds would get >>> SystemFirmwareType="edk2" and "SystemFirmwareAlias="uefi" for example. >> >> I hope I understand you right -- I think your suggestion ties in with my >> other email I just sent in this thread. So, we could tell libvirtd, >> "this firmware is of type 'UEFI', and you must use the 'ovmf_smm' >> mapping method to run it, with this file or that file as varstore template". >> >> We could even describe the parameters for this or that mapping method >> structurally in the schema (in a discriminated union in QAPI JSON, or in >> an XSD choice element). For example, "ovmf" and "ovmf_smm" would both >> take "OvmfSplitFileOptions" -- a list of single varstore template files >> with feature enum contants attached --, while "SeaBiosOptions" would be >> an empty structure. > > Sorry, I've got no clue about ovmf_smm and the other things you've > mentioned here ;-) > >> I feel the key question here is whether we are allowed to directly >> reference a mapping method we know libvirt implements. If we are, that >> makes things a lot clearer (and easier, I should hope). > > Key question is maybe rather: Do you want to design / implement > something that is libvirt-only here, or rather something generic that > could also be used for other upper layer tools that do not use libvirt? > (... and looks like Daniel just had the same comment in another mail in > this thread ...) Yeah, we can't target libvirtd as the sole consumer. Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 11:26, Thomas Huth wrote: > On 10.04.2018 11:16, Laszlo Ersek wrote: >> On 04/10/18 08:27, Gerd Hoffmann wrote: >>> Hi, >>> - I considered adding wildcards (say, blacklist "all" i440fx machtypes, present and future, for SMM-requiring OVMF builds), but then you get into version sorting and similar mess. I considered fnmatch() -- basically simple ? and * wildcards -- but that's not expressive enough. >>> >>> I'd suggest whitelist with wildcards. So the smm builds would get >>> "pc-q35-*". >>> >>> libvirt knows about aliases, so it should be able to handle the "q35" >>> shortcut like "pc-q35-${latest}". >>> >>> Or do you see another issue? >> >> Well, one issue I see is version sorting; I should say "Q35 but no >> earlier than 2.4", and lexicographically, "2.11" sorts before "2.4". >> >> Anyway (also asking for Thomas's input here): if we run with your idea >> to refer to exact mapping methods / firmware *implementation* types that >> we know libvirt implements / supports as a "white box", do we still deem >> machine type identification necessary? Because, libvirt already knows >> (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just >> have to make a *reference* to that knowledge in the JSON file. > > I think you really need a way to specify the machine there. Latest > example from QEMU 2.12: We've now got two uboot binaries in the tree, > pc-bios/u-boot.e500 and pc-bios/u-boot-sam460-20100605.bin. Both are > uboot, both are for ppc, but u-boot.e500 only works with the "ppce500" > machine and the other one only works with the "sam460ex" machine. How > would you teach libvirt such a relationship without an explicit machine > type identification field there? My idea was to assign different "map method" enumeration constants to them, and libvirtd would associate those with different machtype requirements. But, as Daniel explained, we cannot reference libvirtd features, so I agree we have to express machine types somehow. I don't know how though. For example, can we take it for granted that a machtype version number, if it exists in the first place, always follows the last hyphen in the machtype identifier? Say, "virt-2.11" / aarch64 conforms, "pc-q35-2.4" and "pc-i440fx-2.12" conform too. But, is that a guarantee that covers all arches and all boards? Because, I don't think: - machine-type-family = q35 - minimum-machine-type = 2.4 will work. Will every application that manages QEMU learn that "q35" is short-hand for "pc-q35-XXX", and (again) that 2.12 sorts *after* 2.4? Thanks, Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On Tue, Apr 10, 2018 at 12:48:28PM +0100, Peter Maydell wrote: > On 10 April 2018 at 12:34, Daniel P. Berrangé wrote: > > On Tue, Apr 10, 2018 at 01:27:18PM +0200, Laszlo Ersek wrote: > > > >> Please go through the rest of the emails in this thread, and advise: > >> - if the firmware descriptor schema may perhaps live in the libvirt tree, > >> - accordingly, if the schema could be expressed as an XSD (and firmware > >> packages should provide the descriptor documents as XMLs) > >> - if you agree that the descriptor document can uniquely reference > >> mapping methods implemented in libvirtd by simple enum constants (with > >> necessary parameters provided). > > > > No to all three. This is the responsibility of QEMU to define, because > > this information is relevant to anything managing QEMU not just libvirt. > > (Please consider this as more of a grenade lobbed into the conversation > rather than a carefully thought out proposal...) > > My inclination is to say that it's not really the responsibility > of QEMU to define either -- we provide emulated models of hardware, > and it's up to the user or the management layer or the provider > of the firmware to specify what guest code they want to run and how > it needs to run on that emulated hardware... > > Where the QEMU upstream itself is providing firmware blobs > (in tarballs etc) it's probably our job to specify how they work, > but if the firmware is compiled and provided by the distro (as eg happens > for Arm UEFI blobs at the moment) then I don't see how upstream QEMU > can reliably define how that firmware needs to be loaded. QEMU should not provide the actual metadata files themselves - it just has to the define the file format. The relevant firmware upstreams and or distros, can provide the metadata files for the blobs they choose to ship for use with QEMU. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On Tue, Apr 10, 2018 at 01:44:13PM +0200, Laszlo Ersek wrote: > On 04/10/18 13:34, Daniel P. Berrangé wrote: > > On Tue, Apr 10, 2018 at 01:27:18PM +0200, Laszlo Ersek wrote: > > > >> Please go through the rest of the emails in this thread, and advise: > >> - if the firmware descriptor schema may perhaps live in the libvirt tree, > >> - accordingly, if the schema could be expressed as an XSD (and firmware > >> packages should provide the descriptor documents as XMLs) > >> - if you agree that the descriptor document can uniquely reference > >> mapping methods implemented in libvirtd by simple enum constants (with > >> necessary parameters provided). > > > > No to all three. This is the responsibility of QEMU to define, because > > this information is relevant to anything managing QEMU not just libvirt. > > In that case, how do you suggest we describe the QEMU command line > options that are (a) necessary, (b) "discoverable" to the management > application? Should we provide verbatim command line fragments (option > templates)? Is this feature meant to replace the cmdline generation > logic that already exists in libvirtd? Each part of the schema should have docs describing what CLI args it corresponds to. eg document that when device=memory, corresponds to -bios, that device=flash, corresponds to -drive if=pflash, etc We've not trying to replace the cmdline generator in libvirt. We just want to know that when we see a particular field present in the schema, that it corresponds to a particular cli arg. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 10 April 2018 at 12:34, Daniel P. Berrangé wrote: > On Tue, Apr 10, 2018 at 01:27:18PM +0200, Laszlo Ersek wrote: > >> Please go through the rest of the emails in this thread, and advise: >> - if the firmware descriptor schema may perhaps live in the libvirt tree, >> - accordingly, if the schema could be expressed as an XSD (and firmware >> packages should provide the descriptor documents as XMLs) >> - if you agree that the descriptor document can uniquely reference >> mapping methods implemented in libvirtd by simple enum constants (with >> necessary parameters provided). > > No to all three. This is the responsibility of QEMU to define, because > this information is relevant to anything managing QEMU not just libvirt. (Please consider this as more of a grenade lobbed into the conversation rather than a carefully thought out proposal...) My inclination is to say that it's not really the responsibility of QEMU to define either -- we provide emulated models of hardware, and it's up to the user or the management layer or the provider of the firmware to specify what guest code they want to run and how it needs to run on that emulated hardware... Where the QEMU upstream itself is providing firmware blobs (in tarballs etc) it's probably our job to specify how they work, but if the firmware is compiled and provided by the distro (as eg happens for Arm UEFI blobs at the moment) then I don't see how upstream QEMU can reliably define how that firmware needs to be loaded. thanks -- PMM -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 12:09, Paolo Bonzini wrote: > On 10/04/2018 11:23, Daniel P. Berrangé wrote: >>> And, really, this seems to reinforce my point that the schema should >>> live in the libvirtd tree, not in the QEMU tree. In that case, perhaps >>> it would be a better fit to work with an XSD, and firmware packages >>> should install XML files? Personally I'm a lot more attracted to >>> XML/XSD; I think the tooling is better too. I just don't see how QEMU is >>> involved. >> >> This is defining a set of metadata that is required to use various firmware >> files in combination with QEMU, along with defining a mapping to QEMU command >> line arguments and/or features. Essentially, while I wish everyone used >> libvirt, libvirt is not the only thing that manages QEMU. This information is >> relevant to anyone managing QEMU, so it doesn't belong in libvirt's realm, >> it is clear QEMU is best placed to declare this information. > > QEMU is best placed to _standardize_ how to provide this information > (and where in the file system to place it), but really it's up to > firmware packages to provide it. > > We can of course define the schema in QAPI terms for ease of validation > (machine-readable specs are nice to have), but really this should just > be a file in docs/interop/. No code is needed in QEMU. OK -- while we're figuring out the schema, I guess I'll keep posting RFCs that change source code / json, but finally we can move it to docs/interop. Thanks! Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 13:34, Daniel P. Berrangé wrote: > On Tue, Apr 10, 2018 at 01:27:18PM +0200, Laszlo Ersek wrote: > >> Please go through the rest of the emails in this thread, and advise: >> - if the firmware descriptor schema may perhaps live in the libvirt tree, >> - accordingly, if the schema could be expressed as an XSD (and firmware >> packages should provide the descriptor documents as XMLs) >> - if you agree that the descriptor document can uniquely reference >> mapping methods implemented in libvirtd by simple enum constants (with >> necessary parameters provided). > > No to all three. This is the responsibility of QEMU to define, because > this information is relevant to anything managing QEMU not just libvirt. In that case, how do you suggest we describe the QEMU command line options that are (a) necessary, (b) "discoverable" to the management application? Should we provide verbatim command line fragments (option templates)? Is this feature meant to replace the cmdline generation logic that already exists in libvirtd? Thanks Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 11:05, Daniel P. Berrangé wrote: > On Mon, Apr 09, 2018 at 06:34:41PM +0200, Laszlo Ersek wrote: >> On 04/09/18 09:26, Thomas Huth wrote: >>> Hi Laszlo, >>> >>> On 07.04.2018 02:01, Laszlo Ersek wrote: Add a schema that describes the properties of virtual machine firmware. Each firmware executable installed on a host system should come with a JSON file that conforms to this schema, and informs the management applications about the firmware's properties. In addition, a configuration directory with symlinks to the JSON files should exist, with the symlinks carefully named to reflect a priority order. Management applications can then search this directory in priority order for the first firmware executable that satisfies their search criteria. The found JSON file provides the management layer with domain configuration bits that are required to run the firmware binary. >>> [...] +## +# @FirmwareDevice: +# +# Defines the device types that a firmware file can be mapped into. +# +# @memory: The firmware file is to be mapped into memory. +# +# @kernel: The firmware file is to be loaded like a Linux kernel. This is +# similar to @memory but may imply additional processing that is +# specific to the target architecture. +# +# @flash: The firmware file is to be mapped into a pflash chip. +# +# Since: 2.13 +## +{ 'enum' : 'FirmwareDevice', + 'data' : [ 'memory', 'kernel', 'flash' ] } >>> >>> This is not fully clear to me... what is this exactly good for? Is >>> this a way to say how the firmware should be loaded, i.e. via >>> "-bios", "-kernel" or "-pflash" parameter? If so, the term "memory" >>> is quite misleading since files that are loaded via -bios can also >>> end up in an emulated ROM chip. >> >> I threw in "-kernel" because, although it also (usually?) means >> "memory", I expected people would want it separate. > > What platform / scenario actually uses -kernel to load firmware. If > you have loaded firmware using -kernel, how do you then load the > actual kernel ? AAVMF has a build called "ArmVirtQemuKernel" where the firmware is loaded with the -kernel switch. commit 8de84d4242215252af9d2afecd45e2419689ee5f Author: Ard Biesheuvel Date: Fri Feb 5 14:57:57 2016 +0100 ArmVirtPkg: implement ArmVirtQemuKernel This implements a version of ArmVirtQemu that does not execute in place from emulated NOR flash, but implements the Linux kernel boot protocol, and executes from DRAM instead. This allows UEFI to be loaded as a payload by a previous bootloader stage such as ARM Trusted Firmware/OP-TEE. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel Acked-by: Laszlo Ersek My understanding is that in this scenario you cannot use -kernel for loading a Linux kernel; instead you have to boot the Linux OS off of some other media (CD-ROM, disk, network...) Personally I never use this AAVMF build, but I know it exists and Ard uses it at least occasionally. Thanks, Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [qemu RFC] qapi: add "firmware.json"
Hi, > It occurs to me that we are actually over-thinking things, by making it > possible to list a choice of vars files per firmware. We could remove this > special case by just having separate tpo level firmware entries and a main > feature flag to say if it is enrolled or not - see below example That would also make it easier to implement something like ... qemu -firmware json=/path/to/firmware/spec.json ... because you simply have two files for the enrolled/non-enrolled variants. cheers, Gerd -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On Tue, Apr 10, 2018 at 01:27:18PM +0200, Laszlo Ersek wrote: > Please go through the rest of the emails in this thread, and advise: > - if the firmware descriptor schema may perhaps live in the libvirt tree, > - accordingly, if the schema could be expressed as an XSD (and firmware > packages should provide the descriptor documents as XMLs) > - if you agree that the descriptor document can uniquely reference > mapping methods implemented in libvirtd by simple enum constants (with > necessary parameters provided). No to all three. This is the responsibility of QEMU to define, because this information is relevant to anything managing QEMU not just libvirt. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 11:18, Daniel P. Berrangé wrote: > On Mon, Apr 09, 2018 at 07:57:54PM +0200, Laszlo Ersek wrote: >> On 04/09/18 10:49, Daniel P. Berrangé wrote: >>> On Sat, Apr 07, 2018 at 02:01:17AM +0200, Laszlo Ersek wrote: Add a schema that describes the properties of virtual machine firmware. Each firmware executable installed on a host system should come with a JSON file that conforms to this schema, and informs the management applications about the firmware's properties. In addition, a configuration directory with symlinks to the JSON files should exist, with the symlinks carefully named to reflect a priority order. Management applications can then search this directory in priority order for the first firmware executable that satisfies their search criteria. The found JSON file provides the management layer with domain configuration bits that are required to run the firmware binary. Cc: "Daniel P. Berrange" Cc: Alexander Graf Cc: Ard Biesheuvel Cc: David Gibson Cc: Eric Blake Cc: Gary Ching-Pang Lin Cc: Gerd Hoffmann Cc: Kashyap Chamarthy Cc: Markus Armbruster Cc: Michael Roth Cc: Michal Privoznik Cc: Peter Krempa Cc: Peter Maydell Cc: Thomas Huth Signed-off-by: Laszlo Ersek --- Notes: Folks on the CC list, please try to see if the suggested schema is flexible enough to describe the virtual firmware(s) that you are familiar with. Thanks! Makefile | 9 ++ Makefile.objs | 4 + qapi/firmware.json| 343 ++ qapi/qapi-schema.json | 1 + qmp.c | 5 + .gitignore| 4 + 6 files changed, 366 insertions(+) create mode 100644 qapi/firmware.json >>> diff --git a/qapi/firmware.json b/qapi/firmware.json new file mode 100644 index ..f267240f44dd --- /dev/null +++ b/qapi/firmware.json @@ -0,0 +1,343 @@ +# -*- Mode: Python -*- + +## +# = Firmware +## + +## +# @FirmwareDevice: +# +# Defines the device types that a firmware file can be mapped into. +# +# @memory: The firmware file is to be mapped into memory. +# +# @kernel: The firmware file is to be loaded like a Linux kernel. This is +# similar to @memory but may imply additional processing that is +# specific to the target architecture. +# +# @flash: The firmware file is to be mapped into a pflash chip. +# +# Since: 2.13 +## +{ 'enum' : 'FirmwareDevice', + 'data' : [ 'memory', 'kernel', 'flash' ] } + +## +# @FirmwareAccess: +# +# Defines the possible permissions for a given access mode to a device that +# maps a firmware file. +# +# @denied: The access is denied. +# +# @permitted: The access is permitted. +# +# @restricted-to-secure-context: The access is permitted for guest code that +#runs in a secure context; otherwise the access +#is denied. The definition of "secure context" +#is specific to the target architecture. +# +# Since: 2.13 +## +{ 'enum' : 'FirmwareAccess', + 'data' : [ 'denied', 'permitted', 'restricted-to-secure-context' ] } >>> >>> I'm not really understanding the purpose of this - what does it map to >>> on the command line ? >> >> That's difficult to answer generally, because -bios and -kernel have >> different meanings per board type. So I didn't aim at command line >> switches here; instead I tried to capture where and how the firmware >> wants to "end up" in the virtual hardware. How that maps to a particular >> board is a separate question. > > I tend to think that defining a mapping to command line arguments is a key > feature that this should cover. Even if there variations across boards, QEMU > still has a small finite set of approaches to configure firmware, so it does > not feel unreasonable to define what they are and how they map to thes > firmware > files. I agree, now that I've read about Gerd's similar argument. There I made the suggestion that the schema could define enum constants (mapping identifiers) that directly refer to libvirtd's existing logic to map various firmware types. > Your FirmwareDevice enum above with "memory", "kernel" and "flash" has > pretty much suggested the -bios, -kernel or -drive if=flash args anway > >> So, the schema intends to describe the mapping that the firmware expects >> from the board. How that is implemented on the QEMU command line is left >> as an exercise to ... libvirtd. :) > > I think this is pretty unhelpful. Essentially that is saying here
Re: [libvirt] [Qemu-devel] [qemu RFC] qapi: add "firmware.json"
On Tue, Apr 10, 2018 at 11:20:33AM +0100, Daniel P. Berrangé wrote: > On Sat, Apr 07, 2018 at 02:01:17AM +0200, Laszlo Ersek wrote: > > Add a schema that describes the properties of virtual machine firmware. > > > > Each firmware executable installed on a host system should come with a > > JSON file that conforms to this schema, and informs the management > > applications about the firmware's properties. > > > > In addition, a configuration directory with symlinks to the JSON files > > should exist, with the symlinks carefully named to reflect a priority > > order. Management applications can then search this directory in priority > > order for the first firmware executable that satisfies their search > > criteria. The found JSON file provides the management layer with domain > > configuration bits that are required to run the firmware binary. > > > > > diff --git a/qapi/firmware.json b/qapi/firmware.json > > new file mode 100644 > > index ..f267240f44dd > > --- /dev/null > > +++ b/qapi/firmware.json > > [snip] > > > +{ 'struct' : 'SystemFirmware', > > + 'data' : { 'executable' : 'FirmwareFile', > > + 'type' : 'SystemFirmwareType', > > + 'targets': [ 'str' ], > > + 'sysfw-map' : 'FirmwareMapping', > > + '*nvram-slots' : [ 'NVRAMSlot' ], > > + '*supports-uefi-secure-boot' : 'bool', > > + '*supports-amd-sev' : 'bool', > > + '*supports-acpi-s3' : 'bool', > > + '*supports-acpi-s4' : 'bool' } } > > Elsewhere in the thread I mentioned that I think we should try to use a > union approach to isolate which information is relevant to "flash" loader > format and which is relevant to "memory" and "kernel". To try to illustrate > what I mean by that I've knocked up an alternative structure. I also > incorporated the points about features & target/machine types. I've left > out the read/write/etc fields, but they could be put back in at the > relevant position > > > { 'enum' : 'SystemFirmwareType', > 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] } > > { 'enum' : 'SystemFirmwareDevice', > 'data' : [ 'memory', 'kernel', 'flash' ] } > > { 'enum' : 'SystemFirmwareArchitecture', > 'data': ['x86_64', 'i386', ..etc.. ] } > > { 'enum' : 'SystemFirmwareFeature', > 'data': ['acpi-s3', 'acpi-s5', 'secure-boot', 'amd-sev' ]} > > > ## Struct(s) for device==memory > > { 'struct': 'SystemFirmwareBinaryMemory', > 'data': { 'pathname': 'str' } } > > > ## Struct(s) for device==kernel > > { 'struct': 'SystemFirmwareBinaryKernel', > 'data': { 'pathname': 'str' } } > > > ## Struct(s) for device==flash > > { 'struct': 'SystemFirmwareBinaryFlashFile', > 'data': { 'filename': 'str', > 'format': 'BlockdevDriver' } } > > { 'struct': 'SystemFirmwareBinaryFlashCode', > 'base': 'SystemFirmwareBinaryFlashFile' } > > { 'struct': 'SystemFirmwareBinaryFlashVars', > 'base': 'SystemFirmwareBinaryFlashFile', > 'data': { 'secure-boot-key-enroll': 'bool' } } > > { 'struct': 'SystemFirmwareBinaryFlash', > 'data': { 'code': 'SystemFirmwareBinaryFlashCode', > 'vars': ['SystemFirmwareBinaryFlashVars' ] } } > > > ## Discriminated struct for different loading approaches > > { 'union': 'SystemFirmwareBinary', > 'base': { 'device': 'SystemFirmwareDevice' }, > 'discriminator': 'device', > 'data': { 'memory': 'SystemFirmwareBinaryMemory', > 'kernel': 'SystemFirmwareBinaryKernel', > 'flash': 'SystemFirmwareBinaryFlash' } } > > > > { 'struct' : 'SystemFirmwareTarget', > 'data': { 'architecture': 'SystemFirmwareArchitecture', > 'machines': [ 'str' ] } } > > > { 'struct' : 'SystemFirmware', > 'data' : { > 'description' : 'str', > 'type' : 'SystemFirmwareType', > 'binary' : 'SystemFirmwareBinary', > 'targets' : [ 'SystemFirmwareTarget' ], > 'features' : ['SystemFirmwareFeature'] } } > > > > # Examples: > # > # { > #'description': 'SeaBIOS 256k', > #'type': 'bios', > #'binary': { > #'type': 'memory', > #'filename': '/path/to/seabios/rom-256k', > #} > #'targets': { > #'x86_64': [ "pc", "q35"], > #'i386': [ "pc", "q35"], > #} > #'features': ['acpi-s3', 'acpi-s5'], > # } > # { > #'description': 'SeaBIOS 128k', > #'type': 'bios', > #'binary': { > #'type': 'memory', > #'filename': '/path/to/seabios/rom-128k', > #} > #'targets': { > #'x86_64': [ "isapc"], > #'i386': [ "isapc"], > #} > #'features': [], > # } > # { > #'description': 'OVMF', > #'type': 'uefi' > #'binary': { > #'type': 'flash', > #'code': { > # 'filename': '/usr/share/OVMF/OVMF_CODE.secboot.fd', > # 'format': 'raw', > #
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On Sat, Apr 07, 2018 at 02:01:17AM +0200, Laszlo Ersek wrote: > Add a schema that describes the properties of virtual machine firmware. > > Each firmware executable installed on a host system should come with a > JSON file that conforms to this schema, and informs the management > applications about the firmware's properties. > > In addition, a configuration directory with symlinks to the JSON files > should exist, with the symlinks carefully named to reflect a priority > order. Management applications can then search this directory in priority > order for the first firmware executable that satisfies their search > criteria. The found JSON file provides the management layer with domain > configuration bits that are required to run the firmware binary. > > diff --git a/qapi/firmware.json b/qapi/firmware.json > new file mode 100644 > index ..f267240f44dd > --- /dev/null > +++ b/qapi/firmware.json [snip] > +{ 'struct' : 'SystemFirmware', > + 'data' : { 'executable' : 'FirmwareFile', > + 'type' : 'SystemFirmwareType', > + 'targets': [ 'str' ], > + 'sysfw-map' : 'FirmwareMapping', > + '*nvram-slots' : [ 'NVRAMSlot' ], > + '*supports-uefi-secure-boot' : 'bool', > + '*supports-amd-sev' : 'bool', > + '*supports-acpi-s3' : 'bool', > + '*supports-acpi-s4' : 'bool' } } Elsewhere in the thread I mentioned that I think we should try to use a union approach to isolate which information is relevant to "flash" loader format and which is relevant to "memory" and "kernel". To try to illustrate what I mean by that I've knocked up an alternative structure. I also incorporated the points about features & target/machine types. I've left out the read/write/etc fields, but they could be put back in at the relevant position { 'enum' : 'SystemFirmwareType', 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] } { 'enum' : 'SystemFirmwareDevice', 'data' : [ 'memory', 'kernel', 'flash' ] } { 'enum' : 'SystemFirmwareArchitecture', 'data': ['x86_64', 'i386', ..etc.. ] } { 'enum' : 'SystemFirmwareFeature', 'data': ['acpi-s3', 'acpi-s5', 'secure-boot', 'amd-sev' ]} ## Struct(s) for device==memory { 'struct': 'SystemFirmwareBinaryMemory', 'data': { 'pathname': 'str' } } ## Struct(s) for device==kernel { 'struct': 'SystemFirmwareBinaryKernel', 'data': { 'pathname': 'str' } } ## Struct(s) for device==flash { 'struct': 'SystemFirmwareBinaryFlashFile', 'data': { 'filename': 'str', 'format': 'BlockdevDriver' } } { 'struct': 'SystemFirmwareBinaryFlashCode', 'base': 'SystemFirmwareBinaryFlashFile' } { 'struct': 'SystemFirmwareBinaryFlashVars', 'base': 'SystemFirmwareBinaryFlashFile', 'data': { 'secure-boot-key-enroll': 'bool' } } { 'struct': 'SystemFirmwareBinaryFlash', 'data': { 'code': 'SystemFirmwareBinaryFlashCode', 'vars': ['SystemFirmwareBinaryFlashVars' ] } } ## Discriminated struct for different loading approaches { 'union': 'SystemFirmwareBinary', 'base': { 'device': 'SystemFirmwareDevice' }, 'discriminator': 'device', 'data': { 'memory': 'SystemFirmwareBinaryMemory', 'kernel': 'SystemFirmwareBinaryKernel', 'flash': 'SystemFirmwareBinaryFlash' } } { 'struct' : 'SystemFirmwareTarget', 'data': { 'architecture': 'SystemFirmwareArchitecture', 'machines': [ 'str' ] } } { 'struct' : 'SystemFirmware', 'data' : { 'description' : 'str', 'type' : 'SystemFirmwareType', 'binary' : 'SystemFirmwareBinary', 'targets' : [ 'SystemFirmwareTarget' ], 'features' : ['SystemFirmwareFeature'] } } # Examples: # # { #'description': 'SeaBIOS 256k', #'type': 'bios', #'binary': { #'type': 'memory', #'filename': '/path/to/seabios/rom-256k', #} #'targets': { #'x86_64': [ "pc", "q35"], #'i386': [ "pc", "q35"], #} #'features': ['acpi-s3', 'acpi-s5'], # } # { #'description': 'SeaBIOS 128k', #'type': 'bios', #'binary': { #'type': 'memory', #'filename': '/path/to/seabios/rom-128k', #} #'targets': { #'x86_64': [ "isapc"], #'i386': [ "isapc"], #} #'features': [], # } # { #'description': 'OVMF', #'type': 'uefi' #'binary': { #'type': 'flash', #'code': { # 'filename': '/usr/share/OVMF/OVMF_CODE.secboot.fd', # 'format': 'raw', #}, #'vars': [ # { # 'filename': '/usr/share/OVMF/OVMF_VARS.fd', # 'format': 'raw', # 'secure=boot-key-enroll': false, # }, # { # 'filename': '/usr/share/OVMF/OVMF_VARS.secboot.fd', # 'format': 'raw', # 'secure=boot-key-enroll': true, # } #
Re: [libvirt] [Qemu-devel] [qemu RFC] qapi: add "firmware.json"
On 10/04/2018 11:23, Daniel P. Berrangé wrote: >> And, really, this seems to reinforce my point that the schema should >> live in the libvirtd tree, not in the QEMU tree. In that case, perhaps >> it would be a better fit to work with an XSD, and firmware packages >> should install XML files? Personally I'm a lot more attracted to >> XML/XSD; I think the tooling is better too. I just don't see how QEMU is >> involved. > > This is defining a set of metadata that is required to use various firmware > files in combination with QEMU, along with defining a mapping to QEMU command > line arguments and/or features. Essentially, while I wish everyone used > libvirt, libvirt is not the only thing that manages QEMU. This information is > relevant to anyone managing QEMU, so it doesn't belong in libvirt's realm, > it is clear QEMU is best placed to declare this information. QEMU is best placed to _standardize_ how to provide this information (and where in the file system to place it), but really it's up to firmware packages to provide it. We can of course define the schema in QAPI terms for ease of validation (machine-readable specs are nice to have), but really this should just be a file in docs/interop/. No code is needed in QEMU. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On Tue, Apr 10, 2018 at 11:51:31AM +0200, Gerd Hoffmann wrote: > > > Hmm, I'm wondering whenever it is useful to model things this way. It's > > > not like you can actually configure things for -bios seabios.rom or > > > -kernel uboot.elf. Only pflash allows to actually configure things, and > > > there are not that many useful combinations. The code needs > > > Read+Execute. Allowing Write could be useful in theory, to allow the > > > guest doing firmware updates. But I think nobody actually does that, so > > > in practice it is fixed. The varstore can have different permissions, > > > but it's only two useful combinations. Either allow access > > > unconditionally, or allow access in secure contect (aka smm) only. > > > > (I hope I understand your point right:) > > > > I'm also fine if we simply define a fixed (but extensible) set of > > mapping methods, basically a new enum type, that simply tells libvirtd > > what this firmware *is*. IOW, directly reference a mapping method we > > know libvirt implements, rather than give vague hints. > > > > This could repurpose SystemFirmwareType, but it should become more > > detailed. I'm thinking like: > > - ovmf: split files without requiring SMM > > - ovmf_smm: split files with SMM requirement > > - seabios: exactly that > > - ... other things others suggest. > > I wouldn't name them by firmware, that is misleading. Basically we have > three cases: > > (1) single firmware image (seabios, OVMF.fd, ...). > (2) split firmware image (OVMF_{CODE,VARS}.fd), where vars can be > writable unconditinally. > (3) split firmware image, where access to vars should be restricted > to smm mode. > > (2) + (3) requires pflash. (1) works with both pflash and -bios. A big chunk of the data in the schema looks specific to the pflash case, but this is not expressed except in the docs. Most of the time with QAPI when we have data that is only relevant in certain types, we use a discriminated union to describe it. It feels like a unioon approach could be better suited to this Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
> > Hmm, I'm wondering whenever it is useful to model things this way. It's > > not like you can actually configure things for -bios seabios.rom or > > -kernel uboot.elf. Only pflash allows to actually configure things, and > > there are not that many useful combinations. The code needs > > Read+Execute. Allowing Write could be useful in theory, to allow the > > guest doing firmware updates. But I think nobody actually does that, so > > in practice it is fixed. The varstore can have different permissions, > > but it's only two useful combinations. Either allow access > > unconditionally, or allow access in secure contect (aka smm) only. > > (I hope I understand your point right:) > > I'm also fine if we simply define a fixed (but extensible) set of > mapping methods, basically a new enum type, that simply tells libvirtd > what this firmware *is*. IOW, directly reference a mapping method we > know libvirt implements, rather than give vague hints. > > This could repurpose SystemFirmwareType, but it should become more > detailed. I'm thinking like: > - ovmf: split files without requiring SMM > - ovmf_smm: split files with SMM requirement > - seabios: exactly that > - ... other things others suggest. I wouldn't name them by firmware, that is misleading. Basically we have three cases: (1) single firmware image (seabios, OVMF.fd, ...). (2) split firmware image (OVMF_{CODE,VARS}.fd), where vars can be writable unconditinally. (3) split firmware image, where access to vars should be restricted to smm mode. (2) + (3) requires pflash. (1) works with both pflash and -bios. There also is (4) elf binary loadable with -kernel. Not sure we should include that case. u-boot can be loaded that way. The elf binary seems to be more a side product of the build proccess, I always have both u-boot (elf binary) and u-boot.bin (binary blob loadable with -bios). So maybe we should put aside -kernel for now, and maybe reconsider once a real need for it shows up. So maybe Firmware{Device,Access,Mapping} should be replaced with a FirmwareImageType [ 'single', 'code+vars', 'code+protectedvars' ] ? cheers, Gerd -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On Tue, Apr 10, 2018 at 11:16:01AM +0200, Laszlo Ersek wrote: > On 04/10/18 08:27, Gerd Hoffmann wrote: > > Hi, > > > >> - I considered adding wildcards (say, blacklist "all" i440fx machtypes, > >> present and future, for SMM-requiring OVMF builds), but then you get > >> into version sorting and similar mess. I considered fnmatch() -- > >> basically simple ? and * wildcards -- but that's not expressive enough. > > > > I'd suggest whitelist with wildcards. So the smm builds would get > > "pc-q35-*". > > > > libvirt knows about aliases, so it should be able to handle the "q35" > > shortcut like "pc-q35-${latest}". > > > > Or do you see another issue? > > Well, one issue I see is version sorting; I should say "Q35 but no > earlier than 2.4", and lexicographically, "2.11" sorts before "2.4". > > Anyway (also asking for Thomas's input here): if we run with your idea > to refer to exact mapping methods / firmware *implementation* types that > we know libvirt implements / supports as a "white box", do we still deem > machine type identification necessary? Because, libvirt already knows > (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just > have to make a *reference* to that knowledge in the JSON file. BTW, that's not quite correct - when libvirt handles the "smm" arg it checks if machine type == q35, and QEMU version >= 2.4. It is *not* checking the version of the machine type. ie it will happily use smm with pc-q35-2.0, as long as QEMU version is 2.4. Perhaps this is not quite right, but we don't try to parse the version number out of the machine type, because we can't assume a specific format for the machine type version part. eg version can be "2.4", or it can be "rhel-7.0.0" or something else again on Ubuntu. IMHO it would be valid to just keep life simple and only record the base machine type name that can use the firmware ie "pc", "q35", and ignore the fact that in some cases the firmware might require a specific version of the machine type. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 10.04.2018 11:22, Laszlo Ersek wrote: > On 04/10/18 09:33, Thomas Huth wrote: [...] >> Alternatively, what about providing some kind of "alias" or "nickname" >> setting here, too? So the EDK2 builds would get >> SystemFirmwareType="edk2" and "SystemFirmwareAlias="uefi" for example. > > I hope I understand you right -- I think your suggestion ties in with my > other email I just sent in this thread. So, we could tell libvirtd, > "this firmware is of type 'UEFI', and you must use the 'ovmf_smm' > mapping method to run it, with this file or that file as varstore template". > > We could even describe the parameters for this or that mapping method > structurally in the schema (in a discriminated union in QAPI JSON, or in > an XSD choice element). For example, "ovmf" and "ovmf_smm" would both > take "OvmfSplitFileOptions" -- a list of single varstore template files > with feature enum contants attached --, while "SeaBiosOptions" would be > an empty structure. Sorry, I've got no clue about ovmf_smm and the other things you've mentioned here ;-) > I feel the key question here is whether we are allowed to directly > reference a mapping method we know libvirt implements. If we are, that > makes things a lot clearer (and easier, I should hope). Key question is maybe rather: Do you want to design / implement something that is libvirt-only here, or rather something generic that could also be used for other upper layer tools that do not use libvirt? (... and looks like Daniel just had the same comment in another mail in this thread ...) Thomas -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 10.04.2018 11:16, Laszlo Ersek wrote: > On 04/10/18 08:27, Gerd Hoffmann wrote: >> Hi, >> >>> - I considered adding wildcards (say, blacklist "all" i440fx machtypes, >>> present and future, for SMM-requiring OVMF builds), but then you get >>> into version sorting and similar mess. I considered fnmatch() -- >>> basically simple ? and * wildcards -- but that's not expressive enough. >> >> I'd suggest whitelist with wildcards. So the smm builds would get >> "pc-q35-*". >> >> libvirt knows about aliases, so it should be able to handle the "q35" >> shortcut like "pc-q35-${latest}". >> >> Or do you see another issue? > > Well, one issue I see is version sorting; I should say "Q35 but no > earlier than 2.4", and lexicographically, "2.11" sorts before "2.4". > > Anyway (also asking for Thomas's input here): if we run with your idea > to refer to exact mapping methods / firmware *implementation* types that > we know libvirt implements / supports as a "white box", do we still deem > machine type identification necessary? Because, libvirt already knows > (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just > have to make a *reference* to that knowledge in the JSON file. I think you really need a way to specify the machine there. Latest example from QEMU 2.12: We've now got two uboot binaries in the tree, pc-bios/u-boot.e500 and pc-bios/u-boot-sam460-20100605.bin. Both are uboot, both are for ppc, but u-boot.e500 only works with the "ppce500" machine and the other one only works with the "sam460ex" machine. How would you teach libvirt such a relationship without an explicit machine type identification field there? Thomas -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On Tue, Apr 10, 2018 at 11:16:01AM +0200, Laszlo Ersek wrote: > On 04/10/18 08:27, Gerd Hoffmann wrote: > > Hi, > > > >> - I considered adding wildcards (say, blacklist "all" i440fx machtypes, > >> present and future, for SMM-requiring OVMF builds), but then you get > >> into version sorting and similar mess. I considered fnmatch() -- > >> basically simple ? and * wildcards -- but that's not expressive enough. > > > > I'd suggest whitelist with wildcards. So the smm builds would get > > "pc-q35-*". > > > > libvirt knows about aliases, so it should be able to handle the "q35" > > shortcut like "pc-q35-${latest}". > > > > Or do you see another issue? > > Well, one issue I see is version sorting; I should say "Q35 but no > earlier than 2.4", and lexicographically, "2.11" sorts before "2.4". > > Anyway (also asking for Thomas's input here): if we run with your idea > to refer to exact mapping methods / firmware *implementation* types that > we know libvirt implements / supports as a "white box", do we still deem > machine type identification necessary? Because, libvirt already knows > (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just > have to make a *reference* to that knowledge in the JSON file. > > And, really, this seems to reinforce my point that the schema should > live in the libvirtd tree, not in the QEMU tree. In that case, perhaps > it would be a better fit to work with an XSD, and firmware packages > should install XML files? Personally I'm a lot more attracted to > XML/XSD; I think the tooling is better too. I just don't see how QEMU is > involved. This is defining a set of metadata that is required to use various firmware files in combination with QEMU, along with defining a mapping to QEMU command line arguments and/or features. Essentially, while I wish everyone used libvirt, libvirt is not the only thing that manages QEMU. This information is relevant to anyone managing QEMU, so it doesn't belong in libvirt's realm, it is clear QEMU is best placed to declare this information. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 09:33, Thomas Huth wrote: > On 09.04.2018 18:50, Laszlo Ersek wrote: >> On 04/09/18 10:19, Gerd Hoffmann wrote: > +{ 'enum' : 'SystemFirmwareType', > + 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] } The naming here is quite a bad mixture between firmware interface ('bios', 'uefi') and firmware implementations ('slof', 'uboot'). There could be other implementations of BIOS or UEFI than SeaBIOS and EDK2 ... so I'd suggest to rather name them 'seabios' and 'edk2' here instead. >>> >>> uboot for example implements uefi unterfaces too (dunno how complete, >>> but reportly recent versions can run uefi shell and grub just fine). >> >> Indeed: when I was struggling with this enum type and tried to look for >> more firmware types to add, my googling turned up the "UEFI on Top of >> U-Boot" whitepaper, from Alex and Andreas :) >> >> Again, this reaches to the root of the problem: when a user creates a >> new domain, using high-level tools, they just want to tick "UEFI". (Dan >> has emphasized this to me several times, so I think I get the idea by >> now, if not the full environment.) We cannot ask the user, "please be >> more specific, do you want UEFI from edk2, or UEFI on top of U-Boot?" > > But you are designing a rather low-level interface here, which should > IMHO rather be precise than fuzzy. So should this "just want to tick > UEFI" rather be handled in the high-level tools instead? > > Alternatively, what about providing some kind of "alias" or "nickname" > setting here, too? So the EDK2 builds would get > SystemFirmwareType="edk2" and "SystemFirmwareAlias="uefi" for example. I hope I understand you right -- I think your suggestion ties in with my other email I just sent in this thread. So, we could tell libvirtd, "this firmware is of type 'UEFI', and you must use the 'ovmf_smm' mapping method to run it, with this file or that file as varstore template". We could even describe the parameters for this or that mapping method structurally in the schema (in a discriminated union in QAPI JSON, or in an XSD choice element). For example, "ovmf" and "ovmf_smm" would both take "OvmfSplitFileOptions" -- a list of single varstore template files with feature enum contants attached --, while "SeaBiosOptions" would be an empty structure. I feel the key question here is whether we are allowed to directly reference a mapping method we know libvirt implements. If we are, that makes things a lot clearer (and easier, I should hope). Thanks Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 10.04.2018 11:05, Daniel P. Berrangé wrote: > On Mon, Apr 09, 2018 at 06:34:41PM +0200, Laszlo Ersek wrote: >> On 04/09/18 09:26, Thomas Huth wrote: >>> Hi Laszlo, >>> >>> On 07.04.2018 02:01, Laszlo Ersek wrote: Add a schema that describes the properties of virtual machine firmware. Each firmware executable installed on a host system should come with a JSON file that conforms to this schema, and informs the management applications about the firmware's properties. In addition, a configuration directory with symlinks to the JSON files should exist, with the symlinks carefully named to reflect a priority order. Management applications can then search this directory in priority order for the first firmware executable that satisfies their search criteria. The found JSON file provides the management layer with domain configuration bits that are required to run the firmware binary. >>> [...] +## +# @FirmwareDevice: +# +# Defines the device types that a firmware file can be mapped into. +# +# @memory: The firmware file is to be mapped into memory. +# +# @kernel: The firmware file is to be loaded like a Linux kernel. This is +# similar to @memory but may imply additional processing that is +# specific to the target architecture. +# +# @flash: The firmware file is to be mapped into a pflash chip. +# +# Since: 2.13 +## +{ 'enum' : 'FirmwareDevice', + 'data' : [ 'memory', 'kernel', 'flash' ] } >>> >>> This is not fully clear to me... what is this exactly good for? Is this >>> a way to say how the firmware should be loaded, i.e. via "-bios", >>> "-kernel" or "-pflash" parameter? If so, the term "memory" is quite >>> misleading since files that are loaded via -bios can also end up in an >>> emulated ROM chip. >> >> I threw in "-kernel" because, although it also (usually?) means >> "memory", I expected people would want it separate. > > What platform / scenario actually uses -kernel to load firmware. I think uboot uses -kernel in certain cases, see e.g.: https://balau82.wordpress.com/2010/03/10/u-boot-for-arm-on-qemu/ > If you > have loaded firmware using -kernel, how do you then load the actual > kernel ? The kernel is then loaded from disk or network or another boot device. Thomas -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On Mon, Apr 09, 2018 at 07:57:54PM +0200, Laszlo Ersek wrote: > On 04/09/18 10:49, Daniel P. Berrangé wrote: > > On Sat, Apr 07, 2018 at 02:01:17AM +0200, Laszlo Ersek wrote: > >> Add a schema that describes the properties of virtual machine firmware. > >> > >> Each firmware executable installed on a host system should come with a > >> JSON file that conforms to this schema, and informs the management > >> applications about the firmware's properties. > >> > >> In addition, a configuration directory with symlinks to the JSON files > >> should exist, with the symlinks carefully named to reflect a priority > >> order. Management applications can then search this directory in priority > >> order for the first firmware executable that satisfies their search > >> criteria. The found JSON file provides the management layer with domain > >> configuration bits that are required to run the firmware binary. > >> > >> Cc: "Daniel P. Berrange" > >> Cc: Alexander Graf > >> Cc: Ard Biesheuvel > >> Cc: David Gibson > >> Cc: Eric Blake > >> Cc: Gary Ching-Pang Lin > >> Cc: Gerd Hoffmann > >> Cc: Kashyap Chamarthy > >> Cc: Markus Armbruster > >> Cc: Michael Roth > >> Cc: Michal Privoznik > >> Cc: Peter Krempa > >> Cc: Peter Maydell > >> Cc: Thomas Huth > >> Signed-off-by: Laszlo Ersek > >> --- > >> > >> Notes: > >> Folks on the CC list, please try to see if the suggested schema is > >> flexible enough to describe the virtual firmware(s) that you are > >> familiar with. Thanks! > >> > >> Makefile | 9 ++ > >> Makefile.objs | 4 + > >> qapi/firmware.json| 343 > >> ++ > >> qapi/qapi-schema.json | 1 + > >> qmp.c | 5 + > >> .gitignore| 4 + > >> 6 files changed, 366 insertions(+) > >> create mode 100644 qapi/firmware.json > >> > > > >> diff --git a/qapi/firmware.json b/qapi/firmware.json > >> new file mode 100644 > >> index ..f267240f44dd > >> --- /dev/null > >> +++ b/qapi/firmware.json > >> @@ -0,0 +1,343 @@ > >> +# -*- Mode: Python -*- > >> + > >> +## > >> +# = Firmware > >> +## > >> + > >> +## > >> +# @FirmwareDevice: > >> +# > >> +# Defines the device types that a firmware file can be mapped into. > >> +# > >> +# @memory: The firmware file is to be mapped into memory. > >> +# > >> +# @kernel: The firmware file is to be loaded like a Linux kernel. This is > >> +# similar to @memory but may imply additional processing that is > >> +# specific to the target architecture. > >> +# > >> +# @flash: The firmware file is to be mapped into a pflash chip. > >> +# > >> +# Since: 2.13 > >> +## > >> +{ 'enum' : 'FirmwareDevice', > >> + 'data' : [ 'memory', 'kernel', 'flash' ] } > >> + > >> +## > >> +# @FirmwareAccess: > >> +# > >> +# Defines the possible permissions for a given access mode to a device > >> that > >> +# maps a firmware file. > >> +# > >> +# @denied: The access is denied. > >> +# > >> +# @permitted: The access is permitted. > >> +# > >> +# @restricted-to-secure-context: The access is permitted for guest code > >> that > >> +#runs in a secure context; otherwise the > >> access > >> +#is denied. The definition of "secure > >> context" > >> +#is specific to the target architecture. > >> +# > >> +# Since: 2.13 > >> +## > >> +{ 'enum' : 'FirmwareAccess', > >> + 'data' : [ 'denied', 'permitted', 'restricted-to-secure-context' ] } > > > > I'm not really understanding the purpose of this - what does it map to > > on the command line ? > > That's difficult to answer generally, because -bios and -kernel have > different meanings per board type. So I didn't aim at command line > switches here; instead I tried to capture where and how the firmware > wants to "end up" in the virtual hardware. How that maps to a particular > board is a separate question. I tend to think that defining a mapping to command line arguments is a key feature that this should cover. Even if there variations across boards, QEMU still has a small finite set of approaches to configure firmware, so it does not feel unreasonable to define what they are and how they map to thes firmware files. Your FirmwareDevice enum above with "memory", "kernel" and "flash" has pretty much suggested the -bios, -kernel or -drive if=flash args anway > So, the schema intends to describe the mapping that the firmware expects > from the board. How that is implemented on the QEMU command line is left > as an exercise to ... libvirtd. :) I think this is pretty unhelpful. Essentially that is saying here is a big pile of information about firmware, but we're not going to tell you how to use it correctly, everyone must figure it out themselves. > >> +## > >> +# @FirmwareFile: > >> +# > >> +# Gathers the common traits of system firmware executables and NVRAM > >> templates. > >> +# > >> +# @pathname: absolute p
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 08:27, Gerd Hoffmann wrote: > Hi, > >> - I considered adding wildcards (say, blacklist "all" i440fx machtypes, >> present and future, for SMM-requiring OVMF builds), but then you get >> into version sorting and similar mess. I considered fnmatch() -- >> basically simple ? and * wildcards -- but that's not expressive enough. > > I'd suggest whitelist with wildcards. So the smm builds would get > "pc-q35-*". > > libvirt knows about aliases, so it should be able to handle the "q35" > shortcut like "pc-q35-${latest}". > > Or do you see another issue? Well, one issue I see is version sorting; I should say "Q35 but no earlier than 2.4", and lexicographically, "2.11" sorts before "2.4". Anyway (also asking for Thomas's input here): if we run with your idea to refer to exact mapping methods / firmware *implementation* types that we know libvirt implements / supports as a "white box", do we still deem machine type identification necessary? Because, libvirt already knows (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just have to make a *reference* to that knowledge in the JSON file. And, really, this seems to reinforce my point that the schema should live in the libvirtd tree, not in the QEMU tree. In that case, perhaps it would be a better fit to work with an XSD, and firmware packages should install XML files? Personally I'm a lot more attracted to XML/XSD; I think the tooling is better too. I just don't see how QEMU is involved. Opinions please :) Thanks! Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 08:18, Gerd Hoffmann wrote: > Hi, > >>> uboot for example implements uefi unterfaces too (dunno how complete, >>> but reportly recent versions can run uefi shell and grub just fine). >> >> Indeed: when I was struggling with this enum type and tried to look for >> more firmware types to add, my googling turned up the "UEFI on Top of >> U-Boot" whitepaper, from Alex and Andreas :) > > In case you wanna play: uboot supports x86 qemu meanwhile, so you can > try install u-boot.git-x86 from my firmware repo, then run > "qemu-system-x86_64 -bios /usr/share/u-boot.git/x86/qemu-pc/u-boot.rom". > > It certainly isn't a useful edk2 replacement atm. It has no virtio > drivers. And even when using ide storage its not like it would happily > boot a fedora live iso. So I certainly wouldn't tag that as uefi today. > That might change at some point in the future though. > >> Again, this reaches to the root of the problem: when a user creates a >> new domain, using high-level tools, they just want to tick "UEFI". (Dan >> has emphasized this to me several times, so I think I get the idea by >> now, if not the full environment.) We cannot ask the user, "please be >> more specific, do you want UEFI from edk2, or UEFI on top of U-Boot?" > > Well, in case the uefi support in u-boot is good enough some day then it > doesn't matter to the user whenever uboot or edk2 boots the efi guest > from disk/iso, right? I believe that's correct. Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On Mon, Apr 09, 2018 at 06:50:12PM +0200, Laszlo Ersek wrote: > On 04/09/18 10:19, Gerd Hoffmann wrote: > >>> +{ 'enum' : 'SystemFirmwareType', > >>> + 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] } > >> > >> The naming here is quite a bad mixture between firmware interface > >> ('bios', 'uefi') and firmware implementations ('slof', 'uboot'). There > >> could be other implementations of BIOS or UEFI than SeaBIOS and EDK2 ... > >> so I'd suggest to rather name them 'seabios' and 'edk2' here instead. > > > > uboot for example implements uefi unterfaces too (dunno how complete, > > but reportly recent versions can run uefi shell and grub just fine). > > Indeed: when I was struggling with this enum type and tried to look for > more firmware types to add, my googling turned up the "UEFI on Top of > U-Boot" whitepaper, from Alex and Andreas :) > > Again, this reaches to the root of the problem: when a user creates a > new domain, using high-level tools, they just want to tick "UEFI". (Dan > has emphasized this to me several times, so I think I get the idea by > now, if not the full environment.) We cannot ask the user, "please be > more specific, do you want UEFI from edk2, or UEFI on top of U-Boot?" > > Instead, each of those firmware images will have to come with a JSON > document that states "uefi" in SystemFirmware.type, and the host admin > will be responsible for establishing a priority order between them. > Then, when the user asks for "UEFI" (and no more details), they'll get > (compatibly with the target architecture) whichever firmware the host > admin marked as "higher priority". Yep, I don't think there's any problem here. If they have asked for "uefi", they'll get whichever UEFI implementation is the default for that host. Today it'll be an EDK2 impl, but if there's a uboot impl of UEFI available instead, that's fine too. If both are available we'll have some deterministic manner in which we pick one, even if it as simple as which has alphabetically first filename. This is really only about getting good default choices. If the user really badly wants to have a specific firmware, they can still provide a path to it themselves instead of having libvirt choose it. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 07:59, Gerd Hoffmann wrote: > Hi, > >> I threw in "-kernel" because, although it also (usually?) means >> "memory", I expected people would want it separate. >> >> Regarding memory vs. pflash, I thought that these two, combined with the >> access permissions, could cover all of RAM, ROM, and read-only and >> read-write pflash too. >> >> So, "-bios" (-> ROM) boils down to "memory", with write access denied -- >> please see the SeaBIOS example near the end. > > Hmm, I'm wondering whenever it is useful to model things this way. It's > not like you can actually configure things for -bios seabios.rom or > -kernel uboot.elf. Only pflash allows to actually configure things, and > there are not that many useful combinations. The code needs > Read+Execute. Allowing Write could be useful in theory, to allow the > guest doing firmware updates. But I think nobody actually does that, so > in practice it is fixed. The varstore can have different permissions, > but it's only two useful combinations. Either allow access > unconditionally, or allow access in secure contect (aka smm) only. (I hope I understand your point right:) I'm also fine if we simply define a fixed (but extensible) set of mapping methods, basically a new enum type, that simply tells libvirtd what this firmware *is*. IOW, directly reference a mapping method we know libvirt implements, rather than give vague hints. This could repurpose SystemFirmwareType, but it should become more detailed. I'm thinking like: - ovmf: split files without requiring SMM - ovmf_smm: split files with SMM requirement - seabios: exactly that - ... other things others suggest. So "ovmf" would refer precisely to point (3) in my email <3381bdf1-62ea-9da7-c654-032c0c11fb4e@redhat.com">http://mid.mail-archive.com/3381bdf1-62ea-9da7-c654-032c0c11fb4e@redhat.com>, and "ovmf_smm" would refer to point (4) in that email. Let me post the next version soon with this idea, focusing just on OVMF and maybe SeaBIOS. Then let us see if that RFCv2 format lends itself easily to extensions by Thomas. :) Thanks! Laszlo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On Mon, Apr 09, 2018 at 06:34:41PM +0200, Laszlo Ersek wrote: > On 04/09/18 09:26, Thomas Huth wrote: > > Hi Laszlo, > > > > On 07.04.2018 02:01, Laszlo Ersek wrote: > >> Add a schema that describes the properties of virtual machine firmware. > >> > >> Each firmware executable installed on a host system should come with a > >> JSON file that conforms to this schema, and informs the management > >> applications about the firmware's properties. > >> > >> In addition, a configuration directory with symlinks to the JSON files > >> should exist, with the symlinks carefully named to reflect a priority > >> order. Management applications can then search this directory in priority > >> order for the first firmware executable that satisfies their search > >> criteria. The found JSON file provides the management layer with domain > >> configuration bits that are required to run the firmware binary. > > [...] > >> +## > >> +# @FirmwareDevice: > >> +# > >> +# Defines the device types that a firmware file can be mapped into. > >> +# > >> +# @memory: The firmware file is to be mapped into memory. > >> +# > >> +# @kernel: The firmware file is to be loaded like a Linux kernel. This is > >> +# similar to @memory but may imply additional processing that is > >> +# specific to the target architecture. > >> +# > >> +# @flash: The firmware file is to be mapped into a pflash chip. > >> +# > >> +# Since: 2.13 > >> +## > >> +{ 'enum' : 'FirmwareDevice', > >> + 'data' : [ 'memory', 'kernel', 'flash' ] } > > > > This is not fully clear to me... what is this exactly good for? Is this > > a way to say how the firmware should be loaded, i.e. via "-bios", > > "-kernel" or "-pflash" parameter? If so, the term "memory" is quite > > misleading since files that are loaded via -bios can also end up in an > > emulated ROM chip. > > I threw in "-kernel" because, although it also (usually?) means > "memory", I expected people would want it separate. What platform / scenario actually uses -kernel to load firmware. If you have loaded firmware using -kernel, how do you then load the actual kernel ? Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 04/10/18 09:44, Thomas Huth wrote: > On 09.04.2018 18:34, Laszlo Ersek wrote: >> On 04/09/18 09:26, Thomas Huth wrote: >>> Hi Laszlo, >>> >>> On 07.04.2018 02:01, Laszlo Ersek wrote: Add a schema that describes the properties of virtual machine firmware. Each firmware executable installed on a host system should come with a JSON file that conforms to this schema, and informs the management applications about the firmware's properties. In addition, a configuration directory with symlinks to the JSON files should exist, with the symlinks carefully named to reflect a priority order. Management applications can then search this directory in priority order for the first firmware executable that satisfies their search criteria. The found JSON file provides the management layer with domain configuration bits that are required to run the firmware binary. >>> [...] +## +# @FirmwareDevice: +# +# Defines the device types that a firmware file can be mapped into. +# +# @memory: The firmware file is to be mapped into memory. +# +# @kernel: The firmware file is to be loaded like a Linux kernel. This is +# similar to @memory but may imply additional processing that is +# specific to the target architecture. +# +# @flash: The firmware file is to be mapped into a pflash chip. +# +# Since: 2.13 +## +{ 'enum' : 'FirmwareDevice', + 'data' : [ 'memory', 'kernel', 'flash' ] } >>> >>> This is not fully clear to me... what is this exactly good for? Is this >>> a way to say how the firmware should be loaded, i.e. via "-bios", >>> "-kernel" or "-pflash" parameter? If so, the term "memory" is quite >>> misleading since files that are loaded via -bios can also end up in an >>> emulated ROM chip. >> >> I threw in "-kernel" because, although it also (usually?) means >> "memory", I expected people would want it separate. >> >> Regarding memory vs. pflash, I thought that these two, combined with the >> access permissions, could cover all of RAM, ROM, and read-only and >> read-write pflash too. >> >> So, "-bios" (-> ROM) boils down to "memory", with write access denied -- >> please see the SeaBIOS example near the end. > > Let me ask the other way round: How does a high-level tool know whether > it should use "-bios", "-kernel", "-pflash", "-device generic-loader" or > "-younameit" for loading the firmware? I expect it knows that because its developers investigate all the supported firmware options and write dedicated code for those. My understanding is that this JSON is not supposed to inform the mgmt layer about unknown firmware, but to expose enough information for the mgmt layer to pick a firmware and to compose a known & supported cmdline config for it. + 'nvram-map' : 'FirmwareMapping', + 'templates' : [ 'FirmwareFile' ] } } + +## +# @SystemFirmwareType: +# +# Lists system firmware types commonly used with QEMU virtual machines. +# +# @bios: The system firmware was built from the SeaBIOS project. +# +# @slof: The system firmware was built from the Slimline Open Firmware project. +# +# @uboot: The system firmware was built from the U-Boot project. +# +# @uefi: The system firmware was built from the edk2 (EFI Development Kit II) +#project. +# +# Since: 2.13 +## +{ 'enum' : 'SystemFirmwareType', + 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] } >>> >>> The naming here is quite a bad mixture between firmware interface >>> ('bios', 'uefi') and firmware implementations ('slof', 'uboot'). There >>> could be other implementations of BIOS or UEFI than SeaBIOS and EDK2 ... >>> so I'd suggest to rather name them 'seabios' and 'edk2' here instead. >> >> Sure, I'm totally ready to follow community advice here (too). >> >> In fact this is the one element I dislike the most about the schema -- >> it's the fuzziest part, yet it is the most important element for >> libvirt. Because users and higher level apps just want to say "give me >> UEFI". If I have to ask "OK, but UEFI built from edk2 or something >> else?", then it's a lost cause already. >> >> It's hard to find the right level of abstraction in the naming when the >> higher level tools (and/or ultimately the users) don't know enough to >> ask for specifics -- I'm not saying that's bad; it's quite natural, but >> makes things very difficult. So this enum aims to match the user story >> "gimme UEFI and be done with it". I figure users will just utter the >> most common buzzword form of the concept they have in mind. "edk2" >> doesn't tell them as much as "uefi". > > OK, I see your point. But I still think we should not design fuzzy > interfaces here at this low level, this will only lead to other trouble > later. ... thinking about this again, users seem to mix up firmware >
Re: [libvirt] [PATCH] qemuDomainNamespaceSetupDisk: Fix const correctness
On Tue, Apr 10, 2018 at 08:58 AM +0200, Michal Privoznik wrote: > The array of strings we are building is indeed array of const > strings. We are not STRDUP()-ing them nor FREE()-ing them. > > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_domain.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index f03aa03e8a..0486c5527d 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -11196,7 +11196,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, > virStorageSourcePtr src) > { > virStorageSourcePtr next; > -char **paths = NULL; > +const char **paths = NULL; > size_t npaths = 0; > int ret = -1; > > @@ -11214,7 +11214,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, > goto cleanup; > } > > -if (qemuDomainNamespaceMknodPaths(vm, (const char **)paths, npaths) < 0) > +if (qemuDomainNamespaceMknodPaths(vm, paths, npaths) < 0) > return -1; > > ret = 0; > -- > 2.16.1 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > Reviewed-by: Marc Hartmayer -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/9] Be consistent with vir*Obj*Remove* APIs
On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan wrote: > v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html > > NB: This can wait until 4.2.0 is release, but I figured I'd post this > now just to put it on the radar and of course in hopes that someone > will look during the idle moment or two before the release. > > Changes since v1: > > Short story: Rework the processing of the code > > Longer story: > > In his review Erik noted that there's a "fire dance" when processing > the vir*Obj*Remove APIs of requiring a locked object upon entry, then > adding a reference to that object, unlocking the object, locking the > list to which it is contained, and then relocking the object. > > So it took some time to think about it and during one lengthy meeting > today I had the aha moment that the *Remove API's could take the same > key (e.g., uuid or name) used to Add or Find the object and use it for > the Remove API. This allows the Remove API to not require a locked (and > reffed) object upon entry and perform the lock dance, remove the object, > and return just just a reffed object forcing the caller to know to Unref > object. > > Instead, let's simplify things. The *Remove API can take the key, Find > the object in the list, remove it from the hash tables, and dispose of > the object. In essence the antecedent to the Add or AssignDef API's > taking a def, creating an object, and adding it the object to the hash > table(s). If there are two *Remove threads competing, one will win and > perform the removal, while the other will call *Remove, but won't find > the object in the hash table, and just return none the wiser. > > And yes, I think this can also work for the Domain code, but it's going > to take a few patch series to get there as that code is not consistent > between consumers. > > John Ferlan (9): > secret: Rework LoadAllConfigs > secret: Alter virSecretObjListRemove processing > interface: Alter virInterfaceObjListRemove processing > nodedev: Alter virNodeDeviceObjListRemove processing > conf: Clean up virStoragePoolObjLoad error processing > storage: Clean up storagePoolCreateXML error processing > test: Clean up testStoragePoolCreateXML error processing > conf: Move virStoragePoolObjRemove closer to AssignDef > storagepool: Alter virStoragePoolObjRemove processing Side note: Wouldn’t is be useful to refactor all the vir*ObjList* things as they’re looking quite similar? Not sure if it’s easily feasible in all places. […snip] -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v2 03/44] Force QMP capability probing
On Mon, 2018-04-09 at 16:42 +0100, Daniel P. Berrangé wrote: > > @@ -5114,7 +4261,7 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, > > goto error; > > } > > > > -if (qmpOnly && !qemuCaps->usedQMP) { > > +if (!qemuCaps->usedQMP) { > > virReportError(VIR_ERR_INTERNAL_ERROR, > > _("Failed to probe QEMU binary with QMP: %s"), > > qmperr ? qmperr : _("unknown error")); > > We should be able to kill this now. We only have "usedQMP" because we > want to gracefully get out of virQEMUCapsInitQMPCommandRun() when > launching with QMP fails. We can make that method return -1 or 0 only > now, and drop "usedQMP" I assumed he would drop it later in the series. Even if that's not the case, I'd still rather see it dropped as a follow-up cleanup patch instead of cramming even more changes into this single commit. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] news: announce dropping of legacy Xen driver
On Mon, 2018-04-09 at 15:00 -0600, Jim Fehlig wrote: > Signed-off-by: Jim Fehlig > --- > > Not sure if removal of a feature is a feature, but this seems better > placed under "New features" than "Improvements" or "Bug fixes". It definitely counts as an improvement for those of us working on the codebase ;) I think it would make sense to have a "Removed features" section in between "New features" and "Improvements", at least for this release: the old Xen driver is not the only chunk of legacy code being dropped... > docs/news.xml | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/docs/news.xml b/docs/news.xml > index 798ab6da4..106979cc1 100644 > --- a/docs/news.xml > +++ b/docs/news.xml > @@ -44,6 +44,16 @@ >add this controller when traditional PCI devices are in use. > > > + > + > + Xen: Drop the legacy xend-based driver > + > + > + The xm/xend toolstack was deprecated in Xen 4.2 and removed > + from the Xen sources in the 4.5 development cycle. The libvirt > + driver based on xend is now removed from the libvirt sources. > + > + > > > With a "Removed features" section introduced and the entry moved to it, Reviewed-by: Andrea Bolognani but maybe don't push right away to give other people a chance to chime in. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 0/2] jobs: Simplify Python jobs
On Thu, Apr 05, 2018 at 05:23:58PM +0200, Andrea Bolognani wrote: > By reverting a bunch of semi-recent changes. > > Requires > > https://www.redhat.com/archives/libvir-list/2018-April/msg00356.html > > Andrea Bolognani (2): > jobs: Don't set $PYTHONPATH for python-distutil jobs > jobs: Build using $PYTHON Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 0/2] Add libvirt-dbus project
On Thu, Apr 05, 2018 at 04:10:15PM +0200, Andrea Bolognani wrote: > Andrea Bolognani (2): > guests: Add libvirt-dbus project > projects: Add libvirt-dbus project Thanks for adding libvirt-dbus. Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 09.04.2018 18:34, Laszlo Ersek wrote: > On 04/09/18 09:26, Thomas Huth wrote: >> Hi Laszlo, >> >> On 07.04.2018 02:01, Laszlo Ersek wrote: >>> Add a schema that describes the properties of virtual machine firmware. >>> >>> Each firmware executable installed on a host system should come with a >>> JSON file that conforms to this schema, and informs the management >>> applications about the firmware's properties. >>> >>> In addition, a configuration directory with symlinks to the JSON files >>> should exist, with the symlinks carefully named to reflect a priority >>> order. Management applications can then search this directory in priority >>> order for the first firmware executable that satisfies their search >>> criteria. The found JSON file provides the management layer with domain >>> configuration bits that are required to run the firmware binary. >> [...] >>> +## >>> +# @FirmwareDevice: >>> +# >>> +# Defines the device types that a firmware file can be mapped into. >>> +# >>> +# @memory: The firmware file is to be mapped into memory. >>> +# >>> +# @kernel: The firmware file is to be loaded like a Linux kernel. This is >>> +# similar to @memory but may imply additional processing that is >>> +# specific to the target architecture. >>> +# >>> +# @flash: The firmware file is to be mapped into a pflash chip. >>> +# >>> +# Since: 2.13 >>> +## >>> +{ 'enum' : 'FirmwareDevice', >>> + 'data' : [ 'memory', 'kernel', 'flash' ] } >> >> This is not fully clear to me... what is this exactly good for? Is this >> a way to say how the firmware should be loaded, i.e. via "-bios", >> "-kernel" or "-pflash" parameter? If so, the term "memory" is quite >> misleading since files that are loaded via -bios can also end up in an >> emulated ROM chip. > > I threw in "-kernel" because, although it also (usually?) means > "memory", I expected people would want it separate. > > Regarding memory vs. pflash, I thought that these two, combined with the > access permissions, could cover all of RAM, ROM, and read-only and > read-write pflash too. > > So, "-bios" (-> ROM) boils down to "memory", with write access denied -- > please see the SeaBIOS example near the end. Let me ask the other way round: How does a high-level tool know whether it should use "-bios", "-kernel", "-pflash", "-device generic-loader" or "-younameit" for loading the firmware? >>> + 'nvram-map' : 'FirmwareMapping', >>> + 'templates' : [ 'FirmwareFile' ] } } >>> + >>> +## >>> +# @SystemFirmwareType: >>> +# >>> +# Lists system firmware types commonly used with QEMU virtual machines. >>> +# >>> +# @bios: The system firmware was built from the SeaBIOS project. >>> +# >>> +# @slof: The system firmware was built from the Slimline Open Firmware >>> project. >>> +# >>> +# @uboot: The system firmware was built from the U-Boot project. >>> +# >>> +# @uefi: The system firmware was built from the edk2 (EFI Development Kit >>> II) >>> +#project. >>> +# >>> +# Since: 2.13 >>> +## >>> +{ 'enum' : 'SystemFirmwareType', >>> + 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] } >> >> The naming here is quite a bad mixture between firmware interface >> ('bios', 'uefi') and firmware implementations ('slof', 'uboot'). There >> could be other implementations of BIOS or UEFI than SeaBIOS and EDK2 ... >> so I'd suggest to rather name them 'seabios' and 'edk2' here instead. > > Sure, I'm totally ready to follow community advice here (too). > > In fact this is the one element I dislike the most about the schema -- > it's the fuzziest part, yet it is the most important element for > libvirt. Because users and higher level apps just want to say "give me > UEFI". If I have to ask "OK, but UEFI built from edk2 or something > else?", then it's a lost cause already. > > It's hard to find the right level of abstraction in the naming when the > higher level tools (and/or ultimately the users) don't know enough to > ask for specifics -- I'm not saying that's bad; it's quite natural, but > makes things very difficult. So this enum aims to match the user story > "gimme UEFI and be done with it". I figure users will just utter the > most common buzzword form of the concept they have in mind. "edk2" > doesn't tell them as much as "uefi". OK, I see your point. But I still think we should not design fuzzy interfaces here at this low level, this will only lead to other trouble later. ... thinking about this again, users seem to mix up firmware interfaces / families with concrete implementations. So maybe we need something like: { 'enum' : 'SystemFirmwareType', 'data' : [ 'seabios', 'slof', 'uboot', 'edk2', 'openbios' ] } *and* : { 'enum' : 'SystemFirmwareInterface', /* or: 'SystemFirmwareFamily' */ 'data' : [ 'bios', 'uefi', 'openfirmware' ] } Then a high level tool can check both and pick the best match? Thomas -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [qemu RFC] qapi: add "firmware.json"
On 09.04.2018 18:50, Laszlo Ersek wrote: > On 04/09/18 10:19, Gerd Hoffmann wrote: +{ 'enum' : 'SystemFirmwareType', + 'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] } >>> >>> The naming here is quite a bad mixture between firmware interface >>> ('bios', 'uefi') and firmware implementations ('slof', 'uboot'). There >>> could be other implementations of BIOS or UEFI than SeaBIOS and EDK2 ... >>> so I'd suggest to rather name them 'seabios' and 'edk2' here instead. >> >> uboot for example implements uefi unterfaces too (dunno how complete, >> but reportly recent versions can run uefi shell and grub just fine). > > Indeed: when I was struggling with this enum type and tried to look for > more firmware types to add, my googling turned up the "UEFI on Top of > U-Boot" whitepaper, from Alex and Andreas :) > > Again, this reaches to the root of the problem: when a user creates a > new domain, using high-level tools, they just want to tick "UEFI". (Dan > has emphasized this to me several times, so I think I get the idea by > now, if not the full environment.) We cannot ask the user, "please be > more specific, do you want UEFI from edk2, or UEFI on top of U-Boot?" But you are designing a rather low-level interface here, which should IMHO rather be precise than fuzzy. So should this "just want to tick UEFI" rather be handled in the high-level tools instead? Alternatively, what about providing some kind of "alias" or "nickname" setting here, too? So the EDK2 builds would get SystemFirmwareType="edk2" and "SystemFirmwareAlias="uefi" for example. Thomas -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemuDomainNamespaceSetupDisk: Fix const correctness
The array of strings we are building is indeed array of const strings. We are not STRDUP()-ing them nor FREE()-ing them. Signed-off-by: Michal Privoznik --- src/qemu/qemu_domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f03aa03e8a..0486c5527d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11196,7 +11196,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, virStorageSourcePtr src) { virStorageSourcePtr next; -char **paths = NULL; +const char **paths = NULL; size_t npaths = 0; int ret = -1; @@ -11214,7 +11214,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, goto cleanup; } -if (qemuDomainNamespaceMknodPaths(vm, (const char **)paths, npaths) < 0) +if (qemuDomainNamespaceMknodPaths(vm, paths, npaths) < 0) return -1; ret = 0; -- 2.16.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list