coreboot

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.

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.

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?

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.

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