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

[Alexey Kardashevskiy]

On 11/19/2015 12: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?
>
> [...]
>> 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 ;-)
>
>> + * 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!
>
>> +#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.
>
>> +#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)

Right. Because it is quite different from another crq in 
board-qemu/slof/vio-vscsi.fs.



>> +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).

+1




-- 
Alexey