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@gmail.com>
---
 src/conf/domain_conf.c | 47 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01a32f62d1..33b7d69318 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;
+    }
+
     if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
         goto error;
 
@@ -21972,15 +21980,41 @@ 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++) {
+            virDomainTPMDefPtr dev = virDomainTPMDefParseXML(xmlopt, nodes[i], ctxt, flags);
+
+            if (!dev)
+                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"));
+                    VIR_FREE(dev);
+                    goto error;
+                }
+                def->tpmproxy = g_steal_pointer(&dev);
+            } else {
+                /* all other TPM devices goes to def->tpm */
+                if (def->tpm) {
+                    virReportError(VIR_ERR_XML_ERROR, "%s",
+                                   _("only a single TPM non-proxy device is supported"));
+                    VIR_FREE(dev);
+                    goto error;
+                }
+                def->tpm = g_steal_pointer(&dev);
+            }
+        }
     }
     VIR_FREE(nodes);
 
@@ -29807,6 +29841,11 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def,
             goto error;
     }
 
+    if (def->tpmproxy) {
+        if (virDomainTPMDefFormat(buf, def->tpmproxy, flags) < 0)
+            goto error;
+    }
+
     for (n = 0; n < def->ngraphics; n++) {
         if (virDomainGraphicsDefFormat(buf, def->graphics[n], flags) < 0)
             goto error;
-- 
2.26.2

On 5/13/20 10:10 AM, 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@gmail.com>
> ---
>   src/conf/domain_conf.c | 47 ++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 01a32f62d1..33b7d69318 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;
> +    }
> +
>       if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
>           goto error;
>   
> @@ -21972,15 +21980,41 @@ 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++) {
> +            virDomainTPMDefPtr dev = virDomainTPMDefParseXML(xmlopt, nodes[i], ctxt, flags);
> +
> +            if (!dev)
> +                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"));
> +                    VIR_FREE(dev);
> +                    goto error;
> +                }
> +                def->tpmproxy = g_steal_pointer(&dev);


Is g_steal_pointer necessary ?


> +            } else {
> +                /* all other TPM devices goes to def->tpm */
> +                if (def->tpm) {
> +                    virReportError(VIR_ERR_XML_ERROR, "%s",
> +                                   _("only a single TPM non-proxy device is supported"));
> +                    VIR_FREE(dev);
> +                    goto error;
> +                }
> +                def->tpm = g_steal_pointer(&dev);
> +            }
> +        }
>       }
>       VIR_FREE(nodes);
>   
> @@ -29807,6 +29841,11 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def,
>               goto error;
>       }
>   
> +    if (def->tpmproxy) {
> +        if (virDomainTPMDefFormat(buf, def->tpmproxy, flags) < 0)
> +            goto error;
> +    }
> +
>       for (n = 0; n < def->ngraphics; n++) {
>           if (virDomainGraphicsDefFormat(buf, def->graphics[n], flags) < 0)
>               goto error;

On 5/13/20 11:35 AM, Stefan Berger wrote:
> On 5/13/20 10:10 AM, 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@gmail.com>
>> ---
>>   src/conf/domain_conf.c | 47 ++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 43 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 01a32f62d1..33b7d69318 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;
>> +    }
>> +
>>       if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
>>           goto error;
>> @@ -21972,15 +21980,41 @@ 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++) {
>> +            virDomainTPMDefPtr dev = virDomainTPMDefParseXML(xmlopt, nodes[i], ctxt, flags);
>> +
>> +            if (!dev)
>> +                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"));
>> +                    VIR_FREE(dev);
>> +                    goto error;
>> +                }
>> +                def->tpmproxy = g_steal_pointer(&dev);
> 
> 
> Is g_steal_pointer necessary ?


Not in this case, since the VIR_FREE() call is being done before the jump
and I'm not using g_autopt() with this pointer. Thing is that we should
use g_autoptr() in these scenarios to avoid the manual cleanup. Also, I
don't think I can use VIR_FREE() with this pointer - I should've used
virDomainTPMDefFree().

I'll change the code to using g_autoptr() and g_steal_pointer().


Thanks,


DHB



> 
> 
>> +            } else {
>> +                /* all other TPM devices goes to def->tpm */
>> +                if (def->tpm) {
>> +                    virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                                   _("only a single TPM non-proxy device is supported"));
>> +                    VIR_FREE(dev);
>> +                    goto error;
>> +                }
>> +                def->tpm = g_steal_pointer(&dev);
>> +            }
>> +        }
>>       }
>>       VIR_FREE(nodes);
>> @@ -29807,6 +29841,11 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def,
>>               goto error;
>>       }
>> +    if (def->tpmproxy) {
>> +        if (virDomainTPMDefFormat(buf, def->tpmproxy, flags) < 0)
>> +            goto error;
>> +    }
>> +
>>       for (n = 0; n < def->ngraphics; n++) {
>>           if (virDomainGraphicsDefFormat(buf, def->graphics[n], flags) < 0)
>>               goto error;
> 
>