[V19,1/7] 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            |   28 +++++++
 hmp.h            |    1 +
 hw/tpm_tis.h     |   78 ++++++++++++++++++++
 monitor.c        |    8 ++
 qapi-schema.json |   29 ++++++++
 qemu-config.c    |   20 +++++
 qemu-options.hx  |   33 +++++++++
 qmp-commands.hx  |    5 ++
 tpm.c            |  213 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tpm.h            |   81 +++++++++++++++++++++
 vl.c             |   17 +++++
 12 files changed, 515 insertions(+), 0 deletions(-)
 create mode 100644 hw/tpm_tis.h
 create mode 100644 tpm.c
 create mode 100644 tpm.h

On 06/04/2012 03:37 PM, 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            |   28 +++++++
>   hmp.h            |    1 +
>   hw/tpm_tis.h     |   78 ++++++++++++++++++++
>   monitor.c        |    8 ++
>   qapi-schema.json |   29 ++++++++
>   qemu-config.c    |   20 +++++
>   qemu-options.hx  |   33 +++++++++
>   qmp-commands.hx  |    5 ++
>   tpm.c            |  213 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   tpm.h            |   81 +++++++++++++++++++++
>   vl.c             |   17 +++++
>   12 files changed, 515 insertions(+), 0 deletions(-)
>   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 18cb415..08f6942 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1401,6 +1401,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 bb0952e..9aedaff 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -548,6 +548,34 @@ 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\n",
> +                       ti->id, ti->type,
> +                       ti->has_path ? ",path=" : "",
> +                       ti->has_path ? ti->path : "");
> +        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 443b812..8e2a858 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -33,6 +33,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..5e1f731
> --- /dev/null
> +++ b/hw/tpm_tis.h
> @@ -0,0 +1,78 @@
> +/*
> + * tpm_tis.c - QEMU's TPM TIS interface emulator
> + *
> + * Copyright (C) 2006,2010,2011 IBM Corporation
> + *
> + * 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_STATUS_IDLE = 0,
> +    TPM_TIS_STATUS_READY,
> +    TPM_TIS_STATUS_COMPLETION,
> +    TPM_TIS_STATUS_EXECUTION,
> +    TPM_TIS_STATUS_RECEPTION,
> +} TPMTISStatus;
> +
> +/* locality data  -- all fields are persisted */
> +typedef struct TPMLocality {
> +    TPMTISStatus status;
> +    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 TPMTISState {
> +    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;
> +} TPMTISState;
> +
> +#endif /* HW_TPM_TIS_H */
> diff --git a/monitor.c b/monitor.c
> index 12a6fe2..aec06ce 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"
> @@ -2602,6 +2603,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 2ca7195..8799322 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1755,3 +1755,32 @@
>   # Since: 0.14.0
>   ##
>   { 'command': 'device_del', 'data': {'id': 'str'} }
> +
> +##
> +# @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
> +#
> +# Since: 1.1
> +##
> +{ 'type': 'TPMInfo',
> +  'data': {'model': 'str', 'id': 'str', 'type': 'str', '*path': 'str' } }
> +
> +##
> +# @query-tpm
> +#
> +# Return information about the TPM device.
> +#
> +# Returns: @TPMInfo on success
> +#
> +# Since: 1.1
> +##
> +{ 'command': 'query-tpm', 'returns': ['TPMInfo'] }
> diff --git a/qemu-config.c b/qemu-config.c
> index be84a03..edc8d5d 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -613,6 +613,25 @@ QemuOptsList qemu_boot_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",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>   static QemuOptsList *vm_config_groups[32] = {
>       &qemu_drive_opts,
>       &qemu_chardev_opts,
> @@ -628,6 +647,7 @@ static QemuOptsList *vm_config_groups[32] = {
>       &qemu_machine_opts,
>       &qemu_boot_opts,
>       &qemu_iscsi_opts,
> +    &qemu_tpmdev_opts,
>       NULL,
>   };
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8b66264..143d92d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2012,6 +2012,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 db980fa..82f1a6d 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2142,3 +2142,8 @@ EQMP
>           .args_type  = "implements:s?,abstract:b?",
>           .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
>       },
> +    {
> +        .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..778de42
> --- /dev/null
> +++ b/tpm.c
> @@ -0,0 +1,213 @@
> +/*
> + * 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.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 *bes[] = {

I think bes[] would be more descriptive if it were named be_drivers[] or 
be_ops[]?

> +    NULL,
> +};
> +
> +const TPMDriverOps *tpm_get_backend_driver(const char *id)
> +{
> +    int i;
> +
> +    for (i = 0; bes[i] != NULL; i++) {
> +        if (!strcmp(bes[i]->id, id)) {
> +            break;
> +        }
> +    }
> +
> +    return bes[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; bes[i] != NULL; i++) {
> +        fprintf(stderr, "%12s   %s\n", bes[i]->id, bes[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;
> +    }

A list of tpm_backends is maintained and walked in a few places, but 
only one is allowed to be added to the list.  Will it ever make sense to 
enable multiple backends at one time?

> +
> +    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);

The "type" value is being passed but the tpm_get_backend_driver() 
defines the parameter as "id".  Maybe "id" could be renamed to "type" 
for consistency.  See similar comment further down in this email.

> +    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.
> + * To display all available TPM backends the user may use '-tpmdev ?'
> + */
> +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
> +{
> +    QemuOpts *opts;
> +
> +    if (strcmp("none", optarg) != 0) {

What's the point of supporting "-tpmdev none"?

> +        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->type = g_strdup(drv->ops->id);
> +
> +    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..7fdd1db
> --- /dev/null
> +++ b/tpm.h
> @@ -0,0 +1,81 @@
> +/*
> + * 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;

For consistency, this could be named "type" instead of "id" since it 
corresponds to -tpmdev's type.

> +    const char *fe_model;
> +    char *path;
> +    const TPMDriverOps *ops;
> +
> +    QLIST_ENTRY(TPMBackend) list;
> +} TPMBackend;
> +
> +/* overall state of the TPM interface */
> +typedef struct TPMState {
> +    ISADevice busdev;
> +    MemoryRegion mmio;
> +
> +    union {
> +        TPMTISState tis;
> +    } s;
> +
> +    uint8_t     command_locty;

It would be easier to read if locality was spelled out fully, instead of 
locty.  But if that causes lines to be too long then maybe it's not 
worth it.

> +    TPMLocality *cmd_locty;

There's a cmd_locty and a command_locty. command_locty is the locality 
number and cmd_locty is the locality data.  Could these variable names 
be updated to be more unique and descriptive?

> +
> +    char *backend;
> +    TPMBackend *be_driver;
> +} TPMState;
> +
> +typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty);
> +
> +struct TPMDriverOps {
> +    const char *id;
> +    /* 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);
> +
> +    bool (*get_tpm_established_flag)(TPMBackend *t);
> +};
> +
> +#define TPM_DEFAULT_DEVICE_MODEL "tpm-tis"
> +
> +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 23ab3a3..3e784f5 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -139,6 +139,7 @@ int main(int argc, char **argv)
>   #include "block.h"
>   #include "blockdev.h"
>   #include "block-migration.h"
> +#include "tpm.h"
>   #include "dma.h"
>   #include "audio/audio.h"
>   #include "migration.h"
> @@ -2674,6 +2675,13 @@ int main(int argc, char **argv, char **envp)
>                   ram_size = value;
>                   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;
> @@ -3405,6 +3413,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);
> @@ -3657,6 +3671,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 09/27/2012 10:12 AM, Corey Bryant wrote:
>
>
> On 06/04/2012 03:37 PM, Stefan Berger wrote:
>> +
>> +#ifdef CONFIG_TPM
>> +
>> +static const TPMDriverOps *bes[] = {
>
> I think bes[] would be more descriptive if it were named be_drivers[] 
> or be_ops[]?
>

Renamed to be_drivers.

>> +    if (!QLIST_EMPTY(&tpm_backends)) {
>> +        error_report("Only one TPM is allowed.\n");
>> +        return 1;
>> +    }
>
> A list of tpm_backends is maintained and walked in a few places, but 
> only one is allowed to be added to the list.  Will it ever make sense 
> to enable multiple backends at one time?
>

A list is also returned through the monitor. This list can at the moment 
only have maximum of one entry. I would keep that list there unless 
someone else opposes. It may be possible to create different types of 
hardware emulation interfaces or simply replicate the TPM TIS at 
different addresses. So I cannot say whether it will 'ever make sense' 
to do that but I'd rather keep the opportunity there than close it and 
with that also let the monitor return a list of items rather than a 
single item.

I removed the processing of the lists in this part of the code at least.

>> +
>> +    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);
>
> The "type" value is being passed but the tpm_get_backend_driver() 
> defines the parameter as "id".  Maybe "id" could be renamed to "type" 
> for consistency.  See similar comment further down in this email.
>

Done.

>> + */
>> +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
>> +{
>> +    QemuOpts *opts;
>> +
>> +    if (strcmp("none", optarg) != 0) {
>
> What's the point of supporting "-tpmdev none"?
>

Removed.

>> +typedef struct TPMBackend {
>> +    char *id;
>
> For consistency, this could be named "type" instead of "id" since it 
> corresponds to -tpmdev's type.
>

Yes.

>> +    uint8_t     command_locty;
>
> It would be easier to read if locality was spelled out fully, instead 
> of locty.  But if that causes lines to be too long then maybe it's not 
> worth it.

I rather keep it 'locty'.

>
>> +    TPMLocality *cmd_locty;
>
> There's a cmd_locty and a command_locty. command_locty is the locality 
> number and cmd_locty is the locality data.  Could these variable names 
> be updated to be more unique and descriptive?
>

Will rename them command_locty -> locty_number and cmd_locty -> locty_data.

Regards,

    Stefan

On 10/24/2012 03:06 PM, Stefan Berger wrote:
> On 09/27/2012 10:12 AM, Corey Bryant wrote:
>>
>>
>> On 06/04/2012 03:37 PM, Stefan Berger wrote:
>>> +
>>> +#ifdef CONFIG_TPM
>>> +
>>> +static const TPMDriverOps *bes[] = {
>>
>> I think bes[] would be more descriptive if it were named be_drivers[]
>> or be_ops[]?
>>
>
> Renamed to be_drivers.
>
>>> +    if (!QLIST_EMPTY(&tpm_backends)) {
>>> +        error_report("Only one TPM is allowed.\n");
>>> +        return 1;
>>> +    }
>>
>> A list of tpm_backends is maintained and walked in a few places, but
>> only one is allowed to be added to the list.  Will it ever make sense
>> to enable multiple backends at one time?
>>
>
> A list is also returned through the monitor. This list can at the moment
> only have maximum of one entry. I would keep that list there unless
> someone else opposes. It may be possible to create different types of
> hardware emulation interfaces or simply replicate the TPM TIS at
> different addresses. So I cannot say whether it will 'ever make sense'
> to do that but I'd rather keep the opportunity there than close it and
> with that also let the monitor return a list of items rather than a
> single item.
>
> I removed the processing of the lists in this part of the code at least.
>

Ok and it doesn't hurt to keep the list processing.  In that case you 
might as well keep the list processing code everywhere that you already 
have it.

>>> +
>>> +    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);
>>
>> The "type" value is being passed but the tpm_get_backend_driver()
>> defines the parameter as "id".  Maybe "id" could be renamed to "type"
>> for consistency.  See similar comment further down in this email.
>>
>
> Done.
>
>>> + */
>>> +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
>>> +{
>>> +    QemuOpts *opts;
>>> +
>>> +    if (strcmp("none", optarg) != 0) {
>>
>> What's the point of supporting "-tpmdev none"?
>>
>
> Removed.
>

There must have been a reason you added it in the first place that I'm 
just not aware of.  Did someone else suggest adding it?

>>> +typedef struct TPMBackend {
>>> +    char *id;
>>
>> For consistency, this could be named "type" instead of "id" since it
>> corresponds to -tpmdev's type.
>>
>
> Yes.
>
>>> +    uint8_t     command_locty;
>>
>> It would be easier to read if locality was spelled out fully, instead
>> of locty.  But if that causes lines to be too long then maybe it's not
>> worth it.
>
> I rather keep it 'locty'.
>
>>
>>> +    TPMLocality *cmd_locty;
>>
>> There's a cmd_locty and a command_locty. command_locty is the locality
>> number and cmd_locty is the locality data.  Could these variable names
>> be updated to be more unique and descriptive?
>>
>
> Will rename them command_locty -> locty_number and cmd_locty -> locty_data.
>
> Regards,
>
>     Stefan
>
>

On 11/08/2012 10:52 AM, Corey Bryant wrote:
>
>
> On 10/24/2012 03:06 PM, Stefan Berger wrote:
>> On 09/27/2012 10:12 AM, Corey Bryant wrote:
>>>
>>>
>>> On 06/04/2012 03:37 PM, Stefan Berger wrote:
>>
>>>> +    if (!QLIST_EMPTY(&tpm_backends)) {
>>>> +        error_report("Only one TPM is allowed.\n");
>>>> +        return 1;
>>>> +    }
>>>
>>> A list of tpm_backends is maintained and walked in a few places, but
>>> only one is allowed to be added to the list.  Will it ever make sense
>>> to enable multiple backends at one time?
>>>
>>
>> A list is also returned through the monitor. This list can at the moment
>> only have maximum of one entry. I would keep that list there unless
>> someone else opposes. It may be possible to create different types of
>> hardware emulation interfaces or simply replicate the TPM TIS at
>> different addresses. So I cannot say whether it will 'ever make sense'
>> to do that but I'd rather keep the opportunity there than close it and
>> with that also let the monitor return a list of items rather than a
>> single item.
>>
>> I removed the processing of the lists in this part of the code at least.
>>
>
> Ok and it doesn't hurt to keep the list processing.  In that case you 
> might as well keep the list processing code everywhere that you 
> already have it.
>

I was only going to keep it in the monitor part now...
+ */
>>>> +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
>>>> +{
>>>> +    QemuOpts *opts;
>>>> +
>>>> +    if (strcmp("none", optarg) != 0) {
>>>
>>> What's the point of supporting "-tpmdev none"?
>>>
>>
>> Removed.
>>
>
> There must have been a reason you added it in the first place that I'm 
> just not aware of.  Did someone else suggest adding it?

Not that I can remember.The option would have been useful if every VM by 
default had a TPM, similar to the physical world today, but it's 
unlikely that this will happen.


Regards,
     Stefan