Re: [PATCH v3 5/9] domain_conf.c: XML parsing for VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY

On a Wednesday in 2020, Daniel Henrique Barboza wrote: > Aside from trivial XML parsing/format changes, this patch adds > additional rules for TPM device support to better accomodate > all the available scenarios with the new TPM Proxy. > > The changes make no impact to existing domains. This means that > the scenario of a domain with a single TPM device is still > supported in the same way.  The restriction of multiple TPM devices > got alleviated to allow a TPM Proxy device to be added together > with a TPM device in the same domain. All other combinations > are still forbidden. > > To summarize, after this patch, the following combinations in the same > domain are valid: > > - a single TPM device > - a single TPM Proxy device > - a single TPM + single TPM Proxy devices > > These combinations in the same domain are NOT allowed: > > - 2 or more TPM devices > - 2 or more TPM Proxy devices > > Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; > --- > src/conf/domain_conf.c | 45 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 41 insertions(+), 4 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 01a32f62d1..8164cd58c9 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -13730,6 +13730,14 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, >         goto error; >     } > > +    /* TPM Proxy devices have 'passthrough' backend */ > +    if (def->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY && > +        def->type != VIR_DOMAIN_TPM_TYPE_PASSTHROUGH) { > +        virReportError(VIR_ERR_XML_ERROR, "%s", > +                       _("'Passthrough' backend is required for TPM Proxy devices")); > +        goto error; > +    } > + This check should be in a Validate function, not the parser.

>     if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) >         goto error; > > @@ -21972,15 +21980,39 @@ virDomainDefParseXML(xmlDocPtr xml, >     if ((n = virXPathNodeSet("./devices/tpm", ctxt, &nodes)) < 0) >         goto error; > > -    if (n > 1) { > +    if (n > 2) { >         virReportError(VIR_ERR_XML_ERROR, "%s", > -                       _("only a single TPM device is supported")); > +                       _("a maximum of two TPM devices is supported, one of " > +                         "them being a TPM Proxy device")); >         goto error; >     } > >     if (n > 0) { > -        if (!(def->tpm = virDomainTPMDefParseXML(xmlopt, nodes[0], ctxt, flags))) > -            goto error; > +        for (i = 0; i < n; i++) { > +            g_autoptr(virDomainTPMDef) dev = NULL; > + > +            if (!(dev = virDomainTPMDefParseXML(xmlopt, nodes[i], ctxt, flags))) > +                goto error; > + > +            /* TPM Proxy devices must be held in def->tpmproxy. Error > +             * out if there's a TPM Proxy declared already */ > +            if (dev->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) { > +                if (def->tpmproxy) { > +                    virReportError(VIR_ERR_XML_ERROR, "%s", > +                                   _("only a single TPM Proxy device is supported")); > +                    goto error; > +                } > +                def->tpmproxy = g_steal_pointer(&dev); > +            } else { > +                /* all other TPM devices goes to def->tpm */ Any reason why you store them separately? It seems they are treated the same in every place except when building QEMU command line. Switching to a def->tpms array would better reflect the XML. The Validate function would then check wheteher there's just one copy of each device type.

Jano > +                if (def->tpm) { > +                    virReportError(VIR_ERR_XML_ERROR, "%s", > +                                   _("only a single TPM non-proxy device is supported")); > +                    goto error; > +                } > +                def->tpm = g_steal_pointer(&dev); > +            } > +        } >     } >     VIR_FREE(nodes);