[SLOF] [PATCH v2 12/20] Add TPM firmware API calls hash-all, log-event, hash-log-extend-event

[Thomas Huth]

On 19/11/15 19:20, Stefan Berger wrote:
> On 11/19/2015 06:30 AM, Thomas Huth wrote:
>> On 17/11/15 18:02, Stefan Berger wrote:
>>> From: Stefan Berger <stefanb at linux.vnet.ibm.com>
>>>
>>> Add the TPM firmware API calls hash-all, log-event, and
>>> hash-log-extend-event.
>>> These firmware calls are implemented in /vdevice/vtpm and /ibm,vtpm
>>> but the
>>> former merely forwards the calls to the latter. The implementation
>>> follows
>>> the Virtual TPM firmware documentation.
>>>
>>> These particular 3 API calls enable trusted grub extensions.
>>>
>>> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
>>> ---
[...]
>>> +: vtpm-call-forward ( arg ... arg name namelen -- failure? ret ...
>>> ret )
>>> +    \ assign /ibm,vtpm node to vtpm-ihandle, if not assigned
>>> +    vtpm-ihandle 0= IF
>>> +        s" /ibm,vtpm" open-dev to vtpm-ihandle
>>> +    THEN
>>> +
>>> +    vtpm-ihandle 0<> IF
>>> +        vtpm-ihandle                   ( arg ... arg name namelen
>>> ihandle)
>>> +        $call-method                   ( -- ret ... ret )
>>> +        false                          ( ret ... ret --- ret ... ret
>>> false )
>>> +    ELSE
>>> +        true                           ( -- true )
>>> +    THEN
>>> +;
>> Do you need an instance for your functions at all? If not, please use
>> "$call-static" instead, that's easier.
> 
> I guess I need someone's guidance here who is more familiar with the
> differences between $call-static and $call-method than I am.

$call-static can be used without an instance (thus it needs an phandle
as parameter), $call-method needs an instance (thus it needs an ihandle
as parameter).
Are you familiar with the concept of phandles and ihandles or shall I
elaborate a little bit?

> In the above implementation I tried to partially follow the spec that
> even goes to the detail of stating that /ibm,vtpm is to be opened and
> the handle should be assigned to a (global) vtpm-handle variable.

That sounds like a strange design. Any chance that there will be a
public version of that spec in the near future? I'd really like to have
a look at that one...

An instance is a temporary incarnation of a device tree node - you could
compare it to an instance in C++, or when comparing it to a file system
for example: think of an ihandle like a file descriptor while the
phandle is the inode. So storing an ihandle in a global variable without
ever closing it again after the work is done sounds ugly to me.

> So following that I am invoking $call-method on the vtpm-ihandle. What
> would I need to put on the stack (which function to call that achieves
> that?) to be able to invoke $call-static with the 'node' parameter?

You need the phandle that you get with the "find-node" function for example.

>> [...]
>>> diff --git a/slof/fs/tpm/tpm-static.fs b/slof/fs/tpm/tpm-static.fs
>>> index 66bd36f..a40117f 100644
>>> --- a/slof/fs/tpm/tpm-static.fs
>>> +++ b/slof/fs/tpm/tpm-static.fs
>>> @@ -72,6 +72,46 @@ false VALUE vtpm-debug?
>>>       THEN
>>>   ;
>>>   +\ firmware API function
>>> +: vtpm-log-event ( event-ptr -- ok? )
>>> +    vtpm-available? IF
>>> +        tpm-log-event
>>> +        dup 0= IF
>>> +            ." VTPM: Returned bool from tpm-log-event: " dup . cr
>>> +        THEN
>>> +    ELSE
>>> +        drop
>>> +        false
>>> +    THEN
>>> +;
>>> +
>>> +\ firmware API function
>>> +: vtpm-hash-log-extend-event ( event-ptr -- rc )
>>> +    vtpm-available? IF
>>> +        tpm-hash-log-extend-event
>>> +        dup 0<> IF
>>> +            ." VTPM: Error code from tpm-hash-log-extend-event: "
>>> dup . cr
>>> +        THEN
>>> +    ELSE
>>> +        drop
>>> +        9  \ Tpm-fail failure reason
>>> +    THEN
>>> +;
>>> +
>>> +\ firmware API function
>>> +: vtpm-hash-all ( data-ptr data-len hash-ptr -- )
>>> +    vtpm-available? IF
>>> +        tpm-hash-all                               ( -- errcode )
>>> +        dup 0<> IF
>>> +            ." VTPM: Error code from tpm-hash-all: " . cr
>>> +        ELSE
>>> +            drop
>>> +        THEN
>>> +    ELSE
>>> +        3drop
>>> +    THEN
>>> +;
>> Why do you need wrappers for these in tpm-static.fs at all? The
>> functions only seem to be necessary from vtpm-sml.fs, so you could
>> directly implement them only there instead.
> 
> 
> Some of the functions here in tpm-static.fs will be called directly,
> others only via the firmware API and node. All the functions in this
> file follow a similar pattern:
> 
> : vtpm-xyz
>     vtpm-available? IF
>         tpm-xyz  \ invoke C code
>         [...]
>     THEN
> ;
> 
> I am not sure whether partially putting this pattern into other files
> helps much.

I guess I should explain my thoughts a little bit: Since Forth in SLOF
is interpreted, each file that you add to the boot flow unconditionally
(like tpm-static.fs) will slow down the boot process a little bit - and
additional Forth functions will clutter the common Forth dictionary (try
to type "words" at the SLOF prompt to see what I mean).

So if we've got the possibility to put the code in device tree nodes
instead, whose .fs files are only included if the device is really
there, this is IMHO the better solution.

But as always - it's a question what the maintainers prefer... Nikunj?
Alexey?

 Thomas