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

[Daniel Henrique Barboza <danielhb413@xxxxxxxxx>]

On 5/27/20 3:42 PM, Stefan Berger wrote:
> 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?


It won't. qemuDomainAssignSpaprVIOAddress() does a check for info.type equals
SPAPRVIO, being a NO-OP in case it doesn't:


static int
qemuDomainAssignSpaprVIOAddress(virDomainDefPtr def,
                                virDomainDeviceInfoPtr info,
                                unsigned long long default_reg)
{
    bool user_reg;
    int ret;

    if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO)
        return 0;
(...)


This chunk of code will prevent the type to be SPAPRVIO for the proxy:


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



At first I wasn't bothering preventing a SPAPRVIO address for the proxy, but
qemu_command.c is adding additional stuff in this case:


    } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
        if (info->addr.spaprvio.has_reg)
            virBufferAsprintf(buf, ",reg=0x%08llx", info->addr.spaprvio.reg);


And this extra argument broke the TPM Proxy tests I've made before.


Thanks,


DHB


> >               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@xxxxxxxxxxxxx>