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

[Thomas Huth]

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?

[...]
>>> diff --git a/lib/libtpm/tcgbios_int.h b/lib/libtpm/tcgbios_int.h
>>> new file mode 100644
>>> index 0000000..a6a0d8f
>>> --- /dev/null
>>> +++ b/lib/libtpm/tcgbios_int.h
[...]
>>> +
>>> +/* event types */
>>> +#define EV_POST_CODE             1
>>> +#define EV_SEPARATOR             4
>>> +#define EV_ACTION                5
>>> +#define EV_EVENT_TAG             6
>>> +#define EV_COMPACT_HASH         12
>>> +#define EV_IPL                  13
>>> +#define EV_IPL_PARTITION_DATA   14
>>> +
>>> +#define STATUS_FLAG_SHUTDOWN        (1 << 0)
>>> +
>>> +#define SHA1_BUFSIZE                20
>>> +
>>> +struct iovec {
>>> +    size_t length;
>>> +    const void *data;
>>> +};
>> That "struct iovec" should rather be put into the right header file of
>> SLOF's libc instead - otherwise we'll end up again with multiple
>> definitions of this struct type as soon as some other code needs it, too.
> 
> This one has a const void * for data, which may not be what others want.
> So under what name should that go?

Ah, right, it's not the same as the normal POSIX iovec ... so please
either introduce a POSIX-style iovec into SLOF's libc, or name your
iovec differently (tpm_iovec or so).

 Thomas