Hi Peter,
Thank you for your comments. A question
1. Here is one code snippet where you indicated the Formatting is broken.
If I understood correctly, I must not call virReportError() - but simply
goto the cleanup like the last condition where you seem to be fine ?
__start
if (def->ntpms > 1) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("VMware only supports a single TPM device"));
goto cleanup;
}
if (virtualHW_version < 14) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("vTPM requires virtual hardware version 14 or
higher"));
goto cleanup;
}
if (def->os.firmware !=
VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("vTPM requires EFI firmware"));
goto cleanup;
}
__end
On Wed, Apr 29, 2026 at 1:56 PM Peter Krempa <[email protected]> wrote:
> On Thu, Apr 23, 2026 at 17:48:17 +0530, Srihari Parimi via Devel wrote:
> > Parses vtpm.present from VMX files and converts to libvirt TPM
> > device with CRB model and emulator backend. VMware vTPM uses
> > TPM 2.0 as specified in the document below
> >
> >
> https://techdocs.broadcom.com/us/en/vmware-cis/vsphere/vsphere/8-0/vsphere-security/securing-virtual-machines-with-virtual-trusted-platform-module/vtpm-overview.html
> >
> > implement support for formatting TPM devices in virVMXFormatConfig
> >
> > Signed-off-by: Srihari Parimi <[email protected]>
> > ---
> > src/vmx/vmx.c | 74 ++++++++++++++++++++++++++++++++++++++
> > tests/vmx2xmldata/vtpm.vmx | 22 ++++++++++++
> > tests/vmx2xmldata/vtpm.xml | 32 +++++++++++++++++
> > tests/vmx2xmltest.c | 2 ++
> > 4 files changed, 130 insertions(+)
> > create mode 100644 tests/vmx2xmldata/vtpm.vmx
> > create mode 100644 tests/vmx2xmldata/vtpm.xml
> >
> > diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> > index 57dfd57cfc..977fd3b346 100644
> > --- a/src/vmx/vmx.c
> > +++ b/src/vmx/vmx.c
> > @@ -599,6 +599,7 @@ static int virVMXParseSerial(virVMXContext *ctx,
> virConf *conf, int port,
> > static int virVMXParseParallel(virVMXContext *ctx, virConf *conf, int
> port,
> > virDomainChrDef **def);
> > static int virVMXParseSVGA(virConf *conf, virDomainVideoDef **def);
> > +static int virVMXParseTPM(virConf *conf, virDomainTPMDef **def);
> >
> > static int virVMXFormatVNC(virDomainGraphicsDef *def, virBuffer
> *buffer);
> > static int virVMXFormatDisk(virVMXContext *ctx, virDomainDiskDef *def,
> > @@ -609,6 +610,7 @@ static int virVMXFormatFileSystem(virDomainFSDef
> *def, int number,
> > virBuffer *buffer);
> > static int virVMXFormatEthernet(virDomainNetDef *def, int controller,
> > virBuffer *buffer, int
> virtualHW_version);
> > +static int virVMXFormatTPM(virDomainTPMDef *def, virBuffer *buffer);
> > static int virVMXFormatSerial(virVMXContext *ctx, virDomainChrDef *def,
> > virBuffer *buffer);
> > static int virVMXFormatParallel(virVMXContext *ctx, virDomainChrDef
> *def,
> > @@ -1938,6 +1940,15 @@ virVMXParseConfig(virVMXContext *ctx,
> >
> > def->nvideos = 1;
> >
> > + /* def:tpms */
> > + {
> > + virDomainTPMDef *tpm = NULL;
> > + if (virVMXParseTPM(conf, &tpm) < 0)
> > + goto cleanup;
> > + if (tpm)
> > + VIR_APPEND_ELEMENT(def->tpms, def->ntpms, tpm);
> > + }
> > +
> > /* def:sounds */
> > /* FIXME */
> >
> > @@ -3367,6 +3378,27 @@ virVMXParseSVGA(virConf *conf, virDomainVideoDef
> **def)
> > return result;
> > }
> >
> > +static int
> > +virVMXParseTPM(virConf *conf, virDomainTPMDef **def)
> > +{
> > + bool vtpm_present = false;
> > +
> > + /* vmx:vtpm.present */
> > + if (virVMXGetConfigBoolean(conf, "vtpm.present", &vtpm_present,
> > + false, true) < 0) {
> > + return -1;
> > + }
> > +
> > + if (!vtpm_present)
> > + return 0;
> > +
> > + *def = g_new0(virDomainTPMDef, 1);
> > + (*def)->type = VIR_DOMAIN_TPM_TYPE_EMULATOR;
> > + (*def)->model = VIR_DOMAIN_TPM_MODEL_CRB;
> > + (*def)->data.emulator.version = VIR_DOMAIN_TPM_VERSION_2_0;
> > +
> > + return 0;
> > +}
> >
> >
> > /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> * * * *
> > @@ -3701,6 +3733,31 @@ virVMXFormatConfig(virVMXContext *ctx,
> virDomainXMLOption *xmlopt, virDomainDef
> > goto cleanup;
> > }
> >
> > + /* def:vTPM */
> > + if (def->ntpms > 0) {
> > + /* Validate TPM requirements */
> > + if (def->ntpms > 1) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("VMware only supports a single TPM device"));
>
> Formatting is broken here ....
>
> > + goto cleanup;
> > + }
> > +
> > + if (virtualHW_version < 14) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("vTPM requires virtual hardware version 14 or
> higher"));
>
> ... here ...
>
> > + goto cleanup;
> > + }
> > +
> > + if (def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("vTPM requires EFI firmware"));
>
> .... here.
>
> Also
>
> > + goto cleanup;
> > + }
> > +
> > + if (virVMXFormatTPM(def->tpms[0], &buffer) < 0)
> > + goto cleanup;
> > + }
> > +
> > /* def:inputs */
> > /* FIXME */
> >
> > @@ -4231,6 +4288,23 @@ virVMXFormatEthernet(virDomainNetDef *def, int
> controller,
> > }
> >
> >
> > +static int virVMXFormatTPM(virDomainTPMDef *def, virBuffer *buffer)
>
> The coding style is off here too ...
>
> > +{
> > + /* VMware vTPM specifically requires TPM 2.0 */
> > + if (def->model != VIR_DOMAIN_TPM_MODEL_CRB ||
> > + def->type != VIR_DOMAIN_TPM_TYPE_EMULATOR ||
> > + def->data.emulator.version != VIR_DOMAIN_TPM_VERSION_2_0) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("VMware driver only supports TPM 2.0 with the
> CRB model"));
> > + return -1;
> > + }
> > +
> > + /* VMX parameters for vTPM */
> > + virBufferAddLit(buffer, "vtpm.present = \"TRUE\"\n");
> > +
> > + return 0;
> > +}
> > +
> >
> > static int
> > virVMXFormatSerial(virVMXContext *ctx, virDomainChrDef *def,
>
> ... ^^^.
>
> I also don't quite understand why the validation is split partly between
> virVMXFormatConfig and virVMXFormatTPM. E.g. if you have most of checks
> in virVMXFormatConfig adding the last check and the formatting wouldn't
> make it much worse. Same way you could put all the checks into
> virVMXFormatTPM instead.
>
> With the coding style fixed and all validation moved into
> virVMXFormatTPM:
>
> Reviewed-by: Peter Krempa <[email protected]>
>
>
