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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
