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

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 10/24/2012 03:06 PM, Stefan Berger wrote:
> 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.
>

Ok and it doesn't hurt to keep the list processing.  In that case you 
might as well keep the list processing code everywhere that you already 
have it.

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

There must have been a reason you added it in the first place that I'm 
just not aware of.  Did someone else suggest adding it?

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

-- 
Regards,
Corey Bryant