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 | 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;
+ }
+
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 */
+ 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);
@@ -29807,6 +29839,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 3:30 PM, 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>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
> 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;
> + }
> +
> 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 */
> + 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);
>
> @@ -29807,6 +29839,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 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@gmail.com>
>---
> 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);
On 5/14/20 11:09 AM, Ján Tomko wrote:
> 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@gmail.com>
>> ---
>> 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.
Good catch.
>
>> 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.
It is an attempt to minimize the amount of changes needed. For example, making
a def->tpms array would impact all the code related to the 'emulator' TPM type,
in particular the files qemu_tpm.c and qemu_extdevice.c, that would need to
handle an array instead of the def->tpm pointer.
It is also an attempt of making it easier for any future "TPM Proxy" device
style to be added in the future. Instead of revisiting the logic inside each
def->tpms loop one would simply deal with the def->tpmproxy pointer directly.
And, in the case this is wrong and no one else cares about it, at the very
least we didn't change the design of all TPM devices because of one single
PPC64 specific model.
This is all up to debate, of course. If we we decide that changing to a def->tpms
array is worth the extra lines of code I'll make it happen in the v4.
Thanks,
DHB
>
> 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);
On 5/14/20 11:32 AM, Daniel Henrique Barboza wrote:
>
>
> On 5/14/20 11:09 AM, Ján Tomko wrote:
>> 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
[...]
>>
>> 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.
>
>
> It is an attempt to minimize the amount of changes needed. For example, making
> a def->tpms array would impact all the code related to the 'emulator' TPM type,
> in particular the files qemu_tpm.c and qemu_extdevice.c, that would need to
> handle an array instead of the def->tpm pointer.
>
> It is also an attempt of making it easier for any future "TPM Proxy" device
> style to be added in the future. Instead of revisiting the logic inside each
> def->tpms loop one would simply deal with the def->tpmproxy pointer directly.
> And, in the case this is wrong and no one else cares about it, at the very
> least we didn't change the design of all TPM devices because of one single
> PPC64 specific model.
>
>
> This is all up to debate, of course. If we we decide that changing to a def->tpms
> array is worth the extra lines of code I'll make it happen in the v4.
I'm experimenting with the idea of def->tpms array and, aside from having to
change additional files, it's not as bad as I thought. What I'm doing here
to mitigate the changes in the 'emulator' backend code is to always assure that
the 'emulator' device, if present, will be always at def->tpms[0]. This
makes most changes in qemu_tpm.c and qemu_extdevice.c a replace of "def->tpm" to
"def->tpms[0]", instead of having to insert a for loop to interact with
the array.
This also has the benefit of not having to duplicate the handling of def->tpm
for the def->tpmproxy pointer, as Jano mentioned above.
Jano, what do you think about this idea of making def->tpms[0] always a non-proxy
device?
DHB
>
>
>
> Thanks,
>
>
> DHB
>
>
>
>
>
>>
>> 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);
On 5/14/20 10:07 PM, Daniel Henrique Barboza wrote: > > > On 5/14/20 11:32 AM, Daniel Henrique Barboza wrote: >> >> >> On 5/14/20 11:09 AM, Ján Tomko wrote: >>> 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 > > [...] > >>> >>> 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. Just sent a v4 with this approach. I attempted to do the trick I mentioned previously of having def->tpms[0] to always hold a non-proxy device, but in the end I decided it wasn't worth it - I would make everyone reading qemu_extdevices.c to wonder "why is this code passing def->tpms[0] instead of interacting the whole array". It was too messy for someone who doesn't know about the TPM Proxy history. Thanks, DHB
© 2016 - 2026 Red Hat, Inc.