Re: [PATCH v5 05/10] conf, qemu, security, tests: introducing 'def->tpms' array

On 5/21/20 9:07 AM, Daniel Henrique Barboza wrote: > A TPM Proxy device can coexist with a regular TPM, but the

> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c > index 07431343ed..4c26070022 100644 > --- a/src/qemu/qemu_domain_address.c > +++ b/src/qemu/qemu_domain_address.c > @@ -268,10 +268,13 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def) >               return -1; >       } > -    if (def->tpm) { > -        if (qemuDomainIsPSeries(def)) > -            def->tpm->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; > -        if (qemuDomainAssignSpaprVIOAddress(def, &def->tpm->info, > +    for (i = 0; i < def->ntpms; i++) { > +        virDomainTPMDefPtr tpm = def->tpms[i]; > + > +        if (tpm->model != VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY && > +            qemuDomainIsPSeries(def)) > +            tpm->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; > +        if (qemuDomainAssignSpaprVIOAddress(def, &tpm->info, >                                               VIO_ADDR_TPM) < 0) It looks like tike proxy will also get a VIOAddress. Is that necessary?

> + if (tpm->model != VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY && > + qemuDomainIsPSeries(def)) > + tpm->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;

>               return -1; >       } > diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c > index 2ff3f68f5d..db18c82640 100644 > --- a/src/qemu/qemu_extdevice.c > +++ b/src/qemu/qemu_extdevice.c > @@ -73,7 +73,7 @@ static int >   qemuExtDevicesInitPaths(virQEMUDriverPtr driver, >                           virDomainDefPtr def) >   { > -    if (def->tpm) > +    if (def->ntpms > 0) >           return qemuExtTPMInitPaths(driver, def); >       return 0; > @@ -132,7 +132,7 @@ qemuExtDevicesPrepareHost(virQEMUDriverPtr driver, >       virDomainDefPtr def = vm->def; >       size_t i; > -    if (def->tpm && > +    if (def->ntpms > 0 && >           qemuExtTPMPrepareHost(driver, def) < 0) >           return -1; > @@ -155,7 +155,7 @@ qemuExtDevicesCleanupHost(virQEMUDriverPtr driver, >       if (qemuExtDevicesInitPaths(driver, def) < 0) >           return; > -    if (def->tpm) > +    if (def->ntpms > 0) >           qemuExtTPMCleanupHost(def); >   } > @@ -181,7 +181,7 @@ qemuExtDevicesStart(virQEMUDriverPtr driver, >           } >       } > -    if (def->tpm && qemuExtTPMStart(driver, vm, incomingMigration) < 0) > +    if (def->ntpms > 0 && qemuExtTPMStart(driver, vm, incomingMigration) < 0) >           return -1; >       for (i = 0; i < def->nnets; i++) { > @@ -223,7 +223,7 @@ qemuExtDevicesStop(virQEMUDriverPtr driver, >               qemuExtVhostUserGPUStop(driver, vm, video); >       } > -    if (def->tpm) > +    if (def->ntpms > 0) >           qemuExtTPMStop(driver, vm); >       for (i = 0; i < def->nnets; i++) { > @@ -253,8 +253,10 @@ qemuExtDevicesHasDevice(virDomainDefPtr def) >               return true; >       } > -    if (def->tpm && def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) > -        return true; > +    for (i = 0; i < def->ntpms; i++) { > +        if (def->tpms[i]->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) > +            return true; > +    } >       for (i = 0; i < def->nfss; i++) { >           virDomainFSDefPtr fs = def->fss[i]; > @@ -294,7 +296,7 @@ qemuExtDevicesSetupCgroup(virQEMUDriverPtr driver, >               return -1; >       } > -    if (def->tpm && > +    if (def->ntpms > 0 && >           qemuExtTPMSetupCgroup(driver, def, cgroup) < 0) >           return -1; > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c > index afec0e5328..8adb0e42b8 100644 > --- a/src/qemu/qemu_tpm.c > +++ b/src/qemu/qemu_tpm.c > @@ -679,10 +679,15 @@ qemuExtTPMInitPaths(virQEMUDriverPtr driver, >                       virDomainDefPtr def) >   { >       g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); > +    size_t i; > -    if (def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) > -        return qemuTPMEmulatorInitPaths(def->tpm, cfg->swtpmStorageDir, > +    for (i = 0; i < def->ntpms; i++) { > +        if (def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) > +            continue; > + > +        return qemuTPMEmulatorInitPaths(def->tpms[i], cfg->swtpmStorageDir, >                                           def->uuid); > +    } >       return 0; >   } > @@ -694,13 +699,17 @@ qemuExtTPMPrepareHost(virQEMUDriverPtr driver, >   { >       g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); >       g_autofree char *shortName = NULL; > +    size_t i; > + > +    for (i = 0; i < def->ntpms; i++) { > +        if (def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) > +            continue; > -    if (def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) { >           shortName = virDomainDefGetShortName(def); >           if (!shortName) >               return -1; > -        return qemuTPMEmulatorPrepareHost(def->tpm, cfg->swtpmLogDir, > +        return qemuTPMEmulatorPrepareHost(def->tpms[i], cfg->swtpmLogDir, >                                             def->name, cfg->swtpm_user, >                                             cfg->swtpm_group, >                                             cfg->swtpmStateDir, cfg->user, > @@ -714,8 +723,14 @@ qemuExtTPMPrepareHost(virQEMUDriverPtr driver, >   void >   qemuExtTPMCleanupHost(virDomainDefPtr def) >   { > -    if (def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) > -        qemuTPMDeleteEmulatorStorage(def->tpm); > +    size_t i; > + > +    for (i = 0; i < def->ntpms; i++) { > +        if (def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) > +            continue; > + > +        qemuTPMDeleteEmulatorStorage(def->tpms[i]); > +    } >   } > @@ -733,13 +748,13 @@ qemuExtTPMCleanupHost(virDomainDefPtr def) >   static int >   qemuExtTPMStartEmulator(virQEMUDriverPtr driver, >                           virDomainObjPtr vm, > +                        virDomainTPMDefPtr tpm, >                           bool incomingMigration) >   { >       g_autoptr(virCommand) cmd = NULL; >       int exitstatus = 0; >       g_autofree char *errbuf = NULL; >       g_autoptr(virQEMUDriverConfig) cfg = NULL; > -    virDomainTPMDefPtr tpm = vm->def->tpm; >       g_autofree char *shortName = virDomainDefGetShortName(vm->def); >       int cmdret = 0, timeout, rc; >       pid_t pid; > @@ -807,10 +822,15 @@ qemuExtTPMStart(virQEMUDriverPtr driver, >                   virDomainObjPtr vm, >                   bool incomingMigration) >   { > -    virDomainTPMDefPtr tpm = vm->def->tpm; > +    size_t i; > + > +    for (i = 0; i < vm->def->ntpms; i++) { > +        if (vm->def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) > +            continue; > -    if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) > -        return qemuExtTPMStartEmulator(driver, vm, incomingMigration); > +        return qemuExtTPMStartEmulator(driver, vm, vm->def->tpms[i], > +                                       incomingMigration); > +    } >       return 0; >   } > @@ -822,8 +842,12 @@ qemuExtTPMStop(virQEMUDriverPtr driver, >   { >       g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); >       g_autofree char *shortName = NULL; > +    size_t i; > + > +    for (i = 0; i < vm->def->ntpms; i++) { > +        if (vm->def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) > +            continue; > -    if (vm->def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) { >           shortName = virDomainDefGetShortName(vm->def); >           if (!shortName) >               return; > @@ -845,8 +869,12 @@ qemuExtTPMSetupCgroup(virQEMUDriverPtr driver, >       g_autofree char *shortName = NULL; >       int rc; >       pid_t pid; > +    size_t i; > + > +    for (i = 0; i < def->ntpms; i++) { > +        if (def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) > +            continue; > -    if (def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) { >           shortName = virDomainDefGetShortName(def); >           if (!shortName) >               return -1; > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > index bdc2d7edf3..79123f384c 100644 > --- a/src/security/security_dac.c > +++ b/src/security/security_dac.c > @@ -1973,10 +1973,10 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr, >                                  &chardevData) < 0) >           rc = -1; > -    if (def->tpm) { > +    for (i = 0; i < def->ntpms; i++) { >           if (virSecurityDACRestoreTPMFileLabel(mgr, >                                                 def, > -                                              def->tpm) < 0) > +                                              def->tpms[i]) < 0) >               rc = -1; >       } > @@ -2152,10 +2152,10 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, >                                  &chardevData) < 0) >           return -1; > -    if (def->tpm) { > +    for (i = 0; i < def->ntpms; i++) { >           if (virSecurityDACSetTPMFileLabel(mgr, >                                             def, > -                                          def->tpm) < 0) > +                                          def->tpms[i]) < 0) >               return -1; >       } > diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c > index 914a252df1..39928aef3e 100644 > --- a/src/security/security_selinux.c > +++ b/src/security/security_selinux.c > @@ -2758,8 +2758,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, >               return -1; >       } > -    if (def->tpm) { > -        if (virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, def->tpm) < 0) > +    for (i = 0; i < def->ntpms; i++) { > +        if (virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, def->tpms[i]) < 0) >               rc = -1; >       } > @@ -3166,8 +3166,8 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr, >               return -1; >       } > -    if (def->tpm) { > -        if (virSecuritySELinuxSetTPMFileLabel(mgr, def, def->tpm) < 0) > +    for (i = 0; i < def->ntpms; i++) { > +        if (virSecuritySELinuxSetTPMFileLabel(mgr, def, def->tpms[i]) < 0) >               return -1; >       } > @@ -3487,19 +3487,23 @@ virSecuritySELinuxSetTPMLabels(virSecurityManagerPtr mgr, >                                  virDomainDefPtr def) >   { >       int ret = 0; > +    size_t i; >       virSecurityLabelDefPtr seclabel; >       seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); >       if (seclabel == NULL) >           return 0; > -    if (def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) { > +    for (i = 0; i < def->ntpms; i++) { > +        if (def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) > +            continue; > + >           ret = virSecuritySELinuxSetFileLabels( > -            mgr, def->tpm->data.emulator.storagepath, > +            mgr, def->tpms[i]->data.emulator.storagepath, >               seclabel); > -        if (ret == 0 && def->tpm->data.emulator.logfile) > +        if (ret == 0 && def->tpms[i]->data.emulator.logfile) >               ret = virSecuritySELinuxSetFileLabels( > -                mgr, def->tpm->data.emulator.logfile, > +                mgr, def->tpms[i]->data.emulator.logfile, >                   seclabel); >       } > @@ -3512,13 +3516,17 @@ virSecuritySELinuxRestoreTPMLabels(virSecurityManagerPtr mgr, >                                      virDomainDefPtr def) >   { >       int ret = 0; > +    size_t i; > + > +    for (i = 0; i < def->ntpms; i++) { > +        if (def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) > +            continue; > -    if (def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) { >           ret = virSecuritySELinuxRestoreFileLabels( > -            mgr, def->tpm->data.emulator.storagepath); > -        if (ret == 0 && def->tpm->data.emulator.logfile) > +            mgr, def->tpms[i]->data.emulator.storagepath); > +        if (ret == 0 && def->tpms[i]->data.emulator.logfile) >               ret = virSecuritySELinuxRestoreFileLabels( > -                mgr, def->tpm->data.emulator.logfile); > +                mgr, def->tpms[i]->data.emulator.logfile); >       } >       return ret; > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > index 6e8f77e4dd..7abb6e70be 100644 > --- a/src/security/virt-aa-helper.c > +++ b/src/security/virt-aa-helper.c > @@ -1206,14 +1206,17 @@ get_files(vahControl * ctl) >       } > -    if (ctl->def->tpm) { > +    if (ctl->def->ntpms > 0) { >           char *shortName = NULL; >           const char *tpmpath = NULL; > -        if (ctl->def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) { > +        for (i = 0; i < ctl->def->ntpms; i++) { > +            if (ctl->def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) > +                continue; > + >               shortName = virDomainDefGetShortName(ctl->def); > -            switch (ctl->def->tpm->version) { > +            switch (ctl->def->tpms[i]->version) { >               case VIR_DOMAIN_TPM_VERSION_1_2: >                   tpmpath = "tpm1.2"; >                   break; > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index c40ce64cbf..5b27cf9ae4 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -437,12 +437,13 @@ testCompareXMLToArgvCreateArgs(virQEMUDriverPtr drv, >           vsockPriv->vhostfd = 6789; >       } > -    if (vm->def->tpm) { > -        if (vm->def->tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) { > -            VIR_FREE(vm->def->tpm->data.emulator.source.data.file.path); > -            vm->def->tpm->data.emulator.source.data.file.path = g_strdup("/dev/test"); > -            vm->def->tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_FILE; > -       } > +    for (i = 0; i < vm->def->ntpms; i++) { > +        if (vm->def->tpms[i]->type != VIR_DOMAIN_TPM_TYPE_EMULATOR) > +            continue; > + > +        VIR_FREE(vm->def->tpms[i]->data.emulator.source.data.file.path); > +        vm->def->tpms[i]->data.emulator.source.data.file.path = g_strdup("/dev/test"); > +        vm->def->tpms[i]->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_FILE; >       } >       for (i = 0; i < vm->def->nvideos; i++) { Maybe you need to address the comment above: Reviewed-by: Stefan Berger <stefanb(a)linux.ibm.com&gt;