[libvirt] [PATCH v3 14/14] qemu: Add swtpm to emulator cgroup
Stefan Berger
stefanb at linux.vnet.ibm.com
Thu May 10 14:18:02 UTC 2018
On 05/09/2018 11:23 AM, John Ferlan wrote:
>
> On 05/04/2018 04:21 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/conf/domain_conf.c | 1 +
>> src/conf/domain_conf.h | 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/virtpm.c | 33 +++++++++++++++++++++++++++++
>> 7 files changed, 112 insertions(+)
>>
> So that I don't forget ;-) - there should be a "next" patch as well that
> adds some text to docs/news.xml in order to describe "at a high level"
> the new feature or improvement being done. Perhaps something that should
> be done for the existing series I pushed, but neglected to remember to
> ask about while reviewing.
>
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index c98d26a..f542c8e 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2624,6 +2624,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 826ff26..49c77f7 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -1311,6 +1311,7 @@ struct _virDomainTPMDef {
>> virDomainChrSourceDef source;
>> char *storagepath;
>> char *logfile;
>> + char *pidfile;
>> } emulator;
>> } data;
>> };
> There are varying "arguments" about using/saving a pidfile or pid of a
> thread in recent posts vis-a-vis the pid stored in some file doesn't
> match what's running for various reasons. How does one know that that
> pid in the pidfile matches the pid of the running process without checking.
>
> You may want to look at src/util/virpidfile.c and various virPidFile*
> API's to see if they're useful. Somehow I have a feeling this will be
> similar to other consumers.
>
> Also, see the discussion during pr_helper review v3, patch 9:
>
> https://www.redhat.com/archives/libvir-list/2018-March/msg00749.html
>
> The pr_helper in the end is a process/thread being started to manage
> something - much like swtpm is a process/thread managing something
> (blackbox view).
>
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index 1a5adca..f86ac3f 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -38,6 +38,7 @@
>> #include "virnuma.h"
>> #include "virsystemd.h"
>> #include "virdevmapper.h"
>> +#include "virpidfile.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_QEMU
>>
>> @@ -1146,6 +1147,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 */
>> +
> Since this only matters "if (tpm && tpm->type ==
> VIR_DOMAIN_TPM_TYPE_EMULATOR)", then why not check that here and
> simplify the rest a bit more.
I'll add this type of check calling qemuExtDevicesHasDevice(). That at
least lets us isolate all the external device code.
>
>> + /*
>> + * 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)
> Currently, the VIR_CGROUP_THREAD_EMULATOR is used for the qemu-kvm
> emulator (e.g. qemu-kvm process/thread). There's also VCPU and IOTHREAD
> cgroup enum's.
>
> So that makes me wonder if there should be a new "group" for "any"
> similar thread that's created based on domain configuration.
It probably should be a separate group.
>
> This type of conversation is a bit out of my comfort zone though.
> Usually I defer to danpb on this although IIRC mkletzan also has some
> expertise here.
>
> Since this "concept" of having to limit the thread via cgroups has an
> impact on another series I've recently reviewed (the pr_helper), you
> should see that I responded today to that series with a question of
> mprivozn about whether it would be needed there as well, see:
>
> https://www.redhat.com/archives/libvir-list/2018-May/msg00579.html
Ok, I will have a look
>
>
>> + goto cleanup;
>> +
>> + if (tpm) {
>> + switch (tpm->type) {
>> + case VIR_DOMAIN_TPM_TYPE_EMULATOR:
>> + if (virPidFileReadPath(tpm->data.emulator.pidfile, &pid) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Could not read swtpm's pidfile %s"),
>> + tpm->data.emulator.pidfile);
>> + goto cleanup;
>> + }
This here was probably the strongest reason to carry the pidfile around
in the tpm->data.emulator.pidfile. I now moved this into a function
qemuExtDevicesSetupCgroup() so that I don't have to create this pidfile
over here. It should probably have followed the pattern of caling into
qemu_extdevice.c before.
>> + if (virCgroupAddTask(cgroup_temp, pid) < 0)
>> + goto cleanup;
>> + break;
>> + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
>> + case VIR_DOMAIN_TPM_TYPE_LAST:
>> + break;
>> + }
>> + }
>> +
>> + ret = 0;
>> +
>> +cleanup:
>> + virCgroupFree(&cgroup_temp);
>> +
>> + return ret;
>> +}
>> +
>> +
>> +int
>> qemuSetupGlobalCpuCgroup(virDomainObjPtr vm)
>> {
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
>> index 3b8ff60..478bf7e 100644
>> --- a/src/qemu/qemu_cgroup.h
>> +++ b/src/qemu/qemu_cgroup.h
>> @@ -69,6 +69,7 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup,
>> long long quota);
>> int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask);
>> int qemuSetupGlobalCpuCgroup(virDomainObjPtr vm);
>> +int qemuSetupCgroupForExtDevices(virDomainObjPtr vm);
>> int qemuRemoveCgroup(virDomainObjPtr vm);
>>
>> typedef struct _qemuCgroupEmulatorAllNodesData qemuCgroupEmulatorAllNodesData;
>> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
>> index eb7220d..779e759 100644
>> --- a/src/qemu/qemu_extdevice.c
>> +++ b/src/qemu/qemu_extdevice.c
>> @@ -148,6 +148,9 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> virDomainTPMDefPtr tpm = def->tpm;
>> char *shortName = virDomainDefGetShortName(def);
>> + char *pidfiledata = NULL;
>> + int timeout;
>> + int len;
>>
>> if (!shortName)
>> return -1;
>> @@ -195,6 +198,22 @@ qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
>> goto error;
>> }
>>
>> + /* check that the swtpm has written its pid into the file */
>> + timeout = 1000; /* ms */
>> + while ((len = virFileReadHeaderQuiet(tpm->data.emulator.pidfile,
>> + 10, &pidfiledata)) <= 0) {
>> + if (len == 0 && timeout > 0) {
>> + timeout -= 50;
>> + usleep(50 * 1000);
>> + continue;
>> + }
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("swtpm did not write pidfile '%s'"),
>> + tpm->data.emulator.pidfile);
>> + goto error;
>> + }
>> + VIR_FREE(pidfiledata);
>> +
> I think the virPidFile definitely helps here although I haven't dug too
> deep into those API's.
>
>> ret = 0;
>>
>> cleanup:
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 2b07530..bf0e4da 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -6076,6 +6076,10 @@ qemuProcessLaunch(virConnectPtr conn,
>> if (qemuProcessSetupEmulator(vm) < 0)
>> goto cleanup;
>>
>> + VIR_DEBUG("Setting cgroup for external devices (if required)");
>> + if (qemuSetupCgroupForExtDevices(vm) < 0)
>> + goto cleanup;
>> +
>> VIR_DEBUG("Setting up resctrl");
>> if (qemuProcessResctrlCreate(driver, vm) < 0)
>> goto cleanup;
>> diff --git a/src/util/virtpm.c b/src/util/virtpm.c
>> index 0617326..3d8a195 100644
>> --- a/src/util/virtpm.c
>> +++ b/src/util/virtpm.c
>> @@ -39,6 +39,7 @@
>> #include "virlog.h"
>> #include "virtpm.h"
>> #include "virutil.h"
>> +#include "virpidfile.h"
>> #include "configmake.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_NONE
>> @@ -376,6 +377,25 @@ int virTPMEmulatorInitPaths(virDomainTPMDefPtr tpm,
>> }
>>
>> /*
>> + * virTPMCreatePidFilename
>> + */
>> +static char *virTPMCreatePidFilename(const char *swtpmStateDir,
>> + const char *shortName)
>> +{
>> + char *pidfile = NULL;
>> + char *devname = NULL;
>> +
>> + if (virAsprintf(&devname, "%s-swtpm", shortName) < 0)
>> + return NULL;
>> +
>> + pidfile = virPidFileBuildPath(swtpmStateDir, devname);
>> +
>> + VIR_FREE(devname);
>> +
>> + return pidfile;
>> +}
>> +
>> +/*
>> * virTPMEmulatorPrepareHost:
>> *
>> * @tpm: tpm definition
>> @@ -442,6 +462,10 @@ int virTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
>> goto cleanup;
>> tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_UNIX;
>>
>> + if (!(tpm->data.emulator.pidfile =
>> + virTPMCreatePidFilename(swtpmStateDir, shortName)))
>> + goto cleanup;
>> +
> Not quite sure why we need to save this especially since during stop
> processing we don't use it and we can recreate it fairly easily.
>
>> ret = 0;
>>
>> cleanup:
>> @@ -619,6 +643,9 @@ virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, const char *vmname,
>> break;
>> }
>>
>> + virCommandAddArg(cmd, "--pid");
>> + virCommandAddArgFormat(cmd, "file=%s", tpm->data.emulator.pidfile);
>> +
> This code could just have a local @pidfile = virTPMCreatePidFilename,
> then once added to the arglist, VIR_FREE(pidfile). Especially since we
> can build it up on the fly using information we have.
I changed it towards that now. The patch will look quite different, though.
>
>> return cmd;
>>
>> error:
>> @@ -646,6 +673,7 @@ virTPMEmulatorStop(const char *swtpmStateDir, const char *shortName)
>> virCommandPtr cmd;
>> char *pathname;
>> char *errbuf = NULL;
>> + char *pidfile;
>>
>> if (virTPMEmulatorInit() < 0)
>> return;
>> @@ -674,6 +702,11 @@ virTPMEmulatorStop(const char *swtpmStateDir, const char *shortName)
>> unlink(pathname);
>>
>> cleanup:
>> + /* clean up the PID file */
>> + if ((pidfile = virTPMCreatePidFilename(swtpmStateDir, shortName))) {
>> + unlink(pidfile);
>> + VIR_FREE(pidfile);
>> + }
> So why not use tpm->data.emulator.pidfile or pass it and only clean up
> "if (pidfile) {...}"... IOW: Pass as an argument @pidfile; otherwise,
> what's the purpose of keeping it around?
>
> Curious why is it libvirt's responsibility for this cleanup? If adding
> "--pid $path" to the swtpm startup has the result of creating a pid file
> that a consumer can read once started/running, then shouldn't it be the
> responsibility of that same image/code to remove the pidfile as part of
> it's clean up processing indicating that the process no longer exists?
> OK so yeah, swtpm could die, not clean things up, and then what? Well
> for sure the domain isn't going to last, right? So perhaps on cleanup I
> could see reason for ensuring the pidfile doesn't exist, but I also
> assume that swtpm would write a "clean" pid if it finds --pid on the
> command line. So does it really matter beyond needing to somehow know
> that swtpm succeeded in starting "this" time.
swtpm does clean up after itself and libvirt shouldn't need to do that.
So, yeah, I will not hold onto this and drop it.
Stefan
>
> John
>
>> VIR_FREE(pathname);
>> VIR_FREE(errbuf);
>> }
>>
More information about the libvir-list
mailing list