[SLOF] [PATCH v2 02/20] Add TPM initialization support

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> On 19/11/15 13:15, Stefan Berger wrote:
>> On 11/19/2015 04:04 AM, Thomas Huth wrote:
>>> On 17/11/15 18:02, Stefan Berger wrote:
> [...]
>>>> diff --git a/board-qemu/slof/vtpm-sml.fs b/board-qemu/slof/vtpm-sml.fs
>>>> new file mode 100644
>>>> index 0000000..40f1b7e
>>>> --- /dev/null
>>>> +++ b/board-qemu/slof/vtpm-sml.fs
>>>> @@ -0,0 +1,49 @@
>>>> +\
>>>> *****************************************************************************
>>>>
>>>> +\ * Copyright (c) 2015 IBM Corporation
>>>> +\ * All rights reserved.
>>>> +\ * This program and the accompanying materials
>>>> +\ * are made available under the terms of the BSD License
>>>> +\ * which accompanies this distribution, and is available at
>>>> +\ * http://www.opensource.org/licenses/bsd-license.php
>>>> +\ *
>>>> +\ * Contributors:
>>>> +\ *     IBM Corporation - initial implementation
>>>> +\
>>>> ****************************************************************************/
>>>>
>>>> +
>>>> +\ KVM/qemu TPM SML entries in /ibm,vtpm
>>> What does SML mean? ... being a little bit more verbose the first time
>>> you use TLAs (Three Letter Acronyms) would be nice.
>> 
>> Stored Measurement Log.
>> 
>>>
>>>> +" /" find-device
>>>> +
>>>> +new-device
>>>> +
>>>> +false VALUE    vtpm-debug?
>>> Again, yet another vtpm-debug? variable that shadows the global one?
>> 
>> What is preferable ? One global one or local ones?
>
> Depends on what you want ... but I'd rather expect one knob only to turn
> on debugging instead of three.
>
>>>> diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c
>>>> new file mode 100644
>>>> index 0000000..0dae810
>>>> --- /dev/null
>>>> +++ b/lib/libtpm/tcgbios.c
> [...]
>>>> +/********************************************************
>>>> +  Extensions for TCG-enabled BIOS
>>>> + *******************************************************/
>>>> +
>>>> +static bool is_tpm_present(void)
>>>> +{
>>>> +    bool rc = false;
>>>> +    unsigned int i;
>>>> +
>>>> +    for (i = 0; i < TPM_NUM_DRIVERS; i++) {
>>>> +        struct tpm_driver *td = &tpm_drivers[i];
>>>> +        if (td->probe()) {
>>>> +            td->init();
>>>> +            tpm_state.tpm_drv = td;
>>>> +            rc = true;
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return rc;
>>>> +}
>>> Uh, unless you really really want to support multiple TPM drivers in the
>>> near future, I really would prefer if you could keep this simple instead
>>> and get rid of that indirect "struct tpm_driver *" function pointer
>>> magic. That really looks over-engineered to me right now.
>> 
>> ... is this really necessary?
>
> Sounds like a question for the maintainers ... Nikunj? Alexey?

If we dont intend to support more than 3 drivers, i dont see an
advantage here. So I agree here with Thomas, we can get rid of the
function pointer stuff.

Regards
Nikunj