[PATCH] conf: Set SPAPR TPM default to 2.0 and prevent 1.2 choice
From: Stefan Berger <stefanb@linux.ibm.com> The firmware (SLOF) on QEMU for ppc64 does not support TPM 1.2, so prevent the choice of TPM 1.2 when the SPAPR device model is chosen and use a default of '2.0' (TPM 2) for the backend. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_validate.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index bd7590a00a..f4d71aebf5 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3645,8 +3645,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, virQEMUCapsFlags flag; /* TPM 1.2 and 2 are not compatible, so we choose a specific version here */ - if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) - tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) { + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) + tpm->version = VIR_DOMAIN_TPM_VERSION_2_0; + else + tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + } switch (tpm->version) { case VIR_DOMAIN_TPM_VERSION_1_2: @@ -3658,6 +3662,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, virDomainTPMModelTypeToString(tpm->model)); return -1; } + /* TPM 1.2 + SPAPR do not work with any 'type' (backend) */ + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TPM 1.2 is not supported with the SPAPR device model")); + return -1; + } break; case VIR_DOMAIN_TPM_VERSION_2_0: case VIR_DOMAIN_TPM_VERSION_DEFAULT: -- 2.17.1
On Wed, Jul 08, 2020 at 09:57:56 -0400, Stefan Berger wrote:
From: Stefan Berger <stefanb@linux.ibm.com>
The firmware (SLOF) on QEMU for ppc64 does not support TPM 1.2, so prevent the choice of TPM 1.2 when the SPAPR device model is chosen and use a default of '2.0' (TPM 2) for the backend.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_validate.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index bd7590a00a..f4d71aebf5 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3645,8 +3645,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, virQEMUCapsFlags flag;
/* TPM 1.2 and 2 are not compatible, so we choose a specific version here */ - if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) - tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) { + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) + tpm->version = VIR_DOMAIN_TPM_VERSION_2_0; + else + tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + }
This is the validation callback, which must not modify the config. qemuDomainDefTPMsPostParse is the correct place. Beaware that the function has a short-circuit condition at the beginning.
switch (tpm->version) { case VIR_DOMAIN_TPM_VERSION_1_2: @@ -3658,6 +3662,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, virDomainTPMModelTypeToString(tpm->model)); return -1; } + /* TPM 1.2 + SPAPR do not work with any 'type' (backend) */ + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TPM 1.2 is not supported with the SPAPR device model")); + return -1; + }
This is okay as it validates the configuration.
break; case VIR_DOMAIN_TPM_VERSION_2_0: case VIR_DOMAIN_TPM_VERSION_DEFAULT: -- 2.17.1
On 7/8/20 10:10 AM, Peter Krempa wrote:
On Wed, Jul 08, 2020 at 09:57:56 -0400, Stefan Berger wrote:
From: Stefan Berger <stefanb@linux.ibm.com>
The firmware (SLOF) on QEMU for ppc64 does not support TPM 1.2, so prevent the choice of TPM 1.2 when the SPAPR device model is chosen and use a default of '2.0' (TPM 2) for the backend.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_validate.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index bd7590a00a..f4d71aebf5 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3645,8 +3645,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, virQEMUCapsFlags flag;
/* TPM 1.2 and 2 are not compatible, so we choose a specific version here */ - if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) - tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) { + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) + tpm->version = VIR_DOMAIN_TPM_VERSION_2_0; + else + tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + } This is the validation callback, which must not modify the config.
qemuDomainDefTPMsPostParse is the correct place. Beaware that the function has a short-circuit condition at the beginning.
So should I remove the existing code that already set the version to VIR_DOMAIN_TPM_VERSION_1_2 and move this into that other function?
switch (tpm->version) { case VIR_DOMAIN_TPM_VERSION_1_2: @@ -3658,6 +3662,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, virDomainTPMModelTypeToString(tpm->model)); return -1; } + /* TPM 1.2 + SPAPR do not work with any 'type' (backend) */ + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("TPM 1.2 is not supported with the SPAPR device model")); + return -1; + }
This is okay as it validates the configuration.
break; case VIR_DOMAIN_TPM_VERSION_2_0: case VIR_DOMAIN_TPM_VERSION_DEFAULT: -- 2.17.1
On Wed, Jul 08, 2020 at 10:19:11 -0400, Stefan Berger wrote:
On 7/8/20 10:10 AM, Peter Krempa wrote:
On Wed, Jul 08, 2020 at 09:57:56 -0400, Stefan Berger wrote:
From: Stefan Berger <stefanb@linux.ibm.com>
The firmware (SLOF) on QEMU for ppc64 does not support TPM 1.2, so prevent the choice of TPM 1.2 when the SPAPR device model is chosen and use a default of '2.0' (TPM 2) for the backend.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_validate.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index bd7590a00a..f4d71aebf5 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3645,8 +3645,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, virQEMUCapsFlags flag; /* TPM 1.2 and 2 are not compatible, so we choose a specific version here */ - if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) - tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) { + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) + tpm->version = VIR_DOMAIN_TPM_VERSION_2_0; + else + tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + } This is the validation callback, which must not modify the config.
qemuDomainDefTPMsPostParse is the correct place. Beaware that the function has a short-circuit condition at the beginning.
So should I remove the existing code that already set the version to VIR_DOMAIN_TPM_VERSION_1_2 and move this into that other function?
Ideally yes as a refactor prior to your change. The XML parser shouldn't really be setting defaults or validating any form of interdependance of values.
On 7/8/20 10:22 AM, Peter Krempa wrote:
On Wed, Jul 08, 2020 at 10:19:11 -0400, Stefan Berger wrote:
On 7/8/20 10:10 AM, Peter Krempa wrote:
On Wed, Jul 08, 2020 at 09:57:56 -0400, Stefan Berger wrote:
From: Stefan Berger <stefanb@linux.ibm.com>
The firmware (SLOF) on QEMU for ppc64 does not support TPM 1.2, so prevent the choice of TPM 1.2 when the SPAPR device model is chosen and use a default of '2.0' (TPM 2) for the backend.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_validate.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index bd7590a00a..f4d71aebf5 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3645,8 +3645,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, virQEMUCapsFlags flag; /* TPM 1.2 and 2 are not compatible, so we choose a specific version here */ - if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) - tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) { + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) + tpm->version = VIR_DOMAIN_TPM_VERSION_2_0; + else + tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + } This is the validation callback, which must not modify the config.
qemuDomainDefTPMsPostParse is the correct place. Beaware that the function has a short-circuit condition at the beginning.
So should I remove the existing code that already set the version to VIR_DOMAIN_TPM_VERSION_1_2 and move this into that other function? Ideally yes as a refactor prior to your change. The XML parser shouldn't really be setting defaults or validating any form of interdependance of values.
Well, the issue is that the validation function works on a single device (TPM) config but for validating a correct configuration for the SPAPR proxy device we would need to see both configs. So this is presumably the reason why this was done in the post parse function that has visibility into all TPM configs.
On Wed, Jul 08, 2020 at 10:41:11 -0400, Stefan Berger wrote:
On 7/8/20 10:22 AM, Peter Krempa wrote:
On Wed, Jul 08, 2020 at 10:19:11 -0400, Stefan Berger wrote:
On 7/8/20 10:10 AM, Peter Krempa wrote:
On Wed, Jul 08, 2020 at 09:57:56 -0400, Stefan Berger wrote:
From: Stefan Berger <stefanb@linux.ibm.com>
The firmware (SLOF) on QEMU for ppc64 does not support TPM 1.2, so prevent the choice of TPM 1.2 when the SPAPR device model is chosen and use a default of '2.0' (TPM 2) for the backend.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_validate.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index bd7590a00a..f4d71aebf5 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3645,8 +3645,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm, virQEMUCapsFlags flag; /* TPM 1.2 and 2 are not compatible, so we choose a specific version here */ - if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) - tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) { + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) + tpm->version = VIR_DOMAIN_TPM_VERSION_2_0; + else + tpm->version = VIR_DOMAIN_TPM_VERSION_1_2; + } This is the validation callback, which must not modify the config.
qemuDomainDefTPMsPostParse is the correct place. Beaware that the function has a short-circuit condition at the beginning.
So should I remove the existing code that already set the version to VIR_DOMAIN_TPM_VERSION_1_2 and move this into that other function? Ideally yes as a refactor prior to your change. The XML parser shouldn't really be setting defaults or validating any form of interdependance of values.
Well, the issue is that the validation function works on a single device (TPM) config but for validating a correct configuration for the SPAPR proxy device we would need to see both configs. So this is presumably the reason why this was done in the post parse function that has visibility into all TPM configs.
It is okay to do validation in PostParse, if it's being added as new code, which never worked before. The difference between PostParse and Validate is that PostParse is called always after parsing an XML. This means even those meant for internal use such as status XMLs. Failing to parse an existing one would be bad as a VM would vanish after libvirtd restart. The Validate callbacks are selectively used only in cases when a new XML is used. This is e.g. when defining via the API or any other place where we can return reasonable error to the user. In the Validate code we can reject even previously accepted configs as there is no problem with vanishing VMs. Thus as per the above, you can leave the validation in the PostParse callback, but you must not add any new one which was accepted previously and there might be VMs running.
participants (3)
-
Peter Krempa -
Stefan Berger -
Stefan Berger