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

[Daniel Henrique Barboza]

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);