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

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 12/12/2011 06:16 PM, Anthony Liguori wrote:
> On 12/12/2011 01:12 PM, Stefan Berger wrote:
>> @@ -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.
>

This will then require tpm.c to always be compiled. You'll find the 
CONFIG_TPM there then.
@@ -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?
>

Must be your mailer.@@ -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.

ok.

>> +
>> +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.
>

Will try to convert but keep this function ?

>> +
>> +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.

True...

>> +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.

I implemented this following along the lines of

qemu-system-x86_64 -soundhw ?

which then also shows and error code 0. What error code should it return 
to the shell ?


>
>> +        }
>> +        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.

Ok.

>> +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.

Do you have an alternative for this function? Assuming it's useful for 
debugging, should I just move it into tpm.c ?


>
>> +#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.

Will document them in tpm.c where their implementation is.


@@ -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.

Can I have an #ifdef-#else-#endif construct there along the lines of 
CONFIG_SDL with an exit(1) in the #else branch?


    Stefan