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

[Stefan Berger]

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>
> ...
>> diff --git a/lib/libtpm/Makefile b/lib/libtpm/Makefile
>> new file mode 100644
>> index 0000000..a174815
>> --- /dev/null
>> +++ b/lib/libtpm/Makefile
>> @@ -0,0 +1,51 @@
>> +# *****************************************************************************
>> +# * 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
>> +# ****************************************************************************/
>> +
>> +TOPCMNDIR ?= ../..
>> +
>> +ASFLAGS = $(FLAG) $(RELEASE) $(CPUARCHDEF) -Wa,-mregnames
> Do you need the ASFLAGS at all? As far as I can see, you don't introduce
> any assembler files in this libtpm directory, do you?

Can probably drop it.


>
> [...]
>> diff --git a/lib/libtpm/tpm_drivers.c b/lib/libtpm/tpm_drivers.c
>> new file mode 100644
>> index 0000000..900ba38
>> --- /dev/null
>> +++ b/lib/libtpm/tpm_drivers.c
>> @@ -0,0 +1,495 @@
>> +/*****************************************************************************
>> + * 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
> IMHO it would be nice if you could give a brief description about the
> TPM code here, since this is not something very common yet ... otherwise
> somebody might think that TPM is about the Tivoli Provisioning Manager
> from IBM ;-)


Ok.

>
>> + * Contributors:
>> + *     IBM Corporation - initial implementation
>> + *****************************************************************************/
>> +
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <stdbool.h>
>> +
>> +#include "string.h"
>> +#include "helpers.h"
>> +#include "byteorder.h"
>> +#include "tpm_drivers.h"
>> +#include "tcgbios.h"
> Looks like tcgbios.h does not exist before patch #2 ... so compiling
> this first patch only certainly does not work. Please try to maintain
> the bisectability of the revisions!

Yes. Removed.

>
>> +#include "libhvcall.h"
>>   +#include "paflof.h"
>> +
>> +#undef PAPR_VTPM_DEBUG
>> +//#define PAPR_VTPM_DEBUG
>> +#ifdef PAPR_VTPM_DEBUG
>> +#define dprintf(_x ...) do { printf("VTPM CRQ: " _x); } while(0)
>> +#else
>> +#define dprintf(_x ...)
>> +#endif
> I'd really appreciate if you could name that dprintf differently to
> avoid the name clash with the POSIX function.

libusb, libvirtio etc use it as well. Don't necessarily want to be 
different here...

>
>> +#define MIN(a, b) ((a) > (b) ? (b) : (a))
>> +
> Please add a short comment before that struct below - what does CRQ
> mean? (well, I can guess, but that's really something you can avoid with
> a proper comment)
>
>> +struct crq {
>> +	uint8_t valid;
>> +	uint8_t msg;
>> +	uint16_t len;
>> +	uint32_t data;
>> +	uint64_t reserved;
>> +} __attribute__((packed));
>> +
>> +#define PAPR_VTPM_INIT_CRQ_COMMAND      0xC0
>> +#define PAPR_VTPM_VALID_COMMAND         0x80
>> +#define PAPR_VTPM_MSG_RESULT            0x80
>> +
>> +/* crq.msg request types when crq.valid = PAPR_VTPM_INIT_CRQ_COMMAND */
>> +#define PAPR_VTPM_INIT_CRQ_RESULT       0x1
>> +
>> +/* crq.msg request types when crq.valid = PAPR_VTPM_VALID_COMMAND */
>> +#define PAPR_VTPM_GET_VERSION           0x1
>> +#define PAPR_VTPM_TPM_COMMAND           0x2
>> +#define PAPR_VTPM_GET_RTCE_BUFFER_SIZE  0x3
>> +
>> +/* check for error state */
>> +#define SPAPR_CHECK_FAILURE_MODE \
>> +	if (vtpm_drv_error_get() != VTPM_DRV_ERROR_NO_FAILURE) {	\
>> +		printf("VTPM CRQ: In failure mode\n");		\
>> +		return false;					\
>> +	}
> Hiding "return" statements in macros is IMHO a very bad style - you
> later don't see the function flow anymore, you can not "grep" for
> "return" statements in the code, etc. So I'd vote for not using such
> macros (but I leave the decision to the maintainers of course whether
> they accept such code or not).


I'll let others comment.


>
>> +static const uint32_t tpm_default_durations[TPM_NUM_DURATIONS] = {
>> +	TPM_DEFAULT_DURATION_SHORT,
>> +	TPM_DEFAULT_DURATION_MEDIUM,
>> +	TPM_DEFAULT_DURATION_LONG,
>> +};
>> +
>> +#define PAGE_SIZE 4096
> What does that macro mean? Page size of the CPU (this can vary on PPC -
> and we're running here in real mode anyway)? Of the TPM chip?

I'll rename it.
>> +err_exit:
>> +	hv_free_crq(spapr_vtpm.unit);
>> +
>> +	return false;
>> +}
>> +
>> +static bool spapr_vtpm_init(void)
>> +{
>> +        spapr_vtpm_set_durations(tpm_default_durations);
> Bad indentation (use TABs)

Fixed.

>
>> +	return true;
>> +}
>> +
>> +/*
>> + * Check whether we have a CRQ underneath us
>> + */
>> +static bool spapr_vtpm_probe(void)
>> +{
>> +	spapr_vtpm_init();
>> +
>> +	if (!spapr_vtpm.qaddr) {
>> +		spapr_vtpm.qaddr = SLOF_alloc_mem(spapr_vtpm.qsize);
>> +		if (!spapr_vtpm.qaddr) {
>> +			printf("spapr-vtpm: Unable to allocate memory\n");
>> +			return false;
>> +		}
>> +		memset(spapr_vtpm.qaddr, 0, spapr_vtpm.qsize);
>> +
>> +		dprintf("getting FORTH vtpm-unit\n");
>> +		spapr_vtpm.unit = SLOF_get_vtpm_unit();
>> +		if (!spapr_vtpm.unit) {
>> +			printf("spapr-vtpm: Could not get valid vtpm-unit\n");
>> +			return false;
>> +		}
>> +	}
>> +
>> +	dprintf("vtpm_unit = %lx, buffer = %p\n",
>> +	        spapr_vtpm.unit, spapr_vtpm.qaddr);
>> +
>> +	if (!spapr_vtpm_activate(0))
>> +		return false;
>> +
>> +	hv_free_crq(spapr_vtpm.unit);
>> +
>> +	return true;
>> +}
> [...]
>> +/**** 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.



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

    Stefan