Re: [Qemu-devel] [PATCH V21 1/7] Support for TPM command line options

[Corey Bryant]

On 02/08/2013 09:55 PM, Stefan Berger wrote:
> On 02/08/2013 05:01 PM, Eric Blake wrote:
>> On 02/08/2013 02:42 PM, Stefan Berger wrote:
>>> This patch adds support for TPM command line options.
>>> The command line options supported here are
>>>
>>> ./qemu-... -tpmdev passthrough,path=<path to TPM device>,id=<id>
>>>             -device tpm-tis,tpmdev=<id>
>>>
>>> and
>>>
>>> ./qemu-... -tpmdev ?
>> I though we preferred '-tpmdev help' instead of '-tpmdev ?' these days,
>> as that doesn't need shell quoting.
> 
> Will fix it. I only used this syntax because the existing  '-soundhw ?'.
> 
>>> where the latter works similar to -soundhw ? and shows a list of
>>> available TPM backends (for example 'passthrough').
>> What is the QMP counterpart command for listing all possible TPM
>> backends?  Libvirt refuses to use command-line probing since qemu 1.3,
>> so we need some way for libvirt to get at the same list from QMP without
>> having to use '-tpmdev ?'.
> 
> Now added query-tpm-models and query-tpm-types listing the below shown 
> enumerations.
> 
>>
>>> +++ b/qapi-schema.json
>>> @@ -3184,3 +3184,59 @@
>>>   # Since: 1.4
>>>   ##
>>>   { 'command': 'chardev-remove', 'data': {'id': 'str'} }
>>> +
>>> +##
>>> +# @TpmModel
>>> +#
>>> +# An enumeration of TPM models.
>>> +#
>>> +# @tpm-tis: TPM TIS model
>>> +#
>>> +# Since: 1.5
>>> +##
>>> +{ 'enum': 'TpmModel',
>>> +  'data': [ 'tpm-tis' ] }
>>> +
>>> +##
>>> +# @TpmType
>>> +#
>>> +# An enumeration of TPM types.
>>> +#
>>> +# @passthrough: TPM passthrough
>>> +#
>>> +# Since: 1.5
>>> +##
>>> +{ 'enum': 'TpmType',
>>> +  'data': [ 'passthrough' ] }
>>> +
>>> +##
>>> +# @TpmInfo:
>>> +#
>>> +# Information about the TPM
>>> +#
>>> +# @model: The TPM frontend model, i.e., tpm-tis
>>> +#
>>> +# @id: The ID of the TPM
>>> +#
>>> +# @type: The type of TPM backend, i.e., passthrough
>>> +#
>>> +# @path: #optional Path to the TPM backend device
>>> +#
>>> +# @cancel_path: #optional Path to TPM backend device's cancel sysfs entry
>> Prefer '-' over '_' in QMP; this should be cancel-path.
>>
> 
> Fine, I changed it and also changed it for the command line (6/7). I 
> looked around in this file and searched for '_' versus '-' and found 
> both, rolled the dice afterwards...
> 
>>> +#
>>> +# Since: 1.5
>>> +##
>>> +{ 'type': 'TPMInfo',
>>> +  'data': {'model': 'TpmModel', 'id': 'str', 'type': 'TpmType', '*path': 
>>> 'str',
>>> +           '*cancel_path': 'str' } }
>> Is this a case where the choice of which optional parameters are present
>> depends on which model was in use?  That is, if we add a new model that
>> uses a new field, would it be better to have a union type, something like:
>>
>> { 'type': 'TPMTis', 'data': {'path':'str', '*cancel-path':'str'} }
>> { 'union': 'TPMBackend',
>>    'data': { 'tpm-tis' : 'TPMTis',
>>              'tpm-future': 'TPMFUture' } }
>> { 'type': 'TPMInfo',
>>    'data': { 'id': 'str', 'type': 'TpmType', 'model': 'TPMBackend' } }
>>
>> so that the user sees:
>> { 'id': 'tpm0', 'type':'passthrough',
>>    'model': { 'type':'tpm-tis', 'data':{'path':'/dev/tpm'} } }
>>
>> not necessarily better, just food for thought.
>>
> 
> Above is not entirely reflecting the real world. What about the following?
> 
> The backend:
> 
> { 'type': 'TPMPassthrough', 'data': { 'type' : 'TpmType', 'path':'str', 
> '*cancel-path':'str'} }
> 
> { 'union': 'TPMBE',
>    'data': { 'tpm-passthrough'   : 'TPMPassthrough',
>              'tpm-future-backend': 'TPMFutureBackend' } }
> 
> 
> The hardware device 'model' (frontend):
> 
> 
> { 'type' : 'TPMTis', 'data' : { model : 'TpmModel' }
> 
> { 'union': 'TPMFE',
>    'data': { 'tpm-tis'         : 'TPMTis',
>              'tpm-future-model': 'TPMFutureModel' } }
> 
> { 'type' : 'TPMInfo'
>    'data' : { 'id' : 'str', 'model' : 'TPMFE', 'type' : 'TPMBE' }
> 
> The user should then see:
> 
> { 'id': 'tpm0',
>    'model' : { 'model' : 'tpm-tis'},
>    'type'  : { 'type'  : 'passthrough', 'path' : '/dev/tpm0' , 'cancel-path' 
> = '/dev/fdset/2' } }
> 
> 
> Stefan
> 

It might be cleaner if you use a hybrid of what you have here and what you 
already had before (plus you might be able to re-use some of these for 
query-tpm-models and query-tpm-types).  Maybe something like this?

{ 'enum': 'TpmModel',
  'data': [ 'tpm-tis' ] }

{ 'enum': 'TpmType',
  'data': [ 'passthrough' ] }

{ 'type': 'TPMPassthroughOptions', 'data': { '*path':'str', 
'*cancel-path':'str'} }

{ 'union': 'TpmTypeOptions',
   'data': { 'tpm-passthrough-options'   : 'TPMPassthroughOptions',
             'tpm-future-backend-options': 'TPMFutureBackendOptions' } }

{ 'type': 'TPMInfo',
  'data': {'id': 'str', 'model': 'TpmModel', 'type': 'TpmType', 'type-options' 
'TpmTypeOptions' } }

-- 
Regards,
Corey Bryant