| Message ID | 20250320152433.144083-3-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: add send_recv() op and use it in tpm_ftpm_tee driver | expand |
On Thu, Mar 20, 2025 at 04:24:33PM +0100, Stefano Garzarella wrote: > From: Stefano Garzarella <sgarzare@redhat.com> > > This driver does not support interrupts, and receiving the response is > synchronous with sending the command. > > It used an internal buffer to cache the response when .send() is called, > and then return it when .recv() is called. > > Let's simplify the driver by implementing the new send_recv() op, so that > we can also remove the 4KB internal buffer used to cache the response. > > Tested-by: Jens Wiklander <jens.wiklander@linaro.org> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > v1: > - added Jens' T-b > --- > drivers/char/tpm/tpm_ftpm_tee.h | 4 -- > drivers/char/tpm/tpm_ftpm_tee.c | 86 ++++++++------------------------- > 2 files changed, 21 insertions(+), 69 deletions(-) > Reviewed-by: Sumit Garg <sumit.garg@kernel.org> -Sumit > diff --git a/drivers/char/tpm/tpm_ftpm_tee.h b/drivers/char/tpm/tpm_ftpm_tee.h > index e39903b7ea07..8d5c3f0d2879 100644 > --- a/drivers/char/tpm/tpm_ftpm_tee.h > +++ b/drivers/char/tpm/tpm_ftpm_tee.h > @@ -22,16 +22,12 @@ > * struct ftpm_tee_private - fTPM's private data > * @chip: struct tpm_chip instance registered with tpm framework. > * @session: fTPM TA session identifier. > - * @resp_len: cached response buffer length. > - * @resp_buf: cached response buffer. > * @ctx: TEE context handler. > * @shm: Memory pool shared with fTPM TA in TEE. > */ > struct ftpm_tee_private { > struct tpm_chip *chip; > u32 session; > - size_t resp_len; > - u8 resp_buf[MAX_RESPONSE_SIZE]; > struct tee_context *ctx; > struct tee_shm *shm; > }; > diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c > index 8d9209dfc384..d472199c0a7b 100644 > --- a/drivers/char/tpm/tpm_ftpm_tee.c > +++ b/drivers/char/tpm/tpm_ftpm_tee.c > @@ -31,45 +31,19 @@ static const uuid_t ftpm_ta_uuid = > 0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96); > > /** > - * ftpm_tee_tpm_op_recv() - retrieve fTPM response. > - * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h. > - * @buf: the buffer to store data. > - * @count: the number of bytes to read. > - * > - * Return: > - * In case of success the number of bytes received. > - * On failure, -errno. > - */ > -static int ftpm_tee_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count) > -{ > - struct ftpm_tee_private *pvt_data = dev_get_drvdata(chip->dev.parent); > - size_t len; > - > - len = pvt_data->resp_len; > - if (count < len) { > - dev_err(&chip->dev, > - "%s: Invalid size in recv: count=%zd, resp_len=%zd\n", > - __func__, count, len); > - return -EIO; > - } > - > - memcpy(buf, pvt_data->resp_buf, len); > - pvt_data->resp_len = 0; > - > - return len; > -} > - > -/** > - * ftpm_tee_tpm_op_send() - send TPM commands through the TEE shared memory. > + * ftpm_tee_tpm_op_send_recv() - send TPM commands through the TEE shared memory > + * and retrieve the response. > * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h > - * @buf: the buffer to send. > - * @len: the number of bytes to send. > + * @buf: the buffer to send and to store the response. > + * @buf_len: the size of the buffer. > + * @cmd_len: the number of bytes to send. > * > * Return: > - * In case of success, returns 0. > + * In case of success, returns the number of bytes received. > * On failure, -errno > */ > -static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len) > +static int ftpm_tee_tpm_op_send_recv(struct tpm_chip *chip, u8 *buf, > + size_t buf_len, size_t cmd_len) > { > struct ftpm_tee_private *pvt_data = dev_get_drvdata(chip->dev.parent); > size_t resp_len; > @@ -80,16 +54,15 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len) > struct tee_param command_params[4]; > struct tee_shm *shm = pvt_data->shm; > > - if (len > MAX_COMMAND_SIZE) { > + if (cmd_len > MAX_COMMAND_SIZE) { > dev_err(&chip->dev, > "%s: len=%zd exceeds MAX_COMMAND_SIZE supported by fTPM TA\n", > - __func__, len); > + __func__, cmd_len); > return -EIO; > } > > memset(&transceive_args, 0, sizeof(transceive_args)); > memset(command_params, 0, sizeof(command_params)); > - pvt_data->resp_len = 0; > > /* Invoke FTPM_OPTEE_TA_SUBMIT_COMMAND function of fTPM TA */ > transceive_args = (struct tee_ioctl_invoke_arg) { > @@ -103,7 +76,7 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len) > .attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT, > .u.memref = { > .shm = shm, > - .size = len, > + .size = cmd_len, > .shm_offs = 0, > }, > }; > @@ -115,7 +88,7 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len) > return PTR_ERR(temp_buf); > } > memset(temp_buf, 0, (MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE)); > - memcpy(temp_buf, buf, len); > + memcpy(temp_buf, buf, cmd_len); > > command_params[1] = (struct tee_param) { > .attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT, > @@ -156,38 +129,21 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len) > __func__, resp_len); > return -EIO; > } > + if (resp_len > buf_len) { > + dev_err(&chip->dev, > + "%s: Invalid size in recv: buf_len=%zd, resp_len=%zd\n", > + __func__, buf_len, resp_len); > + return -EIO; > + } > > - /* sanity checks look good, cache the response */ > - memcpy(pvt_data->resp_buf, temp_buf, resp_len); > - pvt_data->resp_len = resp_len; > - > - return 0; > -} > - > -static void ftpm_tee_tpm_op_cancel(struct tpm_chip *chip) > -{ > - /* not supported */ > -} > - > -static u8 ftpm_tee_tpm_op_status(struct tpm_chip *chip) > -{ > - return 0; > -} > + memcpy(buf, temp_buf, resp_len); > > -static bool ftpm_tee_tpm_req_canceled(struct tpm_chip *chip, u8 status) > -{ > - return false; > + return resp_len; > } > > static const struct tpm_class_ops ftpm_tee_tpm_ops = { > .flags = TPM_OPS_AUTO_STARTUP, > - .recv = ftpm_tee_tpm_op_recv, > - .send = ftpm_tee_tpm_op_send, > - .cancel = ftpm_tee_tpm_op_cancel, > - .status = ftpm_tee_tpm_op_status, > - .req_complete_mask = 0, > - .req_complete_val = 0, > - .req_canceled = ftpm_tee_tpm_req_canceled, > + .send_recv = ftpm_tee_tpm_op_send_recv, > }; > > /* > -- > 2.48.1 >
On Tue, Mar 25, 2025 at 10:49:38AM +0530, Sumit Garg wrote: > On Thu, Mar 20, 2025 at 04:24:33PM +0100, Stefano Garzarella wrote: > > From: Stefano Garzarella <sgarzare@redhat.com> > > > > This driver does not support interrupts, and receiving the response is > > synchronous with sending the command. > > > > It used an internal buffer to cache the response when .send() is called, > > and then return it when .recv() is called. > > > > Let's simplify the driver by implementing the new send_recv() op, so that > > we can also remove the 4KB internal buffer used to cache the response. > > > > Tested-by: Jens Wiklander <jens.wiklander@linaro.org> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > --- > > v1: > > - added Jens' T-b > > --- > > drivers/char/tpm/tpm_ftpm_tee.h | 4 -- > > drivers/char/tpm/tpm_ftpm_tee.c | 86 ++++++++------------------------- > > 2 files changed, 21 insertions(+), 69 deletions(-) > > > > Reviewed-by: Sumit Garg <sumit.garg@kernel.org> I've knowingly even peeked at these patches because of stuff in-flight. Generally speaking I don't see enough value in complicating callback interface. It's better to handle complications in the leaves (i.e. dictatorship of majority ;-) ). BR, Jarkko
On Wed, Mar 26, 2025 at 02:11:12PM +0200, Jarkko Sakkinen wrote: > Generally speaking I don't see enough value in complicating > callback interface. It's better to handle complications in > the leaves (i.e. dictatorship of majority ;-) ). That is very much not the way most driver subsystems view the world. We want to pull logical things into the core code and remove them from drivers to make the drivers simpler and more robust. The amount of really dumb driver boiler plate that this series obviously removes is exactly the sort of stuff we should be fixing by improving the core code. The callback interface was never really sanely designed, it was just built around the idea of pulling the timout processing into the core code for TIS hardware. It should be revised to properly match these new HW types that don't have this kind of timeout mechanism. Jason
On Wed, Mar 26, 2025 at 11:34:01AM -0300, Jason Gunthorpe wrote: > On Wed, Mar 26, 2025 at 02:11:12PM +0200, Jarkko Sakkinen wrote: > > > Generally speaking I don't see enough value in complicating > > callback interface. It's better to handle complications in > > the leaves (i.e. dictatorship of majority ;-) ). > > That is very much not the way most driver subsystems view the > world. We want to pull logical things into the core code and remove > them from drivers to make the drivers simpler and more robust. > > The amount of really dumb driver boiler plate that this series > obviously removes is exactly the sort of stuff we should be fixing by > improving the core code. > > The callback interface was never really sanely designed, it was just > built around the idea of pulling the timout processing into the core > code for TIS hardware. It should be revised to properly match these > new HW types that don't have this kind of timeout mechanism. Both TIS and CRB, which are TCG standards and they span to many different types of drivers and busses. I don't have the figures but probably they cover vast majority of the hardware. We are talking about 39 lines of reduced complexity at the cost of complicating branching at the top level. I doubt that there is either any throughput or latency issues. What is measurable benefit? The rationale is way way too abstract for me to cope, sorry. > Jason BR, Jarkko
On Wed, Mar 26, 2025 at 04:57:47PM +0200, Jarkko Sakkinen wrote: > On Wed, Mar 26, 2025 at 11:34:01AM -0300, Jason Gunthorpe wrote: > > On Wed, Mar 26, 2025 at 02:11:12PM +0200, Jarkko Sakkinen wrote: > > > > > Generally speaking I don't see enough value in complicating > > > callback interface. It's better to handle complications in > > > the leaves (i.e. dictatorship of majority ;-) ). > > > > That is very much not the way most driver subsystems view the > > world. We want to pull logical things into the core code and remove > > them from drivers to make the drivers simpler and more robust. > > > > The amount of really dumb driver boiler plate that this series > > obviously removes is exactly the sort of stuff we should be fixing by > > improving the core code. > > > > The callback interface was never really sanely designed, it was just > > built around the idea of pulling the timout processing into the core > > code for TIS hardware. It should be revised to properly match these > > new HW types that don't have this kind of timeout mechanism. > > Both TIS and CRB, which are TCG standards and they span to many > different types of drivers and busses. I don't have the figures but > probably they cover vast majority of the hardware. > > We are talking about 39 lines of reduced complexity at the cost > of complicating branching at the top level. I doubt that there > is either any throughput or latency issues. > > What is measurable benefit? The rationale is way way too abstract > for me to cope, sorry. E.g., here's how you can get rid of extra cruft in tpm_ftpm_tee w/o any new callbacks. BR, Jarkko
On Wed, Mar 26, 2025 at 05:58:33PM +0200, Jarkko Sakkinen wrote: > On Wed, Mar 26, 2025 at 04:57:47PM +0200, Jarkko Sakkinen wrote: > > On Wed, Mar 26, 2025 at 11:34:01AM -0300, Jason Gunthorpe wrote: > > > On Wed, Mar 26, 2025 at 02:11:12PM +0200, Jarkko Sakkinen wrote: > > > > > > > Generally speaking I don't see enough value in complicating > > > > callback interface. It's better to handle complications in > > > > the leaves (i.e. dictatorship of majority ;-) ). > > > > > > That is very much not the way most driver subsystems view the > > > world. We want to pull logical things into the core code and remove > > > them from drivers to make the drivers simpler and more robust. > > > > > > The amount of really dumb driver boiler plate that this series > > > obviously removes is exactly the sort of stuff we should be fixing by > > > improving the core code. > > > > > > The callback interface was never really sanely designed, it was just > > > built around the idea of pulling the timout processing into the core > > > code for TIS hardware. It should be revised to properly match these > > > new HW types that don't have this kind of timeout mechanism. > > > > Both TIS and CRB, which are TCG standards and they span to many > > different types of drivers and busses. I don't have the figures but > > probably they cover vast majority of the hardware. > > > > We are talking about 39 lines of reduced complexity at the cost > > of complicating branching at the top level. I doubt that there > > is either any throughput or latency issues. > > > > What is measurable benefit? The rationale is way way too abstract > > for me to cope, sorry. > > E.g., here's how you can get rid of extra cruft in tpm_ftpm_tee w/o > any new callbacks. Measurable benefit: no need to allocate memory buffer. Let's take that as a starting point ;-) On that basis I can consider this (i.e. something to measure). BR, Jarkko
diff --git a/drivers/char/tpm/tpm_ftpm_tee.h b/drivers/char/tpm/tpm_ftpm_tee.h index e39903b7ea07..8d5c3f0d2879 100644 --- a/drivers/char/tpm/tpm_ftpm_tee.h +++ b/drivers/char/tpm/tpm_ftpm_tee.h @@ -22,16 +22,12 @@ * struct ftpm_tee_private - fTPM's private data * @chip: struct tpm_chip instance registered with tpm framework. * @session: fTPM TA session identifier. - * @resp_len: cached response buffer length. - * @resp_buf: cached response buffer. * @ctx: TEE context handler. * @shm: Memory pool shared with fTPM TA in TEE. */ struct ftpm_tee_private { struct tpm_chip *chip; u32 session; - size_t resp_len; - u8 resp_buf[MAX_RESPONSE_SIZE]; struct tee_context *ctx; struct tee_shm *shm; }; diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c index 8d9209dfc384..d472199c0a7b 100644 --- a/drivers/char/tpm/tpm_ftpm_tee.c +++ b/drivers/char/tpm/tpm_ftpm_tee.c @@ -31,45 +31,19 @@ static const uuid_t ftpm_ta_uuid = 0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96); /** - * ftpm_tee_tpm_op_recv() - retrieve fTPM response. - * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h. - * @buf: the buffer to store data. - * @count: the number of bytes to read. - * - * Return: - * In case of success the number of bytes received. - * On failure, -errno. - */ -static int ftpm_tee_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count) -{ - struct ftpm_tee_private *pvt_data = dev_get_drvdata(chip->dev.parent); - size_t len; - - len = pvt_data->resp_len; - if (count < len) { - dev_err(&chip->dev, - "%s: Invalid size in recv: count=%zd, resp_len=%zd\n", - __func__, count, len); - return -EIO; - } - - memcpy(buf, pvt_data->resp_buf, len); - pvt_data->resp_len = 0; - - return len; -} - -/** - * ftpm_tee_tpm_op_send() - send TPM commands through the TEE shared memory. + * ftpm_tee_tpm_op_send_recv() - send TPM commands through the TEE shared memory + * and retrieve the response. * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h - * @buf: the buffer to send. - * @len: the number of bytes to send. + * @buf: the buffer to send and to store the response. + * @buf_len: the size of the buffer. + * @cmd_len: the number of bytes to send. * * Return: - * In case of success, returns 0. + * In case of success, returns the number of bytes received. * On failure, -errno */ -static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len) +static int ftpm_tee_tpm_op_send_recv(struct tpm_chip *chip, u8 *buf, + size_t buf_len, size_t cmd_len) { struct ftpm_tee_private *pvt_data = dev_get_drvdata(chip->dev.parent); size_t resp_len; @@ -80,16 +54,15 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len) struct tee_param command_params[4]; struct tee_shm *shm = pvt_data->shm; - if (len > MAX_COMMAND_SIZE) { + if (cmd_len > MAX_COMMAND_SIZE) { dev_err(&chip->dev, "%s: len=%zd exceeds MAX_COMMAND_SIZE supported by fTPM TA\n", - __func__, len); + __func__, cmd_len); return -EIO; } memset(&transceive_args, 0, sizeof(transceive_args)); memset(command_params, 0, sizeof(command_params)); - pvt_data->resp_len = 0; /* Invoke FTPM_OPTEE_TA_SUBMIT_COMMAND function of fTPM TA */ transceive_args = (struct tee_ioctl_invoke_arg) { @@ -103,7 +76,7 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len) .attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT, .u.memref = { .shm = shm, - .size = len, + .size = cmd_len, .shm_offs = 0, }, }; @@ -115,7 +88,7 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len) return PTR_ERR(temp_buf); } memset(temp_buf, 0, (MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE)); - memcpy(temp_buf, buf, len); + memcpy(temp_buf, buf, cmd_len); command_params[1] = (struct tee_param) { .attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT, @@ -156,38 +129,21 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len) __func__, resp_len); return -EIO; } + if (resp_len > buf_len) { + dev_err(&chip->dev, + "%s: Invalid size in recv: buf_len=%zd, resp_len=%zd\n", + __func__, buf_len, resp_len); + return -EIO; + } - /* sanity checks look good, cache the response */ - memcpy(pvt_data->resp_buf, temp_buf, resp_len); - pvt_data->resp_len = resp_len; - - return 0; -} - -static void ftpm_tee_tpm_op_cancel(struct tpm_chip *chip) -{ - /* not supported */ -} - -static u8 ftpm_tee_tpm_op_status(struct tpm_chip *chip) -{ - return 0; -} + memcpy(buf, temp_buf, resp_len); -static bool ftpm_tee_tpm_req_canceled(struct tpm_chip *chip, u8 status) -{ - return false; + return resp_len; } static const struct tpm_class_ops ftpm_tee_tpm_ops = { .flags = TPM_OPS_AUTO_STARTUP, - .recv = ftpm_tee_tpm_op_recv, - .send = ftpm_tee_tpm_op_send, - .cancel = ftpm_tee_tpm_op_cancel, - .status = ftpm_tee_tpm_op_status, - .req_complete_mask = 0, - .req_complete_val = 0, - .req_canceled = ftpm_tee_tpm_req_canceled, + .send_recv = ftpm_tee_tpm_op_send_recv, }; /*