Issue #433 has been updated by Michał Żygowski. Julius Werner wrote in #note-1:

If we want to do major changes to the TPM API I would prefer to use that opportunity to rather redesign it from scratch instead of perpetuating a bunch of weird design choices that haven't made sense in a while (or ever, really). A lot of that code was haphazardly copied from U-Boot in the early prototyping phase for TPM support and then never cleaned up or reevaluated to check if it actually makes any sense for coreboot.

For example, why do we have tis_init(), tis_open() and tis_close()? init() and open() are always called right after each other, and nothing in coreboot ever calls close(). The tpm_chip structure also makes no sense when it's just a container for tpm_vendor_specific where all the relevant things are stored in (and which isn't actually vendor-specific in all cases). The name "tis" (which technically stands for TPM Interface Specification) is also used in places where that descriptor doesn't actually make sense (to distinguish from the things just prefixed "tpm_").

For coreboot, the unifying TPM layer we have is in src/security/tss, specifically tpm_process_command() and tlcl_lib_init(). I don't think we really need any more interface-independent layers beneath that, those two can directly call into an init() and a sendrecv() implemented by the individual drivers (and those drivers can just keep what information they need in global variables because they're never instantiated more than once, no need for some complicated partially-common/partially-driver-specific structure construction). If you want to be able to enable more then one driver, then tlcl_lib_init() could call the init function for all of them and have the one that succeeds return a function pointer that is then used for sendrecv() or something like that.

Thank Julius for the feedback. True, most important is sendrecv, open and close seem to be obsolete. As for calling all the inits, that will be a little problem since in the current shape it could return success for multiple TPMs, e.g. some Infineon TPMs have the same IDs for TPM 1.2 and TPM 2.0. So rewriting it into a probe function (probing TPM_INTF_CAPABILITY and TPM_INTERFACE_ID) is necessary. What I meant was to make src/security/tss directory code compile for both TPMs and make it aware of which TPM it is dealing with by using the runtime detection (of course detection needs to be done once per stage only). Some parts of the API in src/security/tpm/tss.h are common for both TPM families, but some have different definitions and this must be dealt with too. ---------------------------------------- Feature #433: Unify TPM drivers in coreboot https://ticket.coreboot.org/issues/433#change-1224 * Author: Michał Żygowski * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-24 ---------------------------------------- Add an option to compile all drivers for TPM 1.2, 2.0 TIS and CRB. The motivation is to not build multiple coreboot ROMs for each possible TPM supported by the platform. The tasks would include: - runtime TPM detection (probing TPM_INTF_CAPABILITY and TPM_INTERFACE_ID) - rename the TPM driver functions, make them static and expose them as a driver structure, e.g. struct tpm_driver { void (*init)(void); int (*open)(void); int (*close)(void); int (*sendrecv)(const uint8_t *sendbuf, size_t send_size, uint8_t *recvbuf, size_t *recv_len); } - based on the detected TPM, hook the tpm_driver functions to provide the global TPM API: tis_open, tis_close, tis_init, tis_sendrecv. Some additional API to get vendor/device name could also be considered. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account

Issue #433 has been updated by Maciej Pijanowski. Sergii Dmytruk wrote in #note-3:

...

Add an option to compile all drivers for TPM 1.2, 2.0 TIS and CRB.

An option like "compile all of them" or just allow selecting any subset of the drivers?

I'd say select which drivers to include. Currently, one can include only one IIUC. There would be no point in including driver if we know that given platform will never come with given set of TPMs. For example, we have a laptop which can use either discrete TPM 2.0 or fTPM. We could give user selection which TPM should be used. Or, for example, use fTPM if discrete TPM 2.0 module is not present. Etc. ---------------------------------------- Feature #433: Unify TPM drivers in coreboot https://ticket.coreboot.org/issues/433#change-1226 * Author: Michał Żygowski * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-24 ---------------------------------------- Add an option to compile all drivers for TPM 1.2, 2.0 TIS and CRB. The motivation is to not build multiple coreboot ROMs for each possible TPM supported by the platform. The tasks would include: - runtime TPM detection (probing TPM_INTF_CAPABILITY and TPM_INTERFACE_ID) - rename the TPM driver functions, make them static and expose them as a driver structure, e.g. struct tpm_driver { void (*init)(void); int (*open)(void); int (*close)(void); int (*sendrecv)(const uint8_t *sendbuf, size_t send_size, uint8_t *recvbuf, size_t *recv_len); } - based on the detected TPM, hook the tpm_driver functions to provide the global TPM API: tis_open, tis_close, tis_init, tis_sendrecv. Some additional API to get vendor/device name could also be considered. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account

Issue #433 has been updated by Michał Żygowski. Sergii Dmytruk wrote in #note-5:

https://review.coreboot.org/c/coreboot/+/69162 was merged today, so I think this can be closed.

I have added 3 more patches that ought to be merged before we can consider it closed: https://review.coreboot.org/c/coreboot/+/80453 https://review.coreboot.org/c/coreboot/+/80454 https://review.coreboot.org/c/coreboot/+/80455 Without them, one cannot really use either fTPM or dTPM on Intel platform. ---------------------------------------- Feature #433: Unify TPM drivers in coreboot https://ticket.coreboot.org/issues/433#change-1808 * Author: Michał Żygowski * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-24 ---------------------------------------- Add an option to compile all drivers for TPM 1.2, 2.0 TIS and CRB. The motivation is to not build multiple coreboot ROMs for each possible TPM supported by the platform. The tasks would include: - runtime TPM detection (probing TPM_INTF_CAPABILITY and TPM_INTERFACE_ID) - rename the TPM driver functions, make them static and expose them as a driver structure, e.g. struct tpm_driver { void (*init)(void); int (*open)(void); int (*close)(void); int (*sendrecv)(const uint8_t *sendbuf, size_t send_size, uint8_t *recvbuf, size_t *recv_len); } - based on the detected TPM, hook the tpm_driver functions to provide the global TPM API: tis_open, tis_close, tis_init, tis_sendrecv. Some additional API to get vendor/device name could also be considered. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account