Re: [libvirt] [PATCH v5 11/11] qemu: Add swtpm to emulator cgroup

On 05/15/2018 08:26 PM, Stefan Berger wrote: > 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 1-testvm-swtpm.pid > srw-rw----. 1 qemu qemu system_u:object_r:svirt_image_t:s0:c597,c632 0 Apr 10 12:26 1-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/1-testvm-swtpm.sock,mode=0600 --tpmstate dir=/var/lib/libvirt/swtpm/485d0004-a48f-436a-8457-8a3b73e28568/tpm1.2/ --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log --pid file=/var/run/libvirt/qemu/swtpm/1-testvm-swtpm.pid > > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > --- > src/qemu/qemu_cgroup.c | 35 ++++++++++++ > src/qemu/qemu_cgroup.h | 2 + > src/qemu/qemu_extdevice.c | 23 ++++++++ > src/qemu/qemu_extdevice.h | 6 +++ > src/qemu/qemu_process.c | 4 ++ > src/qemu/qemu_tpm.c | 135 ++++++++++++++++++++++++++++++++++++++++++++-- > src/qemu/qemu_tpm.h | 6 +++ > 7 files changed, 208 insertions(+), 3 deletions(-) > > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c > index 54b00a5da5..12b3f3bf40 100644 > --- a/src/qemu/qemu_cgroup.c > +++ b/src/qemu/qemu_cgroup.c > @@ -26,6 +26,7 @@ > #include "qemu_cgroup.h" > #include "qemu_domain.h" > #include "qemu_process.h" > +#include "qemu_extdevice.h" > #include "vircgroup.h" > #include "virlog.h" > #include "viralloc.h" > @@ -1172,6 +1173,40 @@ qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, > } > > > +int > +qemuSetupCgroupForExtDevices(virDomainObjPtr vm, > + virQEMUDriverPtr driver) > +{ > + qemuDomainObjPrivatePtr priv = vm->privateData; > + virCgroupPtr cgroup_temp = NULL; > + int ret = -1; > + > + if (!qemuExtDevicesHasDevice(vm->def) || > + priv->cgroup == NULL) > + return 0; /* Not supported, so claim success */ > + > + /* > + * If CPU cgroup controller is not initialized here, then we need > + * neither period nor quota settings. And if CPUSET controller is > + * not initialized either, then there's nothing to do anyway. > + */ > + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && > + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) > + return 0; > + > + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, > + false, &cgroup_temp) < 0) > + goto cleanup; > + > + ret = qemuExtDevicesSetupCgroup(driver, vm->def, cgroup_temp); > + > + cleanup: > + virCgroupFree(&cgroup_temp); > + > + return ret; > +} > + > + > int > qemuSetupGlobalCpuCgroup(virDomainObjPtr vm) > { > diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h > index 3b8ff6055d..c2fca7fc1d 100644 > --- a/src/qemu/qemu_cgroup.h > +++ b/src/qemu/qemu_cgroup.h > @@ -69,6 +69,8 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, > long long quota); > int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask); > int qemuSetupGlobalCpuCgroup(virDomainObjPtr vm); > +int qemuSetupCgroupForExtDevices(virDomainObjPtr vm, > + virQEMUDriverPtr driver); > int qemuRemoveCgroup(virDomainObjPtr vm); > > typedef struct _qemuCgroupEmulatorAllNodesData qemuCgroupEmulatorAllNodesData; > diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c > index 790b19be9e..dd664208df 100644 > --- a/src/qemu/qemu_extdevice.c > +++ b/src/qemu/qemu_extdevice.c > @@ -30,6 +30,8 @@ > #include "virlog.h" > #include "virstring.h" > #include "virtime.h" > +#include "virtpm.h" > +#include "virpidfile.h" > > #define VIR_FROM_THIS VIR_FROM_QEMU > > @@ -152,3 +154,24 @@ qemuExtDevicesStop(virQEMUDriverPtr driver, > if (def->tpm) > qemuExtTPMStop(driver, def); > } > + > + > +bool > +qemuExtDevicesHasDevice(virDomainDefPtr def) > +{ > + return def->tpm != NULL; I think this should be: if (def->tpm && def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) return true; return false; Since we only need this for the emulator process, right?

> +} > + > + > +int > +qemuExtDevicesSetupCgroup(virQEMUDriverPtr driver, > + virDomainDefPtr def, > + virCgroupPtr cgroup) > +{ > + int ret = 0; > + > + if (def->tpm) > + ret = qemuExtTPMSetupCgroup(driver, def, cgroup); The def->tpm would seem to be superfluous based on qemuSetupCgroupForExtDevices calling qemuExtDevicesHasDevice first anyway.

> + > + return ret; > +} [...] > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c > index 508685c455..9f50d9b9b2 100644 > --- a/src/qemu/qemu_tpm.c > +++ b/src/qemu/qemu_tpm.c [...] > > if (qemuExtDeviceLogCommand(logCtxt, cmd, "TPM Emulator") < 0) > @@ -756,6 +823,22 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver, > goto cleanup; > } > > + /* check that the swtpm has written its pid into the file */ > + timeout = 1000; /* ms */ > + while (timeout > 0) { > + rc = qemuTPMEmulatorGetPid(cfg->swtpmStateDir, shortName, &pid); > + if (rc < 0) { > + timeout -= 50; > + usleep(50 * 1000); > + continue; > + } > + if (rc == 0 && pid == (pid_t)-1) > + goto error; s/ goto/goto/ e.g. there's one extra space.

> + break; > + } > + if (timeout <= 0) > + goto error; > + > ret = 0; > [...] Things look good to me... Let me know about patch 6 and thoughts here. I don't mind making the adjustments before pushing. I do need a patch 12 which alters docs/news.xml to briefly describe the changes to support TPM/emulator...

Tks - Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt;

John