[PATCH v5 05/10] conf, qemu, security, tests: introducing 'def->tpms' array
Daniel Henrique Barboza
danielhb413 at gmail.com
Wed May 27 18:57:36 UTC 2020
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 at linux.ibm.com>
>
>
More information about the libvir-list
mailing list