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

[Stefan Berger]

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<address@hidden>
> > + *
> > + * 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