Hi Aidan,
On 2026-05-13T00:26:04, Aidan Garske <[email protected]> wrote:
> cmd: refactor tpm2 command into frontend/backend architecture
>
> Split the tpm2 command implementation into a shared frontend and
> two selectable backends. This allows wolfTPM to provide its own
> TPM2 command implementations while keeping the command table,
> dispatcher, and help text shared.
>
> Architecture:
>
> cmd/tpm-v2.c (frontend - always compiled):
> Contains the tpm2_commands[] table, get_tpm2_commands(), the
> do_tpm2() dispatcher, and the U_BOOT_CMD help text. References
> backend functions via cmd/tpm2-backend.h. When CONFIG_TPM_WOLF
> is enabled, additional wolfTPM-only commands (caps, pcr_print,
> firmware_update, firmware_cancel) are added to the table.
>
> cmd/tpm2-backend.h (new):
> Declares all backend function prototypes that both backends must
> implement: do_tpm2_device, do_tpm2_info, do_tpm2_init,
> do_tpm2_startup, do_tpm2_selftest, do_tpm2_clear,
> [...]
>
> cmd/Kconfig | 11 +
> cmd/Makefile | 10 +-
> cmd/native_tpm2.c | 516 +++++++++++++++++++++++
> cmd/tpm-v2.c | 559 +++----------------------
> cmd/tpm2-backend.h | 66 +++
> cmd/wolftpm.c | 1170
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 1840 insertions(+), 492 deletions(-)
> diff --git a/cmd/wolftpm.c b/cmd/wolftpm.c
> @@ -0,0 +1,1170 @@
> +#define LOG_CATEGORY UCLASS_BOOTSTD
wolftpm.c has nothing to do with the bootstd uclass. Should be
UCLASS_TPM (or LOGC_NONE). Likely applies to other files in this
series too.
> diff --git a/cmd/wolftpm.c b/cmd/wolftpm.c
> @@ -0,0 +1,1170 @@
> + /* full test */
> + if (fullTest == YES) {
> + rc = wolfTPM2_SelfTest(&dev);
> + if (rc != TPM_RC_SUCCESS) {
> + log_debug("TPM2 Self Test: Result = 0x%x
> (%s)\n", rc,
> + TPM2_GetRCString(rc));
> + }
> + /* continue test */
> + } else {
> + rc = wolfTPM2_SelfTest(&dev);
> + if (rc != TPM_RC_SUCCESS) {
> + log_debug("TPM2 Self Test: Result = 0x%x
> (%s)\n", rc,
> + TPM2_GetRCString(rc));
> + }
> + }
Both branches do the same thing - continue is meant to be incremental,
not full. The native backend passes TPMI_YES/TPMI_NO to
tpm2_self_test() to distinguish. Please call the appropriate wolfTPM
API for the incremental case, or collapse the branches and document
that there's no difference.
> diff --git a/cmd/wolftpm.c b/cmd/wolftpm.c
> @@ -0,0 +1,1170 @@
> + rc = wolfTPM2_ExtendPCR(&dev, pcrIndex, algo, digest, digestLen);
> + if (rc != TPM_RC_SUCCESS) {
> + log_debug("TPM2_PCR_Extend failed 0x%x: %s\n", rc,
> + TPM2_GetRCString(rc));
> + }
> +
> + unmap_sysmem(digest);
> + wolfTPM2_Cleanup(&dev);
> +
> + log_debug("tpm2 pcr_extend: rc = %d (%s)\n", rc, TPM2_GetRCString(rc));
> +
> + return rc;
Most wolfTPM handlers return the raw TPM2 return code directly to the
shell. The native backend funnels everything through
report_return_code() which maps non-zero rc to CMD_RET_FAILURE and
prints a useful diagnostic. Returning a raw TPM_RC from a cmd_tbl
handler is not what the shell expects - please use
report_return_code() so the two backends behave the same from the
user's perspective.
> diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> @@ -511,7 +55,22 @@ struct cmd_tbl *get_tpm2_commands(unsigned int *size)
> +#ifdef CONFIG_TPM_WOLF
> + U_BOOT_CMD_MKENT(caps, 0, 1, do_tpm2_caps, "", ""),
> + U_BOOT_CMD_MKENT(pcr_print, 0, 1, do_tpm2_pcr_print, "", ""),
> +#ifdef WOLFTPM_FIRMWARE_UPGRADE
> +#if defined(WOLFTPM_SLB9672) || defined(WOLFTPM_SLB9673)
> + U_BOOT_CMD_MKENT(firmware_update, 0, 1,
> + do_tpm2_firmware_update, "", ""),
> + U_BOOT_CMD_MKENT(firmware_cancel, 0, 1,
> + do_tpm2_firmware_cancel, "", ""),
> +#endif /* WOLFTPM_SLB9672 || WOLFTPM_SLB9673 */
> +#endif /* WOLFTPM_FIRMWARE_UPGRADE */
> +#endif /* CONFIG_TPM_WOLF */
The supposedly-shared frontend now contains four levels of
backend-specific #ifdef in both the table and the help text. The
shared-command-table argument for splitting the file is weaker than it
looks. Worse, the inner #if defined(WOLFTPM_SLB9672)... tests a symbol
from a wolfTPM-private header, not a Kconfig - so tpm-v2.c now needs
the wolfTPM header search path even though it otherwise has nothing to
do with wolfTPM. A Kconfig (e.g. CONFIG_TPM_WOLF_FW_UPGRADE) would be
much cleaner.
> diff --git a/cmd/wolftpm.c b/cmd/wolftpm.c
> @@ -0,0 +1,1170 @@
> +int do_tpm2_device(struct cmd_tbl *cmdtp, int flag,
> + int argc, char *const argv[])
> +{
> + WOLFTPM2_DEV dev;
> + WOLFTPM2_CAPS caps;
> + int rc;
> +
> + /* Expected 1 arg only in native SPI mode (no device switching) */
> + if (argc != 1)
> + return CMD_RET_USAGE;
The shared help text still advertises 'device [num device]' but this
implementation rejects any argument. Either implement device switching
or trim the help text when CONFIG_TPM_WOLF is set - quietly diverging
from the documented interface is worse than either.
> diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> @@ -521,7 +80,7 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a
> TPMv2.x command",
> 'autostart\n'
> -" Initalize the tpm, perform a Startup(clear) and run a full selftest\n"
> +" Initialize the tpm, perform a Startup(clear) and run a full selftest\n"
> @@ -573,6 +132,10 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a
> TPMv2.x command",
> "change_auth <hierarchy> <new_pw> [<old_pw>]\n"
> " <hierarchy>: the hierarchy\n"
> +" * TPM2_RH_LOCKOUT\n"
> +" * TPM2_RH_ENDORSEMENT\n"
> +" * TPM2_RH_OWNER\n"
> +" * TPM2_RH_PLATFORM\n"
These are independent improvements - please split into their own
patches so they can be picked up even if the wolfTPM bits don't land.
> diff --git a/cmd/wolftpm.c b/cmd/wolftpm.c
> @@ -0,0 +1,1170 @@
> +#include <wolftpm/tpm2.h>
> +#include <wolftpm/tpm2_wrap.h>
> +#include <wolftpm/tpm2_packet.h>
> +#include <wolftpm.h>
> +
> +#include <stdio.h>
> +#include <hash.h>
Include ordering is also upside-down: U-Boot convention is <command.h>
and core headers first, then driver/library headers. The file also
uses camelCase locals
(doStartup/fullTest/pcrIndex/digestLen/manifest_bufSz) and
uint8_t/uint16_t/uint32_t throughout; U-Boot style is snake_case and
u8/u16/u32. Please run patman / checkpatch and adjust.
Regards,
Simon
