[U-Boot] [PATCH 15/25] dm: tpm: Add a uclass for Trusted Platform Modules

christophe.ricard
2015-08-11T21:44:01Z

Hi Simon,

I think we are pretty inline for the uclass.
Please find below some few remarks.

On 11/08/2015 16:48, Simon Glass wrote:
> Add a new uclass for TPMs which uses almost the same TIS (TPM Interface
> Specification) as is currently implemented. Since init() is handled by the
> normal driver model probe() method, we don't need to implement that. Also
> rename the transfer method to xfer() which is a less clumbsy name.
>
> Once all drivers and users are converted to driver model we can remove the
> old code.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> drivers/tpm/Kconfig | 9 +++++
> drivers/tpm/Makefile | 2 +
> drivers/tpm/tpm-uclass.c | 57 ++++++++++++++++++++++++++++
> include/dm/uclass-id.h | 1 +
> include/tis.h | 97 ++++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 166 insertions(+)
> create mode 100644 drivers/tpm/tpm-uclass.c
>
> diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig
> index 993d2d7..800239e 100644
> --- a/drivers/tpm/Kconfig
> +++ b/drivers/tpm/Kconfig
> @@ -1,3 +1,12 @@
> +config DM_TPM
> + bool "Enable driver model for Trusted Platform Module drivers"
> + depends on DM && TPM
> + help
> + Enable driver model for TPMs. The TIS interface (tis_open(),
> + tis_sendrecv(), etc.) is then implemented by the TPM uclass. Note
> + that even with driver model only a single TPM is currently
> + supported, since the tpm library assumes this.
> +
> config TPM_TIS_SANDBOX
> bool "Enable sandbox TPM driver"
> depends on SANDBOX
> diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile
> index 597966c..0d328f8 100644
> --- a/drivers/tpm/Makefile
> +++ b/drivers/tpm/Makefile
> @@ -3,6 +3,8 @@
> # SPDX-License-Identifier: GPL-2.0+
> #
>
> +obj-$(CONFIG_DM_TPM) += tpm-uclass.o
> +
> obj-$(CONFIG_TPM_ATMEL_TWI) += tpm_atmel_twi.o
> obj-$(CONFIG_TPM_TIS_I2C) += tpm_tis_i2c.o
> obj-$(CONFIG_TPM_TIS_LPC) += tpm_tis_lpc.o
> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> new file mode 100644
> index 0000000..ccade5b
> --- /dev/null
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright (c) 2015 Google, Inc
> + * Written by Simon Glass <sjg@chromium.org>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <tis.h>
> +
> +int tis_open(struct udevice *dev)
> +{
> + struct tpm_ops *ops = tpm_get_ops(dev);
> +
> + if (!ops->open)
> + return -ENOSYS;
> +
> + return ops->open(dev);
> +}
> +
> +int tis_close(struct udevice *dev)
> +{
> + struct tpm_ops *ops = tpm_get_ops(dev);
> +
> + if (!ops->close)
> + return -ENOSYS;
> +
> + return ops->close(dev);
> +}
> +
> +int tis_get_desc(struct udevice *dev, char *buf, int size)
> +{
> + struct tpm_ops *ops = tpm_get_ops(dev);
> +
> + if (!ops->get_desc)
> + return -ENOSYS;
> +
> + return ops->get_desc(dev, buf, size);
> +}
> +
> +int tis_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> + uint8_t *recvbuf, size_t *recv_size)
> +{
> + struct tpm_ops *ops = tpm_get_ops(dev);
> +
> + if (!ops->xfer)
> + return -ENOSYS;
> +
> + return ops->xfer(dev, sendbuf, send_size, recvbuf, recv_size);
> +}
tis_xfer could be more generic and rely on tpm_transmit from original tpm.c.
The command duration could be calculated at probe time during driver
initialisation running one single getcapability command.
> +
> +UCLASS_DRIVER(tpm) = {
> + .id = UCLASS_TPM,
> + .name = "tpm",
> + .flags = DM_UC_FLAG_SEQ_ALIAS,
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index c744044..3eff895 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -54,6 +54,7 @@ enum uclass_id {
> UCLASS_SPI_GENERIC, /* Generic SPI flash target */
> UCLASS_SYSCON, /* System configuration device */
> UCLASS_THERMAL, /* Thermal sensor */
> + UCLASS_TPM, /* Trusted Platform Module TIS interface */
> UCLASS_USB, /* USB bus */
> UCLASS_USB_DEV_GENERIC, /* USB generic device */
> UCLASS_USB_HUB, /* USB hub */
> diff --git a/include/tis.h b/include/tis.h
> index 40a1f86..6620554 100644
> --- a/include/tis.h
> +++ b/include/tis.h
> @@ -7,6 +7,102 @@
> #ifndef __TIS_H
> #define __TIS_H
>
> +#ifdef CONFIG_DM_TPM
> +struct tpm_ops {
As per a previous comment, an init handler could be usefull.
> + /**
> + * open() - Request access to locality 0 for the caller
> + *
> + * After all commands have been completed the caller should call
> + * tis_close().
> + *
> + * @dev: Device to close
> + * @return 0 ok OK, -ve on error
> + */
> + int (*open)(struct udevice *dev);
> +
> + /**
> + * tis_close() - Close the current session
> + *
> + * Releasing the locked locality. Returns 0 on success, -ve 1 on
> + * failure (in case lock removal did not succeed).
> + *
> + * @dev: Device to close
> + * @return 0 ok OK, -ve on error
> + */
> + int (*close)(struct udevice *dev);
> +
> + /**
> + * get_desc() - Get a text description of the TPM
> + *
> + * @dev: Device to check
> + * @buf: Buffer to put the string
> + * @size: Maximum size of buffer
> + * @return length of string, or -ENOSPC it no space
> + */
> + int (*get_desc)(struct udevice *dev, char *buf, int size);
> +
> + /**
> + * xfer() - send data to the TPM and get response
> + *
> + * @dev: Device to talk to
> + * @sendbuf: Buffer of the data to send
> + * @send_size: Size of the data to send
> + * @recvbuf: Buffer to save the response to
> + * @recv_size: Pointer to the size of the response buffer
> + *
> + * Returns 0 on success (and places the number of response bytes at
> + * recv_size) or -ve on failure.
> + */
> + int (*xfer)(struct udevice *dev, const uint8_t *sendbuf,
> + size_t send_size, uint8_t *recvbuf, size_t *recv_size);
> +};
> +
> +#define tpm_get_ops(dev) ((struct tpm_ops *)(dev)->driver->ops)
why not device_get_ops(dev) ?
> +
> +/*
> + * open() - Request access to locality 0 for the caller
> + *
> + * After all commands have been completed the caller is supposed to
> + * call tis_close().
> + *
> + * Returns 0 on success, -ve on failure.
> + */
> +int tis_open(struct udevice *dev);
> +
> +/*
> + * tis_close() - Close the current session
> + *
> + * Releasing the locked locality. Returns 0 on success, -ve 1 on
> + * failure (in case lock removal did not succeed).
> + */
> +int tis_close(struct udevice *dev);
> +
> +/**
> + * tis_get_desc() - Get a text description of the TPM
> + *
> + * @dev: Device to check
> + * @buf: Buffer to put the string
> + * @size: Maximum size of buffer
> + * @return length of string, or -ENOSPC it no space
> + */
> +int tis_get_desc(struct udevice *dev, char *buf, int size);
> +
> +/*
> + * tis_sendrecv() - send data to the TPM and get response
> + *
> + * @sendbuf - buffer of the data to send
> + * @send_size size of the data to send
> + * @recvbuf - memory to save the response to
> + * @recv_len - pointer to the size of the response buffer
> + *
> + * Returns 0 on success (and places the number of response bytes at
> + * recv_len) or -ve on failure.
> + */
> +int tis_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> + uint8_t *recvbuf, size_t *recv_size);
> +
As at the moment there is a 1 - 1 link with TPM and a platform, are you
sure udevice should be a parameter ?
> +#else
> +
> #include <common.h>
>
> /* Low-level interface to access TPM */
> @@ -53,5 +149,6 @@ int tis_close(void);
> */
> int tis_sendrecv(const uint8_t *sendbuf, size_t send_size, uint8_t *recvbuf,
> size_t *recv_len);
> +#endif
>
> #endif /* __TIS_H */
Best Regards
Christophe