Re: [Qemu-devel] [PATCH V8 01/14] Support for TPM command line options

[Stefan Berger]

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=<path to blockstorage file>
> >
> > and
> >
> > ./qemu-... -tpmdev builtin,path=<path to blockstorage file>,id=<id>
> >             -device tpm-tis,tpmdev=<id>
> 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=<path to blockstorage file>,id=<id>
         -device tpm-tis,tpmdev=<id>


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<address@hidden>
> >
> > ---
> >   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=<path>[,model=<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:
> > address@hidden @option
> > +
> > address@hidden -tpmdev @var{backend} ,address@hidden [,@var{options}]
> > address@hidden -tpmdev
> > +Backend type must be:
> > address@hidden
> > +
> > +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.
> > address@hidden
> > +qemu -tpmdev ?
> > address@hidden example
> > +
> > address@hidden -tpmdev builtin ,address@hidden, address@hidden
> > +
> > +Creates an instance of the built-in TPM.
> > +
> > address@hidden 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:
> > address@hidden
> > +-tpmdev builtin,id=tpm0,path=<path_to_qcow2>  -device tpm-tis,tpmdev=tpm0
> > address@hidden example
> > +Not that the @code{-tpmdev} id is @code{tpm0} and is referenced by
> > address@hidden in the device option.
> > +
> > address@hidden table
> > +
> > +The short form of a TPM device option is:
> > address@hidden @option
> > +
> > address@hidden -tpm @var{backend-type}, address@hidden [,address@hidden
> > address@hidden -tpm
> > +
> > address@hidden specifies the device model. The default device model is a
> > address@hidden device model. @code{model} is optional.
> > +
> > +Use ? to print all available TPM models.
> > address@hidden
> > +qemu -tpm model=?
> > address@hidden example
> > +
> > +The other options have the same meaning as explained above.
> > +
> > +To create a built-in TPM use the following option:
> > address@hidden
> > +-tpm builtin, path=<path_to_qcow2>
> > address@hidden example
> > +
> > address@hidden 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<address@hidden>
> > + *         David Safford<address@hidden>
> > + *
> > + * 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<stdint.h>
> > +
> > +#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