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

[Stefan Berger <stefanb@linux.vnet.ibm.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=<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<stefanb@linux.vnet.ibm.com>
>>
>> ---
>>   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:
>> +@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=<path_to_qcow2>  -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=<path_to_qcow2>
>> +@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<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.
>> + */
>> +#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