[libvirt] [PATCH v2 4/6] tpm: Label the external swtpm with SELinux labels

[Stefan Berger]

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, );
 
-if (virCommandRun(cmd, ) < 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, );
+
+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 @@ 

[libvirt] [PATCH v2 2/6] tpm: Add support for external swtpm TPM emulator

[Stefan Berger]

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
 /devices
 ...
 
+
+
+  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
+
+
+  ...
+  devices
+tpm model='tpm-tis'
+  backend type='emulator'
+  /backend
+/tpm
+  /devices
+  ...
+
 
   model
   
@@ 

[libvirt] [PATCH v2 3/6] tpm: Add test cases for external swtpm TPM emulator

[Stefan Berger]

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(, 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, 

[libvirt] [PATCH v2 6/6] tpm: Add swtpm to emulator cgroup

[Stefan Berger]

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, _temp) < 0)
+goto cleanup;
+
+if (tpm) {
+switch (tpm->type) {
+case VIR_DOMAIN_TPM_TYPE_EMULATOR:
+if (virPidFileReadPath(tpm->data.emulator.pidfile, ) < 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(_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 

[libvirt] [PATCH v2 5/6] tpm: Add support for choosing emulation of a TPM 2

[Stefan Berger]

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
   ...
   devices
 tpm model='tpm-tis'
-  backend type='emulator'
+  backend type='emulator' tpmversion='2'
   /backend
 /tpm
   /devices
@@ -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,
+} 

[libvirt] [PATCH v2 0/6] Add support for TPM emulator and CRB interface

[Stefan Berger]

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 

[libvirt] [PATCH v2 1/6] tpm: Enable TPM CRB interface

[Stefan Berger]

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 \

Re: [libvirt] [PATCH v3] qemu: add virQEMUBuildBufferEscapeComma in qemu_command.c

[John Ferlan]

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(, "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 

Re: [libvirt] [PATCH V2 2/8] tests: move sexpr2xml tests to WITH_LIBXL

[Jim Fehlig]

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

[Jim Fehlig]

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 

Re: [libvirt] [PATCH v6.1 2/9] libxl: pass driver config to libxlMakeDomBuildInfo

[Jim Fehlig]

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(, 0, sizeof(info));
+
+return 
+/* 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

[Daniel P . Berrangé]

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

[Michal Privoznik]

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, ) < 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 \

[libvirt] [PATCH v4 14/14] qemu: Detect pr-manager-helper capability

[Michal Privoznik]

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

[Michal Privoznik]

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

[Michal Privoznik]

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, >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

[Michal Privoznik]

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, ) < 0)
 goto error;
 
+if (qemuMaybeBuildPRManagerInfoProps(vm, prd, ) < 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(_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

[Michal Privoznik]

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

[Michal Privoznik]

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", >bridgeHelperName) < 
0)
 goto cleanup;
 
+if (virConfGetValueString(conf, "pr_helper", >prHelperName) < 0)
+goto cleanup;
+
 if (virConfGetValueBool(conf, "mac_filter", >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

[Michal Privoznik]

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(>secinfo);
 qemuDomainSecretInfoFree(>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(, "%s/pr-helper0.sock", priv->libDir) < 0)
+goto cleanup;
+
+} else {
+if (virAsprintf(, "pr-helper-%s", disk->info.alias) < 0)
+return ret;
+
+

[libvirt] [PATCH v4 10/14] qemu: Start PR daemon on domain startup

[Michal Privoznik]

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(_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(_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, , ) < 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, );
+
+/* 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, ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("pr helper %s didn't show up"), prd->alias);
+goto cleanup;
+}
+
+if (virTimeBackOffStart(, 1, timeout) < 0)
+goto cleanup;
+while (virTimeBackOffWait()) {
+char errbuf[1024] = { 0 };
+
+if (virFileExists(prd->path))
+break;
+
+if (virProcessKill(cpid, 0) == 0)
+continue;
+
+  

[libvirt] [PATCH v4 03/14] qemu: Introduce pr-manager-helper capability

[Michal Privoznik]

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

[Michal Privoznik]

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

[Michal Privoznik]

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 @@
   /disk
   disk type='block' device='lun'
 driver name='qemu' type='raw'/
-source dev='/dev/sda'/
+source dev='/dev/sda'
+  reservations enabled='yes' managed='no'
+source type='unix' path='/path/to/qemu-pr-helper' 
mode='client'/
+  /reservations
 target dev='sda' bus='scsi'/
 address type='drive' controller='0' bus='0' target='3' unit='0'/
   /disk
@@ -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

[Michal Privoznik]

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, ))
 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

[Michal Privoznik]

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

[Michal Privoznik]

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

[Ján Tomko]

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 qemus 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

[Ján Tomko]

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

[Ján Tomko]

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

[Ján Tomko]

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

[Ján Tomko]

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

[Ján Tomko]

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

[Pavel Hrdina]

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"

[Laszlo Ersek]

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"

[Laszlo Ersek]

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

[John Ferlan]

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"

[Laszlo Ersek]

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"

[Laszlo Ersek]

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"

[Laszlo Ersek]

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"

[Daniel P . Berrangé]

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"

[Daniel P . Berrangé]

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"

[Peter Maydell]

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"

[Laszlo Ersek]

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"

[Laszlo Ersek]

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"

[Laszlo Ersek]

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"

[Gerd Hoffmann]

  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"

[Daniel P . Berrangé]

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"

[Laszlo Ersek]

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 

Re: [libvirt] [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

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"

[Daniel P . Berrangé]

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"

[Paolo Bonzini]

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"

[Daniel P . Berrangé]

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"

[Gerd Hoffmann]

> > 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"

[Daniel P . Berrangé]

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"

[Thomas Huth]

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"

[Thomas Huth]

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"

[Daniel P . Berrangé]

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"

[Laszlo Ersek]

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"

[Thomas Huth]

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"

[Daniel P . Berrangé]

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 

Re: [libvirt] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

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"

[Laszlo Ersek]

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"

[Daniel P . Berrangé]

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"

[Laszlo Ersek]

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"

[Daniel P . Berrangé]

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"

[Laszlo Ersek]

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

[Marc Hartmayer]

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

[Marc Hartmayer]

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

[Andrea Bolognani]

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

[Andrea Bolognani]

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

[Pavel Hrdina]

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

[Pavel Hrdina]

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"

[Thomas Huth]

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"

[Thomas Huth]

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

[Michal Privoznik]

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

Re: [libvirt] [PATCH] util: don't check for parallel iteration in hash-related functions

[Vincent Bernat]

 ❦  6 avril 2018 12:01 +0200, Michal Privoznik  :

> So I went through all callbacks (even transitive ones) and I've found
> two problems:
>
> umlProcessAutoDestroyRun -> umlProcessAutoDestroyDom -> virHashRemoveEntry
>
> qemuDomainSnapshotDiscardAllMetadata -> qemuDomainSnapshotDiscardAll ->
> qemuDomainSnapshotDiscard -> virDomainSnapshotObjListRemove ->
> virHashRemoveEntry
>
> While me (and probably Peter :-)) don't care about the first one, the
> second one is a real issue. I guess we need to fix that one before this
> can be merged.
>
> On a positive side, I haven't spotted any other problem. So once qemu
> (and possibly uml) are fixed this can be merged as is.

I updated the patch with the other small issue you noticed, but not this
one (didn't spot an immediate lock to use and got not time to dig
further).

FI, we didn't run into any problem so far and we are running a patched
libvirt on all our hypervisors (with QEMU).
-- 
Zounds!  I was never so bethumped with words
since I first called my brother's father dad.
-- William Shakespeare, "Kind John"

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [qemu RFC] qapi: add "firmware.json"

[Gerd Hoffmann]

  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?

cheers,
  Gerd

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2] util: don't check for parallel iteration in hash-related functions

[Vincent Bernat]

This is the responsability of the caller to apply the correct lock
before using these functions. Moreover, the use of a simple boolean
was still racy: two threads may check the boolean and "lock" it
simultaneously.

Users of functions from src/util/virhash.c have to be checked for
correctness. Lookups and iteration should hold a RO
lock. Modifications should hold a RW lock.

Most important uses seem to be covered. Callers have now a greater
responsability, notably the ability to execute some operations while
iterating were reliably forbidden before are now accepted.
---
 src/util/virhash.c  | 37 
 tests/virhashtest.c | 83 -
 2 files changed, 120 deletions(-)

diff --git a/src/util/virhash.c b/src/util/virhash.c
index 0ffbfcce2c64..475c2b0281b2 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -41,12 +41,6 @@ VIR_LOG_INIT("util.hash");
 
 /* #define DEBUG_GROW */
 
-#define virHashIterationError(ret) \
-do { \
-VIR_ERROR(_("Hash operation not allowed during iteration")); \
-return ret; \
-} while (0)
-
 /*
  * A single entry in the hash table
  */
@@ -66,10 +60,6 @@ struct _virHashTable {
 uint32_t seed;
 size_t size;
 size_t nbElems;
-/* True iff we are iterating over hash entries. */
-bool iterating;
-/* Pointer to the current entry during iteration. */
-virHashEntryPtr current;
 virHashDataFree dataFree;
 virHashKeyCode keyCode;
 virHashKeyEqual keyEqual;
@@ -339,9 +329,6 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void 
*name,
 if ((table == NULL) || (name == NULL))
 return -1;
 
-if (table->iterating)
-virHashIterationError(-1);
-
 key = virHashComputeKey(table, name);
 
 /* Check for duplicate entry */
@@ -551,9 +538,6 @@ virHashRemoveEntry(virHashTablePtr table, const void *name)
 nextptr = table->table + virHashComputeKey(table, name);
 for (entry = *nextptr; entry; entry = entry->next) {
 if (table->keyEqual(entry->name, name)) {
-if (table->iterating && table->current != entry)
-virHashIterationError(-1);
-
 if (table->dataFree)
 table->dataFree(entry->payload, entry->name);
 if (table->keyFree)
@@ -593,18 +577,11 @@ virHashForEach(virHashTablePtr table, virHashIterator 
iter, void *data)
 if (table == NULL || iter == NULL)
 return -1;
 
-if (table->iterating)
-virHashIterationError(-1);
-
-table->iterating = true;
-table->current = NULL;
 for (i = 0; i < table->size; i++) {
 virHashEntryPtr entry = table->table[i];
 while (entry) {
 virHashEntryPtr next = entry->next;
-table->current = entry;
 ret = iter(entry->payload, entry->name, data);
-table->current = NULL;
 
 if (ret < 0)
 goto cleanup;
@@ -615,7 +592,6 @@ virHashForEach(virHashTablePtr table, virHashIterator iter, 
void *data)
 
 ret = 0;
  cleanup:
-table->iterating = false;
 return ret;
 }
 
@@ -643,11 +619,6 @@ virHashRemoveSet(virHashTablePtr table,
 if (table == NULL || iter == NULL)
 return -1;
 
-if (table->iterating)
-virHashIterationError(-1);
-
-table->iterating = true;
-table->current = NULL;
 for (i = 0; i < table->size; i++) {
 virHashEntryPtr *nextptr = table->table + i;
 
@@ -667,7 +638,6 @@ virHashRemoveSet(virHashTablePtr table,
 }
 }
 }
-table->iterating = false;
 
 return count;
 }
@@ -723,23 +693,16 @@ void *virHashSearch(const virHashTable *ctable,
 if (table == NULL || iter == NULL)
 return NULL;
 
-if (table->iterating)
-virHashIterationError(NULL);
-
-table->iterating = true;
-table->current = NULL;
 for (i = 0; i < table->size; i++) {
 virHashEntryPtr entry;
 for (entry = table->table[i]; entry; entry = entry->next) {
 if (iter(entry->payload, entry->name, data)) {
-table->iterating = false;
 if (name)
 *name = table->keyCopy(entry->name);
 return entry->payload;
 }
 }
 }
-table->iterating = false;
 
 return NULL;
 }
diff --git a/tests/virhashtest.c b/tests/virhashtest.c
index 3b85b62c301d..e9c03c1afbbc 100644
--- a/tests/virhashtest.c
+++ b/tests/virhashtest.c
@@ -221,32 +221,6 @@ testHashRemoveForEachAll(void *payload ATTRIBUTE_UNUSED,
 }
 
 
-const int testHashCountRemoveForEachForbidden = ARRAY_CARDINALITY(uuids);
-
-static int
-testHashRemoveForEachForbidden(void *payload ATTRIBUTE_UNUSED,
-   const void *name,
-   void *data)
-{
-virHashTablePtr hash = data;
-size_t i;
-
-for (i = 0; i < ARRAY_CARDINALITY(uuids_subset); i++) {
-if (STREQ(uuids_subset[i], name)) {
-int next = (i + 1) % 

Re: [libvirt] [qemu RFC] qapi: add "firmware.json"

[Gerd Hoffmann]

  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?

cheers,
  Gerd

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list