From c03cb022305b40e3efd7271c055e8bc6aefa7f75 Mon Sep 17 00:00:00 2001 From: Tim Harvey Date: Fri, 16 Jul 2021 15:44:12 -0700 Subject: [PATCH 01/23] common: board_r: print error if binman_init fails Display an error if binman_init fails. Signed-off-by: Tim Harvey --- common/board_r.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/common/board_r.c b/common/board_r.c index 3f824047727..e3e6248a1fd 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -323,10 +323,16 @@ static int initr_manual_reloc_cmdtable(void) static int initr_binman(void) { + int ret; + if (!CONFIG_IS_ENABLED(BINMAN_FDT)) return 0; - return binman_init(); + ret = binman_init(); + if (ret) + printf("binman_init failed:%d\n", ret); + + return ret; } #if defined(CONFIG_MTD_NOR_FLASH) From 974c98f26c765f323433b0c52e02ea3a777bc80f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 18 Jul 2021 14:17:57 -0600 Subject: [PATCH 02/23] sandbox: tpm: Split out common nvdata code We want to support nvdata in TPM2 as well. To avoid code duplicating the associated code, move it into a common file. Drop the special-case logic for the kernel space. This can be handled by the higher-level code now, i.e. in vboot itself. Signed-off-by: Simon Glass --- drivers/tpm/Makefile | 4 +- drivers/tpm/sandbox_common.c | 66 ++++++++++++++++++++ drivers/tpm/sandbox_common.h | 96 +++++++++++++++++++++++++++++ drivers/tpm/tpm_tis_sandbox.c | 111 +++------------------------------- 4 files changed, 172 insertions(+), 105 deletions(-) create mode 100644 drivers/tpm/sandbox_common.c create mode 100644 drivers/tpm/sandbox_common.h diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile index f64d20067f8..c65be526700 100644 --- a/drivers/tpm/Makefile +++ b/drivers/tpm/Makefile @@ -6,11 +6,11 @@ obj-$(CONFIG_$(SPL_TPL_)TPM) += tpm-uclass.o obj-$(CONFIG_TPM_ATMEL_TWI) += tpm_atmel_twi.o obj-$(CONFIG_TPM_TIS_INFINEON) += tpm_tis_infineon.o obj-$(CONFIG_TPM_TIS_LPC) += tpm_tis_lpc.o -obj-$(CONFIG_TPM_TIS_SANDBOX) += tpm_tis_sandbox.o +obj-$(CONFIG_TPM_TIS_SANDBOX) += tpm_tis_sandbox.o sandbox_common.o obj-$(CONFIG_TPM_ST33ZP24_I2C) += tpm_tis_st33zp24_i2c.o obj-$(CONFIG_TPM_ST33ZP24_SPI) += tpm_tis_st33zp24_spi.o obj-$(CONFIG_$(SPL_TPL_)TPM2_CR50_I2C) += cr50_i2c.o -obj-$(CONFIG_TPM2_TIS_SANDBOX) += tpm2_tis_sandbox.o +obj-$(CONFIG_TPM2_TIS_SANDBOX) += tpm2_tis_sandbox.o sandbox_common.o obj-$(CONFIG_TPM2_TIS_SPI) += tpm2_tis_spi.o obj-$(CONFIG_TPM2_FTPM_TEE) += tpm2_ftpm_tee.o diff --git a/drivers/tpm/sandbox_common.c b/drivers/tpm/sandbox_common.c new file mode 100644 index 00000000000..13f5e030a5f --- /dev/null +++ b/drivers/tpm/sandbox_common.c @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Common features for sandbox TPM1 and TPM2 implementations + * + * Copyright 2021 Google LLC + */ + +#define LOG_CATEGORY UCLASS_TPM + +#include +#include +#include +#include +#include "sandbox_common.h" + +#define TPM_ERR_CODE_OFS (2 + 4) /* after tag and size */ + +int sb_tpm_index_to_seq(u32 index) +{ + index &= ~HR_NV_INDEX; + switch (index) { + case FIRMWARE_NV_INDEX: + return NV_SEQ_FIRMWARE; + case KERNEL_NV_INDEX: + return NV_SEQ_KERNEL; + case BACKUP_NV_INDEX: + return NV_SEQ_BACKUP; + case FWMP_NV_INDEX: + return NV_SEQ_FWMP; + case MRC_REC_HASH_NV_INDEX: + return NV_SEQ_REC_HASH; + case 0: + return NV_SEQ_GLOBAL_LOCK; + case TPM_NV_INDEX_LOCK: + return NV_SEQ_ENABLE_LOCKING; + } + + printf("Invalid nv index %#x\n", index); + return -1; +} + +void sb_tpm_read_data(const struct nvdata_state nvdata[NV_SEQ_COUNT], + enum sandbox_nv_space seq, u8 *buf, int data_ofs, + int length) +{ + const struct nvdata_state *nvd = &nvdata[seq]; + + if (!nvd->present) + put_unaligned_be32(TPM_BADINDEX, buf + TPM_ERR_CODE_OFS); + else if (length > nvd->length) + put_unaligned_be32(TPM_BAD_DATASIZE, buf + TPM_ERR_CODE_OFS); + else + memcpy(buf + data_ofs, &nvd->data, length); +} + +void sb_tpm_write_data(struct nvdata_state nvdata[NV_SEQ_COUNT], + enum sandbox_nv_space seq, const u8 *buf, int data_ofs, + int length) +{ + struct nvdata_state *nvd = &nvdata[seq]; + + if (length > nvd->length) + log_err("Invalid length %x (max %x)\n", length, nvd->length); + else + memcpy(&nvdata[seq].data, buf + data_ofs, length); +} diff --git a/drivers/tpm/sandbox_common.h b/drivers/tpm/sandbox_common.h new file mode 100644 index 00000000000..aa5292d7945 --- /dev/null +++ b/drivers/tpm/sandbox_common.h @@ -0,0 +1,96 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Common features for sandbox TPM1 and TPM2 implementations + * + * Copyright 2021 Google LLC + */ + +#ifndef __TPM_SANDBOX_COMMON_H +#define __TPM_SANDBOX_COMMON_H + +/* + * These numbers derive from adding the sizes of command fields as shown in + * the TPM commands manual. + */ +#define TPM_HDR_LEN 10 + +/* These are the different non-volatile spaces that we emulate */ +enum sandbox_nv_space { + NV_SEQ_ENABLE_LOCKING, + NV_SEQ_GLOBAL_LOCK, + NV_SEQ_FIRMWARE, + NV_SEQ_KERNEL, + NV_SEQ_BACKUP, + NV_SEQ_FWMP, + NV_SEQ_REC_HASH, + + NV_SEQ_COUNT, +}; + +/* TPM NVRAM location indices */ +#define FIRMWARE_NV_INDEX 0x1007 +#define KERNEL_NV_INDEX 0x1008 +#define BACKUP_NV_INDEX 0x1009 +#define FWMP_NV_INDEX 0x100a +#define MRC_REC_HASH_NV_INDEX 0x100b + +/* Size of each non-volatile space */ +#define NV_DATA_SIZE 0x28 + +/** + * struct nvdata_state - state of a single non-volatile-data 'space' + * + * @present: true if present + * @length: length in bytes (max NV_DATA_SIZE) + * @data: contents of non-volatile space + */ +struct nvdata_state { + bool present; + int length; + u8 data[NV_DATA_SIZE]; +}; + +/** + * sb_tpm_index_to_seq() - convert an index into a space sequence number + * + * This converts the index as used by the vboot code into an internal sequence + * number used by the sandbox emulation. + * + * @index: Index to use (FIRMWARE_NV_INDEX, etc.) + * @return associated space (enum sandbox_nv_space) + */ +int sb_tpm_index_to_seq(uint index); + +/** + * sb_tpm_read_data() - Read non-volatile data + * + * This handles a TPM read of nvdata. If the nvdata is not present, a + * TPM_BADINDEX error is put in the buffer. If @length is too large, + * TPM_BAD_DATASIZE is put in the buffer. + * + * @nvdata: Current nvdata state + * @seq: Sequence number to read + * @recvbuf: Buffer to update with the TPM response, assumed to contain zeroes + * @data_ofs: Offset of the 'data' portion of @recvbuf + * @length: Number of bytes to read + */ +void sb_tpm_read_data(const struct nvdata_state nvdata[NV_SEQ_COUNT], + enum sandbox_nv_space seq, u8 *recvbuf, int data_ofs, + int length); + +/** + * sb_tpm_write_data() - Write non-volatile data + * + * If @length is too large, an error is logged and nothing is written. + * + * @nvdata: Current nvdata state + * @seq: Sequence number to read + * @buf: Buffer containing the data to write + * @data_ofs: Offset of the 'data' portion of @buf + * @length: Number of bytes to write + */ +void sb_tpm_write_data(struct nvdata_state nvdata[NV_SEQ_COUNT], + enum sandbox_nv_space seq, const u8 *buf, int data_ofs, + int length); + +#endif diff --git a/drivers/tpm/tpm_tis_sandbox.c b/drivers/tpm/tpm_tis_sandbox.c index 67139cea3be..294d98da606 100644 --- a/drivers/tpm/tpm_tis_sandbox.c +++ b/drivers/tpm/tpm_tis_sandbox.c @@ -9,61 +9,10 @@ #include #include #include - -/* TPM NVRAM location indices. */ -#define FIRMWARE_NV_INDEX 0x1007 -#define KERNEL_NV_INDEX 0x1008 -#define BACKUP_NV_INDEX 0x1009 -#define FWMP_NV_INDEX 0x100a -#define REC_HASH_NV_INDEX 0x100b -#define REC_HASH_NV_SIZE VB2_SHA256_DIGEST_SIZE +#include "sandbox_common.h" #define NV_DATA_PUBLIC_PERMISSIONS_OFFSET 60 -/* Kernel TPM space - KERNEL_NV_INDEX, locked with physical presence */ -#define ROLLBACK_SPACE_KERNEL_VERSION 2 -#define ROLLBACK_SPACE_KERNEL_UID 0x4752574C /* 'GRWL' */ - -struct rollback_space_kernel { - /* Struct version, for backwards compatibility */ - uint8_t struct_version; - /* Unique ID to detect space redefinition */ - uint32_t uid; - /* Kernel versions */ - uint32_t kernel_versions; - /* Reserved for future expansion */ - uint8_t reserved[3]; - /* Checksum (v2 and later only) */ - uint8_t crc8; -} __packed rollback_space_kernel; - -/* - * These numbers derive from adding the sizes of command fields as shown in - * the TPM commands manual. - */ -#define TPM_REQUEST_HEADER_LENGTH 10 -#define TPM_RESPONSE_HEADER_LENGTH 10 - -/* These are the different non-volatile spaces that we emulate */ -enum { - NV_GLOBAL_LOCK, - NV_SEQ_FIRMWARE, - NV_SEQ_KERNEL, - NV_SEQ_BACKUP, - NV_SEQ_FWMP, - NV_SEQ_REC_HASH, - - NV_SEQ_COUNT, -}; - -/* Size of each non-volatile space */ -#define NV_DATA_SIZE 0x20 - -struct nvdata_state { - bool present; - u8 data[NV_DATA_SIZE]; -}; - /* * Information about our TPM emulation. This is preserved in the sandbox * state file if enabled. @@ -140,27 +89,6 @@ static int sandbox_tpm_write_state(void *blob, int node) SANDBOX_STATE_IO(sandbox_tpm, "google,sandbox-tpm", sandbox_tpm_read_state, sandbox_tpm_write_state); -static int index_to_seq(uint32_t index) -{ - switch (index) { - case FIRMWARE_NV_INDEX: - return NV_SEQ_FIRMWARE; - case KERNEL_NV_INDEX: - return NV_SEQ_KERNEL; - case BACKUP_NV_INDEX: - return NV_SEQ_BACKUP; - case FWMP_NV_INDEX: - return NV_SEQ_FWMP; - case REC_HASH_NV_INDEX: - return NV_SEQ_REC_HASH; - case 0: - return NV_GLOBAL_LOCK; - } - - printf("Invalid nv index %#x\n", index); - return -1; -} - static void handle_cap_flag_space(u8 **datap, uint index) { struct tpm_nv_data_public pub; @@ -246,48 +174,25 @@ static int sandbox_tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, case TPM_CMD_NV_WRITE_VALUE: index = get_unaligned_be32(sendbuf + 10); length = get_unaligned_be32(sendbuf + 18); - seq = index_to_seq(index); + seq = sb_tpm_index_to_seq(index); if (seq < 0) return -EINVAL; printf("tpm: nvwrite index=%#02x, len=%#02x\n", index, length); - memcpy(&tpm->nvdata[seq].data, sendbuf + 22, length); - tpm->nvdata[seq].present = true; - *recv_len = 12; - memset(recvbuf, '\0', *recv_len); + sb_tpm_write_data(tpm->nvdata, seq, sendbuf, 22, length); break; case TPM_CMD_NV_READ_VALUE: /* nvread */ index = get_unaligned_be32(sendbuf + 10); length = get_unaligned_be32(sendbuf + 18); - seq = index_to_seq(index); + seq = sb_tpm_index_to_seq(index); if (seq < 0) return -EINVAL; printf("tpm: nvread index=%#02x, len=%#02x, seq=%#02x\n", index, length, seq); - *recv_len = TPM_RESPONSE_HEADER_LENGTH + sizeof(uint32_t) + - length; + *recv_len = TPM_HDR_LEN + sizeof(uint32_t) + length; memset(recvbuf, '\0', *recv_len); - put_unaligned_be32(length, recvbuf + - TPM_RESPONSE_HEADER_LENGTH); - if (seq == NV_SEQ_KERNEL) { - struct rollback_space_kernel rsk; - - data = recvbuf + TPM_RESPONSE_HEADER_LENGTH + - sizeof(uint32_t); - memset(&rsk, 0, sizeof(struct rollback_space_kernel)); - rsk.struct_version = 2; - rsk.uid = ROLLBACK_SPACE_KERNEL_UID; - rsk.crc8 = crc8(0, (unsigned char *)&rsk, - offsetof(struct rollback_space_kernel, - crc8)); - memcpy(data, &rsk, sizeof(rsk)); - } else if (!tpm->nvdata[seq].present) { - put_unaligned_be32(TPM_BADINDEX, recvbuf + - sizeof(uint16_t) + sizeof(uint32_t)); - } else { - memcpy(recvbuf + TPM_RESPONSE_HEADER_LENGTH + - sizeof(uint32_t), &tpm->nvdata[seq].data, - length); - } + put_unaligned_be32(length, recvbuf + TPM_HDR_LEN); + sb_tpm_read_data(tpm->nvdata, seq, recvbuf, TPM_HDR_LEN + 4, + length); break; case TPM_CMD_EXTEND: *recv_len = 30; From 1db235a186c001a50e5f5db9b587bbaa4e397ee7 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 18 Jul 2021 14:17:58 -0600 Subject: [PATCH 03/23] sandbox: tpm: Tidy up reading and writing of device state At present this code assumes that the TPM data has been read but this may not be the case. Refactor the code to use a separate pointer so we know the current state of the data. Add error checking for the data size. Signed-off-by: Simon Glass --- drivers/tpm/tpm_tis_sandbox.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/drivers/tpm/tpm_tis_sandbox.c b/drivers/tpm/tpm_tis_sandbox.c index 294d98da606..f22ed846f0a 100644 --- a/drivers/tpm/tpm_tis_sandbox.c +++ b/drivers/tpm/tpm_tis_sandbox.c @@ -20,7 +20,7 @@ static struct tpm_state { bool valid; struct nvdata_state nvdata[NV_SEQ_COUNT]; -} g_state; +} s_state, *g_state; /** * sandbox_tpm_read_state() - read the sandbox EC state from the state file @@ -33,6 +33,7 @@ static struct tpm_state { */ static int sandbox_tpm_read_state(const void *blob, int node) { + struct tpm_state *state = &s_state; const char *prop; int len; int i; @@ -41,22 +42,27 @@ static int sandbox_tpm_read_state(const void *blob, int node) return 0; for (i = 0; i < NV_SEQ_COUNT; i++) { + struct nvdata_state *nvd = &state->nvdata[i]; char prop_name[20]; sprintf(prop_name, "nvdata%d", i); prop = fdt_getprop(blob, node, prop_name, &len); - if (prop && len == NV_DATA_SIZE) { - memcpy(g_state.nvdata[i].data, prop, NV_DATA_SIZE); - g_state.nvdata[i].present = true; + if (len >= NV_DATA_SIZE) + return log_msg_ret("nvd", -E2BIG); + if (prop) { + memcpy(nvd->data, prop, len); + nvd->length = len; + nvd->present = true; } } - g_state.valid = true; + + s_state.valid = true; return 0; } /** - * cros_ec_write_state() - Write out our state to the state file + * sandbox_tpm_write_state() - Write out our state to the state file * * The caller will ensure that there is a node ready for the state. The node * may already contain the old state, in which case it is overridden. @@ -66,20 +72,25 @@ static int sandbox_tpm_read_state(const void *blob, int node) */ static int sandbox_tpm_write_state(void *blob, int node) { + const struct tpm_state *state = g_state; int i; + if (!state) + return 0; + /* * We are guaranteed enough space to write basic properties. * We could use fdt_add_subnode() to put each set of data in its * own node - perhaps useful if we add access informaiton to each. */ for (i = 0; i < NV_SEQ_COUNT; i++) { + const struct nvdata_state *nvd = &state->nvdata[i]; char prop_name[20]; - if (g_state.nvdata[i].present) { - sprintf(prop_name, "nvdata%d", i); - fdt_setprop(blob, node, prop_name, - g_state.nvdata[i].data, NV_DATA_SIZE); + if (nvd->present) { + snprintf(prop_name, sizeof(prop_name), "nvdata%d", i); + fdt_setprop(blob, node, prop_name, nvd->data, + nvd->length); } } @@ -233,7 +244,9 @@ static int sandbox_tpm_probe(struct udevice *dev) { struct tpm_state *tpm = dev_get_priv(dev); - memcpy(tpm, &g_state, sizeof(*tpm)); + if (s_state.valid) + memcpy(tpm, &s_state, sizeof(*tpm)); + g_state = tpm; return 0; } From f9143c12003aabbad3a2485f8ad305f5dff5fae9 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 18 Jul 2021 14:17:59 -0600 Subject: [PATCH 04/23] sandbox: tpm: Support the define-space command Add support for this command, moving away from the previous approach of hard-coding the initial data in the driver, now that the kernel-space data has to be set up by the higher-level vboot code. Signed-off-by: Simon Glass --- drivers/tpm/sandbox_common.c | 11 +++++++++++ drivers/tpm/sandbox_common.h | 12 ++++++++++++ drivers/tpm/tpm_tis_sandbox.c | 11 +++++++++++ 3 files changed, 34 insertions(+) diff --git a/drivers/tpm/sandbox_common.c b/drivers/tpm/sandbox_common.c index 13f5e030a5f..7e0b2502e35 100644 --- a/drivers/tpm/sandbox_common.c +++ b/drivers/tpm/sandbox_common.c @@ -64,3 +64,14 @@ void sb_tpm_write_data(struct nvdata_state nvdata[NV_SEQ_COUNT], else memcpy(&nvdata[seq].data, buf + data_ofs, length); } + +void sb_tpm_define_data(struct nvdata_state nvdata[NV_SEQ_COUNT], + enum sandbox_nv_space seq, int length) +{ + struct nvdata_state *nvd = &nvdata[seq]; + + if (length > NV_DATA_SIZE) + log_err("Invalid length %x (max %x)\n", length, NV_DATA_SIZE); + nvd->length = length; + nvd->present = true; +} diff --git a/drivers/tpm/sandbox_common.h b/drivers/tpm/sandbox_common.h index aa5292d7945..e822a200fd3 100644 --- a/drivers/tpm/sandbox_common.h +++ b/drivers/tpm/sandbox_common.h @@ -93,4 +93,16 @@ void sb_tpm_write_data(struct nvdata_state nvdata[NV_SEQ_COUNT], enum sandbox_nv_space seq, const u8 *buf, int data_ofs, int length); +/** + * sb_tpm_define_data() - Set up non-volatile data + * + * If @length is too large, an error is logged and nothing is written. + * + * @nvdata: Current nvdata state + * @seq: Sequence number to set up + * @length: Length of space in bytes + */ +void sb_tpm_define_data(struct nvdata_state nvdata[NV_SEQ_COUNT], + enum sandbox_nv_space seq, int length); + #endif diff --git a/drivers/tpm/tpm_tis_sandbox.c b/drivers/tpm/tpm_tis_sandbox.c index f22ed846f0a..85b22afa4d9 100644 --- a/drivers/tpm/tpm_tis_sandbox.c +++ b/drivers/tpm/tpm_tis_sandbox.c @@ -210,6 +210,17 @@ static int sandbox_tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, memset(recvbuf, '\0', *recv_len); break; case TPM_CMD_NV_DEFINE_SPACE: + index = get_unaligned_be32(sendbuf + 12); + length = get_unaligned_be32(sendbuf + 77); + seq = sb_tpm_index_to_seq(index); + if (seq < 0) + return -EINVAL; + printf("tpm: define_space index=%#02x, len=%#02x, seq=%#02x\n", + index, length, seq); + sb_tpm_define_data(tpm->nvdata, seq, length); + *recv_len = 12; + memset(recvbuf, '\0', *recv_len); + break; case 0x15: /* pcr read */ case 0x5d: /* force clear */ case 0x6f: /* physical enable */ From 7f350a959c1a9562b1b84d145e1084e90fa86353 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 18 Jul 2021 14:18:00 -0600 Subject: [PATCH 05/23] sandbox: tpm: Correct handling of get-capability This function current handles the kernel case incorrectly. Fix it, and use the shorter TPM_HDR_LEN while we are here. Signed-off-by: Simon Glass --- drivers/tpm/tpm_tis_sandbox.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/drivers/tpm/tpm_tis_sandbox.c b/drivers/tpm/tpm_tis_sandbox.c index 85b22afa4d9..efbeb00ab63 100644 --- a/drivers/tpm/tpm_tis_sandbox.c +++ b/drivers/tpm/tpm_tis_sandbox.c @@ -140,16 +140,13 @@ static int sandbox_tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, printf("Get flags index %#02x\n", index); *recv_len = 22; memset(recvbuf, '\0', *recv_len); - data = recvbuf + TPM_RESPONSE_HEADER_LENGTH + - sizeof(uint32_t); + data = recvbuf + TPM_HDR_LEN + sizeof(uint32_t); switch (index) { case FIRMWARE_NV_INDEX: break; case KERNEL_NV_INDEX: handle_cap_flag_space(&data, index); - *recv_len = data - recvbuf - - TPM_RESPONSE_HEADER_LENGTH - - sizeof(uint32_t); + *recv_len = data - recvbuf; break; case TPM_CAP_FLAG_PERMANENT: { struct tpm_permanent_flags *pflags; @@ -166,15 +163,12 @@ static int sandbox_tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, printf(" ** Unknown flags index %x\n", index); return -ENOSYS; } - put_unaligned_be32(*recv_len, - recvbuf + - TPM_RESPONSE_HEADER_LENGTH); + put_unaligned_be32(*recv_len, recvbuf + TPM_HDR_LEN); break; case TPM_CAP_NV_INDEX: index = get_unaligned_be32(sendbuf + 18); printf("Get cap nv index %#02x\n", index); - put_unaligned_be32(22, recvbuf + - TPM_RESPONSE_HEADER_LENGTH); + put_unaligned_be32(22, recvbuf + TPM_HDR_LEN); break; default: printf(" ** Unknown 0x65 command type %#02x\n", From 46aed06cb7bbdc0e01c6c43a5d446d12e75e62b4 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 18 Jul 2021 14:18:01 -0600 Subject: [PATCH 06/23] sandbox: tpm: Finish comments for struct sandbox_tpm2 Tidy up the missing comments for this struct. Signed-off-by: Simon Glass --- drivers/tpm/tpm2_tis_sandbox.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c index 24c804a5645..5e0bd304699 100644 --- a/drivers/tpm/tpm2_tis_sandbox.c +++ b/drivers/tpm/tpm2_tis_sandbox.c @@ -45,19 +45,31 @@ static const u8 sandbox_extended_once_pcr[] = { 0xea, 0x98, 0x31, 0xa9, 0x27, 0x59, 0xfb, 0x4b, }; +/* + * Information about our TPM emulation. This is preserved in the sandbox + * state file if enabled. + * + * @init_done: true if open() has been called + * @startup_done: true if TPM2_CC_STARTUP has been processed + * @tests_done: true if TPM2_CC_SELF_TEST has be processed + * @pw: TPM password per hierarchy + * @pw_sz: Size of each password in bytes + * @properties: TPM properties + * @pcr: TPM Platform Configuration Registers. Each of these holds a hash and + * can be 'extended' a number of times, meaning another hash is added into + * its value (initial value all zeroes) + * @pcr_extensions: Number of times each PCR has been extended (starts at 0) + * @nvdata: non-volatile data, used to store important things for the platform + */ struct sandbox_tpm2 { /* TPM internal states */ bool init_done; bool startup_done; bool tests_done; - /* TPM password per hierarchy */ char pw[TPM2_HIERARCHY_NB][TPM2_DIGEST_LEN + 1]; int pw_sz[TPM2_HIERARCHY_NB]; - /* TPM properties */ u32 properties[TPM2_PROPERTY_NB]; - /* TPM PCRs */ u8 pcr[SANDBOX_TPM_PCR_NB][TPM2_DIGEST_LEN]; - /* TPM PCR extensions */ u32 pcr_extensions[SANDBOX_TPM_PCR_NB]; }; From 0c0ddada656f9d642a5c3904bc225b2b4fb49fe4 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 18 Jul 2021 14:18:02 -0600 Subject: [PATCH 07/23] sandbox: tpm: Track whether the state is valid Add checking as to whether the current TPM state is valid, so we can implement reading/writing the state. Signed-off-by: Simon Glass --- drivers/tpm/tpm2_tis_sandbox.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c index 5e0bd304699..c287ca2278f 100644 --- a/drivers/tpm/tpm2_tis_sandbox.c +++ b/drivers/tpm/tpm2_tis_sandbox.c @@ -49,6 +49,7 @@ static const u8 sandbox_extended_once_pcr[] = { * Information about our TPM emulation. This is preserved in the sandbox * state file if enabled. * + * @valid: true if this is valid (only used in s_state) * @init_done: true if open() has been called * @startup_done: true if TPM2_CC_STARTUP has been processed * @tests_done: true if TPM2_CC_SELF_TEST has be processed @@ -62,6 +63,7 @@ static const u8 sandbox_extended_once_pcr[] = { * @nvdata: non-volatile data, used to store important things for the platform */ struct sandbox_tpm2 { + bool valid; /* TPM internal states */ bool init_done; bool startup_done; @@ -73,6 +75,8 @@ struct sandbox_tpm2 { u32 pcr_extensions[SANDBOX_TPM_PCR_NB]; }; +static struct sandbox_tpm2 s_state, *g_state; + /* * Check the tag validity depending on the command (authentication required or * not). If authentication is required, check it is valid. Update the auth @@ -606,11 +610,13 @@ static int sandbox_tpm2_probe(struct udevice *dev) /* Use the TPM v2 stack */ priv->version = TPM_V2; - memset(tpm, 0, sizeof(*tpm)); - priv->pcr_count = 32; priv->pcr_select_min = 2; + if (s_state.valid) + memcpy(tpm, &s_state, sizeof(*tpm)); + g_state = tpm; + return 0; } From d8f105dd7170bcb0370b8ce18503834cdeeec7c1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 18 Jul 2021 14:18:03 -0600 Subject: [PATCH 08/23] sandbox: tpm: Support nvdata in TPM2 Add support for this feature in the TPM2 emulator, to support Chromium OS vboot. Signed-off-by: Simon Glass --- drivers/tpm/tpm2_tis_sandbox.c | 68 ++++++++++++++++++++++++++++++++++ include/tpm-v2.h | 2 + 2 files changed, 70 insertions(+) diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c index c287ca2278f..1d38a79a867 100644 --- a/drivers/tpm/tpm2_tis_sandbox.c +++ b/drivers/tpm/tpm2_tis_sandbox.c @@ -11,6 +11,7 @@ #include #include #include +#include "sandbox_common.h" /* Hierarchies */ enum tpm2_hierarchy { @@ -73,6 +74,7 @@ struct sandbox_tpm2 { u32 properties[TPM2_PROPERTY_NB]; u8 pcr[SANDBOX_TPM_PCR_NB][TPM2_DIGEST_LEN]; u32 pcr_extensions[SANDBOX_TPM_PCR_NB]; + struct nvdata_state nvdata[NV_SEQ_COUNT]; }; static struct sandbox_tpm2 s_state, *g_state; @@ -109,6 +111,10 @@ static int sandbox_tpm2_check_session(struct udevice *dev, u32 command, u16 tag, case TPM2_CC_DAM_RESET: case TPM2_CC_DAM_PARAMETERS: case TPM2_CC_PCR_EXTEND: + case TPM2_CC_NV_READ: + case TPM2_CC_NV_WRITE: + case TPM2_CC_NV_WRITELOCK: + case TPM2_CC_NV_DEFINE_SPACE: if (tag != TPM2_ST_SESSIONS) { printf("Session required for command 0x%x\n", command); return TPM2_RC_AUTH_CONTEXT; @@ -137,6 +143,10 @@ static int sandbox_tpm2_check_session(struct udevice *dev, u32 command, u16 tag, break; case TPM2_RH_PLATFORM: *hierarchy = TPM2_HIERARCHY_PLATFORM; + if (command == TPM2_CC_NV_READ || + command == TPM2_CC_NV_WRITE || + command == TPM2_CC_NV_WRITELOCK) + *auth += sizeof(u32); break; default: printf("Wrong handle 0x%x\n", handle); @@ -573,6 +583,64 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); break; + case TPM2_CC_NV_READ: { + int index, seq; + + index = get_unaligned_be32(sendbuf + TPM2_HDR_LEN + 4); + length = get_unaligned_be16(sent); + /* ignore offset */ + seq = sb_tpm_index_to_seq(index); + if (seq < 0) + return log_msg_ret("index", -EINVAL); + printf("tpm: nvread index=%#02x, len=%#02x, seq=%#02x\n", index, + length, seq); + *recv_len = TPM2_HDR_LEN + 6 + length; + memset(recvbuf, '\0', *recv_len); + put_unaligned_be32(length, recvbuf + 2); + sb_tpm_read_data(tpm->nvdata, seq, recvbuf, + TPM2_HDR_LEN + 4 + 2, length); + break; + } + case TPM2_CC_NV_WRITE: { + int index, seq; + + index = get_unaligned_be32(sendbuf + TPM2_HDR_LEN + 4); + length = get_unaligned_be16(sent); + sent += sizeof(u16); + + /* ignore offset */ + seq = sb_tpm_index_to_seq(index); + if (seq < 0) + return log_msg_ret("index", -EINVAL); + printf("tpm: nvwrite index=%#02x, len=%#02x, seq=%#02x\n", index, + length, seq); + memcpy(&tpm->nvdata[seq].data, sent, length); + tpm->nvdata[seq].present = true; + *recv_len = TPM2_HDR_LEN + 2; + memset(recvbuf, '\0', *recv_len); + break; + } + case TPM2_CC_NV_DEFINE_SPACE: { + int policy_size, index, seq; + + policy_size = get_unaligned_be16(sent + 12); + index = get_unaligned_be32(sent + 2); + sent += 14 + policy_size; + length = get_unaligned_be16(sent); + seq = sb_tpm_index_to_seq(index); + if (seq < 0) + return -EINVAL; + printf("tpm: define_space index=%x, len=%x, seq=%x, policy_size=%x\n", + index, length, seq, policy_size); + sb_tpm_define_data(tpm->nvdata, seq, length); + *recv_len = 12; + memset(recvbuf, '\0', *recv_len); + break; + } + case TPM2_CC_NV_WRITELOCK: + *recv_len = 12; + memset(recvbuf, '\0', *recv_len); + break; default: printf("TPM2 command %02x unknown in Sandbox\n", command); rc = TPM2_RC_COMMAND_CODE; diff --git a/include/tpm-v2.h b/include/tpm-v2.h index 247b3869676..949a13c917a 100644 --- a/include/tpm-v2.h +++ b/include/tpm-v2.h @@ -32,6 +32,8 @@ struct udevice; #define TPM2_MAX_TPM_PROPERTIES ((TPM2_MAX_CAP_BUFFER - sizeof(u32) /* TPM2_CAP */ - \ sizeof(u32)) / sizeof(struct tpms_tagged_property)) +#define TPM2_HDR_LEN 10 + /* * We deviate from this draft of the specification by increasing the value of * TPM2_NUM_PCR_BANKS from 3 to 16 to ensure compatibility with TPM2 From a986216e348153705e0a019afc95da65baa1fff0 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 18 Jul 2021 14:18:04 -0600 Subject: [PATCH 09/23] sandbox: tpm: Support storing device state in tpm2 At present the tpm2 emulator does not support storing the device state. Add this so we can handle the normal vboot flow through the sandbox executables (VPL->SPL etc.) with the TPM contents staying in place. Note: sandbox has not yet been converted to use livetree for the state information, since livetree does not yet support writing to the tree. Signed-off-by: Simon Glass --- drivers/tpm/tpm2_tis_sandbox.c | 139 +++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c index 1d38a79a867..ed9c9a0bc9f 100644 --- a/drivers/tpm/tpm2_tis_sandbox.c +++ b/drivers/tpm/tpm2_tis_sandbox.c @@ -79,6 +79,145 @@ struct sandbox_tpm2 { static struct sandbox_tpm2 s_state, *g_state; +/** + * sandbox_tpm2_read_state() - read the sandbox EC state from the state file + * + * If data is available, then blob and node will provide access to it. If + * not this function sets up an empty TPM. + * + * @blob: Pointer to device tree blob, or NULL if no data to read + * @node: Node offset to read from + */ +static int sandbox_tpm2_read_state(const void *blob, int node) +{ + struct sandbox_tpm2 *state = &s_state; + char prop_name[20]; + const char *prop; + int len; + int i; + + if (!blob) + return 0; + state->tests_done = fdtdec_get_int(blob, node, "tests-done", 0); + + for (i = 0; i < TPM2_HIERARCHY_NB; i++) { + snprintf(prop_name, sizeof(prop_name), "pw%d", i); + + prop = fdt_getprop(blob, node, prop_name, &len); + if (len > TPM2_DIGEST_LEN) + return log_msg_ret("pw", -E2BIG); + if (prop) { + memcpy(state->pw[i], prop, len); + state->pw_sz[i] = len; + } + } + + for (i = 0; i < TPM2_PROPERTY_NB; i++) { + snprintf(prop_name, sizeof(prop_name), "properties%d", i); + state->properties[i] = fdtdec_get_uint(blob, node, prop_name, + 0); + } + + for (i = 0; i < SANDBOX_TPM_PCR_NB; i++) { + int subnode; + + snprintf(prop_name, sizeof(prop_name), "pcr%d", i); + subnode = fdt_subnode_offset(blob, node, prop_name); + if (subnode < 0) + continue; + prop = fdt_getprop(blob, subnode, "value", &len); + if (len != TPM2_DIGEST_LEN) + return log_msg_ret("pcr", -E2BIG); + memcpy(state->pcr[i], prop, TPM2_DIGEST_LEN); + state->pcr_extensions[i] = fdtdec_get_uint(blob, subnode, + "extensions", 0); + } + + for (i = 0; i < NV_SEQ_COUNT; i++) { + struct nvdata_state *nvd = &state->nvdata[i]; + + sprintf(prop_name, "nvdata%d", i); + prop = fdt_getprop(blob, node, prop_name, &len); + if (len > NV_DATA_SIZE) + return log_msg_ret("nvd", -E2BIG); + if (prop) { + memcpy(nvd->data, prop, len); + nvd->length = len; + nvd->present = true; + } + } + s_state.valid = true; + + return 0; +} + +/** + * sandbox_tpm2_write_state() - Write out our state to the state file + * + * The caller will ensure that there is a node ready for the state. The node + * may already contain the old state, in which case it is overridden. + * + * @blob: Device tree blob holding state + * @node: Node to write our state into + */ +static int sandbox_tpm2_write_state(void *blob, int node) +{ + const struct sandbox_tpm2 *state = g_state; + char prop_name[20]; + int i; + + if (!state) + return 0; + + /* + * We are guaranteed enough space to write basic properties. This is + * SANDBOX_STATE_MIN_SPACE. + * + * We could use fdt_add_subnode() to put each set of data in its + * own node - perhaps useful if we add access information to each. + */ + fdt_setprop_u32(blob, node, "tests-done", state->tests_done); + + for (i = 0; i < TPM2_HIERARCHY_NB; i++) { + if (state->pw_sz[i]) { + snprintf(prop_name, sizeof(prop_name), "pw%d", i); + fdt_setprop(blob, node, prop_name, state->pw[i], + state->pw_sz[i]); + } + } + + for (i = 0; i < TPM2_PROPERTY_NB; i++) { + snprintf(prop_name, sizeof(prop_name), "properties%d", i); + fdt_setprop_u32(blob, node, prop_name, state->properties[i]); + } + + for (i = 0; i < SANDBOX_TPM_PCR_NB; i++) { + int subnode; + + snprintf(prop_name, sizeof(prop_name), "pcr%d", i); + subnode = fdt_add_subnode(blob, node, prop_name); + fdt_setprop(blob, subnode, "value", state->pcr[i], + TPM2_DIGEST_LEN); + fdt_setprop_u32(blob, subnode, "extensions", + state->pcr_extensions[i]); + } + + for (i = 0; i < NV_SEQ_COUNT; i++) { + const struct nvdata_state *nvd = &state->nvdata[i]; + + if (nvd->present) { + snprintf(prop_name, sizeof(prop_name), "nvdata%d", i); + fdt_setprop(blob, node, prop_name, nvd->data, + nvd->length); + } + } + + return 0; +} + +SANDBOX_STATE_IO(sandbox_tpm2, "sandbox,tpm2", sandbox_tpm2_read_state, + sandbox_tpm2_write_state); + /* * Check the tag validity depending on the command (authentication required or * not). If authentication is required, check it is valid. Update the auth From 9f0b53564f035743a2ce60636cadd17c97937dee Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 18 Jul 2021 14:18:05 -0600 Subject: [PATCH 10/23] sandbox: tpm: Correct handling of SANDBOX_TPM_PCR_NB This is the number of PCRs, so the current check is off by one. Also the map itself should not be checked, just the resulting pcr_index, to avoid confusing people who read the code. Fix these problems. Signed-off-by: Simon Glass --- drivers/tpm/tpm2_tis_sandbox.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c index ed9c9a0bc9f..3c4bbcdf2ee 100644 --- a/drivers/tpm/tpm2_tis_sandbox.c +++ b/drivers/tpm/tpm2_tis_sandbox.c @@ -642,15 +642,8 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, for (i = 0; i < pcr_array_sz; i++) pcr_map += (u64)sent[i] << (i * 8); - if (pcr_map >> SANDBOX_TPM_PCR_NB) { - printf("Sandbox TPM handles up to %d PCR(s)\n", - SANDBOX_TPM_PCR_NB); - rc = TPM2_RC_VALUE; - return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); - } - if (!pcr_map) { - printf("Empty PCR map.\n"); + printf("Empty PCR map\n"); rc = TPM2_RC_VALUE; return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); } @@ -659,6 +652,13 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, if (pcr_map & BIT(i)) pcr_index = i; + if (pcr_index >= SANDBOX_TPM_PCR_NB) { + printf("Invalid index %d, sandbox TPM handles up to %d PCR(s)\n", + pcr_index, SANDBOX_TPM_PCR_NB); + rc = TPM2_RC_VALUE; + return sandbox_tpm2_fill_buf(recv, recv_len, tag, rc); + } + /* Write tag */ put_unaligned_be16(tag, recv); recv += sizeof(tag); @@ -692,9 +692,9 @@ static int sandbox_tpm2_xfer(struct udevice *dev, const u8 *sendbuf, pcr_index = get_unaligned_be32(sendbuf + sizeof(tag) + sizeof(length) + sizeof(command)); - if (pcr_index > SANDBOX_TPM_PCR_NB) { - printf("Sandbox TPM handles up to %d PCR(s)\n", - SANDBOX_TPM_PCR_NB); + if (pcr_index >= SANDBOX_TPM_PCR_NB) { + printf("Invalid index %d, sandbox TPM handles up to %d PCR(s)\n", + pcr_index, SANDBOX_TPM_PCR_NB); rc = TPM2_RC_VALUE; } From 1c6608bd92acfa3a4c269ccdcb92905a4e512813 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 18 Jul 2021 14:18:06 -0600 Subject: [PATCH 11/23] sandbox: tpm: Support extending a PCR multiple times It is fairly easy to handle this case and it makes the emulator more useful, since PCRs are commonly extended several times. Add support for this, using U-Boot's sha256 support. For now sandbox only supports a single PCR, but that is enough for the tests that currently exist. Signed-off-by: Simon Glass --- drivers/tpm/tpm2_tis_sandbox.c | 24 ++++++++++-------------- test/py/tests/test_tpm2.py | 18 +++++++++++++++++- 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c index 3c4bbcdf2ee..ac6eb143539 100644 --- a/drivers/tpm/tpm2_tis_sandbox.c +++ b/drivers/tpm/tpm2_tis_sandbox.c @@ -11,6 +11,7 @@ #include #include #include +#include #include "sandbox_common.h" /* Hierarchies */ @@ -39,13 +40,6 @@ enum tpm2_cap_tpm_property { #define SANDBOX_TPM_PCR_NB 1 -static const u8 sandbox_extended_once_pcr[] = { - 0xf5, 0xa5, 0xfd, 0x42, 0xd1, 0x6a, 0x20, 0x30, - 0x27, 0x98, 0xef, 0x6e, 0xd3, 0x09, 0x97, 0x9b, - 0x43, 0x00, 0x3d, 0x23, 0x20, 0xd9, 0xf0, 0xe8, - 0xea, 0x98, 0x31, 0xa9, 0x27, 0x59, 0xfb, 0x4b, -}; - /* * Information about our TPM emulation. This is preserved in the sandbox * state file if enabled. @@ -407,15 +401,17 @@ static int sandbox_tpm2_extend(struct udevice *dev, int pcr_index, const u8 *extension) { struct sandbox_tpm2 *tpm = dev_get_priv(dev); - int i; + sha256_context ctx; - /* Only simulate the first extensions from all '0' with only '0' */ - for (i = 0; i < TPM2_DIGEST_LEN; i++) - if (tpm->pcr[pcr_index][i] || extension[i]) - return TPM2_RC_FAILURE; + /* Zero the PCR if this is the first use */ + if (!tpm->pcr_extensions[pcr_index]) + memset(tpm->pcr[pcr_index], '\0', TPM2_DIGEST_LEN); + + sha256_starts(&ctx); + sha256_update(&ctx, tpm->pcr[pcr_index], TPM2_DIGEST_LEN); + sha256_update(&ctx, extension, TPM2_DIGEST_LEN); + sha256_finish(&ctx, tpm->pcr[pcr_index]); - memcpy(tpm->pcr[pcr_index], sandbox_extended_once_pcr, - TPM2_DIGEST_LEN); tpm->pcr_extensions[pcr_index]++; return 0; diff --git a/test/py/tests/test_tpm2.py b/test/py/tests/test_tpm2.py index 70f906da511..ac04f7191ec 100644 --- a/test/py/tests/test_tpm2.py +++ b/test/py/tests/test_tpm2.py @@ -216,7 +216,9 @@ def test_tpm2_pcr_extend(u_boot_console): output = u_boot_console.run_command('echo $?') assert output.endswith('0') - read_pcr = u_boot_console.run_command('tpm2 pcr_read 0 0x%x' % ram) + # Read the value back into a different place so we can still use 'ram' as + # our zero bytes + read_pcr = u_boot_console.run_command('tpm2 pcr_read 0 0x%x' % (ram + 0x20)) output = u_boot_console.run_command('echo $?') assert output.endswith('0') assert 'f5 a5 fd 42 d1 6a 20 30 27 98 ef 6e d3 09 97 9b' in read_pcr @@ -226,6 +228,20 @@ def test_tpm2_pcr_extend(u_boot_console): new_updates = int(re.findall(r'\d+', str)[0]) assert (updates + 1) == new_updates + u_boot_console.run_command('tpm2 pcr_extend 0 0x%x' % ram) + output = u_boot_console.run_command('echo $?') + assert output.endswith('0') + + read_pcr = u_boot_console.run_command('tpm2 pcr_read 0 0x%x' % (ram + 0x20)) + output = u_boot_console.run_command('echo $?') + assert output.endswith('0') + assert '7a 05 01 f5 95 7b df 9c b3 a8 ff 49 66 f0 22 65' in read_pcr + assert 'f9 68 65 8b 7a 9c 62 64 2c ba 11 65 e8 66 42 f5' in read_pcr + + str = re.findall(r'\d+ known updates', read_pcr)[0] + new_updates = int(re.findall(r'\d+', str)[0]) + assert (updates + 2) == new_updates + @pytest.mark.buildconfigspec('cmd_tpm_v2') def test_tpm2_cleanup(u_boot_console): """Ensure the TPM is cleared from password or test related configuration.""" From 0c929631a237be26042b87259c3404d9756a0773 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 21 Jul 2021 14:55:25 -0600 Subject: [PATCH 12/23] fdt: Tidy up the code a bit with fdt addr Clean up the code a little before changing it. Signed-off-by: Simon Glass --- cmd/fdt.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/cmd/fdt.c b/cmd/fdt.c index f1e2fc2fd8b..5acc3ebaf33 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -115,26 +115,20 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) if (argc < 2) return CMD_RET_USAGE; - /* - * Set the address of the fdt - */ + /* fdt addr: Set the address of the fdt */ if (strncmp(argv[1], "ad", 2) == 0) { unsigned long addr; int control = 0; struct fdt_header *blob; - /* - * Set the address [and length] of the fdt. - */ + + /* Set the address [and length] of the fdt */ argc -= 2; argv += 2; -/* Temporary #ifdef - some archs don't have fdt_blob yet */ -#ifdef CONFIG_OF_CONTROL if (argc && !strcmp(*argv, "-c")) { control = 1; argc--; argv++; } -#endif if (argc == 0) { if (control) blob = (struct fdt_header *)gd->fdt_blob; @@ -160,22 +154,18 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) if (argc >= 2) { int len; int err; - /* - * Optional new length - */ + + /* Optional new length */ len = simple_strtoul(argv[1], NULL, 16); if (len < fdt_totalsize(blob)) { - printf ("New length %d < existing length %d, " - "ignoring.\n", - len, fdt_totalsize(blob)); + printf("New length %d < existing length %d, ignoring\n", + len, fdt_totalsize(blob)); } else { - /* - * Open in place with a new length. - */ + /* Open in place with a new length */ err = fdt_open_into(blob, blob, len); if (err != 0) { - printf ("libfdt fdt_open_into(): %s\n", - fdt_strerror(err)); + printf("libfdt fdt_open_into(): %s\n", + fdt_strerror(err)); } } } @@ -184,10 +174,9 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) } if (!working_fdt) { - puts( - "No FDT memory address configured. Please configure\n" - "the FDT address via \"fdt addr
\" command.\n" - "Aborting!\n"); + puts("No FDT memory address configured. Please configure\n" + "the FDT address via \"fdt addr
\" command.\n" + "Aborting!\n"); return CMD_RET_FAILURE; } From b29a0dbdc3de43a8574335bef93d50c659eff2e5 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 21 Jul 2021 14:55:26 -0600 Subject: [PATCH 13/23] fdt: Show the type of devicetree with fdt addr It seems useful to show whether the address of the Control or Working devicetree is being shown. Add support for this. Drop the confusing 0x prefix since the command itself only accepts hex. Signed-off-by: Simon Glass --- cmd/fdt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/fdt.c b/cmd/fdt.c index 5acc3ebaf33..baec05529ad 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -136,9 +136,10 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) blob = working_fdt; if (!blob || !fdt_valid(&blob)) return 1; - printf("The address of the fdt is %#08lx\n", + printf("%s fdt: %08lx\n", + control ? "Control" : "Working", control ? (ulong)map_to_sysmem(blob) : - env_get_hex("fdtaddr", 0)); + env_get_hex("fdtaddr", 0)); return 0; } From a6123333aba1b587e39762da675a33bb0eb9ad94 Mon Sep 17 00:00:00 2001 From: Patrick Delaunay Date: Thu, 22 Jul 2021 16:51:42 +0200 Subject: [PATCH 14/23] patman: add warning for invalid tag Add a error in patman tool when the commit message contents an invalid tag "Serie-.*" instead of "Series-.*". Signed-off-by: Patrick Delaunay Reviewed-by: Simon Glass --- tools/patman/func_test.py | 11 +++++++++++ tools/patman/patchstream.py | 9 +++++++++ 2 files changed, 20 insertions(+) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index 1ce6448d00b..9871bb580d0 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -506,6 +506,17 @@ Tested-by: %s 'Reviewed-by': {self.joe, self.mary}, 'Tested-by': {self.leb}}) + def testInvalidTag(self): + """Test invalid tag in a patchstream""" + text = '''This is a patch + +Serie-version: 2 +''' + with self.assertRaises(ValueError) as exc: + pstrm = PatchStream.process_text(text) + self.assertEqual("Line 3: Invalid tag = 'Serie-version: 2'", + str(exc.exception)) + def testMissingEnd(self): """Test a missing END tag""" text = '''This is a patch diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index a44cd861afc..b9602924273 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -59,6 +59,9 @@ RE_DIFF = re.compile(r'^>.*diff --git a/(.*) b/(.*)$') # Detect a context line, like '> @@ -153,8 +153,13 @@ CheckPatch RE_LINE = re.compile(r'>.*@@ \-(\d+),\d+ \+(\d+),\d+ @@ *(.*)') +# Detect line with invalid TAG +RE_INV_TAG = re.compile('^Serie-([a-z-]*): *(.*)') + # States we can be in - can we use range() and still have comments? STATE_MSG_HEADER = 0 # Still in the message header STATE_PATCH_SUBJECT = 1 # In patch subject (first line of log for a commit) @@ -318,6 +321,7 @@ class PatchStream: leading_whitespace_match = RE_LEADING_WHITESPACE.match(line) diff_match = RE_DIFF.match(line) line_match = RE_LINE.match(line) + invalid_match = RE_INV_TAG.match(line) tag_match = None if self.state == STATE_PATCH_HEADER: tag_match = RE_TAG.match(line) @@ -471,6 +475,11 @@ class PatchStream: self._add_warn('Line %d: Ignoring Commit-%s' % (self.linenum, name)) + # Detect invalid tags + elif invalid_match: + raise ValueError("Line %d: Invalid tag = '%s'" % + (self.linenum, line)) + # Detect the start of a new commit elif commit_match: self._close_commit() From 2d754cea4c18f0200ff14c038fe4d3f206b684d9 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 21 Jul 2021 21:35:49 -0600 Subject: [PATCH 15/23] doc: Create an intro section for testing At present this information is hidden away. Make it more visible by putting it first, in an intro section. Signed-off-by: Simon Glass Reviewed-by: Heinrich Schuchardt --- doc/develop/index.rst | 2 +- doc/develop/testing.rst | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/doc/develop/index.rst b/doc/develop/index.rst index 54e14dd77b5..0ae440845f4 100644 --- a/doc/develop/index.rst +++ b/doc/develop/index.rst @@ -41,8 +41,8 @@ Testing .. toctree:: :maxdepth: 1 - coccinelle testing + coccinelle py_testing tests_writing tests_sandbox diff --git a/doc/develop/testing.rst b/doc/develop/testing.rst index ced13ac8bb4..1abe4d7f0f0 100644 --- a/doc/develop/testing.rst +++ b/doc/develop/testing.rst @@ -1,5 +1,7 @@ -Testing in U-Boot -================= +.. SPDX-License-Identifier: GPL-2.0+ + +Introduction to testing +======================= U-Boot has a large amount of code. This file describes how this code is tested and what tests you should write when adding a new feature. From a980e7bbde5becf014a8cda7dd0c6fe9c1b234d9 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 21 Jul 2021 21:35:50 -0600 Subject: [PATCH 16/23] doc: Move coccinelle into its own section This tool has nothing to do with testing. It is for refactoring code automatically using a 'semantic patch' tool. Create a new section for 'refactoring' and move it into there. It is likely that other topics may fall under the same heading, such as using moveconfig and search/replace tools. Signed-off-by: Simon Glass --- doc/develop/index.rst | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/develop/index.rst b/doc/develop/index.rst index 0ae440845f4..d1488d764b6 100644 --- a/doc/develop/index.rst +++ b/doc/develop/index.rst @@ -42,7 +42,14 @@ Testing :maxdepth: 1 testing - coccinelle py_testing tests_writing tests_sandbox + +Refactoring +----------- + +.. toctree:: + :maxdepth: 1 + + coccinelle From 5c72c0e0d2bb5ff8aa38b4cebdb4e1d4dd311b5d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 21 Jul 2021 21:35:51 -0600 Subject: [PATCH 17/23] doc: Add docs for the moveconfig tool Move these docs into htmldocs so they can be read there. Signed-off-by: Simon Glass --- doc/develop/index.rst | 1 + doc/develop/moveconfig.rst | 296 +++++++++++++++++++++++++++++++++++++ tools/moveconfig.py | 291 +----------------------------------- 3 files changed, 298 insertions(+), 290 deletions(-) create mode 100644 doc/develop/moveconfig.rst diff --git a/doc/develop/index.rst b/doc/develop/index.rst index d1488d764b6..3ead7bda8fd 100644 --- a/doc/develop/index.rst +++ b/doc/develop/index.rst @@ -53,3 +53,4 @@ Refactoring :maxdepth: 1 coccinelle + moveconfig diff --git a/doc/develop/moveconfig.rst b/doc/develop/moveconfig.rst new file mode 100644 index 00000000000..aaa155e8c70 --- /dev/null +++ b/doc/develop/moveconfig.rst @@ -0,0 +1,296 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +moveconfig +========== + +Since Kconfig was introduced to U-Boot, we have worked on moving +config options from headers to Kconfig (defconfig). + +This tool intends to help this tremendous work. + +Installing +---------- + +You may need to install 'python3-asteval' for the 'asteval' module. + +Usage +----- + +First, you must edit the Kconfig to add the menu entries for the configs +you are moving. + +Then run this tool giving CONFIG names you want to move. +For example, if you want to move CONFIG_CMD_USB and CONFIG_SYS_TEXT_BASE, +simply type as follows:: + + $ tools/moveconfig.py CONFIG_CMD_USB CONFIG_SYS_TEXT_BASE + +The tool walks through all the defconfig files and move the given CONFIGs. + +The log is also displayed on the terminal. + +The log is printed for each defconfig as follows:: + + + + + + ... + +`` is the name of the defconfig. + +`` shows what the tool did for that defconfig. +It looks like one of the following: + + - Move 'CONFIG\_... ' + This config option was moved to the defconfig + + - CONFIG\_... is not defined in Kconfig. Do nothing. + The entry for this CONFIG was not found in Kconfig. The option is not + defined in the config header, either. So, this case can be just skipped. + + - CONFIG\_... is not defined in Kconfig (suspicious). Do nothing. + This option is defined in the config header, but its entry was not found + in Kconfig. + There are two common cases: + + - You forgot to create an entry for the CONFIG before running + this tool, or made a typo in a CONFIG passed to this tool. + - The entry was hidden due to unmet 'depends on'. + + The tool does not know if the result is reasonable, so please check it + manually. + + - 'CONFIG\_...' is the same as the define in Kconfig. Do nothing. + The define in the config header matched the one in Kconfig. + We do not need to touch it. + + - Compiler is missing. Do nothing. + The compiler specified for this architecture was not found + in your PATH environment. + (If -e option is passed, the tool exits immediately.) + + - Failed to process. + An error occurred during processing this defconfig. Skipped. + (If -e option is passed, the tool exits immediately on error.) + +Finally, you will be asked, Clean up headers? [y/n]: + +If you say 'y' here, the unnecessary config defines are removed +from the config headers (include/configs/\*.h). +It just uses the regex method, so you should not rely on it. +Just in case, please do 'git diff' to see what happened. + + +How does it work? +----------------- + +This tool runs configuration and builds include/autoconf.mk for every +defconfig. The config options defined in Kconfig appear in the .config +file (unless they are hidden because of unmet dependency.) +On the other hand, the config options defined by board headers are seen +in include/autoconf.mk. The tool looks for the specified options in both +of them to decide the appropriate action for the options. If the given +config option is found in the .config, but its value does not match the +one from the board header, the config option in the .config is replaced +with the define in the board header. Then, the .config is synced by +"make savedefconfig" and the defconfig is updated with it. + +For faster processing, this tool handles multi-threading. It creates +separate build directories where the out-of-tree build is run. The +temporary build directories are automatically created and deleted as +needed. The number of threads are chosen based on the number of the CPU +cores of your system although you can change it via -j (--jobs) option. + + +Toolchains +---------- + +Appropriate toolchain are necessary to generate include/autoconf.mk +for all the architectures supported by U-Boot. Most of them are available +at the kernel.org site, some are not provided by kernel.org. This tool uses +the same tools as buildman, so see that tool for setup (e.g. --fetch-arch). + + +Tips and trips +-------------- + +To sync only X86 defconfigs:: + + ./tools/moveconfig.py -s -d <(grep -l X86 configs/*) + +or:: + + grep -l X86 configs/* | ./tools/moveconfig.py -s -d - + +To process CONFIG_CMD_FPGAD only for a subset of configs based on path match:: + + ls configs/{hrcon*,iocon*,strider*} | \ + ./tools/moveconfig.py -Cy CONFIG_CMD_FPGAD -d - + + +Finding implied CONFIGs +----------------------- + +Some CONFIG options can be implied by others and this can help to reduce +the size of the defconfig files. For example, CONFIG_X86 implies +CONFIG_CMD_IRQ, so we can put 'imply CMD_IRQ' under 'config X86' and +all x86 boards will have that option, avoiding adding CONFIG_CMD_IRQ to +each of the x86 defconfig files. + +This tool can help find such configs. To use it, first build a database:: + + ./tools/moveconfig.py -b + +Then try to query it:: + + ./tools/moveconfig.py -i CONFIG_CMD_IRQ + CONFIG_CMD_IRQ found in 311/2384 defconfigs + 44 : CONFIG_SYS_FSL_ERRATUM_IFC_A002769 + 41 : CONFIG_SYS_FSL_ERRATUM_A007075 + 31 : CONFIG_SYS_FSL_DDR_VER_44 + 28 : CONFIG_ARCH_P1010 + 28 : CONFIG_SYS_FSL_ERRATUM_P1010_A003549 + 28 : CONFIG_SYS_FSL_ERRATUM_SEC_A003571 + 28 : CONFIG_SYS_FSL_ERRATUM_IFC_A003399 + 25 : CONFIG_SYS_FSL_ERRATUM_A008044 + 22 : CONFIG_ARCH_P1020 + 21 : CONFIG_SYS_FSL_DDR_VER_46 + 20 : CONFIG_MAX_PIRQ_LINKS + 20 : CONFIG_HPET_ADDRESS + 20 : CONFIG_X86 + 20 : CONFIG_PCIE_ECAM_SIZE + 20 : CONFIG_IRQ_SLOT_COUNT + 20 : CONFIG_I8259_PIC + 20 : CONFIG_CPU_ADDR_BITS + 20 : CONFIG_RAMBASE + 20 : CONFIG_SYS_FSL_ERRATUM_A005871 + 20 : CONFIG_PCIE_ECAM_BASE + 20 : CONFIG_X86_TSC_TIMER + 20 : CONFIG_I8254_TIMER + 20 : CONFIG_CMD_GETTIME + 19 : CONFIG_SYS_FSL_ERRATUM_A005812 + 18 : CONFIG_X86_RUN_32BIT + 17 : CONFIG_CMD_CHIP_CONFIG + ... + +This shows a list of config options which might imply CONFIG_CMD_EEPROM along +with how many defconfigs they cover. From this you can see that CONFIG_X86 +implies CONFIG_CMD_EEPROM. Therefore, instead of adding CONFIG_CMD_EEPROM to +the defconfig of every x86 board, you could add a single imply line to the +Kconfig file: + + config X86 + bool "x86 architecture" + ... + imply CMD_EEPROM + +That will cover 20 defconfigs. Many of the options listed are not suitable as +they are not related. E.g. it would be odd for CONFIG_CMD_GETTIME to imply +CMD_EEPROM. + +Using this search you can reduce the size of moveconfig patches. + +You can automatically add 'imply' statements in the Kconfig with the -a +option:: + + ./tools/moveconfig.py -s -i CONFIG_SCSI \ + -a CONFIG_ARCH_LS1021A,CONFIG_ARCH_LS1043A + +This will add 'imply SCSI' to the two CONFIG options mentioned, assuming that +the database indicates that they do actually imply CONFIG_SCSI and do not +already have an 'imply SCSI'. + +The output shows where the imply is added:: + + 18 : CONFIG_ARCH_LS1021A arch/arm/cpu/armv7/ls102xa/Kconfig:1 + 13 : CONFIG_ARCH_LS1043A arch/arm/cpu/armv8/fsl-layerscape/Kconfig:11 + 12 : CONFIG_ARCH_LS1046A arch/arm/cpu/armv8/fsl-layerscape/Kconfig:31 + +The first number is the number of boards which can avoid having a special +CONFIG_SCSI option in their defconfig file if this 'imply' is added. +The location at the right is the Kconfig file and line number where the config +appears. For example, adding 'imply CONFIG_SCSI' to the 'config ARCH_LS1021A' +in arch/arm/cpu/armv7/ls102xa/Kconfig at line 1 will help 18 boards to reduce +the size of their defconfig files. + +If you want to add an 'imply' to every imply config in the list, you can use:: + + ./tools/moveconfig.py -s -i CONFIG_SCSI -a all + +To control which ones are displayed, use -I where list is a list of +options (use '-I help' to see possible options and their meaning). + +To skip showing you options that already have an 'imply' attached, use -A. + +When you have finished adding 'imply' options you can regenerate the +defconfig files for affected boards with something like:: + + git show --stat | ./tools/moveconfig.py -s -d - + +This will regenerate only those defconfigs changed in the current commit. +If you start with (say) 100 defconfigs being changed in the commit, and add +a few 'imply' options as above, then regenerate, hopefully you can reduce the +number of defconfigs changed in the commit. + + +Available options +----------------- + + -c, --color + Surround each portion of the log with escape sequences to display it + in color on the terminal. + + -C, --commit + Create a git commit with the changes when the operation is complete. A + standard commit message is used which may need to be edited. + + -d, --defconfigs + Specify a file containing a list of defconfigs to move. The defconfig + files can be given with shell-style wildcards. Use '-' to read from stdin. + + -n, --dry-run + Perform a trial run that does not make any changes. It is useful to + see what is going to happen before one actually runs it. + + -e, --exit-on-error + Exit immediately if Make exits with a non-zero status while processing + a defconfig file. + + -s, --force-sync + Do "make savedefconfig" forcibly for all the defconfig files. + If not specified, "make savedefconfig" only occurs for cases + where at least one CONFIG was moved. + + -S, --spl + Look for moved config options in spl/include/autoconf.mk instead of + include/autoconf.mk. This is useful for moving options for SPL build + because SPL related options (mostly prefixed with CONFIG_SPL\_) are + sometimes blocked by CONFIG_SPL_BUILD ifdef conditionals. + + -H, --headers-only + Only cleanup the headers; skip the defconfig processing + + -j, --jobs + Specify the number of threads to run simultaneously. If not specified, + the number of threads is the same as the number of CPU cores. + + -r, --git-ref + Specify the git ref to clone for building the autoconf.mk. If unspecified + use the CWD. This is useful for when changes to the Kconfig affect the + default values and you want to capture the state of the defconfig from + before that change was in effect. If in doubt, specify a ref pre-Kconfig + changes (use HEAD if Kconfig changes are not committed). Worst case it will + take a bit longer to run, but will always do the right thing. + + -v, --verbose + Show any build errors as boards are built + + -y, --yes + Instead of prompting, automatically go ahead with all operations. This + includes cleaning up headers, CONFIG_SYS_EXTRA_OPTIONS, the config whitelist + and the README. + +To see the complete list of supported options, run:: + + tools/moveconfig.py -h diff --git a/tools/moveconfig.py b/tools/moveconfig.py index 41dd803c4ef..f3fd75504f7 100755 --- a/tools/moveconfig.py +++ b/tools/moveconfig.py @@ -7,296 +7,7 @@ """ Move config options from headers to defconfig files. -Since Kconfig was introduced to U-Boot, we have worked on moving -config options from headers to Kconfig (defconfig). - -This tool intends to help this tremendous work. - -Installing ----------- - -You may need to install 'python3-asteval' for the 'asteval' module. - -Usage ------ - -First, you must edit the Kconfig to add the menu entries for the configs -you are moving. - -And then run this tool giving CONFIG names you want to move. -For example, if you want to move CONFIG_CMD_USB and CONFIG_SYS_TEXT_BASE, -simply type as follows: - - $ tools/moveconfig.py CONFIG_CMD_USB CONFIG_SYS_TEXT_BASE - -The tool walks through all the defconfig files and move the given CONFIGs. - -The log is also displayed on the terminal. - -The log is printed for each defconfig as follows: - - - - - - ... - - is the name of the defconfig. - - shows what the tool did for that defconfig. -It looks like one of the following: - - - Move 'CONFIG_... ' - This config option was moved to the defconfig - - - CONFIG_... is not defined in Kconfig. Do nothing. - The entry for this CONFIG was not found in Kconfig. The option is not - defined in the config header, either. So, this case can be just skipped. - - - CONFIG_... is not defined in Kconfig (suspicious). Do nothing. - This option is defined in the config header, but its entry was not found - in Kconfig. - There are two common cases: - - You forgot to create an entry for the CONFIG before running - this tool, or made a typo in a CONFIG passed to this tool. - - The entry was hidden due to unmet 'depends on'. - The tool does not know if the result is reasonable, so please check it - manually. - - - 'CONFIG_...' is the same as the define in Kconfig. Do nothing. - The define in the config header matched the one in Kconfig. - We do not need to touch it. - - - Compiler is missing. Do nothing. - The compiler specified for this architecture was not found - in your PATH environment. - (If -e option is passed, the tool exits immediately.) - - - Failed to process. - An error occurred during processing this defconfig. Skipped. - (If -e option is passed, the tool exits immediately on error.) - -Finally, you will be asked, Clean up headers? [y/n]: - -If you say 'y' here, the unnecessary config defines are removed -from the config headers (include/configs/*.h). -It just uses the regex method, so you should not rely on it. -Just in case, please do 'git diff' to see what happened. - - -How does it work? ------------------ - -This tool runs configuration and builds include/autoconf.mk for every -defconfig. The config options defined in Kconfig appear in the .config -file (unless they are hidden because of unmet dependency.) -On the other hand, the config options defined by board headers are seen -in include/autoconf.mk. The tool looks for the specified options in both -of them to decide the appropriate action for the options. If the given -config option is found in the .config, but its value does not match the -one from the board header, the config option in the .config is replaced -with the define in the board header. Then, the .config is synced by -"make savedefconfig" and the defconfig is updated with it. - -For faster processing, this tool handles multi-threading. It creates -separate build directories where the out-of-tree build is run. The -temporary build directories are automatically created and deleted as -needed. The number of threads are chosen based on the number of the CPU -cores of your system although you can change it via -j (--jobs) option. - - -Toolchains ----------- - -Appropriate toolchain are necessary to generate include/autoconf.mk -for all the architectures supported by U-Boot. Most of them are available -at the kernel.org site, some are not provided by kernel.org. This tool uses -the same tools as buildman, so see that tool for setup (e.g. --fetch-arch). - - -Tips and trips --------------- - -To sync only X86 defconfigs: - - ./tools/moveconfig.py -s -d <(grep -l X86 configs/*) - -or: - - grep -l X86 configs/* | ./tools/moveconfig.py -s -d - - -To process CONFIG_CMD_FPGAD only for a subset of configs based on path match: - - ls configs/{hrcon*,iocon*,strider*} | \ - ./tools/moveconfig.py -Cy CONFIG_CMD_FPGAD -d - - - -Finding implied CONFIGs ------------------------ - -Some CONFIG options can be implied by others and this can help to reduce -the size of the defconfig files. For example, CONFIG_X86 implies -CONFIG_CMD_IRQ, so we can put 'imply CMD_IRQ' under 'config X86' and -all x86 boards will have that option, avoiding adding CONFIG_CMD_IRQ to -each of the x86 defconfig files. - -This tool can help find such configs. To use it, first build a database: - - ./tools/moveconfig.py -b - -Then try to query it: - - ./tools/moveconfig.py -i CONFIG_CMD_IRQ - CONFIG_CMD_IRQ found in 311/2384 defconfigs - 44 : CONFIG_SYS_FSL_ERRATUM_IFC_A002769 - 41 : CONFIG_SYS_FSL_ERRATUM_A007075 - 31 : CONFIG_SYS_FSL_DDR_VER_44 - 28 : CONFIG_ARCH_P1010 - 28 : CONFIG_SYS_FSL_ERRATUM_P1010_A003549 - 28 : CONFIG_SYS_FSL_ERRATUM_SEC_A003571 - 28 : CONFIG_SYS_FSL_ERRATUM_IFC_A003399 - 25 : CONFIG_SYS_FSL_ERRATUM_A008044 - 22 : CONFIG_ARCH_P1020 - 21 : CONFIG_SYS_FSL_DDR_VER_46 - 20 : CONFIG_MAX_PIRQ_LINKS - 20 : CONFIG_HPET_ADDRESS - 20 : CONFIG_X86 - 20 : CONFIG_PCIE_ECAM_SIZE - 20 : CONFIG_IRQ_SLOT_COUNT - 20 : CONFIG_I8259_PIC - 20 : CONFIG_CPU_ADDR_BITS - 20 : CONFIG_RAMBASE - 20 : CONFIG_SYS_FSL_ERRATUM_A005871 - 20 : CONFIG_PCIE_ECAM_BASE - 20 : CONFIG_X86_TSC_TIMER - 20 : CONFIG_I8254_TIMER - 20 : CONFIG_CMD_GETTIME - 19 : CONFIG_SYS_FSL_ERRATUM_A005812 - 18 : CONFIG_X86_RUN_32BIT - 17 : CONFIG_CMD_CHIP_CONFIG - ... - -This shows a list of config options which might imply CONFIG_CMD_EEPROM along -with how many defconfigs they cover. From this you can see that CONFIG_X86 -implies CONFIG_CMD_EEPROM. Therefore, instead of adding CONFIG_CMD_EEPROM to -the defconfig of every x86 board, you could add a single imply line to the -Kconfig file: - - config X86 - bool "x86 architecture" - ... - imply CMD_EEPROM - -That will cover 20 defconfigs. Many of the options listed are not suitable as -they are not related. E.g. it would be odd for CONFIG_CMD_GETTIME to imply -CMD_EEPROM. - -Using this search you can reduce the size of moveconfig patches. - -You can automatically add 'imply' statements in the Kconfig with the -a -option: - - ./tools/moveconfig.py -s -i CONFIG_SCSI \ - -a CONFIG_ARCH_LS1021A,CONFIG_ARCH_LS1043A - -This will add 'imply SCSI' to the two CONFIG options mentioned, assuming that -the database indicates that they do actually imply CONFIG_SCSI and do not -already have an 'imply SCSI'. - -The output shows where the imply is added: - - 18 : CONFIG_ARCH_LS1021A arch/arm/cpu/armv7/ls102xa/Kconfig:1 - 13 : CONFIG_ARCH_LS1043A arch/arm/cpu/armv8/fsl-layerscape/Kconfig:11 - 12 : CONFIG_ARCH_LS1046A arch/arm/cpu/armv8/fsl-layerscape/Kconfig:31 - -The first number is the number of boards which can avoid having a special -CONFIG_SCSI option in their defconfig file if this 'imply' is added. -The location at the right is the Kconfig file and line number where the config -appears. For example, adding 'imply CONFIG_SCSI' to the 'config ARCH_LS1021A' -in arch/arm/cpu/armv7/ls102xa/Kconfig at line 1 will help 18 boards to reduce -the size of their defconfig files. - -If you want to add an 'imply' to every imply config in the list, you can use - - ./tools/moveconfig.py -s -i CONFIG_SCSI -a all - -To control which ones are displayed, use -I where list is a list of -options (use '-I help' to see possible options and their meaning). - -To skip showing you options that already have an 'imply' attached, use -A. - -When you have finished adding 'imply' options you can regenerate the -defconfig files for affected boards with something like: - - git show --stat | ./tools/moveconfig.py -s -d - - -This will regenerate only those defconfigs changed in the current commit. -If you start with (say) 100 defconfigs being changed in the commit, and add -a few 'imply' options as above, then regenerate, hopefully you can reduce the -number of defconfigs changed in the commit. - - -Available options ------------------ - - -c, --color - Surround each portion of the log with escape sequences to display it - in color on the terminal. - - -C, --commit - Create a git commit with the changes when the operation is complete. A - standard commit message is used which may need to be edited. - - -d, --defconfigs - Specify a file containing a list of defconfigs to move. The defconfig - files can be given with shell-style wildcards. Use '-' to read from stdin. - - -n, --dry-run - Perform a trial run that does not make any changes. It is useful to - see what is going to happen before one actually runs it. - - -e, --exit-on-error - Exit immediately if Make exits with a non-zero status while processing - a defconfig file. - - -s, --force-sync - Do "make savedefconfig" forcibly for all the defconfig files. - If not specified, "make savedefconfig" only occurs for cases - where at least one CONFIG was moved. - - -S, --spl - Look for moved config options in spl/include/autoconf.mk instead of - include/autoconf.mk. This is useful for moving options for SPL build - because SPL related options (mostly prefixed with CONFIG_SPL_) are - sometimes blocked by CONFIG_SPL_BUILD ifdef conditionals. - - -H, --headers-only - Only cleanup the headers; skip the defconfig processing - - -j, --jobs - Specify the number of threads to run simultaneously. If not specified, - the number of threads is the same as the number of CPU cores. - - -r, --git-ref - Specify the git ref to clone for building the autoconf.mk. If unspecified - use the CWD. This is useful for when changes to the Kconfig affect the - default values and you want to capture the state of the defconfig from - before that change was in effect. If in doubt, specify a ref pre-Kconfig - changes (use HEAD if Kconfig changes are not committed). Worst case it will - take a bit longer to run, but will always do the right thing. - - -v, --verbose - Show any build errors as boards are built - - -y, --yes - Instead of prompting, automatically go ahead with all operations. This - includes cleaning up headers, CONFIG_SYS_EXTRA_OPTIONS, the config whitelist - and the README. - -To see the complete list of supported options, run - - $ tools/moveconfig.py -h - +See doc/develop/moveconfig.rst for documentation. """ import asteval From a8ba35bf2335500013131208f4e90df9f5f936d4 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 21 Jul 2021 21:35:52 -0600 Subject: [PATCH 18/23] doc: Fix up outdated moveconfig docs The examples here are a bit messed up since the command does not match the documentation. Use a different example instead. Signed-off-by: Simon Glass --- doc/develop/moveconfig.rst | 56 ++++++++++++++------------------------ 1 file changed, 21 insertions(+), 35 deletions(-) diff --git a/doc/develop/moveconfig.rst b/doc/develop/moveconfig.rst index aaa155e8c70..dcd4d927e40 100644 --- a/doc/develop/moveconfig.rst +++ b/doc/develop/moveconfig.rst @@ -144,50 +144,36 @@ This tool can help find such configs. To use it, first build a database:: Then try to query it:: - ./tools/moveconfig.py -i CONFIG_CMD_IRQ - CONFIG_CMD_IRQ found in 311/2384 defconfigs - 44 : CONFIG_SYS_FSL_ERRATUM_IFC_A002769 - 41 : CONFIG_SYS_FSL_ERRATUM_A007075 - 31 : CONFIG_SYS_FSL_DDR_VER_44 - 28 : CONFIG_ARCH_P1010 - 28 : CONFIG_SYS_FSL_ERRATUM_P1010_A003549 - 28 : CONFIG_SYS_FSL_ERRATUM_SEC_A003571 - 28 : CONFIG_SYS_FSL_ERRATUM_IFC_A003399 - 25 : CONFIG_SYS_FSL_ERRATUM_A008044 - 22 : CONFIG_ARCH_P1020 - 21 : CONFIG_SYS_FSL_DDR_VER_46 - 20 : CONFIG_MAX_PIRQ_LINKS - 20 : CONFIG_HPET_ADDRESS - 20 : CONFIG_X86 - 20 : CONFIG_PCIE_ECAM_SIZE - 20 : CONFIG_IRQ_SLOT_COUNT - 20 : CONFIG_I8259_PIC - 20 : CONFIG_CPU_ADDR_BITS - 20 : CONFIG_RAMBASE - 20 : CONFIG_SYS_FSL_ERRATUM_A005871 - 20 : CONFIG_PCIE_ECAM_BASE - 20 : CONFIG_X86_TSC_TIMER - 20 : CONFIG_I8254_TIMER - 20 : CONFIG_CMD_GETTIME - 19 : CONFIG_SYS_FSL_ERRATUM_A005812 - 18 : CONFIG_X86_RUN_32BIT - 17 : CONFIG_CMD_CHIP_CONFIG - ... + ./tools/moveconfig.py -i CONFIG_I8042_KEYB + CONFIG_I8042_KEYB found in 33/5155 defconfigs + 28 : CONFIG_X86 + 28 : CONFIG_SA_PCIEX_LENGTH + 28 : CONFIG_HPET_ADDRESS + 28 : CONFIG_MAX_PIRQ_LINKS + 28 : CONFIG_I8254_TIMER + 28 : CONFIG_I8259_PIC + 28 : CONFIG_RAMBASE + 28 : CONFIG_IRQ_SLOT_COUNT + 28 : CONFIG_PCIE_ECAM_SIZE + 28 : CONFIG_APIC + ... -This shows a list of config options which might imply CONFIG_CMD_EEPROM along +This shows a list of config options which might imply CONFIG_I8042_KEYB along with how many defconfigs they cover. From this you can see that CONFIG_X86 -implies CONFIG_CMD_EEPROM. Therefore, instead of adding CONFIG_CMD_EEPROM to +generally implies CONFIG_I8042_KEYB but not always (28 out of 35). Therefore, +instead of adding CONFIG_I8042_KEYB to the defconfig of every x86 board, you could add a single imply line to the -Kconfig file: +Kconfig file:: config X86 bool "x86 architecture" ... imply CMD_EEPROM -That will cover 20 defconfigs. Many of the options listed are not suitable as -they are not related. E.g. it would be odd for CONFIG_CMD_GETTIME to imply -CMD_EEPROM. +That will cover 28 defconfigs and you can perhaps find another condition that +indicates that CONFIG_I8042_KEYB is not needed for the remaining 5 boards. Many +of the options listed are not suitable as they are not related. E.g. it would be +odd for CONFIG_RAMBASE to imply CONFIG_I8042_KEYB. Using this search you can reduce the size of moveconfig patches. From ea40b20431c0c5de1856c56f965757a40df8d3d7 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 21 Jul 2021 21:35:53 -0600 Subject: [PATCH 19/23] moveconfig: Update to newer kconfiglib Some of the more advanced features of this tool don't work anymore since kconfiglib was update. Update the code accordingly. Signed-off-by: Simon Glass --- tools/moveconfig.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/moveconfig.py b/tools/moveconfig.py index f3fd75504f7..373b395fda4 100755 --- a/tools/moveconfig.py +++ b/tools/moveconfig.py @@ -1262,8 +1262,8 @@ def find_kconfig_rules(kconf, config, imply_config): """ sym = kconf.syms.get(imply_config) if sym: - for sel in sym.get_selected_symbols() | sym.get_implied_symbols(): - if sel.get_name() == config: + for sel, cond in (sym.selects + sym.implies): + if sel == config: return sym return None @@ -1288,10 +1288,10 @@ def check_imply_rule(kconf, config, imply_config): sym = kconf.syms.get(imply_config) if not sym: return 'cannot find sym' - locs = sym.get_def_locations() - if len(locs) != 1: - return '%d locations' % len(locs) - fname, linenum = locs[0] + nodes = sym.nodes + if len(nodes) != 1: + return '%d locations' % len(nodes) + fname, linenum = nodes[0].filename, nodes[0].linern cwd = os.getcwd() if cwd and fname.startswith(cwd): fname = fname[len(cwd) + 1:] @@ -1502,9 +1502,9 @@ def do_imply_config(config_list, add_imply, imply_flags, skip_added, iconfig[CONFIG_LEN:]) kconfig_info = '' if sym: - locs = sym.get_def_locations() - if len(locs) == 1: - fname, linenum = locs[0] + nodes = sym.nodes + if len(nodes) == 1: + fname, linenum = nodes[0].filename, nodes[0].linenr if cwd and fname.startswith(cwd): fname = fname[len(cwd) + 1:] kconfig_info = '%s:%d' % (fname, linenum) @@ -1514,9 +1514,9 @@ def do_imply_config(config_list, add_imply, imply_flags, skip_added, sym = kconf.syms.get(iconfig[CONFIG_LEN:]) fname = '' if sym: - locs = sym.get_def_locations() - if len(locs) == 1: - fname, linenum = locs[0] + nodes = sym.nodes + if len(nodes) == 1: + fname, linenum = nodes[0].filename, nodes[0].linenr if cwd and fname.startswith(cwd): fname = fname[len(cwd) + 1:] in_arch_board = not sym or (fname.startswith('arch') or From cb8970092f2cd5f1eaadf7d15d28cb905067e07f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 24 Jul 2021 15:14:39 -0600 Subject: [PATCH 20/23] sandbox: Reduce keyed autoboot delay The autoboot tests are a recent addition to U-Boot, providing much-needed coverage in this area. A side effect of the keyed autoboot test is that this feature is enabled in sandbox always. This changes the autoboot prompt and confuses the pytests. Some tests become slower, for example the vboot tests take about 27s now instead of 3s. We don't actually need this feature enabled to be able to run the tests. Add a switch to allow sandbox to turn it on and off as needed. Use this in the one test that needs it. Add a command-line flag in case this is desired in normal use. Signed-off-by: Simon Glass Fixes: 25c8b9f298e ("test: add first autoboot unit tests") Reviewed-by: Steffen Jaeckel --- arch/sandbox/cpu/start.c | 9 ++++++++ arch/sandbox/cpu/state.c | 18 ++++++++++++++++ arch/sandbox/include/asm/state.h | 1 + common/autoboot.c | 6 +++--- include/autoboot.h | 36 ++++++++++++++++++++++++++++++++ test/common/test_autoboot.c | 6 ++++++ 6 files changed, 73 insertions(+), 3 deletions(-) diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c index 777db4e9522..a74f5ec7ba0 100644 --- a/arch/sandbox/cpu/start.c +++ b/arch/sandbox/cpu/start.c @@ -400,6 +400,15 @@ static int sandbox_cmdline_cb_signals(struct sandbox_state *state, SANDBOX_CMDLINE_OPT_SHORT(signals, 'S', 0, "Handle signals (such as SIGSEGV) in sandbox"); +static int sandbox_cmdline_cb_autoboot_keyed(struct sandbox_state *state, + const char *arg) +{ + state->autoboot_keyed = true; + + return 0; +} +SANDBOX_CMDLINE_OPT(autoboot_keyed, 0, "Allow keyed autoboot"); + static void setup_ram_buf(struct sandbox_state *state) { /* Zero the RAM buffer if we didn't read it, to keep valgrind happy */ diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c index a4d99bade41..4e822538baf 100644 --- a/arch/sandbox/cpu/state.c +++ b/arch/sandbox/cpu/state.c @@ -4,6 +4,7 @@ */ #include +#include #include #include #include @@ -378,6 +379,23 @@ void state_reset_for_test(struct sandbox_state *state) state->next_tag = state->ram_size; } +bool autoboot_keyed(void) +{ + struct sandbox_state *state = state_get_current(); + + return IS_ENABLED(CONFIG_AUTOBOOT_KEYED) && state->autoboot_keyed; +} + +bool autoboot_set_keyed(bool autoboot_keyed) +{ + struct sandbox_state *state = state_get_current(); + bool old_val = state->autoboot_keyed; + + state->autoboot_keyed = autoboot_keyed; + + return old_val; +} + int state_init(void) { state = &main_state; diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h index 1c4c571e28d..10352a587e4 100644 --- a/arch/sandbox/include/asm/state.h +++ b/arch/sandbox/include/asm/state.h @@ -94,6 +94,7 @@ struct sandbox_state { bool run_unittests; /* Run unit tests */ const char *select_unittests; /* Unit test to run */ bool handle_signals; /* Handle signals within sandbox */ + bool autoboot_keyed; /* Use keyed-autoboot feature */ /* Pointer to information for each SPI bus/cs */ struct sandbox_spi_info spi[CONFIG_SANDBOX_SPI_MAX_BUS] diff --git a/common/autoboot.c b/common/autoboot.c index 8b9e9aa8785..5bb2e190895 100644 --- a/common/autoboot.c +++ b/common/autoboot.c @@ -406,7 +406,7 @@ static int abortboot(int bootdelay) int abort = 0; if (bootdelay >= 0) { - if (IS_ENABLED(CONFIG_AUTOBOOT_KEYED)) + if (autoboot_keyed()) abort = abortboot_key_sequence(bootdelay); else abort = abortboot_single_key(bootdelay); @@ -481,7 +481,7 @@ void autoboot_command(const char *s) bool lock; int prev; - lock = IS_ENABLED(CONFIG_AUTOBOOT_KEYED) && + lock = autoboot_keyed() && !IS_ENABLED(CONFIG_AUTOBOOT_KEYED_CTRLC); if (lock) prev = disable_ctrlc(1); /* disable Ctrl-C checking */ @@ -498,4 +498,4 @@ void autoboot_command(const char *s) if (s) run_command_list(s, -1, 0); } -} \ No newline at end of file +} diff --git a/include/autoboot.h b/include/autoboot.h index ac8157e5704..d6915dd0cc6 100644 --- a/include/autoboot.h +++ b/include/autoboot.h @@ -11,6 +11,42 @@ #ifndef __AUTOBOOT_H #define __AUTOBOOT_H +#include + +#ifdef CONFIG_SANDBOX + +/** + * autoboot_keyed() - check whether keyed autoboot should be used + * + * This is only implemented for sandbox since other platforms don't have a way + * of controlling the feature at runtime. + * + * @return true if enabled, false if not + */ +bool autoboot_keyed(void); + +/** + * autoboot_set_keyed() - set whether keyed autoboot should be used + * + * @autoboot_keyed: true to enable the feature, false to disable + * @return old value of the flag + */ +bool autoboot_set_keyed(bool autoboot_keyed); +#else +static inline bool autoboot_keyed(void) +{ + /* There is no runtime flag, so just use the CONFIG */ + return IS_ENABLED(CONFIG_AUTOBOOT_KEYED); +} + +static inline bool autoboot_set_keyed(bool autoboot_keyed) +{ + /* There is no runtime flag to set */ + return false; +} + +#endif + #ifdef CONFIG_AUTOBOOT /** * bootdelay_process() - process the bootd delay diff --git a/test/common/test_autoboot.c b/test/common/test_autoboot.c index 6564ac70496..42a1e4ab1fa 100644 --- a/test/common/test_autoboot.c +++ b/test/common/test_autoboot.c @@ -16,13 +16,19 @@ static int check_for_input(struct unit_test_state *uts, const char *in, bool correct) { + bool old_val; /* The bootdelay is set to 1 second in test_autoboot() */ const char *autoboot_prompt = "Enter password \"a\" in 1 seconds to stop autoboot"; console_record_reset_enable(); console_in_puts(in); + + /* turn on keyed autoboot for the test, if possible */ + old_val = autoboot_set_keyed(true); autoboot_command("echo Autoboot password unlock not successful"); + old_val = autoboot_set_keyed(old_val); + ut_assert_nextline(autoboot_prompt); if (!correct) ut_assert_nextline("Autoboot password unlock not successful"); From df82de805172687e88dd7d72b68a9223b0a4c269 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 28 Jul 2021 19:23:09 -0600 Subject: [PATCH 21/23] dtoc: Rename is_wider_than() to reduce confusion The current name is confusing because the logic is actually backwards from what you might expect. Rename it to needs_widening() and update the comments. Signed-off-by: Simon Glass --- tools/dtoc/fdt.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index 3996971e39c..9749966d5fb 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -24,16 +24,19 @@ from patman import tools # A list of types we support class Type(IntEnum): + # Types in order from widest to narrowest (BYTE, INT, STRING, BOOL, INT64) = range(5) - def is_wider_than(self, other): - """Check if another type is 'wider' than this one + def needs_widening(self, other): + """Check if this type needs widening to hold a value from another type - A wider type is one that holds more information than an earlier one, - similar to the concept of type-widening in C. + A wider type is one that can hold a wider array of information than + another one, or is less restrictive, so it can hold the information of + another type as well as its own. This is similar to the concept of + type-widening in C. This uses a simple arithmetic comparison, since type values are in order - from narrowest (BYTE) to widest (INT64). + from widest (BYTE) to narrowest (INT64). Args: other: Other type to compare against @@ -149,7 +152,7 @@ class Prop: update the current property to be like the second, since it is less specific. """ - if self.type.is_wider_than(newprop.type): + if self.type.needs_widening(newprop.type): if self.type == Type.INT and newprop.type == Type.BYTE: if type(self.value) == list: new_value = [] From ca04494d76bf1152cd9ab1f67af5101c86e0824f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 28 Jul 2021 19:23:10 -0600 Subject: [PATCH 22/23] dtoc: Fix widening an int array to an int An int array can hold a single int so we should not need to do anything in the widening operation. However due to a quirk in the code, an int[3] widened with an int produced an int[4]. Fix this and add a test. Fix a comment typo while we are here. Signed-off-by: Simon Glass Reported-by: Tom Rini --- test/dm/of_platdata.c | 4 +--- tools/dtoc/fdt.py | 13 +++++++------ tools/dtoc/test_dtoc.py | 6 +++--- tools/dtoc/test_fdt.py | 11 ++++++++++- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/test/dm/of_platdata.c b/test/dm/of_platdata.c index 0f89c7a7da8..e3fa01afddf 100644 --- a/test/dm/of_platdata.c +++ b/test/dm/of_platdata.c @@ -35,11 +35,10 @@ static int dm_test_of_plat_props(struct unit_test_state *uts) plat = dev_get_plat(dev); ut_assert(plat->boolval); ut_asserteq(1, plat->intval); - ut_asserteq(4, ARRAY_SIZE(plat->intarray)); + ut_asserteq(3, ARRAY_SIZE(plat->intarray)); ut_asserteq(2, plat->intarray[0]); ut_asserteq(3, plat->intarray[1]); ut_asserteq(4, plat->intarray[2]); - ut_asserteq(0, plat->intarray[3]); ut_asserteq(5, plat->byteval); ut_asserteq(3, ARRAY_SIZE(plat->bytearray)); ut_asserteq(6, plat->bytearray[0]); @@ -61,7 +60,6 @@ static int dm_test_of_plat_props(struct unit_test_state *uts) ut_asserteq(5, plat->intarray[0]); ut_asserteq(0, plat->intarray[1]); ut_asserteq(0, plat->intarray[2]); - ut_asserteq(0, plat->intarray[3]); ut_asserteq(8, plat->byteval); ut_asserteq(3, ARRAY_SIZE(plat->bytearray)); ut_asserteq(1, plat->bytearray[0]); diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index 9749966d5fb..429e95f9a96 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -163,13 +163,14 @@ class Prop: self.value = new_value self.type = newprop.type - if type(newprop.value) == list and type(self.value) != list: - self.value = [self.value] + if type(newprop.value) == list: + if type(self.value) != list: + self.value = [self.value] - if type(self.value) == list and len(newprop.value) > len(self.value): - val = self.GetEmpty(self.type) - while len(self.value) < len(newprop.value): - self.value.append(val) + if len(newprop.value) > len(self.value): + val = self.GetEmpty(self.type) + while len(self.value) < len(newprop.value): + self.value.append(val) @classmethod def GetEmpty(self, type): diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py index 863ede90b7a..44d5d0c354a 100755 --- a/tools/dtoc/test_dtoc.py +++ b/tools/dtoc/test_dtoc.py @@ -296,7 +296,7 @@ struct dtd_sandbox_spl_test { \tbool\t\tboolval; \tunsigned char\tbytearray[3]; \tunsigned char\tbyteval; -\tfdt32_t\t\tintarray[4]; +\tfdt32_t\t\tintarray[3]; \tfdt32_t\t\tintval; \tunsigned char\tlongbytearray[9]; \tunsigned char\tnotstring[5]; @@ -354,7 +354,7 @@ static struct dtd_sandbox_spl_test dtv_spl_test = { \t.boolval\t\t= true, \t.bytearray\t\t= {0x6, 0x0, 0x0}, \t.byteval\t\t= 0x5, -\t.intarray\t\t= {0x2, 0x3, 0x4, 0x0}, +\t.intarray\t\t= {0x2, 0x3, 0x4}, \t.intval\t\t\t= 0x1, \t.longbytearray\t\t= {0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0x10, \t\t0x11}, @@ -377,7 +377,7 @@ static struct dtd_sandbox_spl_test dtv_spl_test2 = { \t.acpi_name\t\t= "\\\\_SB.GPO0", \t.bytearray\t\t= {0x1, 0x23, 0x34}, \t.byteval\t\t= 0x8, -\t.intarray\t\t= {0x5, 0x0, 0x0, 0x0}, +\t.intarray\t\t= {0x5, 0x0, 0x0}, \t.intval\t\t\t= 0x3, \t.longbytearray\t\t= {0x9, 0xa, 0xb, 0xc, 0x0, 0x0, 0x0, 0x0, \t\t0x0}, diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index 856392b1bd9..857861c14ed 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -379,7 +379,7 @@ class TestProp(unittest.TestCase): self.assertEqual(Type.INT, prop.type) self.assertEqual(1, fdt32_to_cpu(prop.value)) - # Convert singla value to array + # Convert single value to array prop2 = self.node.props['intarray'] prop.Widen(prop2) self.assertEqual(Type.INT, prop.type) @@ -422,6 +422,15 @@ class TestProp(unittest.TestCase): self.assertTrue(isinstance(prop.value, list)) self.assertEqual(3, len(prop.value)) + # Widen an array of ints with an int (should do nothing) + prop = self.node.props['intarray'] + prop2 = node2.props['intarray'] + self.assertEqual(Type.INT, prop.type) + self.assertEqual(3, len(prop.value)) + prop.Widen(prop2) + self.assertEqual(Type.INT, prop.type) + self.assertEqual(3, len(prop.value)) + def testAdd(self): """Test adding properties""" self.fdt.pack() From eec44c7218a3c3ce924a282cc46a59e83feb9de1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 28 Jul 2021 19:23:11 -0600 Subject: [PATCH 23/23] dtoc: Support widening a bool value At present if we see 'ranges' property (with no value) we assume it is a boolean, as per the devicetree spec. But another node may define 'ranges' with a value, forcing us to widen it to an int array. At present this is not supported and causes an error. Fix this and add some test cases. Signed-off-by: Simon Glass Reported-by: Tom Rini --- arch/sandbox/dts/sandbox.dtsi | 2 ++ test/dm/of_platdata.c | 3 +++ tools/dtoc/fdt.py | 12 ++++++++++++ tools/dtoc/test/dtoc_test_simple.dts | 2 ++ tools/dtoc/test_dtoc.py | 3 +++ tools/dtoc/test_fdt.py | 18 ++++++++++++++++-- 6 files changed, 38 insertions(+), 2 deletions(-) diff --git a/arch/sandbox/dts/sandbox.dtsi b/arch/sandbox/dts/sandbox.dtsi index 31db50db352..200fcab6a41 100644 --- a/arch/sandbox/dts/sandbox.dtsi +++ b/arch/sandbox/dts/sandbox.dtsi @@ -231,6 +231,7 @@ boolval; intval = <1>; intarray = <2 3 4>; + maybe-empty-int = <>; byteval = [05]; bytearray = [06]; longbytearray = [09 0a 0b 0c 0d 0e 0f 10 11]; @@ -254,6 +255,7 @@ u-boot,dm-pre-reloc; compatible = "sandbox,spl-test"; stringarray = "one"; + maybe-empty-int = <1>; }; spl-test5 { diff --git a/test/dm/of_platdata.c b/test/dm/of_platdata.c index e3fa01afddf..0463cf0b433 100644 --- a/test/dm/of_platdata.c +++ b/test/dm/of_platdata.c @@ -40,6 +40,8 @@ static int dm_test_of_plat_props(struct unit_test_state *uts) ut_asserteq(3, plat->intarray[1]); ut_asserteq(4, plat->intarray[2]); ut_asserteq(5, plat->byteval); + ut_asserteq(1, ARRAY_SIZE(plat->maybe_empty_int)); + ut_asserteq(0, plat->maybe_empty_int[0]); ut_asserteq(3, ARRAY_SIZE(plat->bytearray)); ut_asserteq(6, plat->bytearray[0]); ut_asserteq(0, plat->bytearray[1]); @@ -78,6 +80,7 @@ static int dm_test_of_plat_props(struct unit_test_state *uts) ut_asserteq_str("one", plat->stringarray[0]); ut_asserteq_str("", plat->stringarray[1]); ut_asserteq_str("", plat->stringarray[2]); + ut_asserteq(1, plat->maybe_empty_int[0]); ut_assertok(uclass_next_device_err(&dev)); plat = dev_get_plat(dev); diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index 429e95f9a96..32a7aa98290 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -153,6 +153,18 @@ class Prop: specific. """ if self.type.needs_widening(newprop.type): + + # A boolean has an empty value: if it exists it is True and if not + # it is False. So when widening we always start with an empty list + # since the only valid integer property would be an empty list of + # integers. + # e.g. this is a boolean: + # some-prop; + # and it would be widened to int list by: + # some-prop = <1 2>; + if self.type == Type.BOOL: + self.type = Type.INT + self.value = [self.GetEmpty(self.type)] if self.type == Type.INT and newprop.type == Type.BYTE: if type(self.value) == list: new_value = [] diff --git a/tools/dtoc/test/dtoc_test_simple.dts b/tools/dtoc/test/dtoc_test_simple.dts index b5c1274bb7c..5a6fa88d5cc 100644 --- a/tools/dtoc/test/dtoc_test_simple.dts +++ b/tools/dtoc/test/dtoc_test_simple.dts @@ -14,6 +14,7 @@ u-boot,dm-pre-reloc; compatible = "sandbox,spl-test"; boolval; + maybe-empty-int = <>; intval = <1>; intarray = <2 3 4>; byteval = [05]; @@ -42,6 +43,7 @@ compatible = "sandbox,spl-test"; stringarray = "one"; longbytearray = [09 0a 0b 0c 0d 0e 0f 10]; + maybe-empty-int = <1>; }; i2c@0 { diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py index 44d5d0c354a..752061f27a4 100755 --- a/tools/dtoc/test_dtoc.py +++ b/tools/dtoc/test_dtoc.py @@ -299,6 +299,7 @@ struct dtd_sandbox_spl_test { \tfdt32_t\t\tintarray[3]; \tfdt32_t\t\tintval; \tunsigned char\tlongbytearray[9]; +\tfdt32_t\t\tmaybe_empty_int[1]; \tunsigned char\tnotstring[5]; \tconst char *\tstringarray[3]; \tconst char *\tstringval; @@ -358,6 +359,7 @@ static struct dtd_sandbox_spl_test dtv_spl_test = { \t.intval\t\t\t= 0x1, \t.longbytearray\t\t= {0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0x10, \t\t0x11}, +\t.maybe_empty_int\t= {0x0}, \t.notstring\t\t= {0x20, 0x21, 0x22, 0x10, 0x0}, \t.stringarray\t\t= {"multi-word", "message", ""}, \t.stringval\t\t= "message", @@ -398,6 +400,7 @@ U_BOOT_DRVINFO(spl_test2) = { static struct dtd_sandbox_spl_test dtv_spl_test3 = { \t.longbytearray\t\t= {0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0x10, \t\t0x0}, +\t.maybe_empty_int\t= {0x1}, \t.stringarray\t\t= {"one", "", ""}, }; U_BOOT_DRVINFO(spl_test3) = { diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index 857861c14ed..1119e6b7847 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -122,8 +122,9 @@ class TestFdt(unittest.TestCase): node = self.dtb.GetNode('/spl-test') props = self.dtb.GetProps(node) self.assertEqual(['boolval', 'bytearray', 'byteval', 'compatible', - 'intarray', 'intval', 'longbytearray', 'notstring', - 'stringarray', 'stringval', 'u-boot,dm-pre-reloc'], + 'intarray', 'intval', 'longbytearray', + 'maybe-empty-int', 'notstring', 'stringarray', + 'stringval', 'u-boot,dm-pre-reloc'], sorted(props.keys())) def testCheckError(self): @@ -431,6 +432,19 @@ class TestProp(unittest.TestCase): self.assertEqual(Type.INT, prop.type) self.assertEqual(3, len(prop.value)) + # Widen an empty bool to an int + prop = self.node.props['maybe-empty-int'] + prop3 = node3.props['maybe-empty-int'] + self.assertEqual(Type.BOOL, prop.type) + self.assertEqual(True, prop.value) + self.assertEqual(Type.INT, prop3.type) + self.assertFalse(isinstance(prop.value, list)) + self.assertEqual(4, len(prop3.value)) + prop.Widen(prop3) + self.assertEqual(Type.INT, prop.type) + self.assertTrue(isinstance(prop.value, list)) + self.assertEqual(1, len(prop.value)) + def testAdd(self): """Test adding properties""" self.fdt.pack()