Re: [libvirt] [PATCH v5 06/11] qemu: Extend QEMU with external TPM support

On 05/15/2018 08:26 PM, Stefan Berger wrote: > Implement functions for managing the storage of the external swtpm as well > as starting and stopping it. Also implement functions to use swtpm_setup, > which simulates the manufacturing of a TPM, which includes creation of > certificates for the device. > > Further, the external TPM needs storage on the host that we need to set > up before it can be run. We can clean up the host once the domain is > undefined. > > This patch also implements a small layer for external device support that > calls into the TPM device layer if a domain has an attached TPM. This is > the layer we will wire up later on. > > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > --- > src/qemu/Makefile.inc.am | 4 + > src/qemu/qemu_domain.c | 2 + > src/qemu/qemu_extdevice.c | 154 ++++++++++ > src/qemu/qemu_extdevice.h | 53 ++++ > src/qemu/qemu_migration.c | 3 + > src/qemu/qemu_process.c | 12 + > src/qemu/qemu_tpm.c | 751 ++++++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_tpm.h | 50 +++ > 8 files changed, 1029 insertions(+) > create mode 100644 src/qemu/qemu_extdevice.c > create mode 100644 src/qemu/qemu_extdevice.h > create mode 100644 src/qemu/qemu_tpm.c > create mode 100644 src/qemu/qemu_tpm.h > [...] > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index f753e42d1b..42e68f3ecb 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -39,6 +39,7 @@ > #include "qemu_hotplug.h" > #include "qemu_blockjob.h" > #include "qemu_security.h" > +#include "qemu_extdevice.h" > > #include "domain_audit.h" > #include "virlog.h" > @@ -2917,6 +2918,7 @@ qemuMigrationSrcConfirm(virQEMUDriverPtr driver, > if (!virDomainObjIsActive(vm)) { > if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) { > virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); > + qemuExtDevicesCleanupHost(driver, vm->def); I believe ^^ this ^^ will eventually get called by ... > vm->persistent = 0; > } > qemuDomainRemoveInactiveJob(driver, vm); ... ^^ this ^^ qemuDomainRemoveInactiveJob calls qemuDomainRemoveInactive and seince vm->persistent is not set, the call to qemuExtDevicesCleanupHost will be done unconditionally. IDC if it stays, but unless there's a reason to call this outside of a job, maybe we should allow the RemoveInactiveJob to do the magic? Thoughts? > @@ -4515,6 +4517,7 @@ qemuMigrationSrcPerformJob(virQEMUDriverPtr driver, > if (!virDomainObjIsActive(vm) && ret == 0) { > if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) { > virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); > + qemuExtDevicesCleanupHost(driver, vm->def); Similar here. Which then leaves just qemuDomainRemoveInactive doing that cleanup!

> vm->persistent = 0; > } > qemuDomainRemoveInactiveJob(driver, vm); Beyond that - looks good to me... Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt;

John