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

[Stefan Berger]

On 09/27/2012 10:12 AM, Corey Bryant wrote:
> On 06/04/2012 03:37 PM, Stefan Berger wrote:
> > +
> > +#ifdef CONFIG_TPM
> > +
> > +static const TPMDriverOps *bes[] = {
>
> I think bes[] would be more descriptive if it were named be_drivers[] 
> or be_ops[]?

Renamed to be_drivers.

> > +    if (!QLIST_EMPTY(&tpm_backends)) {
> > +        error_report("Only one TPM is allowed.\n");
> > +        return 1;
> > +    }
>
> A list of tpm_backends is maintained and walked in a few places, but 
> only one is allowed to be added to the list.  Will it ever make sense 
> to enable multiple backends at one time?

A list is also returned through the monitor. This list can at the moment 
only have maximum of one entry. I would keep that list there unless 
someone else opposes. It may be possible to create different types of 
hardware emulation interfaces or simply replicate the TPM TIS at 
different addresses. So I cannot say whether it will 'ever make sense' 
to do that but I'd rather keep the opportunity there than close it and 
with that also let the monitor return a list of items rather than a 
single item.

I removed the processing of the lists in this part of the code at least.

> > +
> > +    value = qemu_opt_get(opts, "type");
> > +    if (!value) {
> > +        qerror_report(QERR_MISSING_PARAMETER, "type");
> > +        tpm_display_backend_drivers();
> > +        return 1;
> > +    }
> > +
> > +    be = tpm_get_backend_driver(value);
>
> The "type" value is being passed but the tpm_get_backend_driver() 
> defines the parameter as "id".  Maybe "id" could be renamed to "type" 
> for consistency.  See similar comment further down in this email.

Done.

> > + */
> > +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
> > +{
> > +    QemuOpts *opts;
> > +
> > +    if (strcmp("none", optarg) != 0) {
>
> What's the point of supporting "-tpmdev none"?

Removed.

> > +typedef struct TPMBackend {
> > +    char *id;
>
> For consistency, this could be named "type" instead of "id" since it 
> corresponds to -tpmdev's type.

Yes.

> > +    uint8_t     command_locty;
>
> It would be easier to read if locality was spelled out fully, instead 
> of locty.  But if that causes lines to be too long then maybe it's not 
> worth it.

I rather keep it 'locty'.

> > +    TPMLocality *cmd_locty;
>
> There's a cmd_locty and a command_locty. command_locty is the locality 
> number and cmd_locty is the locality data.  Could these variable names 
> be updated to be more unique and descriptive?

Will rename them command_locty -> locty_number and cmd_locty -> locty_data.

Regards,

   Stefan