[SLOF] [PATCH v2 01/20] Add a TPM driver implementation
Thomas Huth
thuth at redhat.com
Thu Nov 19 00:07:16 AEDT 2015
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)
> +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).
> +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?
> +/* state of the PAPR CRQ VTPM driver */
> +static struct spapr_vtpm_driver_state {
> + /* whether it driver been initialized */
> + bool initialized;
> +
> + /* durations of short, medium, & long commands */
> + uint32_t durations[TPM_NUM_DURATIONS];
> +
> + /* unit number */
> + unsigned long unit;
> +
> + /* CRQ queue address and size */
> + unsigned char *qaddr;
> + unsigned long qsize;
> +
> + /* current q_entry */
> + unsigned int curr_q_entry;
> +
> + /* current response CRQ */
> + struct crq *response;
> +
> + /* power firmware defined state and error code */
> + vtpm_drv_state driver_state;
> + vtpm_drv_error driver_error;
> +
> + /* version of the TPM we talk to -- from CRQ message */
> + uint32_t tpm_version;
> +
> + /* buffer offset in buffer for sending */
> + unsigned int buffer_offset;
> +
> + /* actual size of the buffer being used */
> + unsigned int buffer_size;
> +
> + /* the buffer; may be bigger than buffer_size */
> + unsigned char buffer[PAPR_VTPM_MAX_BUFFER_SIZE];
> +} spapr_vtpm = {
> + .qsize = PAGE_SIZE,
> + .driver_state = VTPM_DRV_STATE_INVALID,
> + .driver_error = VTPM_DRV_ERROR_NO_FAILURE,
> + .buffer_size = sizeof(spapr_vtpm.buffer),
> +};
[...]
> +/*
> + * Send a message via CRQ and wait for the response
> + */
> +static bool spapr_send_crq_and_wait(unsigned long unit,
> + struct crq *crq,
> + struct crq **response,
> + unsigned timeout,
> + vtpm_drv_state state1,
> + vtpm_drv_state state2)
> +{
> + long rc;
> + unsigned i;
> +
> + *response = get_response_crq();
> +
> + vtpm_drv_state_set(state1, VTPM_DRV_ERROR_NO_FAILURE);
> +
> + rc = hv_send_crq(unit, (uint64_t *)crq);
> + if (rc != H_SUCCESS) {
> + vtpm_drv_state_set(VTPM_DRV_STATE_WAIT_INIT,
> + VTPM_DRV_ERROR_TPM_CRQ_ERROR);
> + return false;
> + }
> +
> + vtpm_drv_state_set(state2,
> + VTPM_DRV_ERROR_NO_FAILURE);
That also fits into one line, I think.
> + for (i = 0; i < timeout; i++) {
> + if (((*response)->valid & PAPR_VTPM_MSG_RESULT))
> + return true;
> + SLOF_msleep(1);
> + }
> +
> + vtpm_drv_state_set(VTPM_DRV_STATE_FAILURE,
> + VTPM_DRV_ERROR_WAIT_TIMEOUT);
> +
> + dprintf("Received no response from CRQ\n");
> + return false;
> +}
> +
> +/*
> + * Get parameters from the CRQ
> + */
> +static bool spapr_vtpm_get_params(void)
> +{
> + struct crq crq, *response;
> + static bool completed = false; /* only once */
> +
> + if (completed)
> + return true;
> +
> + /* get the TPM version */
> + crq.valid = PAPR_VTPM_VALID_COMMAND;
> + crq.msg = PAPR_VTPM_GET_VERSION;
> +
> + if (!spapr_send_crq_and_wait(spapr_vtpm.unit, &crq, &response, 10,
> + VTPM_DRV_STATE_SEND_GET_VERSION,
> + VTPM_DRV_STATE_WAIT_VERSION)) {
> + dprintf("Failure getting TPM version from CRQ\n");
> + return false;
> + }
> +
> + vtpm_drv_state_set(VTPM_DRV_STATE_CHECK_VERSION,
> + VTPM_DRV_ERROR_NO_FAILURE);
> +
> + spapr_vtpm.tpm_version = be32_to_cpu(response->data);
> + dprintf("TPM backend version: %d\n", spapr_vtpm.tpm_version);
> +
> + /* get the TPM's buffer size */
> + crq.valid = PAPR_VTPM_VALID_COMMAND;
> + crq.msg = PAPR_VTPM_GET_RTCE_BUFFER_SIZE;
> +
> + if (!spapr_send_crq_and_wait(spapr_vtpm.unit, &crq, &response, 10,
> + VTPM_DRV_STATE_SEND_BUFSIZE_REQ,
> + VTPM_DRV_STATE_WAIT_BUFSIZE)) {
> + dprintf("Failure getting RTCE buffer size from CRQ\n");
> + return false;
> + }
> +
> + vtpm_drv_state_set(VTPM_DRV_STATE_ALLOC_RTCE_BUF,
> + VTPM_DRV_ERROR_NO_FAILURE);
> +
> + dprintf("RTCE buffer size: %u\n", be16_to_cpu(response->len));
> + spapr_vtpm.buffer_size = MIN(spapr_vtpm.buffer_size,
> + be16_to_cpu(response->len));
> + if (spapr_vtpm.buffer_size < 1024) {
> + dprintf("RTCE buffer size of %u bytes is too small. "
> + "Minimum is 1024 bytes.\n", spapr_vtpm.buffer_size);
> + vtpm_drv_state_set(VTPM_DRV_STATE_FAILURE,
> + VTPM_DRV_ERROR_BAD_RTCE_SIZE);
> + return false;
> + }
> +
> + completed = true;
> +
> + return true;
> +}
> +
> +static bool spapr_vtpm_activate(uint8_t locty)
> +{
> + long rc;
> + struct crq crq, *response;
> +
> + SPAPR_CHECK_FAILURE_MODE
> +
> + spapr_vtpm.buffer_offset = 0;
> +
> + vtpm_drv_state_set(VTPM_DRV_STATE_REG_CRQ,
> + VTPM_DRV_ERROR_NO_FAILURE);
> +
> + rc = hv_reg_crq(spapr_vtpm.unit, (unsigned long)spapr_vtpm.qaddr,
> + spapr_vtpm.qsize);
> + if (rc != H_SUCCESS) {
> + vtpm_drv_state_set(VTPM_DRV_STATE_WAIT_INIT,
> + VTPM_DRV_ERROR_UNEXPECTED_REG_ERROR);
> + dprintf("CRQ registration failed\n");
> + return false;
> + }
> +
> + /* we always start with curr_q_entry 0 */
> + spapr_vtpm.curr_q_entry = 0;
> +
> + if (spapr_vtpm.initialized)
> + goto skip_init;
You could also simply use curly braces around the following code block
instead.
> + crq.valid = PAPR_VTPM_INIT_CRQ_COMMAND;
> + crq.msg = PAPR_VTPM_INIT_CRQ_RESULT;
> +
> + if (!spapr_send_crq_and_wait(spapr_vtpm.unit,
> + &crq,
> + &response,
> + 10,
> + VTPM_DRV_STATE_SEND_INIT,
> + VTPM_DRV_STATE_WAIT_INIT_COMP)) {
> + dprintf("Initializing CRQ failed\n");
> + goto err_exit;
> + }
> + dprintf("Successfully initialized CRQ\n");
> +
> + spapr_vtpm.initialized = true;
> +
> +skip_init:
> + if (!spapr_vtpm_get_params())
> + goto err_exit;
> +
> + return true;
if (spapr_vtpm_get_params())
return true;
... that's shorter and you don't need the ugly goto.
> +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)
> + 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.
> 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?
Thomas
More information about the SLOF
mailing list