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

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

On 03/15/2013 03:36 AM, Markus Armbruster wrote:
> I missed this one, because it wasn't cc'ed to QMP maintainers, the
> subject mentions only command line, not QMP, and even the body talks
> only about the human monitor command, not QMP.  Noticed it only when
> git-pull touched qapi-schema.json.  Please try harder to help Luiz and
> me keep track of QMP changes.
>
> I gave the QMP interface and its documentation a look-over now.  It's
> just a look-over, because passthrough requires a box with TPM enabled,
> which I don't have handy, so I can't test anything.
>
> A few comments inline.
>
> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>
>> 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>,id=<other id>
>>
>> and
>>
>> ./qemu-... -tpmdev help
>>
>> where the latter works similar to -soundhw help and shows a list of
>> available TPM backends (for example 'passthrough').
>>
>> Using the type parameter, the backend is chosen, i.e., 'passthrough' for the
>> passthrough driver. The interpretation of the other parameters along
>> with determining whether enough parameters were provided is pushed into
>> the backend driver, which needs to implement the interface function
>> 'create' and return a TPMDriverOpts structure if the VM can be started or
>> 'NULL' if not enough or bad parameters were provided.
>>
>> Monitor support for 'info tpm' has been added. It for example prints the
>> following:
>>
>> (qemu) info tpm
>> TPM devices:
>>   tpm0: model=tpm-tis
>>    \ tpm0: type=passthrough,path=/dev/tpm0,cancel-path=/sys/devices/pnp0/00:09/cancel
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Reviewed-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>> ---
>>   Makefile.objs     |   1 +
>>   hmp-commands.hx   |   2 +
>>   hmp.c             |  44 +++++++
>>   hmp.h             |   1 +
>>   include/tpm/tpm.h |  21 ++++
>>   monitor.c         |   8 ++
>>   qapi-schema.json  | 104 +++++++++++++++++
>>   qemu-options.hx   |  33 ++++++
>>   qmp-commands.hx   |  18 +++
>>   tpm/Makefile.objs |   1 +
>>   tpm/tpm.c         | 343 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   tpm/tpm_int.h     |  83 +++++++++++++
>>   tpm/tpm_tis.h     |  80 +++++++++++++
>>   vl.c              |  37 ++++++
>>   14 files changed, 776 insertions(+)
>>   create mode 100644 include/tpm/tpm.h
>>   create mode 100644 tpm/Makefile.objs
>>   create mode 100644 tpm/tpm.c
>>   create mode 100644 tpm/tpm_int.h
>>   create mode 100644 tpm/tpm_tis.h
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index a68cdac..047f9c1 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -73,6 +73,7 @@ common-obj-y += bt-host.o bt-vhci.o
>>
>>   common-obj-y += dma-helpers.o
>>   common-obj-y += vl.o
>> +common-obj-y += tpm/
>>
>>   common-obj-$(CONFIG_SLIRP) += slirp/
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index cef7708..9f2c093 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1642,6 +1642,8 @@ show device tree
>>   show qdev device model list
>>   @item info roms
>>   show roms
>> +@item info tpm
>> +show the TPM device
>>   @end table
>>   ETEXI
>>
>> diff --git a/hmp.c b/hmp.c
>> index 2f47a8a..b0a861c 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -607,6 +607,50 @@ void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
>>       }
>>   }
>>
>> +void hmp_info_tpm(Monitor *mon, const QDict *qdict)
>> +{
>> +    TPMInfoList *info_list, *info;
>> +    Error *err = NULL;
>> +    unsigned int c = 0;
>> +    TPMPassthroughOptions *tpo;
>> +
>> +    info_list = qmp_query_tpm(&err);
>> +    if (err) {
>> +        monitor_printf(mon, "TPM device not supported\n");
>> +        error_free(err);
>> +        return;
>> +    }
>> +
>> +    if (info_list) {
>> +        monitor_printf(mon, "TPM device:\n");
>> +    }
>> +
>> +    for (info = info_list; info; info = info->next) {
>> +        TPMInfo *ti = info->value;
>> +        monitor_printf(mon, " tpm%d: model=%s\n",
>> +                       c, TpmModel_lookup[ti->model]);
>> +
>> +        monitor_printf(mon, "  \\ %s: type=%s",
>> +                       ti->id, TpmType_lookup[ti->type]);
>> +
>> +        switch (ti->tpm_options->kind) {
>> +        case TPM_TYPE_OPTIONS_KIND_TPM_PASSTHROUGH_OPTIONS:
>> +            tpo = ti->tpm_options->tpm_passthrough_options;
>> +            monitor_printf(mon, "%s%s%s%s",
>> +                           tpo->has_path ? ",path=" : "",
>> +                           tpo->has_path ? tpo->path : "",
>> +                           tpo->has_cancel_path ? ",cancel-path=" : "",
>> +                           tpo->has_cancel_path ? tpo->cancel_path : "");
>> +            break;
>> +        case TPM_TYPE_OPTIONS_KIND_MAX:
>> +            break;
>> +        }
>> +        monitor_printf(mon, "\n");
>> +        c++;
>> +    }
>> +    qapi_free_TPMInfoList(info_list);
>> +}
>> +
>>   void hmp_quit(Monitor *mon, const QDict *qdict)
>>   {
>>       monitor_suspend(mon);
>> diff --git a/hmp.h b/hmp.h
>> index 30b3c20..95fe76e 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -36,6 +36,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict);
>>   void hmp_info_balloon(Monitor *mon, const QDict *qdict);
>>   void hmp_info_pci(Monitor *mon, const QDict *qdict);
>>   void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
>> +void hmp_info_tpm(Monitor *mon, const QDict *qdict);
>>   void hmp_quit(Monitor *mon, const QDict *qdict);
>>   void hmp_stop(Monitor *mon, const QDict *qdict);
>>   void hmp_system_reset(Monitor *mon, const QDict *qdict);
>> diff --git a/include/tpm/tpm.h b/include/tpm/tpm.h
>> new file mode 100644
>> index 0000000..cc8f20e
>> --- /dev/null
>> +++ b/include/tpm/tpm.h
>> @@ -0,0 +1,21 @@
>> +/*
>> + * Public TPM functions
>> + *
>> + * Copyright (C) 2011-2013 IBM Corporation
>> + *
>> + * Authors:
>> + *  Stefan Berger    <stefanb@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +#ifndef QEMU_TPM_H
>> +#define QEMU_TPM_H
>> +
>> +#include "qemu/option.h"
>> +
>> +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
>> +int tpm_init(void);
>> +void tpm_cleanup(void);
>> +
>> +#endif /* QEMU_TPM_H */
>> diff --git a/monitor.c b/monitor.c
>> index 32a6e74..961494d 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -47,6 +47,7 @@
>>   #include "migration/migration.h"
>>   #include "sysemu/kvm.h"
>>   #include "qemu/acl.h"
>> +#include "tpm/tpm.h"
>>   #include "qapi/qmp/qint.h"
>>   #include "qapi/qmp/qfloat.h"
>>   #include "qapi/qmp/qlist.h"
>> @@ -2722,6 +2723,13 @@ static mon_cmd_t info_cmds[] = {
>>           .mhandler.cmd = do_trace_print_events,
>>       },
>>       {
>> +        .name       = "tpm",
>> +        .args_type  = "",
>> +        .params     = "",
>> +        .help       = "show the TPM device",
>> +        .mhandler.cmd = hmp_info_tpm,
>> +    },
>> +    {
>>           .name       = NULL,
>>       },
>>   };
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 28b070f..4494e53 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3240,3 +3240,107 @@
>>   # 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' ] }
>> +
>> +##
>> +# @query-tpm-models:
>> +#
>> +# Return a list of supported TPM models
>> +#
>> +# Returns: a list of TpmModel
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
>> +
>> +##
>> +# @TpmType:
>> +#
>> +# An enumeration of TPM types
>> +#
>> +# @passthrough: TPM passthrough type
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'enum': 'TpmType', 'data': [ 'passthrough' ] }
>> +
>> +##
>> +# @query-tpm-types:
>> +#
>> +# Return a list of supported TPM types
>> +#
>> +# Returns: a list of TpmType
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'command': 'query-tpm-types', 'returns': ['TpmType'] }
>> +
>> +##
>> +# @TPMPassthroughOptions:
>> +#
>> +# Information about the TPM passthrough type
>> +#
>> +# @path: #optional string describing the path used for accessing the TPM device
>> +#
>> +# @cancel-path: #optional string showing the TPM's sysfs cancel file
>> +#               for cancellation of TPM commands while they are executing
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'type': 'TPMPassthroughOptions', 'data': { '*path' : 'str',
>> +                                             '*cancel-path' : 'str'} }
>> +
>> +##
>> +# @TpmTypeOptions:
>> +#
>> +# A union referencing different TPM backend types' configuration options
>> +#
>> +# @tpm-passthough-options: TPMPassthroughOptions describing the TPM
>> +#                          passthrough configuration options
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'union': 'TpmTypeOptions',
>> +   'data': { 'tpm-passthrough-options' : 'TPMPassthroughOptions' } }
>> +
>> +##
>> +# @TpmInfo:
>> +#
>> +# Information about the TPM
>> +#
>> +# @id: The Id of the TPM
>> +#
>> +# @model: The TPM frontend model
>> +#
>> +# @type: The TPM (backend) type being used
>> +#
>> +# @tpm-options: The TPM (backend) type configuration options
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'type': 'TPMInfo',
>> +  'data': {'id': 'str',
>> +           'model': 'TpmModel',
>> +           'type': 'TpmType',
>> +           'tpm-options': 'TpmTypeOptions' } }
>> +
>> +##
>> +# @query-tpm:
>> +#
>> +# Return information about the TPM device
>> +#
>> +# Returns: @TPMInfo on success
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'command': 'query-tpm', 'returns': ['TPMInfo'] }
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 797d992..3c31806 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -2218,6 +2218,39 @@ STEXI
>>   ETEXI
>>   DEFHEADING()
>>
>> +#ifdef CONFIG_TPM
>> +DEFHEADING(TPM device options:)
>> +
>> +DEF("tpmdev", HAS_ARG, QEMU_OPTION_tpmdev, \
>> +    "-tpmdev [<type>],id=str[,option][,option][,...]\n",
>> +    QEMU_ARCH_ALL)
>> +STEXI
>> +
>> +The general form of a TPM device option is:
>> +@table @option
>> +
>> +@item -tpmdev @var{backend} ,id=@var{id} [,@var{options}]
>> +@findex -tpmdev
>> +Backend type must be:
>> +
>> +The specific backend type will determine the applicable options.
>> +The @code{-tpmdev} option requires a @code{-device} option.
>
> You mean -tpmdev creates just a backend, so for a usable device you also
> need to create a frontend with -device?
>
>> +Options to each backend are described below.
>> +
>> +Use 'help' to print all available TPM backend types.
>> +@example
>> +qemu -tpmdev help
>> +@end example
>> +
>> +@end table
>> +
>> +ETEXI
>> +
>> +DEFHEADING()
>> +
>> +#endif
>> +
>>   DEFHEADING(Linux/Multiboot boot specific:)
>>   STEXI
>>
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 799adea..3454cf8 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -2716,6 +2716,24 @@ EQMP
>>       },
>>
>>       {
>> +        .name       = "query-tpm",
>> +        .args_type  = "",
>> +        .mhandler.cmd_new = qmp_marshal_input_query_tpm,
>> +    },
>> +
>> +    {
>> +        .name       = "query-tpm-models",
>> +        .args_type  = "",
>> +        .mhandler.cmd_new = qmp_marshal_input_query_tpm_models,
>> +    },
>> +
>> +    {
>> +        .name       = "query-tpm-types",
>> +        .args_type  = "",
>> +        .mhandler.cmd_new = qmp_marshal_input_query_tpm_types,
>> +    },
>> +
>> +    {
>>           .name       = "chardev-add",
>>           .args_type  = "id:s,backend:q",
>>           .mhandler.cmd_new = qmp_marshal_input_chardev_add,
>
> You imitated the bad examples that lack documentation instead the good
> ones that have it.  Please fix that in a followup patch.
>
> Each command definition should roughly like this:
>
>          {
>              .name       = "query-tpm-models",
>              .args_type  = "",
>              .mhandler.cmd_new = qmp_marshal_input_query_tpm_models,
>          },
>
>      SQMP
>      query-tpm-models
>      ----------------
>
>      Show available TPM models.
>
>      Arguments: None
>
>      Example:
>
>      -> { "execute": "query-tpm-models" }
>      <- { "return": [ "tpm-tis" ] }
>
>      EQMP
>
> This is particularly important for commands with complex replies such as
> query-tpm.
>

I'll submit a patch for this and the other issue you mentioned above.

> [...]
>> diff --git a/tpm/tpm.c b/tpm/tpm.c
>> new file mode 100644
>> index 0000000..0273549
>> --- /dev/null
>> +++ b/tpm/tpm.c
>> @@ -0,0 +1,343 @@
>> +/*
>> + * TPM configuration
>> + *
>> + * Copyright (C) 2011-2013 IBM Corporation
>> + *
>> + * Authors:
>> + *  Stefan Berger    <stefanb@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + * Based on net.c
>> + */
> [...]
>> +/*
>> + * Walk the list of TPM backend drivers that are in use and call their
>> + * destroy function to have them cleaned up.
>> + */
>> +void tpm_cleanup(void)
>> +{
>> +    TPMBackend *drv, *next;
>> +
>> +    QLIST_FOREACH_SAFE(drv, &tpm_backends, list, next) {
>> +        QLIST_REMOVE(drv, list);
>> +        drv->ops->destroy(drv);
>> +    }
>> +}
>> +
>> +/*
>> + * Initialize the TPM. Process the tpmdev command line options describing the
>> + * TPM backend.
>> + */
>> +int tpm_init(void)
>> +{
>> +    if (qemu_opts_foreach(qemu_find_opts("tpmdev"),
>> +                          tpm_init_tpmdev, NULL, 1) != 0) {
>> +        return -1;
>> +    }
>> +
>> +    atexit(tpm_cleanup);
>
> Routine atexit() question: what happens when the program terminates
> abnormally?  atext() callbacks don't run then.  Impact of not doing
> cleanup on the system?
>

The qemu resource cleanup should be ok (freeing storage, closing files, 
freeing thread pools).  I'm not sure what the implications would be to 
the host TPM state if qemu crashes.  Perhaps there'd be a TPM command 
running at the time of the crash that would need to be cancelled through 
the sysfs interace.  Stefan?

>> +
>> +    return 0;
>> +}
> [...]
>
>
>

-- 
Regards,
Corey Bryant