Re: [Qemu-devel] [PATCH V27 0/7] QEMU Trusted Platform Module (TPM) integration

Re: [Qemu-devel] [PATCH V27 0/7] QEMU Trusted Platform Module (TPM) integration

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

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 <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.

[...]
> 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?

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

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

[Stefan Berger]

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 apologize for that.

>
> 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:
> +
> +@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?

Yes, just like any other device.

      {
+        .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.

Will do by sending it to the list and cc'ing you.

+{
+    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?

In case the TPM is currently not operating on a command there will be no 
impact. In case the TPM is operating on a command, it will hold the 
thread inside /dev/tpm0 until the command has finished. The solution 
here is to write a byte into sysfs file to terminate the TPM from 
further executing the command and return; this code exists and is being 
invoked in all other cases than abnormal termination obviously.

What other choices do we have? Do we need a signal handler that runs on 
SIGSEGV and provide a registration function for those functions that 
need to run on abnormal termination?

     Stefan

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

[Markus Armbruster]

Stefan Berger <stefanb@linux.vnet.ibm.com> writes:

> 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 apologize for that.
>
>>
>> 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:
>> +
>> +@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?
>
> Yes, just like any other device.

Okay.  Corey already posted a patch to clarify the documentation here.
Appreciated!

[...]
> +{
> +    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?
>
> In case the TPM is currently not operating on a command there will be
> no impact. In case the TPM is operating on a command, it will hold the
> thread inside /dev/tpm0 until the command has finished. The solution
> here is to write a byte into sysfs file to terminate the TPM from
> further executing the command and return; this code exists and is
> being invoked in all other cases than abnormal termination obviously.

Unlike other kernel resources such as file descriptors, this one isn't
tied to a process and cleaned up automatically on process termination.
Correct?

Therefore, we try to clean it up manually.  Actual cleanup code is
tpm_passthrough_cancel_cmd().  Correct?

The only existing cleanup function tpm_passthrough_destroy() does a
whole lot more.  Just to make sure I understand what's going on:
everything but tpm_passthrough_cancel_cmd() is effectively a no-op
there, correct?

> What other choices do we have? Do we need a signal handler that runs
> on SIGSEGV and provide a registration function for those functions
> that need to run on abnormal termination?

Not worth it.

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

[Stefan Berger]

On 03/18/2013 09:10 AM, Markus Armbruster wrote:
>>
>> In case the TPM is currently not operating on a command there will be
>> no impact. In case the TPM is operating on a command, it will hold the
>> thread inside /dev/tpm0 until the command has finished. The solution
>> here is to write a byte into sysfs file to terminate the TPM from
>> further executing the command and return; this code exists and is
>> being invoked in all other cases than abnormal termination obviously.
> Unlike other kernel resources such as file descriptors, this one isn't
> tied to a process and cleaned up automatically on process termination.
> Correct?

No, it's also a file descriptor and will be cleaned up upon process 
termination.

>
> Therefore, we try to clean it up manually.  Actual cleanup code is
> tpm_passthrough_cancel_cmd().  Correct?

No, tpm_passthrough_cancel_cmd() sends a byte to the TPM's sysfs entry 
for canceling an ongoing command. QEMU has a thread that sends the 
commands to the TPM via write() and gets the responses in read(). The 
issue being the thread is held inside the write() (!) until the command 
has completed inside the host TPM. Inside the passthrough driver we set 
a flag while the command is executing and if a command is executing when 
for example QEMU is reset, then the sysfs entry is written to to get the 
thread out of the write() due to termination of the ongoing TPM command. 
Depending on which version of Fedora is run, the termination of the 
command may or may not get invoked. F14 for example does not seem to 
care about waiting until all  user-level processes have properly shut 
down and would reset  while F16 seems to wait for all processes to 
properly terminate.
The cancellation of an ongoing command may be initiated inside the guest 
as well and will also cause tpm_passthrough_cancel_cmd() to be invoked 
if a command is executed by the host TPM on behalf of the guest.

There were/are(?) some issues with cancellation of ongoing commands. 
Different vendors seem to set different bits to indicate to the driver 
as to when a command has been canceled. The specs are not clear enough 
in that area. We fixed this for the TPMs that we have access (to wake up 
the thread inside the write()).

>
> The only existing cleanup function tpm_passthrough_destroy() does a
> whole lot more.  Just to make sure I understand what's going on:
> everything but tpm_passthrough_cancel_cmd() is effectively a no-op
> there, correct?

It depends on what you mean with no-op. In tpm_passthrough_destroy() a 
possibly ongoing command is canceled, then the thread is signaled to 
terminate and join()ed. The rest is closing of file descriptors and 
freeing of memory.

    Stefan

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

[Markus Armbruster]

Stefan Berger <stefanb@linux.vnet.ibm.com> writes:

> On 03/18/2013 09:10 AM, Markus Armbruster wrote:
>>>
>>> In case the TPM is currently not operating on a command there will be
>>> no impact. In case the TPM is operating on a command, it will hold the
>>> thread inside /dev/tpm0 until the command has finished. The solution
>>> here is to write a byte into sysfs file to terminate the TPM from
>>> further executing the command and return; this code exists and is
>>> being invoked in all other cases than abnormal termination obviously.
>> Unlike other kernel resources such as file descriptors, this one isn't
>> tied to a process and cleaned up automatically on process termination.
>> Correct?
>
> No, it's also a file descriptor and will be cleaned up upon process
> termination.

Closing the file descriptor does *not* free kernel resources associated
with it?!?

>> Therefore, we try to clean it up manually.  Actual cleanup code is
>> tpm_passthrough_cancel_cmd().  Correct?
>
> No, tpm_passthrough_cancel_cmd() sends a byte to the TPM's sysfs entry
> for canceling an ongoing command. QEMU has a thread that sends the
> commands to the TPM via write() and gets the responses in read(). The
> issue being the thread is held inside the write() (!) until the
> command has completed inside the host TPM.

You mean write() blocks?

>                                            Inside the passthrough
> driver we set a flag while the command is executing and if a command
> is executing when for example QEMU is reset, then the sysfs entry is
> written to to get the thread out of the write() due to termination of
> the ongoing TPM command.

I can understand a need to abort the write() on guest reset, but why on
QEMU exit()?  Shouldn't process termination abort the write() just fine?

>                          Depending on which version of Fedora is run,
> the termination of the command may or may not get invoked.

May or may not get invoked on what condition?

>                                                            F14 for
> example does not seem to care about waiting until all  user-level
> processes have properly shut down and would reset  while F16 seems to
> wait for all processes to properly terminate.

F14 is more than nine months past it's shelf date.  Why should we
support it when its makers don't?

> The cancellation of an ongoing command may be initiated inside the
> guest as well and will also cause tpm_passthrough_cancel_cmd() to be
> invoked if a command is executed by the host TPM on behalf of the
> guest.

Passing through the guest's cancellation makes sense to me.

> There were/are(?) some issues with cancellation of ongoing
> commands. Different vendors seem to set different bits to indicate to
> the driver as to when a command has been canceled. The specs are not
> clear enough in that area. We fixed this for the TPMs that we have
> access (to wake up the thread inside the write()).

I'm not doubting the need to cancel commands.  I'm just trying to
understand why we do it on exit().

>> The only existing cleanup function tpm_passthrough_destroy() does a
>> whole lot more.  Just to make sure I understand what's going on:
>> everything but tpm_passthrough_cancel_cmd() is effectively a no-op
>> there, correct?
>
> It depends on what you mean with no-op. In tpm_passthrough_destroy() a
> possibly ongoing command is canceled, then the thread is signaled to
> terminate and join()ed. The rest is closing of file descriptors and
> freeing of memory.

Closing file descriptors and freeing memory on exit() is a no-op at
best, and a source of bugs otherwise.  The kernel already cleans these
up just fine.

Unless the thread does something interesting in response to its
termination signal, making it terminate is also a no-op.  I can't see
anything interesting happening, please correct me if I'm wrong.

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

[Stefan Berger]

On 03/19/2013 03:45 AM, Markus Armbruster wrote:
> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>
>> On 03/18/2013 09:10 AM, Markus Armbruster wrote:
>>>> In case the TPM is currently not operating on a command there will be
>>>> no impact. In case the TPM is operating on a command, it will hold the
>>>> thread inside /dev/tpm0 until the command has finished. The solution
>>>> here is to write a byte into sysfs file to terminate the TPM from
>>>> further executing the command and return; this code exists and is
>>>> being invoked in all other cases than abnormal termination obviously.
>>> Unlike other kernel resources such as file descriptors, this one isn't
>>> tied to a process and cleaned up automatically on process termination.
>>> Correct?
>> No, it's also a file descriptor and will be cleaned up upon process
>> termination.
> Closing the file descriptor does *not* free kernel resources associated
> with it?!?

That's not what I said. You suggested "this one isn't tied to a process 
and cleaned up automatically on process termination", which is not true 
but the problem lies somewhere else.

>
>>> Therefore, we try to clean it up manually.  Actual cleanup code is
>>> tpm_passthrough_cancel_cmd().  Correct?
>> No, tpm_passthrough_cancel_cmd() sends a byte to the TPM's sysfs entry
>> for canceling an ongoing command. QEMU has a thread that sends the
>> commands to the TPM via write() and gets the responses in read(). The
>> issue being the thread is held inside the write() (!) until the
>> command has completed inside the host TPM.
> You mean write() blocks?

Yep.


>
>>                                             Inside the passthrough
>> driver we set a flag while the command is executing and if a command
>> is executing when for example QEMU is reset, then the sysfs entry is
>> written to to get the thread out of the write() due to termination of
>> the ongoing TPM command.
> I can understand a need to abort the write() on guest reset, but why on
> QEMU exit()?  Shouldn't process termination abort the write() just fine?

Nope, process termination won't get you out of write(). Even if this was 
to be changed today in the kernel driver, the old kernels are out there 
for a while to be.

>
>>                           Depending on which version of Fedora is run,
>> the termination of the command may or may not get invoked.
> May or may not get invoked on what condition?

Behavior of the OS upon shutdown itself. Whether it waits for all 
processes to terminate or not.


>>                                                             F14 for
>> example does not seem to care about waiting until all  user-level
>> processes have properly shut down and would reset  while F16 seems to
>> wait for all processes to properly terminate.
> F14 is more than nine months past it's shelf date.  Why should we
> support it when its makers don't?

F14 is just an example of one OS that can be run just happens to be old 
by by now; there may be others that behave the same way.


>> The cancellation of an ongoing command may be initiated inside the
>> guest as well and will also cause tpm_passthrough_cancel_cmd() to be
>> invoked if a command is executed by the host TPM on behalf of the
>> guest.
> Passing through the guest's cancellation makes sense to me.
>
>> There were/are(?) some issues with cancellation of ongoing
>> commands. Different vendors seem to set different bits to indicate to
>> the driver as to when a command has been canceled. The specs are not
>> clear enough in that area. We fixed this for the TPMs that we have
>> access (to wake up the thread inside the write()).
> I'm not doubting the need to cancel commands.  I'm just trying to
> understand why we do it on exit().

I can write an OS that sends a command to the TPM and then shuts down 
without waiting for the program to terminate or the even the response 
from the TPM to come back . In this case we would need to cancel the 
command on exit()l.


>
>>> The only existing cleanup function tpm_passthrough_destroy() does a
>>> whole lot more.  Just to make sure I understand what's going on:
>>> everything but tpm_passthrough_cancel_cmd() is effectively a no-op
>>> there, correct?
>> It depends on what you mean with no-op. In tpm_passthrough_destroy() a
>> possibly ongoing command is canceled, then the thread is signaled to
>> terminate and join()ed. The rest is closing of file descriptors and
>> freeing of memory.
> Closing file descriptors and freeing memory on exit() is a no-op at
> best, and a source of bugs otherwise.  The kernel already cleans these
> up just fine.
>
> Unless the thread does something interesting in response to its
> termination signal, making it terminate is also a no-op.  I can't see
> anything interesting happening, please correct me if I'm wrong.
>

Per what you are saying only no-ops are happening.

    Stefan

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

[Markus Armbruster]

Stefan Berger <stefanb@linux.vnet.ibm.com> writes:

> On 03/19/2013 03:45 AM, Markus Armbruster wrote:
>> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>>
>>> On 03/18/2013 09:10 AM, Markus Armbruster wrote:
>>>>> In case the TPM is currently not operating on a command there will be
>>>>> no impact. In case the TPM is operating on a command, it will hold the
>>>>> thread inside /dev/tpm0 until the command has finished. The solution
>>>>> here is to write a byte into sysfs file to terminate the TPM from
>>>>> further executing the command and return; this code exists and is
>>>>> being invoked in all other cases than abnormal termination obviously.
>>>> Unlike other kernel resources such as file descriptors, this one isn't
>>>> tied to a process and cleaned up automatically on process termination.
>>>> Correct?
>>> No, it's also a file descriptor and will be cleaned up upon process
>>> termination.
>> Closing the file descriptor does *not* free kernel resources associated
>> with it?!?
>
> That's not what I said. You suggested "this one isn't tied to a
> process and cleaned up automatically on process termination", which is
> not true but the problem lies somewhere else.

Well, if there's kernel action needed beyond the automatic closing of
file descriptors on process termination, there's something weird going
on.  I'm trying to understand what.  Please bear with me.

>>>> Therefore, we try to clean it up manually.  Actual cleanup code is
>>>> tpm_passthrough_cancel_cmd().  Correct?
>>> No, tpm_passthrough_cancel_cmd() sends a byte to the TPM's sysfs entry
>>> for canceling an ongoing command. QEMU has a thread that sends the
>>> commands to the TPM via write() and gets the responses in read(). The
>>> issue being the thread is held inside the write() (!) until the
>>> command has completed inside the host TPM.
>> You mean write() blocks?
>
> Yep.

Got it.

>>>                                             Inside the passthrough
>>> driver we set a flag while the command is executing and if a command
>>> is executing when for example QEMU is reset, then the sysfs entry is
>>> written to to get the thread out of the write() due to termination of
>>> the ongoing TPM command.
>> I can understand a need to abort the write() on guest reset, but why on
>> QEMU exit()?  Shouldn't process termination abort the write() just fine?
>
> Nope, process termination won't get you out of write(). Even if this
> was to be changed today in the kernel driver, the old kernels are out
> there for a while to be.

Now I'm confused.  Are you telling me that exit() hangs until the thread
currently blocked in write() to TPM is made runnable by the completion
of the TPM command?

>>>                           Depending on which version of Fedora is run,
>>> the termination of the command may or may not get invoked.
>> May or may not get invoked on what condition?
>
> Behavior of the OS upon shutdown itself. Whether it waits for all
> processes to terminate or not.

I'm getting more confused.

>>>                                                             F14 for
>>> example does not seem to care about waiting until all  user-level
>>> processes have properly shut down and would reset  while F16 seems to
>>> wait for all processes to properly terminate.
>> F14 is more than nine months past it's shelf date.  Why should we
>> support it when its makers don't?
>
> F14 is just an example of one OS that can be run just happens to be
> old by by now; there may be others that behave the same way.
>
>>> The cancellation of an ongoing command may be initiated inside the
>>> guest as well and will also cause tpm_passthrough_cancel_cmd() to be
>>> invoked if a command is executed by the host TPM on behalf of the
>>> guest.
>> Passing through the guest's cancellation makes sense to me.
>>
>>> There were/are(?) some issues with cancellation of ongoing
>>> commands. Different vendors seem to set different bits to indicate to
>>> the driver as to when a command has been canceled. The specs are not
>>> clear enough in that area. We fixed this for the TPMs that we have
>>> access (to wake up the thread inside the write()).
>> I'm not doubting the need to cancel commands.  I'm just trying to
>> understand why we do it on exit().
>
> I can write an OS that sends a command to the TPM and then shuts down
> without waiting for the program to terminate or the even the response
> from the TPM to come back . In this case we would need to cancel the
> command on exit()l.

Aha, now we're getting somewhere.

Are you talking about the following scenario?

* Guest sends TPM command, QEMU passes it through, QEMU thread blocks in
  write() to TPM.

* Guest powers down, QEMU is configured to terminate on powerdown, calls
  exit().

* Unless you explicitly cancel the TPM command, $BAD_THINGS can happen.

If yes, please describe at least one instance of $BAD_THINGS for me
again.

>>>> The only existing cleanup function tpm_passthrough_destroy() does a
>>>> whole lot more.  Just to make sure I understand what's going on:
>>>> everything but tpm_passthrough_cancel_cmd() is effectively a no-op
>>>> there, correct?
>>> It depends on what you mean with no-op. In tpm_passthrough_destroy() a
>>> possibly ongoing command is canceled, then the thread is signaled to
>>> terminate and join()ed. The rest is closing of file descriptors and
>>> freeing of memory.
>> Closing file descriptors and freeing memory on exit() is a no-op at
>> best, and a source of bugs otherwise.  The kernel already cleans these
>> up just fine.
>>
>> Unless the thread does something interesting in response to its
>> termination signal, making it terminate is also a no-op.  I can't see
>> anything interesting happening, please correct me if I'm wrong.
>>
>
> Per what you are saying only no-ops are happening.

Thanks.

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

[Stefan Berger]

On 03/19/2013 10:28 AM, Markus Armbruster wrote:
> Stefan Berger <stefanb@linux.vnet.ibm.com> writes:
>
>>
>> I can write an OS that sends a command to the TPM and then shuts down
>> without waiting for the program to terminate or the even the response
>> from the TPM to come back . In this case we would need to cancel the
>> command on exit()l.
> Aha, now we're getting somewhere.
>
> Are you talking about the following scenario?
>
> * Guest sends TPM command, QEMU passes it through, QEMU thread blocks in
>    write() to TPM.
>
> * Guest powers down, QEMU is configured to terminate on powerdown, calls
>    exit().
>
> * Unless you explicitly cancel the TPM command, $BAD_THINGS can happen.
>
> If yes, please describe at least one instance of $BAD_THINGS for me
> again.

BAD_THINGS = If a TPM command is executing as in the scenario you are 
describing, the thread will still be caught in the write(), thus the 
QEMU process cannot terminate and will be stuck in the thread-join as it 
is implemented now. If there was no thread-join QEMU would probably 
linger around as a zombie for a while.

    Stefan

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

[Corey Bryant]

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