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

[Stefan Berger]

On 02/01/2013 10:33 AM, Corey Bryant wrote:
> > +    monitor_printf(mon, "TPM device:\n");
> > +
> > +    for (info = info_list; info; info = info->next) {
> > +        TPMInfo *ti = info->value;
> > +        monitor_printf(mon, " tpm%d: model=%s\n",
> > +                       c, ti->model);
> > +        monitor_printf(mon, "  \\ %s: type=%s%s%s%s%s\n",
> > +                       ti->id, ti->type,
> > +                       ti->has_path ? ",path=" : "",
> > +                       ti->has_path ? ti->path : "",
> > +                       ti->has_cancel_path ? ",cancel_path=" : "",
> > +                       ti->has_cancel_path ? ti->cancel_path : "");
> +
>
> Does this cause spacing issues if path is not specified and 
> cancel_path is?

If path is not available we print an empty string "". So I don't see how 
this could cause spacing issues?

> > +++ b/hw/tpm_tis.h
> > @@ -0,0 +1,78 @@
> > +/*
> > + * tpm_tis.c - QEMU's TPM TIS interface emulator
>
> Should me point out somewhere that this is based on the v1.2 TIS?

The implemented TIS spec is v1.21 rev 1.0. I left a not about this now.

> > + *
> > + * Copyright (C) 2006,2010,2011 IBM Corporation
>
> Should this be updated to 2013?  That's a global comment throughout 
> the patch series.

Yes, and 2012 should be added. [...] Fixed.

> It might be preferred that you break the monitor support into it's own 
> patch, and logically it would make more sense to introduce these 
> backend specific members after the backend patch.  But those are just 
> nit comments.

This causes mostly churn with the source code distributing hunks over 
different patches etc.. At this point I would prefer not to do it.

> > +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",
>
> Is this help message correct?  Shouldn't it say it's the path to the 
> host TPM device?

Fixed...


> > +/*
> > + * Parse the TPM configuration options.
> > + * It is possible to pass an option '-tpmdev none' to not activate 
> > any TPM.
>
>
> Is '-tpmdev none' still applicable?

Removed the comment.

> > +struct TPMDriverOps {
> > +    const char *type;
> > +    /* get a descriptive text of the backend to display to the user */
> > +    const char *(*desc)(void);
> > +
> > +    TPMBackend *(*create)(QemuOpts *opts, const char *id);
> > +    void (*destroy)(TPMBackend *t);
> > +
> > +    /* initialize the backend */
> > +    int (*init)(TPMBackend *t, TPMState *s, TPMRecvDataCB *datacb);
> > +    /* start up the TPM on the backend */
> > +    int (*startup_tpm)(TPMBackend *t);
> > +    /* returns true if nothing will ever answer TPM requests */
> > +    bool (*had_startup_error)(TPMBackend *t);
> > +
> > +    size_t (*realloc_buffer)(TPMSizedBuffer *sb);
> > +
> > +    void (*deliver_request)(TPMBackend *t);
> > +
> > +    void (*reset)(TPMBackend *t);
> > +
> > +    void (*cancel_cmd)(TPMBackend *t);
> > +
> > +    bool (*get_tpm_established_flag)(TPMBackend *t);
> > +};
> > +
> > +#define TPM_DEFAULT_DEVICE_MODEL "tpm-tis"
>
>
> TPM_DEFAULT_DEVICE_MODEL doesn't appear to be used anywhere.

Removed. Thanks

 Stefan