[SLOF] [PATCH v2 01/20] Add a TPM driver implementation

[Thomas Huth]

On 19/11/15 12:50, Stefan Berger wrote:
> On 11/18/2015 08:07 AM, Thomas Huth wrote:
>> On 17/11/15 18:02, Stefan Berger wrote:
>>> From: Stefan Berger <stefanb at linux.vnet.ibm.com>
>>>
>>> This patch adds a TPM driver for the CRQ interface as used by
>>> the QEMU PAPR implementation.
>>>
>>> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
[...]
>>> +/**** driver structures ****/
>>> +
>>> +struct tpm_driver tpm_drivers[TPM_NUM_DRIVERS] = {
>>> +    [PAPR_DRIVER_IDX] = {
>>> +        .setdurations      = spapr_vtpm_set_durations,
>>> +        .probe             = spapr_vtpm_probe,
>>> +        .init              = spapr_vtpm_init,
>>> +        .activate          = spapr_vtpm_activate,
>>> +        .ready             = spapr_vtpm_endcycle,
>>> +        .senddata          = spapr_vtpm_senddata,
>>> +        .transfer          = spapr_vtpm_transfer,
>>> +        .waitresponseready = spapr_vtpm_waitresponseready,
>>> +        .readresponse      = spapr_vtpm_readresponse,
>>> +        .sha1threshold     = 100 * 1024,
>>> +        .getbuffersize     = spapr_vtpm_get_buffersize,
>>> +        .getstate          = spapr_vtpm_get_state,
>>> +        .geterror          = spapr_vtpm_get_error,
>>> +    },
>>> +};
>> Do you plan other TPM drivers in the near future? If not, this struct
>> tpm_driver interface with all those function pointers sounds a little
>> bit over-engineered to me right now.
> 
> I think it's a nice separation and allows for other drivers if it ever
> became necessary.

Well, function pointers are always more difficult to read, and to debug
later!
And since I don't see any other TPM driver implementation coming around
in the near future, I really don't think that this is necessary here.

>>> diff --git a/slof/helpers.c b/slof/helpers.c
>>> index d7c1888..dc7f08c 100644
>>> --- a/slof/helpers.c
>>> +++ b/slof/helpers.c
>>> @@ -134,3 +134,9 @@ void *SLOF_translate_my_address(void *addr)
>>>       forth_eval("translate-my-address");
>>>       return (void *)forth_pop();
>>>   }
>>> +
>>> +unsigned long SLOF_get_vtpm_unit(void)
>>> +{
>>> +    forth_eval("vtpm-unit");
>>> +    return forth_pop();
>>> +}
>> vtpm-unit is also not introduced here yet ... So maybe you should change
>> the order of your patches?
> 
> Nothing is going to invoke the driver at this point. Do we really need
> to rearrange the patches?

Ok, should be save to keep it in this order.

 Thomas