[libvirt] [PATCH v5 11/11] qemu: Add swtpm to emulator cgroup
Stefan Berger
stefanb at linux.vnet.ibm.com
Tue May 22 12:51:24 UTC 2018
On 05/21/2018 07:27 PM, John Ferlan wrote:
>
> 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 at 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 at 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 at linux.vnet.ibm.com>
>> ---
>> 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?
Correct. It would do too much in case of passthrough.
>
>> +}
>> +
>> +
>> +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.
For as long as there's not other device, this is of course true.
>
>> +
>> + 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.
Fixed.
One more improvement would be to push this into
qemuTPMEmulatorPollPid(cfg->swtpmStateDir, shortName, &pid, 50, 1000);
I'd do that later, though, since this may end up touching virpidfile.c
again as well with a function to Poll.
>
>> + 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...
Ok. I will post a v6. And then I'll post the AppArmor related patches. I
think auditing also needs some work. It's not being called at all at the
moment.
>
> Tks -
>
> Reviewed-by: John Ferlan <jferlan at redhat.com>
Applied.
>
> John
>
More information about the libvir-list
mailing list