Re: [Qemu-devel] [PATCH] QMP: TPM QMP and man page documentation updates

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 03/20/2013 08:32 AM, Markus Armbruster wrote:
> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>
>> On 03/19/2013 03:26 AM, Markus Armbruster wrote:
>>> [Note cc: Anthony for QAPI schema expertise]
>>>
>>> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>>>
>>>> On 03/18/2013 12:16 PM, Markus Armbruster wrote:
>>>>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>>>>
>>>>>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>     qemu-options.hx |  3 ++-
>>>>>>     qmp-commands.hx | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     2 files changed, 61 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>>>>> index 30fb85d..3b3cd0f 100644
>>>>>> --- a/qemu-options.hx
>>>>>> +++ b/qemu-options.hx
>>>>>> @@ -2237,7 +2237,8 @@ Backend type must be:
>>>>>>     @option{passthrough}.
>>>>>>       The specific backend type will determine the applicable
>>>>>> options.
>>>>>> -The @code{-tpmdev} option requires a @code{-device} option.
>>>>>> +The @code{-tpmdev} option creates the TPM backend and requires a
>>>>>> +@code{-device} option that specifies the TPM frontend interface model.
>>>>>>       Options to each backend are described below.
>>>>>>     diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>>>> index b370060..4eda5ea 100644
>>>>>> --- a/qmp-commands.hx
>>>>>> +++ b/qmp-commands.hx
>>>>>> @@ -2721,18 +2721,77 @@ EQMP
>>>>>>             .mhandler.cmd_new = qmp_marshal_input_query_tpm,
>>>>>>         },
>>>>>>     +SQMP
>>>>>> +query-tpm
>>>>>> +---------
>>>>>> +
>>>>>> +Return information about the TPM device.
>>>>>> +
>>>>>> +Arguments: None
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> +-> { "execute": "query-tpm" }
>>>>>> +<- { "return":
>>>>>> +     [
>>>>>> +       { "model": "tpm-tis",
>>>>>> +         "tpm-options":
>>>>>> +           { "type": "tpm-passthrough-options",
>>>>>> +             "data":
>>>>>> +               { "cancel-path": "/sys/class/misc/tpm0/device/cancel",
>>>>>> +                 "path": "/dev/tpm0"
>>>>>> +               }
>>>>>> +           },
>>>>>> +         "type": "passthrough",
>>>>>> +         "id": "tpm0"
>>>>>> +       }
>>>>>> +     ]
>>>>>> +   }
>>>>>> +
>>>>>> +EQMP
>>>>>> +
>>>>> "tpm-options" is a discriminated union.  How is its discriminator "type"
>>>>> (here: "tpm-passthrough-options") related to the outer "type" (here:
>>>>> "passthrough")?
>>>>
>>>> It gives you similar information twice. So there is a direct
>>>> relationship between the two types.
>>>
>>> Awkward and undocumented.
>>>
>>> Relevant parts of qapi-schema.json:
>>>
>>>       { 'enum': 'TpmType', 'data': [ 'passthrough' ] }
>>>
>>>       { 'union': 'TpmTypeOptions',
>>>          'data': { 'tpm-passthrough-options' : 'TPMPassthroughOptions' } }
>>>
>>>       { 'type': 'TPMInfo',
>>>         'data': {'id': 'str',
>>>                  'model': 'TpmModel',
>>>                  'type': 'TpmType',
>>>                  'tpm-options': 'TpmTypeOptions' } }
>>>
>>> Type Netdev solves the same problem more elegantly:
>>>
>>>       { 'union': 'NetClientOptions',
>>>         'data': {
>>>           'none':     'NetdevNoneOptions',
>>>           'nic':      'NetLegacyNicOptions',
>>>           'user':     'NetdevUserOptions',
>>>           'tap':      'NetdevTapOptions',
>>>           'socket':   'NetdevSocketOptions',
>>>           'vde':      'NetdevVdeOptions',
>>>           'dump':     'NetdevDumpOptions',
>>>           'bridge':   'NetdevBridgeOptions',
>>>           'hubport':  'NetdevHubPortOptions' } }
>>>
>>>       { 'type': 'Netdev',
>>>         'data': {
>>>           'id':   'str',
>>>           'opts': 'NetClientOptions' } }
>>>
>>> Uses the union's discriminator.  Straightforward.
>>>
>>> Following Netdev precedence, we get:
>>>
>>>       { 'union': 'TpmTypeOptions',
>>>         'data': { 'passthrough' : 'TPMPassthroughOptions' } }
>>>
>>>       { 'type': 'TPMInfo',
>>>         'data': {'id': 'str',
>>>                  'model': 'TpmModel',
>>>                  'opts': 'TpmTypeOptions' } }
>>>
>>
>> I can send a patch for this update if you'd like.
>
> Yes, please!
>

Will do.

>>> Duplication of type is gone.  No need for documentation.
>>>
>>> Since enum TpmType is used elsewhere, it still gets duplicated in the
>>> union's discriminator.  Anthony, is there a way to name the implicit
>>> discriminator enum type for reference elsewhere in the schema?
>>>
>>
>> Are you saying it still gets duplicated with this fix?  I'm not sure
>> what you mean.
>
> A union in the schema implicitely defines an C enumeration type to be
> used for its discriminator.  For instance, union TpmTypeOptions
> implicitely defines this one:
>
>      typedef enum TpmTypeOptionsKind
>      {
>          TPM_TYPE_OPTIONS_KIND_TPM_PASSTHROUGH_OPTIONS = 0,
>          TPM_TYPE_OPTIONS_KIND_MAX = 1,
>      } TpmTypeOptionsKind;
>
> QAPI's discriminated union becomes a C struct containing the
> discriminator and the union of the members:
>
>      struct TpmTypeOptions
>      {
>          TpmTypeOptionsKind kind;
>          union {
>              void *data;
>              TPMPassthroughOptions * tpm_passthrough_options;
>          };
>      };
>
> Enum TpmType and type TpmInfo become:
>
>      typedef enum TpmType
>      {
>          TPM_TYPE_PASSTHROUGH = 0,
>          TPM_TYPE_MAX = 1,
>      } TpmType;
>
>      struct TPMInfo
>      {
>          char * id;
>          TpmModel model;
>          TpmType type;
>          TpmTypeOptions * tpm_options;
>      };
>
> With the change I propose, TPM_TYPE_OPTIONS_KIND_TPM_PASSTHROUGH_OPTIONS
> becomes TPM_TYPE_OPTIONS_KIND_TPM_PASSTHROUGH, and TPMInfo member type
> goes away.
>
> Because TpmType is still used elsewhere, it doesn't go away, thus still
> duplicates TpmTypeOptionsKind.  My question to Anthony was about ways to
> avoid that remaining bit of duplication.  I don't expect you do answer
> it :)
>

Ok well thanks for the explanation anyway!

-- 
Regards,
Corey Bryant