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

[Anthony Liguori <anthony@codemonkey.ws>]

On 12/12/2011 01:12 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>
>
> ---
>
> v12:
>   - use all 4 bytes of the message length indicator
>
> v10:
>   - tpm_display_backend_drivers always prints to stderr
>
> v9:
>   - prefixing all functions with tpm_tis_ and all constants with TPM_TIS_
>   - splitting off of -tpm command line support into its own patch
>   - only support TPM passthrough for now
>
> v8:
>   - adjusting formatting of backend drivers output to accomodate better
>     formatting of 'passthrough' backend output
>
> v6:
>   - use #idef CONFIG_TPM to surround TPM calls
>   - use QLIST_FOREACH_SAFE rather than QLIST_FOREACH in tpm_cleanup
>   - commented backend ops in tpm.h
>   - moving to IRQ 5 (11 collided with network cards)
>
> v5:
>   - fixing typo reported by Serge Hallyn
>   - Adapting code to split command line parameters supporting
>     -tpmdev ... -device tpm-tis,tpmdev=...
>   - moved code out of arch_init.c|h into tpm.c|h
>   - increasing reserved memory for ACPI tables to 128kb (from 64kb)
>   - the backend interface has a create() function for interpreting the command
>     line parameters and returning a TPMDevice structure; previoulsy
>     this function was called handle_options()
>   - the backend interface has a destroy() function for cleaning up after
>     the create() function was called
>   - added support for 'info tpm' in monitor
>
> v4:
>   - coding style fixes
>
> v3:
>   - added hw/tpm_tis.h to this patch so Qemu compiles at this stage
> ---
>   hmp-commands.hx |    2 +
>   hw/tpm_tis.h    |   91 ++++++++++++++++++++++++++++++
>   monitor.c       |   10 +++
>   qemu-config.c   |   20 +++++++
>   qemu-options.hx |   34 +++++++++++
>   tpm.c           |  167 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   tpm.h           |   90 +++++++++++++++++++++++++++++
>   vl.c            |   15 +++++
>   8 files changed, 429 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 fdbed15..4506f56 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1352,6 +1352,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/hw/tpm_tis.h b/hw/tpm_tis.h
> new file mode 100644
> index 0000000..e6a9a4b
> --- /dev/null
> +++ b/hw/tpm_tis.h
> @@ -0,0 +1,91 @@
> +/*
> + * 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 program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2 of the
> + * License.
> + *
> + *
> + * Implementation of the TIS interface according to specs found at
> + * http://www.trustedcomputiggroup.org
> + *
> + */
> +#ifndef _HW_TPM_TIS_H
> +#define _HW_TPM_TIS_H

Macros shouldn't start with '_' as that namespace is reserved in C.

> +#include "isa.h"
> +#include "qemu-thread.h"
> +
> +#include<stdint.h>

Just include "qemu-common.h"

> +#define TPM_TIS_ADDR_BASE           0xFED40000
> +
> +#define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
> +#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;
> +
> +enum tpm_tis_state {
> +    TPM_TIS_STATE_IDLE = 0,
> +    TPM_TIS_STATE_READY,
> +    TPM_TIS_STATE_COMPLETION,
> +    TPM_TIS_STATE_EXECUTION,
> +    TPM_TIS_STATE_RECEPTION,
> +};

Typed enums still need to be CamelCase and typedefed.

> +/* locality data  -- all fields are persisted */
> +typedef struct TPMLocality {
> +    enum tpm_tis_state 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 TPMTISState {
> +    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;
> +
> +/* utility functions */
> +
> +static inline uint32_t tpm_tis_get_size_from_buffer(const TPMSizedBuffer *sb)
> +{
> +    return (sb->buffer[2]<<  24) |
> +           (sb->buffer[3]<<  16) |
> +           (sb->buffer[4]<<   8) |
> +            sb->buffer[5];
> +}

I would strongly suggest not making this static inline.

> +
> +#endif /* _HW_TPM_TIS_H */
> diff --git a/monitor.c b/monitor.c
> index 7334401..28873c3 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"
> @@ -2735,6 +2736,15 @@ static mon_cmd_t info_cmds[] = {
>           .help       = "show available trace-events&  their state",
>           .mhandler.info = do_trace_print_events,
>       },
> +#if defined(CONFIG_TPM)
> +    {
> +        .name       = "tpm",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the TPM device",
> +        .mhandler.info = do_info_tpm,
> +    },
> +#endif

Please don't make monitor commands conditional.  Make it fail in a predictable 
fashion if tpm isn't configured.

>       {
>           .name       = NULL,
>       },
> diff --git a/qemu-config.c b/qemu-config.c
> index 18f3020..cc4c31d 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -549,6 +549,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,
> @@ -563,6 +582,7 @@ static QemuOptsList *vm_config_groups[32] = {
>       &qemu_option_rom_opts,
>       &qemu_machine_opts,
>       &qemu_boot_opts,
> +&qemu_tpmdev_opts,

I assume this is my mailer or is the whitespace munged here?

>       NULL,
>   };
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b3db10c..58a5dfa 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1903,6 +1903,40 @@ ETEXI
>
>   DEFHEADING()
>
> +DEFHEADING(TPM device options:)
> +
> +#ifndef _WIN32
> +# ifdef CONFIG_TPM
> +DEF("tpmdev", HAS_ARG, QEMU_OPTION_tpmdev, \
> +    "-tpmdev [<type>],id=str[,option][,option][,...]\n",
> +    QEMU_ARCH_ALL)
> +# endif
> +#endif
> +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()
> +
>   DEFHEADING(Linux/Multiboot boot specific:)
>   STEXI
>
> diff --git a/tpm.c b/tpm.c
> new file mode 100644
> index 0000000..fcab023
> --- /dev/null
> +++ b/tpm.c
> @@ -0,0 +1,167 @@
> +/*
> + * 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.  See
> + * the COPYING file in the top-level directory.
>

v2 or later please.

> + * Based on net.c
> + */
> +#include "config.h"
> +
> +#include "tpm.h"
> +#include "monitor.h"
> +#include "qerror.h"
> +
> +static const TPMDriverOps *bes[] = {
> +    NULL,
> +};
> +
> +static QLIST_HEAD(, TPMBackend) tpm_backends =
> +    QLIST_HEAD_INITIALIZER(tpm_backends);
> +
> +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];
> +}
> +
> +void tpm_display_backend_drivers(void)
> +{
> +    int i;
> +
> +    fprintf(stderr, "Supported TPM types (choose only one):\n");

Having fprintfs to stderr is a sign something is wrong.

In this case, we should have a programatic way to query the backends (like via 
qmp_query_tpm) and then in the option handling code, we should use that function 
and print to the screen from there.

> +    for (i = 0; bes[i] != NULL; i++) {
> +        fprintf(stderr, "%12s   %s\n", bes[i]->id, bes[i]->desc());
> +    }
> +    fprintf(stderr, "\n");
> +}
> +
> +TPMBackend *qemu_find_tpm(const char *id)
> +{
> +    TPMBackend *drv;
> +
> +    QLIST_FOREACH(drv,&tpm_backends, list) {
> +        if (!strcmp(drv->id, id)) {
> +            return drv;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +void do_info_tpm(Monitor *mon)
> +{
> +    TPMBackend *drv;
> +    unsigned int c = 0;
> +
> +    monitor_printf(mon, "TPM device:\n");
> +
> +    QLIST_FOREACH(drv,&tpm_backends, list) {
> +        monitor_printf(mon, " tpm%d: model=%s\n",
> +                       c, drv->fe_model);
> +        monitor_printf(mon, "  \\ %s: type=%s%s%s\n",
> +                       drv->id, drv->ops->id,
> +                       drv->parameters ? "," : "",
> +                       drv->parameters ? drv->parameters : "");
> +        c++;
> +    }
> +}

We should do this through sure QAPI now that it's in the the tree with a proper 
schema entry and an implementation in hmp.c.

> +
> +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);
> +}
> +
> +void tpm_cleanup(void)
> +{
> +    TPMBackend *drv, *next;
> +
> +    QLIST_FOREACH_SAFE(drv,&tpm_backends, list, next) {
> +        QLIST_REMOVE(drv, list);
> +        drv->ops->destroy(drv);
> +    }
> +}
> +
> +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;
> +}
> +
> +void tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
> +{
> +    QemuOpts *opts;
> +
> +    if (strcmp("none", optarg) != 0) {
> +        if (*optarg == '?') {
> +            tpm_display_backend_drivers();
> +            exit(0);

Don't exit from something other than vl.c.  Return an error code and let vl.c exit.

> +        }
> +        opts = qemu_opts_parse(opts_list, optarg, 1);
> +        if (!opts) {
> +            exit(1);
> +        }
> +    }
> +}
> diff --git a/tpm.h b/tpm.h
> new file mode 100644
> index 0000000..85c2a35
> --- /dev/null
> +++ b/tpm.h
> @@ -0,0 +1,90 @@

Needs a copyright.

> +#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 *parameters;
> +    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;
> +    TPMLocality *cmd_locty;
> +
> +    QemuMutex state_lock;
> +    QemuCond  from_tpm_cond;
> +    QemuCond  to_tpm_cond;
> +    bool      to_tpm_execute;
> +
> +    bool      tpm_initialized;
> +
> +    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 (*reset)(TPMBackend *t);
> +
> +    bool (*get_tpm_established_flag)(TPMBackend *t);
> +};
> +
> +static inline void tpm_dump_buffer(FILE *stream,
> +                                   unsigned char *buffer, unsigned int len)
> +{
> +    int i;
> +
> +    for (i = 0; i<  len; i++) {
> +        if (i&&  !(i % 16)) {
> +            fprintf(stream, "\n");
> +        }
> +        fprintf(stream, "%.2X ", buffer[i]);
> +    }
> +    fprintf(stream, "\n");
> +}

This definitely shouldn't be static inline and it's questionable whether it 
should exist in the first place.

> +#define TPM_DEFAULT_DEVICE_MODEL "tpm-tis"
> +
> +void 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 do_info_tpm(Monitor *mon);
> +void tpm_display_backend_drivers(void);
> +const TPMDriverOps *tpm_get_backend_driver(const char *id);

Please document these functions.

> +#endif /* _QEMU_TPM_H */
> diff --git a/vl.c b/vl.c
> index 5372a96..12915d0 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"
> @@ -2550,6 +2551,11 @@ int main(int argc, char **argv, char **envp)
>                   ram_size = value;
>                   break;
>               }
> +#ifdef CONFIG_TPM
> +            case QEMU_OPTION_tpmdev:
> +                tpm_config_parse(qemu_find_opts("tpmdev"), optarg);
> +                break;
> +#endif

Don't make options conditional.

Regards,

Anthony Liguori

>               case QEMU_OPTION_mempath:
>                   mem_path = optarg;
>                   break;
> @@ -3243,6 +3249,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);
> @@ -3487,6 +3499,9 @@ int main(int argc, char **argv, char **envp)
>       pause_all_vcpus();
>       net_cleanup();
>       res_free();
> +#ifdef CONFIG_TPM
> +    tpm_cleanup();
> +#endif
>
>       return 0;
>   }