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

[Markus Armbruster]

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 <address@hidden> 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 <address@hidden>
> Reviewed-by: Corey Bryant <address@hidden>
> ---
>  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
> address@hidden 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    <address@hidden>
> + *
> + * 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:
> address@hidden @option
> +
> address@hidden -tpmdev @var{backend} ,address@hidden [,@var{options}]
> address@hidden -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.
> address@hidden
> +qemu -tpmdev help
> address@hidden example
> +
> address@hidden 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.

[...]
> 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    <address@hidden>
> + *
> + * 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?

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