From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:59208) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzI8c-0008F5-1n for qemu-devel@nongnu.org; Thu, 01 Sep 2011 21:01:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QzI8a-0004lk-1k for qemu-devel@nongnu.org; Thu, 01 Sep 2011 21:01:42 -0400 Received: from e31.co.us.ibm.com ([32.97.110.149]:46907) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzI8Z-0004lE-Ml for qemu-devel@nongnu.org; Thu, 01 Sep 2011 21:01:40 -0400 Received: from /spool/local by us.ibm.com with XMail ESMTP for from ; Thu, 1 Sep 2011 19:01:37 -0600 Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p8211YCu140272 for ; Thu, 1 Sep 2011 19:01:34 -0600 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p8217MrD009317 for ; Thu, 1 Sep 2011 19:07:22 -0600 Message-ID: <4E602AEC.8070805@linux.vnet.ibm.com> Date: Thu, 01 Sep 2011 21:01:32 -0400 From: Stefan Berger MIME-Version: 1.0 References: <20110831143551.127339744@linux.vnet.ibm.com> <20110831143616.411365666@linux.vnet.ibm.com> <20110901171432.GD10989@redhat.com> In-Reply-To: <20110901171432.GD10989@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V8 01/14] Support for TPM command line options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: chrisw@redhat.com, anbang.ruan@cs.ox.ac.uk, qemu-devel@nongnu.org, rrelyea@redhat.com, alevy@redhat.com, andreas.niederl@iaik.tugraz.at, serge@hallyn.com On 09/01/2011 01:14 PM, Michael S. Tsirkin wrote: > On Wed, Aug 31, 2011 at 10:35:52AM -0400, Stefan Berger wrote: >> This patch adds support for TPM command line options. >> The command line supported here (considering the libtpms based >> backend) are >> >> ./qemu-... -tpm builtin,path= >> >> and >> >> ./qemu-... -tpmdev builtin,path=,id= >> -device tpm-tis,tpmdev= > do we really need both? I had chatted with Anthony about this. I am following the existing pattern is use for example for -netdev / -net. >> and >> >> ./qemu-... -tpmdev ? >> >> where the latter works similar to -soundhw ? and shows a list of >> available TPM backends ('builtin'). >> >> To show the available TPM models do: >> >> ./qemu-... -tpm model=? > Can we live with -tpmdev for backend and plain device_add for frontend? Can you give a more specific example? Is device_add a function call or a command line parameter in this context? > Frontend would be connected to backend using a tpmdev matching the id > of the frontend... qemu-... -tpmdev builtin,path=,id= -device tpm-tis,tpmdev= Isn't that what I am doing? >> In case of -tpm, 'type' (above 'builtin') and 'model' are interpreted in tpm.c. >> In case of -tpmdev 'type' and 'id' are interpreted in tpm.c >> Using the type parameter, the backend is chosen, i.e., 'builtin' for the >> libtpms-based builtin TPM. 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. >> >> Since SeaBIOS will now use 128kb for ACPI tables the amount of reserved >> memory for ACPI tables needs to be increased -- increasing it to 128kb. > Increasing from which value to which? From 64kb to 128kb. >> Monitor support for 'info tpm' has been added. It for example prints the >> following: >> >> TPM devices: >> builtin: model=tpm-tis,id=tpm0 > This mixes frontend and backend properties. > There's currently only one frontend 'model' and that's the 'tpm-tis'. In case someone would want to write a virtio equivalent it would show the that the 'builtin' backend is connected to the 'virtio' frontend model. If above is not correct, how should it look like? >> 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 >> >> Signed-off-by: Stefan Berger >> >> --- >> Makefile.target | 1 >> hmp-commands.hx | 2 >> hw/pc.c | 7 + >> hw/tpm_tis.h | 75 +++++++++++++++ >> monitor.c | 10 ++ >> qemu-config.c | 46 +++++++++ >> qemu-options.hx | 80 ++++++++++++++++ >> tpm.c | 279 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> tpm.h | 112 ++++++++++++++++++++++ >> vl.c | 18 +++ >> 10 files changed, 629 insertions(+), 1 deletion(-) >> >> Index: qemu-git/qemu-options.hx >> =================================================================== >> --- qemu-git.orig/qemu-options.hx >> +++ qemu-git/qemu-options.hx >> @@ -1760,6 +1760,86 @@ ETEXI >> >> DEFHEADING() >> >> +DEFHEADING(TPM device options:) >> + >> +#ifndef _WIN32 >> +# ifdef CONFIG_TPM >> +DEF("tpm", HAS_ARG, QEMU_OPTION_tpm, \ >> + "" \ >> + "-tpm builtin,path=[,model=]\n" \ >> + " enable a builtin TPM with state in file in path\n" \ >> + "-tpm model=? to list available TPM device models\n" \ >> + "-tpm ? to list available TPM backend types\n", >> + QEMU_ARCH_I386) >> +DEF("tpmdev", HAS_ARG, QEMU_OPTION_tpmdev, \ >> + "-tpmdev [builtin],id=str[,option][,option][,...]\n", >> + QEMU_ARCH_I386) >> +# 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: >> +@option{builtin}. >> + >> +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 >> + >> +@item -tpmdev builtin ,id=@var{id}, path=@var{path} >> + >> +Creates an instance of the built-in TPM. >> + >> +@option{path} specifies the path to the QCoW2 image that will store >> +the TPM's persistent data. @option{path} is required. >> + >> +To create a built-in TPM use the following two options: >> +@example >> +-tpmdev builtin,id=tpm0,path= -device tpm-tis,tpmdev=tpm0 >> +@end example >> +Not that the @code{-tpmdev} id is @code{tpm0} and is referenced by >> +@code{tpmdev=tpm0} in the device option. >> + >> +@end table >> + >> +The short form of a TPM device option is: >> +@table @option >> + >> +@item -tpm @var{backend-type}, path=@var{path} [,model=@var{model}] >> +@findex -tpm >> + >> +@option{model} specifies the device model. The default device model is a >> +@code{tpm-tis} device model. @code{model} is optional. >> + >> +Use ? to print all available TPM models. >> +@example >> +qemu -tpm model=? >> +@end example >> + >> +The other options have the same meaning as explained above. >> + >> +To create a built-in TPM use the following option: >> +@example >> +-tpm builtin, path= >> +@end example >> + >> +@end table >> + >> +ETEXI >> + >> + >> +DEFHEADING() >> + >> DEFHEADING(Linux/Multiboot boot specific:) >> STEXI >> >> Index: qemu-git/vl.c >> =================================================================== >> --- qemu-git.orig/vl.c >> +++ qemu-git/vl.c >> @@ -137,6 +137,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" >> @@ -2498,6 +2499,14 @@ int main(int argc, char **argv, char **e >> ram_size = value; >> break; >> } >> +#ifdef CONFIG_TPM >> + case QEMU_OPTION_tpm: >> + tpm_config_parse(qemu_find_opts("tpm"), optarg); >> + break; >> + case QEMU_OPTION_tpmdev: >> + tpm_config_parse(qemu_find_opts("tpmdev"), optarg); >> + break; >> +#endif >> case QEMU_OPTION_mempath: >> mem_path = optarg; >> break; >> @@ -3149,6 +3158,12 @@ int main(int argc, char **argv, char **e >> 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); >> @@ -3394,6 +3409,9 @@ int main(int argc, char **argv, char **e >> quit_timers(); >> net_cleanup(); >> res_free(); >> +#ifdef CONFIG_TPM >> + tpm_cleanup(); >> +#endif >> >> return 0; >> } >> Index: qemu-git/qemu-config.c >> =================================================================== >> --- qemu-git.orig/qemu-config.c >> +++ qemu-git/qemu-config.c >> @@ -507,6 +507,50 @@ 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 qemu_tpm_opts = { >> + .name = "tpm", >> + .implied_opt_name = "type", >> + .head = QTAILQ_HEAD_INITIALIZER(qemu_tpm_opts.head), >> + .desc = { >> + { >> + .name = "type", >> + .type = QEMU_OPT_STRING, >> + .help = "Type of TPM backend", >> + }, >> + { >> + .name = "model", >> + .type = QEMU_OPT_STRING, >> + .help = "Model of TPM frontend", >> + }, >> + { >> + .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, >> @@ -523,6 +567,8 @@ static QemuOptsList *vm_config_groups[32 >> &qemu_option_rom_opts, >> &qemu_machine_opts, >> &qemu_boot_opts, >> +&qemu_tpmdev_opts, >> +&qemu_tpm_opts, >> NULL, >> }; >> >> Index: qemu-git/hw/tpm_tis.h >> =================================================================== >> --- /dev/null >> +++ qemu-git/hw/tpm_tis.h >> @@ -0,0 +1,75 @@ >> +/* >> + * tpm_tis.h - include file for tpm_tis.c >> + * >> + * Copyright (C) 2006,2010,2011 IBM Corporation >> + * >> + * Author: 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. >> + */ >> +#ifndef _HW_TPM_TIS_H >> +#define _HW_TPM_TIS_H >> + >> +#include "isa.h" >> +#include "block_int.h" >> +#include "qemu-thread.h" >> + >> +#include >> + >> +#define TIS_ADDR_BASE 0xFED40000 >> + >> +#define NUM_LOCALITIES 5 /* per spec */ >> +#define NO_LOCALITY 0xff > Please use consistent prefixes to avoid namespace > pollution. E.g. tpm_tis_ for stuff in tpm_tis.h, etc. > > Ok. I'll change the functions. Also the #define's ? [...] >> + >> +static int configure_tpm(QemuOpts *opts, int is_tpmdev) >> +{ >> + const char *value; >> + const char *id = TPM_DEFAULT_DEVICE_ID; >> + const char *model = NULL; >> + const TPMDriverOps *be; >> + TPMBackend *drv; >> + >> + if (!QLIST_EMPTY(&tpm_backends)) { >> + fprintf(stderr, "Only one TPM is allowed.\n"); >> + return 1; >> + } >> + >> + if (is_tpmdev) { >> + id = qemu_opts_id(opts); >> + if (id == NULL) { >> + qerror_report(QERR_MISSING_PARAMETER, "id"); >> + return 1; >> + } >> + } else { >> + model = qemu_opt_get(opts, "model"); >> + if (model) { >> + if (strcmp(model, "?") == 0) { >> + tpm_display_models(stdout); >> + return 1; >> + } >> + if (!tpm_check_model(model)) { >> + qerror_report(QERR_INVALID_PARAMETER_VALUE, "model", >> + "a tpm model"); >> + tpm_display_models(stderr); >> + return 1; >> + } >> + } else { >> + model = TPM_DEFAULT_DEVICE_MODEL; >> + } >> + } >> + >> + value = qemu_opt_get(opts, "type"); >> + if (!value) { >> + qerror_report(QERR_MISSING_PARAMETER, "type"); >> + tpm_display_backend_drivers(stderr); >> + 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(stderr); >> + return 1; >> + } >> + >> + assert((is_tpmdev&& model == NULL) || (!is_tpmdev&& model != NULL)); > Why isn't this using qdev for parameter passing? > Can you point me to a device that is using qdev for parameter passing. Also this part is very similar to how the networking works (net.c). Stefan