From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:59491) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RaF70-0005MA-1z for qemu-devel@nongnu.org; Mon, 12 Dec 2011 18:16:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RaF6x-0004eP-Bt for qemu-devel@nongnu.org; Mon, 12 Dec 2011 18:16:45 -0500 Received: from mail-yx0-f173.google.com ([209.85.213.173]:59899) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RaF6w-0004eB-Rq for qemu-devel@nongnu.org; Mon, 12 Dec 2011 18:16:43 -0500 Received: by yenm6 with SMTP id m6so5815861yen.4 for ; Mon, 12 Dec 2011 15:16:42 -0800 (PST) Message-ID: <4EE68B56.30704@codemonkey.ws> Date: Mon, 12 Dec 2011 17:16:38 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1323717136-21661-1-git-send-email-stefanb@linux.vnet.ibm.com> <1323717136-21661-2-git-send-email-stefanb@linux.vnet.ibm.com> In-Reply-To: <1323717136-21661-2-git-send-email-stefanb@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V13 1/7] Support for TPM command line options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Berger Cc: mst@redhat.com, qemu-devel@nongnu.org, andreas.niederl@iaik.tugraz.at 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=,id= > -device tpm-tis,tpmdev= > > 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 > > --- > > 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 > + * David Safford > + * > + * 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 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 [],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 > + * > + * 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; > }