[V20,1/8] Support for TPM command line options

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 ?

where the latter works similar to -soundhw ? 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 TPMDriver 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

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 hmp-commands.hx  |   2 +
 hmp.c            |  30 ++++++++
 hmp.h            |   1 +
 hw/tpm_tis.h     |  78 ++++++++++++++++++++
 monitor.c        |   8 ++
 qapi-schema.json |  32 ++++++++
 qemu-config.c    |  20 +++++
 qemu-options.hx  |  33 +++++++++
 qmp-commands.hx  |   5 ++
 tpm.c            | 217 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tpm.h            |  84 +++++++++++++++++++++
 vl.c             |  17 +++++
 12 files changed, 527 insertions(+)
 create mode 100644 hw/tpm_tis.h
 create mode 100644 tpm.c
 create mode 100644 tpm.h

Thanks for the fixes since the last version.  I have a few comments 
below.  Apologies for not catching some of these the first time through.

On 01/18/2013 11:02 AM, 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 ?
>
> where the latter works similar to -soundhw ? 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 TPMDriver 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
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>   hmp-commands.hx  |   2 +
>   hmp.c            |  30 ++++++++
>   hmp.h            |   1 +
>   hw/tpm_tis.h     |  78 ++++++++++++++++++++
>   monitor.c        |   8 ++
>   qapi-schema.json |  32 ++++++++
>   qemu-config.c    |  20 +++++
>   qemu-options.hx  |  33 +++++++++
>   qmp-commands.hx  |   5 ++
>   tpm.c            | 217 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   tpm.h            |  84 +++++++++++++++++++++
>   vl.c             |  17 +++++
>   12 files changed, 527 insertions(+)
>   create mode 100644 hw/tpm_tis.h
>   create mode 100644 tpm.c
>   create mode 100644 tpm.h
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 010b8c9..b3a1005 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1570,6 +1570,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 180ba2b..8d63d03 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -628,6 +628,36 @@ void hmp_info_block_jobs(Monitor *mon)
>       }
>   }
>
> +void hmp_info_tpm(Monitor *mon)
> +{
> +    TPMInfoList *info_list, *info;
> +    Error *err = NULL;
> +    unsigned int c = 0;
> +
> +    info_list = qmp_query_tpm(&err);
> +    if (err) {
> +        monitor_printf(mon, "TPM device not supported\n");
> +        error_free(err);
> +        return;
> +    }
> +
> +    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, ti->model);
> +        monitor_printf(mon, "  \\ %s: type=%s%s%s%s%s\n",
> +                       ti->id, ti->type,
> +                       ti->has_path ? ",path=" : "",
> +                       ti->has_path ? ti->path : "",
> +                       ti->has_cancel_path ? ",cancel_path=" : "",
> +                       ti->has_cancel_path ? ti->cancel_path : "");

Does this cause spacing issues if path is not specified and cancel_path is?

> +        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 0ab03be..df43f7d 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -36,6 +36,7 @@ void hmp_info_spice(Monitor *mon);
>   void hmp_info_balloon(Monitor *mon);
>   void hmp_info_pci(Monitor *mon);
>   void hmp_info_block_jobs(Monitor *mon);
> +void hmp_info_tpm(Monitor *mon);
>   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/hw/tpm_tis.h b/hw/tpm_tis.h
> new file mode 100644
> index 0000000..7f6dcb9
> --- /dev/null
> +++ b/hw/tpm_tis.h
> @@ -0,0 +1,78 @@
> +/*
> + * tpm_tis.c - QEMU's TPM TIS interface emulator

Should me point out somewhere that this is based on the v1.2 TIS?

> + *
> + * Copyright (C) 2006,2010,2011 IBM Corporation

Should this be updated to 2013?  That's a global comment throughout the 
patch series.

> + *
> + * Authors:
> + *  Stefan Berger <stefanb@us.ibm.com>
> + *  David Safford <safford@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.
> + *
> + * Implementation of the TIS interface according to specs found at
> + * http://www.trustedcomputiggroup.org
> + *
> + */
> +#ifndef HW_TPM_TIS_H
> +#define HW_TPM_TIS_H
> +
> +#include "isa.h"
> +#include "qemu-common.h"
> +
> +#define TPM_TIS_ADDR_BASE           0xFED40000
> +
> +#define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
> +#define TPM_TIS_LOCALITY_SHIFT      12
> +#define TPM_TIS_NO_LOCALITY         0xff
> +
> +#define TPM_TIS_IS_VALID_LOCTY(x)   ((x) < TPM_TIS_NUM_LOCALITIES)
> +
> +#define TPM_TIS_IRQ                 5
> +
> +#define TPM_TIS_BUFFER_MAX          4096
> +
> +
> +typedef struct TPMSizedBuffer {
> +    uint32_t size;
> +    uint8_t  *buffer;
> +} TPMSizedBuffer;
> +
> +typedef enum {
> +    TPM_TIS_STATE_IDLE = 0,
> +    TPM_TIS_STATE_READY,
> +    TPM_TIS_STATE_COMPLETION,
> +    TPM_TIS_STATE_EXECUTION,
> +    TPM_TIS_STATE_RECEPTION,

Just a nit comment.  TPM_TIS_STATE_COMPLETION and 
TPM_TIS_STATE_RECEPTION could be swapped and they'd be in logical order. 
  Same goes for any switch statements.

> +} TPMTISState;
> +
> +/* locality data  -- all fields are persisted */
> +typedef struct TPMLocality {
> +    TPMTISState state;
> +    uint8_t access;
> +    uint8_t sts;
> +    uint32_t inte;
> +    uint32_t ints;
> +
> +    uint16_t w_offset;
> +    uint16_t r_offset;
> +    TPMSizedBuffer w_buffer;
> +    TPMSizedBuffer r_buffer;
> +} TPMLocality;
> +
> +typedef struct TPMTISEmuState {
> +    QEMUBH *bh;
> +    uint32_t offset;
> +    uint8_t buf[TPM_TIS_BUFFER_MAX];
> +
> +    uint8_t active_locty;
> +    uint8_t aborting_locty;
> +    uint8_t next_locty;
> +
> +    TPMLocality loc[TPM_TIS_NUM_LOCALITIES];
> +
> +    qemu_irq irq;
> +    uint32_t irq_num;
> +} TPMTISEmuState;
> +
> +#endif /* HW_TPM_TIS_H */
> diff --git a/monitor.c b/monitor.c
> index c0e32d6..558346c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -47,6 +47,7 @@
>   #include "migration.h"
>   #include "kvm.h"
>   #include "acl.h"
> +#include "tpm.h"
>   #include "qint.h"
>   #include "qfloat.h"
>   #include "qlist.h"
> @@ -2736,6 +2737,13 @@ static mon_cmd_t info_cmds[] = {
>           .mhandler.info = do_trace_print_events,
>       },
>       {
> +        .name       = "tpm",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the TPM device",
> +        .mhandler.info = hmp_info_tpm,
> +    },
> +    {
>           .name       = NULL,
>       },
>   };
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5dfa052..82f753b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3017,3 +3017,35 @@
>   # Since: 1.3.0
>   ##
>   { 'command': 'nbd-server-stop' }
> +
> +##
> +# @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
> +#
> +# Since: 1.5.0
> +##
> +{ 'type': 'TPMInfo',
> +  'data': {'model': 'str', 'id': 'str', 'type': 'str', '*path': 'str',
> +           '*cancel_path': 'str' } }
> +

It might be preferred that you break the monitor support into it's own 
patch, and logically it would make more sense to introduce these backend 
specific members after the backend patch.  But those are just nit comments.

More importantly, I have a question (probably for Luiz) regarding future 
modification of the monitor command.  In the future, if a new vTPM 
backend is introduced (e.g. an emulated software vTPM), can we add new 
optional members to the end of this TPMInfo?  For example, could we 
modify it to this in the future?

{ 'type': 'TPMInfo',
   'data': {'model': 'str', 'id': 'str', 'type': 'str', '*path': 'str',
            '*cancel_path': 'str', '*new_emulated_1': 'str',
            '*new_emulated_2': 'str' } }

And would it make more sense to have something like this?

{ 'type': 'TPMInfoPassthrough',
   'data': {'*path': 'str', '*cancel_path': 'str'}

{ 'type': 'TPMInfoEmulated',
   'data': {'*new_emulated_1': 'str', '*new_emulated_2': 'str' }}

{ 'type': 'TPMInfo',
   'data': {'model': 'str', 'id': 'str', 'type': 'str',
            '*vtpm_passthrough': 'TPMInfoPassthrough',
            '*vtpm_emulated': 'TPMInfoEmulated' } }

> +##
> +# @query-tpm
> +#
> +# Return information about the TPM device.
> +#
> +# Returns: @TPMInfo on success
> +#
> +# Since: 1.5.0
> +##
> +{ 'command': 'query-tpm', 'returns': ['TPMInfo'] }
> diff --git a/qemu-config.c b/qemu-config.c
> index 10d1ba4..59f605a 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -691,6 +691,25 @@ static QemuOptsList qemu_object_opts = {
>       },
>   };
>
> +static QemuOptsList qemu_tpmdev_opts = {
> +    .name = "tpmdev",
> +    .implied_opt_name = "type",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_tpmdev_opts.head),
> +    .desc = {
> +        {
> +            .name = "type",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Type of TPM backend",
> +        },
> +        {
> +            .name = "path",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Persistent storage for TPM state",

Is this help message correct?  Shouldn't it say it's the path to the 
host TPM device?

> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>   static QemuOptsList *vm_config_groups[32] = {
>       &qemu_drive_opts,
>       &qemu_chardev_opts,
> @@ -709,6 +728,7 @@ static QemuOptsList *vm_config_groups[32] = {
>       &qemu_sandbox_opts,
>       &qemu_add_fd_opts,
>       &qemu_object_opts,
> +    &qemu_tpmdev_opts,
>       NULL,
>   };
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index de43b1b..27a968e 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2158,6 +2158,39 @@ 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} options requires a @code{-device} option.
> +
> +Options to each backend are described below.
> +
> +Use ? to print all available TPM backend types.
> +@example
> +qemu -tpmdev ?
> +@end example
> +
> +@end table
> +
> +ETEXI
> +
> +DEFHEADING()
> +
> +#endif
> +
>   DEFHEADING(Linux/Multiboot boot specific:)
>   STEXI
>
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 5c692d0..915e042 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2654,3 +2654,8 @@ EQMP
>           .args_type  = "",
>           .mhandler.cmd_new = qmp_marshal_input_query_target,
>       },
> +    {
> +        .name       = "query-tpm",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_tpm,
> +    },
> diff --git a/tpm.c b/tpm.c
> new file mode 100644
> index 0000000..c9356b6
> --- /dev/null
> +++ b/tpm.c
> @@ -0,0 +1,217 @@
> +/*
> + * TPM configuration
> + *
> + * Copyright (C) 2011 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
> + */
> +#include "config-host.h"
> +
> +#include "monitor.h"
> +#include "qerror.h"
> +#include "tpm.h"
> +#include "qmp-commands.h"
> +
> +static QLIST_HEAD(, TPMBackend) tpm_backends =
> +    QLIST_HEAD_INITIALIZER(tpm_backends);
> +
> +#ifdef CONFIG_TPM
> +
> +static const TPMDriverOps *be_drivers[] = {
> +    NULL,
> +};
> +
> +const TPMDriverOps *tpm_get_backend_driver(const char *type)
> +{
> +    int i;
> +
> +    for (i = 0; be_drivers[i] != NULL; i++) {
> +        if (!strcmp(be_drivers[i]->type, type)) {
> +            break;
> +        }
> +    }
> +
> +    return be_drivers[i];
> +}
> +
> +/*
> + * Walk the list of available TPM backend drivers and display them on the
> + * screen.
> + */
> +void tpm_display_backend_drivers(void)
> +{
> +    int i;
> +
> +    fprintf(stderr, "Supported TPM types (choose only one):\n");
> +
> +    for (i = 0; be_drivers[i] != NULL; i++) {
> +        fprintf(stderr, "%12s   %s\n",
> +                be_drivers[i]->type, be_drivers[i]->desc());
> +    }
> +    fprintf(stderr, "\n");
> +}
> +
> +/*
> + * Find the TPM with the given Id
> + */
> +TPMBackend *qemu_find_tpm(const char *id)
> +{
> +    TPMBackend *drv;
> +
> +    QLIST_FOREACH(drv, &tpm_backends, list) {
> +        if (!strcmp(drv->id, id)) {
> +            return drv;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static int configure_tpm(QemuOpts *opts)
> +{
> +    const char *value;
> +    const char *id;
> +    const TPMDriverOps *be;
> +    TPMBackend *drv;
> +
> +    if (!QLIST_EMPTY(&tpm_backends)) {
> +        error_report("Only one TPM is allowed.\n");
> +        return 1;
> +    }
> +
> +    id = qemu_opts_id(opts);
> +    if (id == NULL) {
> +        qerror_report(QERR_MISSING_PARAMETER, "id");
> +        return 1;
> +    }
> +
> +    value = qemu_opt_get(opts, "type");
> +    if (!value) {
> +        qerror_report(QERR_MISSING_PARAMETER, "type");
> +        tpm_display_backend_drivers();
> +        return 1;
> +    }
> +
> +    be = tpm_get_backend_driver(value);
> +    if (be == NULL) {
> +        qerror_report(QERR_INVALID_PARAMETER_VALUE, "type",
> +                      "a TPM backend type");
> +        tpm_display_backend_drivers();
> +        return 1;
> +    }
> +
> +    drv = be->create(opts, id);
> +    if (!drv) {
> +        return 1;
> +    }
> +
> +    QLIST_INSERT_HEAD(&tpm_backends, drv, list);
> +
> +    return 0;
> +}
> +
> +static int tpm_init_tpmdev(QemuOpts *opts, void *dummy)
> +{
> +    return configure_tpm(opts);
> +}
> +
> +/*
> + * 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);
> +
> +    return 0;
> +}
> +
> +/*
> + * Parse the TPM configuration options.
> + * It is possible to pass an option '-tpmdev none' to not activate any TPM.

Is '-tpmdev none' still applicable?

> + * To display all available TPM backends the user may use '-tpmdev ?'
> + */
> +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
> +{
> +    QemuOpts *opts;
> +
> +    if (*optarg == '?') {
> +        tpm_display_backend_drivers();
> +        return -1;
> +    }
> +    opts = qemu_opts_parse(opts_list, optarg, 1);
> +    if (!opts) {
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +#endif /* CONFIG_TPM */
> +
> +static TPMInfo *qmp_query_tpm_inst(TPMBackend *drv)
> +{
> +    TPMInfo *res = g_new0(TPMInfo, 1);
> +
> +    res->model = g_strdup(drv->fe_model);
> +    res->id = g_strdup(drv->id);
> +    if (drv->path) {
> +        res->path = g_strdup(drv->path);
> +        res->has_path = true;
> +    }
> +    if (drv->cancel_path) {
> +        res->cancel_path = g_strdup(drv->cancel_path);
> +        res->has_cancel_path = true;
> +    }
> +    res->type = g_strdup(drv->ops->type);
> +
> +    return res;
> +}
> +
> +/*
> + * Walk the list of active TPM backends and collect information about them
> + * following the schema description in qapi-schema.json.
> + */
> +TPMInfoList *qmp_query_tpm(Error **errp)
> +{
> +    TPMBackend *drv;
> +    TPMInfoList *info, *head = NULL, *cur_item = NULL;
> +
> +    QLIST_FOREACH(drv, &tpm_backends, list) {
> +        info = g_new0(TPMInfoList, 1);
> +        info->value = qmp_query_tpm_inst(drv);
> +
> +        if (!cur_item) {
> +            head = cur_item = info;
> +        } else {
> +            cur_item->next = info;
> +            cur_item = info;
> +        }
> +    }
> +
> +    return head;
> +}
> diff --git a/tpm.h b/tpm.h
> new file mode 100644
> index 0000000..8943b61
> --- /dev/null
> +++ b/tpm.h
> @@ -0,0 +1,84 @@
> +/*
> + * TPM configuration
> + *
> + * Copyright (C) 2011 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 "memory.h"
> +#include "hw/tpm_tis.h"
> +
> +struct TPMDriverOps;
> +typedef struct TPMDriverOps TPMDriverOps;
> +
> +typedef struct TPMBackend {
> +    char *id;
> +    const char *fe_model;
> +    char *path;
> +    char *cancel_path;
> +    const TPMDriverOps *ops;
> +
> +    QLIST_ENTRY(TPMBackend) list;
> +} TPMBackend;
> +
> +/* overall state of the TPM interface */
> +typedef struct TPMState {
> +    ISADevice busdev;
> +    MemoryRegion mmio;
> +
> +    union {
> +        TPMTISEmuState tis;
> +    } s;
> +
> +    uint8_t     locty_number;
> +    TPMLocality *locty_data;
> +
> +    char *backend;
> +    TPMBackend *be_driver;
> +} TPMState;
> +
> +typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty);
> +
> +struct TPMDriverOps {
> +    const char *type;
> +    /* get a descriptive text of the backend to display to the user */
> +    const char *(*desc)(void);
> +
> +    TPMBackend *(*create)(QemuOpts *opts, const char *id);
> +    void (*destroy)(TPMBackend *t);
> +
> +    /* initialize the backend */
> +    int (*init)(TPMBackend *t, TPMState *s, TPMRecvDataCB *datacb);
> +    /* start up the TPM on the backend */
> +    int (*startup_tpm)(TPMBackend *t);
> +    /* returns true if nothing will ever answer TPM requests */
> +    bool (*had_startup_error)(TPMBackend *t);
> +
> +    size_t (*realloc_buffer)(TPMSizedBuffer *sb);
> +
> +    void (*deliver_request)(TPMBackend *t);
> +
> +    void (*reset)(TPMBackend *t);
> +
> +    void (*cancel_cmd)(TPMBackend *t);
> +
> +    bool (*get_tpm_established_flag)(TPMBackend *t);
> +};
> +
> +#define TPM_DEFAULT_DEVICE_MODEL "tpm-tis"

TPM_DEFAULT_DEVICE_MODEL doesn't appear to be used anywhere.

> +
> +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
> +int tpm_init(void);
> +void tpm_cleanup(void);
> +TPMBackend *qemu_find_tpm(const char *id);
> +void tpm_display_backend_drivers(void);
> +const TPMDriverOps *tpm_get_backend_driver(const char *id);
> +
> +#endif /* QEMU_TPM_H */
> diff --git a/vl.c b/vl.c
> index a3ab384..2cbbc0b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -139,6 +139,7 @@ int main(int argc, char **argv)
>   #include "blockdev.h"
>   #include "hw/block-common.h"
>   #include "block-migration.h"
> +#include "tpm.h"
>   #include "dma.h"
>   #include "audio/audio.h"
>   #include "migration.h"
> @@ -2938,6 +2939,13 @@ int main(int argc, char **argv, char **envp)
>                   }
>                   break;
>               }
> +#ifdef CONFIG_TPM
> +            case QEMU_OPTION_tpmdev:
> +                if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg) < 0) {
> +                    exit(1);
> +                }
> +                break;
> +#endif
>               case QEMU_OPTION_mempath:
>                   mem_path = optarg;
>                   break;
> @@ -3749,6 +3757,12 @@ int main(int argc, char **argv, char **envp)
>           exit(1);
>       }
>
> +#ifdef CONFIG_TPM
> +    if (tpm_init() < 0) {
> +        exit(1);
> +    }
> +#endif
> +
>       /* init the bluetooth world */
>       if (foreach_device_config(DEV_BT, bt_parse))
>           exit(1);
> @@ -4001,6 +4015,9 @@ int main(int argc, char **argv, char **envp)
>       pause_all_vcpus();
>       net_cleanup();
>       res_free();
> +#ifdef CONFIG_TPM
> +    tpm_cleanup();
> +#endif
>
>       return 0;
>   }
>

On Fri, 01 Feb 2013 10:33:01 -0500
Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:

> > +##
> > +# @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
> > +#
> > +# Since: 1.5.0
> > +##
> > +{ 'type': 'TPMInfo',
> > +  'data': {'model': 'str', 'id': 'str', 'type': 'str', '*path': 'str',
> > +           '*cancel_path': 'str' } }
> > +
> 
> It might be preferred that you break the monitor support into it's own 
> patch, and logically it would make more sense to introduce these backend 
> specific members after the backend patch.  But those are just nit comments.
> 
> More importantly, I have a question (probably for Luiz) regarding future 
> modification of the monitor command.  In the future, if a new vTPM 
> backend is introduced (e.g. an emulated software vTPM), can we add new 
> optional members to the end of this TPMInfo?  For example, could we 
> modify it to this in the future?

Usually, extensions like that are only allowed for query- commands. For
non-query commands we prefer adding new commands instead.

Btw, we avoid using free strings like 'type' and 'model', please use
an enumeration instead.

PS: I wonder if it would be possible to extend enumerations and unions
    though.

On 02/04/2013 06:21 AM, Luiz Capitulino wrote:
> On Fri, 01 Feb 2013 10:33:01 -0500
> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>
>>> +##
>>> +# @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
>>> +#
>>> +# Since: 1.5.0
>>> +##
>>> +{ 'type': 'TPMInfo',
>>> +  'data': {'model': 'str', 'id': 'str', 'type': 'str', '*path': 'str',
>>> +           '*cancel_path': 'str' } }
>>> +
>> It might be preferred that you break the monitor support into it's own
>> patch, and logically it would make more sense to introduce these backend
>> specific members after the backend patch.  But those are just nit comments.
>>
>> More importantly, I have a question (probably for Luiz) regarding future
>> modification of the monitor command.  In the future, if a new vTPM
>> backend is introduced (e.g. an emulated software vTPM), can we add new
>> optional members to the end of this TPMInfo?  For example, could we
>> modify it to this in the future?
> Usually, extensions like that are only allowed for query- commands. For
> non-query commands we prefer adding new commands instead.

For 'query-tpm' we should be fine then.

>
> Btw, we avoid using free strings like 'type' and 'model', please use
> an enumeration instead.

Ok, did that.

>
> PS: I wonder if it would be possible to extend enumerations and unions
>      though.
>

Regards,
    Stefan

On 02/01/2013 10:33 AM, Corey Bryant wrote:
>> +    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, ti->model);
>> +        monitor_printf(mon, "  \\ %s: type=%s%s%s%s%s\n",
>> +                       ti->id, ti->type,
>> +                       ti->has_path ? ",path=" : "",
>> +                       ti->has_path ? ti->path : "",
>> +                       ti->has_cancel_path ? ",cancel_path=" : "",
>> +                       ti->has_cancel_path ? ti->cancel_path : "");
> +
>
> Does this cause spacing issues if path is not specified and 
> cancel_path is?

If path is not available we print an empty string "". So I don't see how 
this could cause spacing issues?

>> +++ b/hw/tpm_tis.h
>> @@ -0,0 +1,78 @@
>> +/*
>> + * tpm_tis.c - QEMU's TPM TIS interface emulator
>
> Should me point out somewhere that this is based on the v1.2 TIS?

The implemented TIS spec is v1.21 rev 1.0. I left a not about this now.

>
>> + *
>> + * Copyright (C) 2006,2010,2011 IBM Corporation
>
> Should this be updated to 2013?  That's a global comment throughout 
> the patch series.
>

Yes, and 2012 should be added. [...] Fixed.

> It might be preferred that you break the monitor support into it's own 
> patch, and logically it would make more sense to introduce these 
> backend specific members after the backend patch.  But those are just 
> nit comments.

This causes mostly churn with the source code distributing hunks over 
different patches etc.. At this point I would prefer not to do it.

>> +static QemuOptsList qemu_tpmdev_opts = {
>> +    .name = "tpmdev",
>> +    .implied_opt_name = "type",
>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_tpmdev_opts.head),
>> +    .desc = {
>> +        {
>> +            .name = "type",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "Type of TPM backend",
>> +        },
>> +        {
>> +            .name = "path",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "Persistent storage for TPM state",
>
> Is this help message correct?  Shouldn't it say it's the path to the 
> host TPM device?

Fixed...


>> +/*
>> + * Parse the TPM configuration options.
>> + * It is possible to pass an option '-tpmdev none' to not activate 
>> any TPM.
>
>
> Is '-tpmdev none' still applicable?

Removed the comment.

>> +struct TPMDriverOps {
>> +    const char *type;
>> +    /* get a descriptive text of the backend to display to the user */
>> +    const char *(*desc)(void);
>> +
>> +    TPMBackend *(*create)(QemuOpts *opts, const char *id);
>> +    void (*destroy)(TPMBackend *t);
>> +
>> +    /* initialize the backend */
>> +    int (*init)(TPMBackend *t, TPMState *s, TPMRecvDataCB *datacb);
>> +    /* start up the TPM on the backend */
>> +    int (*startup_tpm)(TPMBackend *t);
>> +    /* returns true if nothing will ever answer TPM requests */
>> +    bool (*had_startup_error)(TPMBackend *t);
>> +
>> +    size_t (*realloc_buffer)(TPMSizedBuffer *sb);
>> +
>> +    void (*deliver_request)(TPMBackend *t);
>> +
>> +    void (*reset)(TPMBackend *t);
>> +
>> +    void (*cancel_cmd)(TPMBackend *t);
>> +
>> +    bool (*get_tpm_established_flag)(TPMBackend *t);
>> +};
>> +
>> +#define TPM_DEFAULT_DEVICE_MODEL "tpm-tis"
>
>
> TPM_DEFAULT_DEVICE_MODEL doesn't appear to be used anywhere.

Removed. Thanks

  Stefan