[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