[PATCH 0/4] qemu: Fix tpm-tis for armv7l and riscv
This fixes tpm-tis usage for armv7l and riscv arches, and then switches qemu tpm validation to use domcaps as the source of truth Cole Robinson (4): qemu: validate: Drop tpm-tis arch validation qemu: command: Use correct tpm device for all non-x86 tests: mock swtpm initialization for all qemu tests qemu: validate: use domcaps for tpm validation src/qemu/qemu_command.c | 2 +- src/qemu/qemu_validate.c | 77 +++++++++------------------------------- tests/domaincapstest.c | 7 ---- tests/testutilsqemu.c | 8 +++++ 4 files changed, 26 insertions(+), 68 deletions(-) -- 2.36.1
Checking against qemu capabilities should be enough here Resolves: https://gitlab.com/libvirt/libvirt/-/issues/321 Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_validate.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index f8aa83c1cb..db47fcaa9c 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4798,12 +4798,6 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, switch (tpm->model) { case VIR_DOMAIN_TPM_MODEL_TIS: - if (!ARCH_IS_X86(def->os.arch) && (def->os.arch != VIR_ARCH_AARCH64)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("TPM model '%s' is only available for x86 and aarch64 guests"), - virDomainTPMModelTypeToString(tpm->model)); - return -1; - } flag = QEMU_CAPS_DEVICE_TPM_TIS; break; case VIR_DOMAIN_TPM_MODEL_CRB: -- 2.36.1
On 6/18/22 2:32 PM, Cole Robinson wrote:
Checking against qemu capabilities should be enough here
Should be https://gitlab.com/libvirt/libvirt/-/issues/329 I've fixed locally
The qemu `tpm-tis` device is an ISA device, so only really applicable to x86 archs. For all non-x86 archs we should use `tpm-tis-device` This fixes tpm-tis usage on armv7l and riscv Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 57334ab246..b307d3139c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9731,7 +9731,7 @@ qemuBuildTPMDevCmd(virCommand *cmd, const char *model = virDomainTPMModelTypeToString(tpm->model); g_autofree char *tpmdev = g_strdup_printf("tpm-%s", tpm->info.alias); - if (tpm->model == VIR_DOMAIN_TPM_MODEL_TIS && def->os.arch == VIR_ARCH_AARCH64) + if (tpm->model == VIR_DOMAIN_TPM_MODEL_TIS && !ARCH_IS_X86(def->os.arch)) model = "tpm-tis-device"; if (virJSONValueObjectAdd(&props, -- 2.36.1
Don't restrict this to domcaps testing only, we will soon need it for qemu command line validation Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/domaincapstest.c | 7 ------- tests/testutilsqemu.c | 8 ++++++++ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index da5c629fd4..3b8216a8f6 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -22,7 +22,6 @@ #include "domain_capabilities.h" #include "virfilewrapper.h" #include "configmake.h" -#include "virtpm.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -129,12 +128,6 @@ fillQemuCaps(virDomainCaps *domCaps, } -/* Enough to tell capabilities code that swtpm is usable */ -bool virTPMHasSwtpm(void) -{ - return true; -} - #endif /* WITH_QEMU */ diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index d26caba5fb..6dabbaf36a 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -11,6 +11,7 @@ # include "qemu/qemu_capspriv.h" # include "virstring.h" # include "virfilecache.h" +# include "virtpm.h" # include <sys/types.h> # include <fcntl.h> @@ -138,6 +139,13 @@ virFindFileInPath(const char *file) } +/* Enough to tell capabilities code that swtpm is usable */ +bool virTPMHasSwtpm(void) +{ + return true; +} + + virCapsHostNUMA * virCapabilitiesHostNUMANewHost(void) { -- 2.36.1
Replace tpm->type and tpm->model qemuCaps validation with the similar logic in domcaps. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_validate.c | 71 ++++++++++------------------------------ 1 file changed, 17 insertions(+), 54 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index db47fcaa9c..39210ba65b 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4750,7 +4750,7 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, const virDomainDef *def, virQEMUCaps *qemuCaps) { - virQEMUCapsFlags flag; + virDomainCapsDeviceTPM tpmCaps = { 0 }; switch (tpm->version) { case VIR_DOMAIN_TPM_VERSION_1_2: @@ -4781,57 +4781,28 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, break; } - switch (tpm->type) { - case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_PASSTHROUGH)) - goto no_support; - break; - - case VIR_DOMAIN_TPM_TYPE_EMULATOR: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_EMULATOR)) - goto no_support; + virQEMUCapsFillDomainDeviceTPMCaps(qemuCaps, &tpmCaps); - break; - case VIR_DOMAIN_TPM_TYPE_LAST: - break; + if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.backendModel, tpm->type)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The QEMU executable %s does not support TPM " + "backend type %s"), + def->emulator, + virDomainTPMBackendTypeToString(tpm->type)); + return -1; } - switch (tpm->model) { - case VIR_DOMAIN_TPM_MODEL_TIS: - flag = QEMU_CAPS_DEVICE_TPM_TIS; - break; - case VIR_DOMAIN_TPM_MODEL_CRB: - flag = QEMU_CAPS_DEVICE_TPM_CRB; - break; - case VIR_DOMAIN_TPM_MODEL_SPAPR: - flag = QEMU_CAPS_DEVICE_TPM_SPAPR; - break; - case VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY: - if (!ARCH_IS_PPC64(def->os.arch)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("TPM Proxy model %s is only available for " - "PPC64 guests"), - virDomainTPMModelTypeToString(tpm->model)); - return -1; - } - - /* TPM Proxy devices have 'passthrough' backend */ - if (tpm->type != VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("TPM Proxy model %s requires " - "'Passthrough' backend"), - virDomainTPMModelTypeToString(tpm->model)); - } - - flag = QEMU_CAPS_DEVICE_SPAPR_TPM_PROXY; - break; - case VIR_DOMAIN_TPM_MODEL_LAST: - default: - virReportEnumRangeError(virDomainTPMModel, tpm->model); + if (ARCH_IS_PPC64(def->os.arch) && + tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY && + tpm->type != VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("TPM Proxy model %s requires " + "'Passthrough' backend"), + virDomainTPMModelTypeToString(tpm->model)); return -1; } - if (!virQEMUCapsGet(qemuCaps, flag)) { + if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.model, tpm->model)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("The QEMU executable %s does not support TPM " "model %s"), @@ -4841,14 +4812,6 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, } return 0; - - no_support: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("The QEMU executable %s does not support TPM " - "backend type %s"), - def->emulator, - virDomainTPMBackendTypeToString(tpm->type)); - return -1; } -- 2.36.1
On 6/18/22 20:32, Cole Robinson wrote:
Replace tpm->type and tpm->model qemuCaps validation with the similar logic in domcaps.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_validate.c | 71 ++++++++++------------------------------ 1 file changed, 17 insertions(+), 54 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index db47fcaa9c..39210ba65b 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4750,7 +4750,7 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, const virDomainDef *def, virQEMUCaps *qemuCaps) { - virQEMUCapsFlags flag; + virDomainCapsDeviceTPM tpmCaps = { 0 };
switch (tpm->version) { case VIR_DOMAIN_TPM_VERSION_1_2: @@ -4781,57 +4781,28 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, break; }
- switch (tpm->type) { - case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_PASSTHROUGH)) - goto no_support; - break; - - case VIR_DOMAIN_TPM_TYPE_EMULATOR: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_EMULATOR)) - goto no_support; + virQEMUCapsFillDomainDeviceTPMCaps(qemuCaps, &tpmCaps);
- break; - case VIR_DOMAIN_TPM_TYPE_LAST: - break; + if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.backendModel, tpm->type)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The QEMU executable %s does not support TPM " + "backend type %s"), + def->emulator, + virDomainTPMBackendTypeToString(tpm->type));
Whoa, very nice idea! And looking around the file I can see it used already. How could this slipped by me? I mean, the more I think about it the more possibilities for code deduplication I see. And on the flip side - we would be motivated to keep domcaps on the bleeding edge. Michal
On 6/21/22 4:11 AM, Michal Prívozník wrote:
On 6/18/22 20:32, Cole Robinson wrote:
Replace tpm->type and tpm->model qemuCaps validation with the similar logic in domcaps.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_validate.c | 71 ++++++++++------------------------------ 1 file changed, 17 insertions(+), 54 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index db47fcaa9c..39210ba65b 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -4750,7 +4750,7 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, const virDomainDef *def, virQEMUCaps *qemuCaps) { - virQEMUCapsFlags flag; + virDomainCapsDeviceTPM tpmCaps = { 0 };
switch (tpm->version) { case VIR_DOMAIN_TPM_VERSION_1_2: @@ -4781,57 +4781,28 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, break; }
- switch (tpm->type) { - case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_PASSTHROUGH)) - goto no_support; - break; - - case VIR_DOMAIN_TPM_TYPE_EMULATOR: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_EMULATOR)) - goto no_support; + virQEMUCapsFillDomainDeviceTPMCaps(qemuCaps, &tpmCaps);
- break; - case VIR_DOMAIN_TPM_TYPE_LAST: - break; + if (!VIR_DOMAIN_CAPS_ENUM_IS_SET(tpmCaps.backendModel, tpm->type)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The QEMU executable %s does not support TPM " + "backend type %s"), + def->emulator, + virDomainTPMBackendTypeToString(tpm->type));
Whoa, very nice idea! And looking around the file I can see it used already. How could this slipped by me? I mean, the more I think about it the more possibilities for code deduplication I see. And on the flip side - we would be motivated to keep domcaps on the bleeding edge.
Heh, it's your code :) https://listman.redhat.com/archives/libvir-list/2020-November/211844.html But yes I agree. domcaps can be a detriment to apps if it's not reliable, and duplicating the qemuCaps checking is always going to lead to issues like this. It would be nice if we could normalize adding domcaps coverage for basic qemuCaps validation cases like this. Thanks, Cole
On 6/18/22 20:32, Cole Robinson wrote:
This fixes tpm-tis usage for armv7l and riscv arches, and then switches qemu tpm validation to use domcaps as the source of truth
Cole Robinson (4): qemu: validate: Drop tpm-tis arch validation qemu: command: Use correct tpm device for all non-x86 tests: mock swtpm initialization for all qemu tests qemu: validate: use domcaps for tpm validation
src/qemu/qemu_command.c | 2 +- src/qemu/qemu_validate.c | 77 +++++++++------------------------------- tests/domaincapstest.c | 7 ---- tests/testutilsqemu.c | 8 +++++ 4 files changed, 26 insertions(+), 68 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Cole Robinson -
Michal Prívozník