From 5c6a8aecd9ab428cd18f86c00d9bfd29ab5e0723 Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Fri, 17 Jul 2020 12:35:48 +0200 Subject: [PATCH 1/5] sof: Calculate source code hash It may be used to check FW compatibility with ldc file. It's much better than comparing DBG_ABI because logs content may be updated without any DBG_ABI change in opposite to source code hash value. Signed-off-by: Karol Trzcinski --- scripts/cmake/version.cmake | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/scripts/cmake/version.cmake b/scripts/cmake/version.cmake index 0223d4982987..b41980bfdbcd 100644 --- a/scripts/cmake/version.cmake +++ b/scripts/cmake/version.cmake @@ -59,6 +59,34 @@ if(NOT SOF_TAG) set(SOF_TAG 0) endif() +# Calculate source hash value, used to check ldc file and firmware compatibility +if(EXISTS ${CMAKE_SOURCE_DIR}/.git/) + # list tracked files from src directory + execute_process(COMMAND git ls-files src + WORKING_DIRECTORY ${SOF_ROOT_SOURCE_DIRECTORY} + OUTPUT_FILE tracked_file_list + ) + # calculate hash of each listed files (from file version saved in file system) + execute_process(COMMAND git hash-object --stdin-paths + WORKING_DIRECTORY ${SOF_ROOT_SOURCE_DIRECTORY} + INPUT_FILE tracked_file_list + OUTPUT_FILE tracked_file_hash_list + ) + # then calculate single hash of previously calculated hash list + execute_process(COMMAND git hash-object --stdin + WORKING_DIRECTORY ${SOF_ROOT_SOURCE_DIRECTORY} + OUTPUT_STRIP_TRAILING_WHITESPACE + INPUT_FILE tracked_file_hash_list + OUTPUT_VARIABLE SOF_SRC_HASH_LONG + ) + file(REMOVE tracked_file_list tracked_file_hash_list) + string(SUBSTRING ${SOF_SRC_HASH_LONG} 0 8 SOF_SRC_HASH) + message(STATUS "Source content hash: ${SOF_SRC_HASH}") +else() + set(SOF_SRC_HASH ${GIT_LOG_HASH}) + message(WARNING "Source content hash can't be calculated, use GIT_LOG_HASH") +endif() + # for SOF_BUILD include(${CMAKE_CURRENT_LIST_DIR}/version-build-counter.cmake) @@ -69,6 +97,7 @@ function(sof_check_version_h) "#define SOF_MICRO ${SOF_MICRO}\n" "#define SOF_TAG \"${SOF_TAG}\"\n" "#define SOF_BUILD ${SOF_BUILD}\n" + "#define SOF_SRC_HASH 0x${SOF_SRC_HASH}\n" ) if(EXISTS "${VERSION_H_PATH}") From a7cf0fd828fa0f56ad44c2fdb7fac177bdf8aed9 Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Fri, 17 Jul 2020 12:37:01 +0200 Subject: [PATCH 2/5] sof: Add `src_hash` to `sof_ipc_fw_version` structure This field will be used to compare ldc file with loaded fw version, to assert validity of trace logs. Value used in sof-logger. Signed-off-by: Karol Trzcinski --- src/include/ipc/info.h | 4 +++- src/init/ext_manifest.c | 1 + src/platform/baytrail/platform.c | 1 + src/platform/haswell/platform.c | 1 + src/platform/imx8/platform.c | 1 + src/platform/imx8m/platform.c | 1 + src/platform/intel/cavs/platform.c | 1 + 7 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/include/ipc/info.h b/src/include/ipc/info.h index 52e5ee78bd27..3f628b174467 100644 --- a/src/include/ipc/info.h +++ b/src/include/ipc/info.h @@ -54,9 +54,11 @@ struct sof_ipc_fw_version { uint8_t time[10]; uint8_t tag[6]; uint32_t abi_version; + /* used to check FW and ldc file compatibility, reproducible value */ + uint32_t src_hash; /* reserved for future use */ - uint32_t reserved[4]; + uint32_t reserved[3]; } __attribute__((packed)); /* FW ready Message - sent by firmware when boot has completed */ diff --git a/src/init/ext_manifest.c b/src/init/ext_manifest.c index d209752977ab..985cbfbfad47 100644 --- a/src/init/ext_manifest.c +++ b/src/init/ext_manifest.c @@ -32,6 +32,7 @@ const struct ext_man_fw_version ext_man_fw_ver #endif .tag = SOF_TAG, .abi_version = SOF_ABI_VERSION, + .src_hash = SOF_SRC_HASH, }, .flags = DEBUG_SET_FW_READY_FLAGS, }; diff --git a/src/platform/baytrail/platform.c b/src/platform/baytrail/platform.c index 63c9e02fef44..3195f47bea75 100644 --- a/src/platform/baytrail/platform.c +++ b/src/platform/baytrail/platform.c @@ -72,6 +72,7 @@ static const struct sof_ipc_fw_ready ready #endif .tag = SOF_TAG, .abi_version = SOF_ABI_VERSION, + .src_hash = SOF_SRC_HASH, }, .flags = DEBUG_SET_FW_READY_FLAGS }; diff --git a/src/platform/haswell/platform.c b/src/platform/haswell/platform.c index dca7a66ca9ca..5762e4f9d10a 100644 --- a/src/platform/haswell/platform.c +++ b/src/platform/haswell/platform.c @@ -58,6 +58,7 @@ static const struct sof_ipc_fw_ready ready #endif .tag = SOF_TAG, .abi_version = SOF_ABI_VERSION, + .src_hash = SOF_SRC_HASH, }, .flags = DEBUG_SET_FW_READY_FLAGS, }; diff --git a/src/platform/imx8/platform.c b/src/platform/imx8/platform.c index 6710241af565..be6801a43a1a 100644 --- a/src/platform/imx8/platform.c +++ b/src/platform/imx8/platform.c @@ -57,6 +57,7 @@ static const struct sof_ipc_fw_ready ready #endif .tag = SOF_TAG, .abi_version = SOF_ABI_VERSION, + .src_hash = SOF_SRC_HASH, }, .flags = DEBUG_SET_FW_READY_FLAGS, }; diff --git a/src/platform/imx8m/platform.c b/src/platform/imx8m/platform.c index f415fdb16964..92383495698f 100644 --- a/src/platform/imx8m/platform.c +++ b/src/platform/imx8m/platform.c @@ -56,6 +56,7 @@ static const struct sof_ipc_fw_ready ready #endif .tag = SOF_TAG, .abi_version = SOF_ABI_VERSION, + .src_hash = SOF_SRC_HASH, }, .flags = DEBUG_SET_FW_READY_FLAGS, }; diff --git a/src/platform/intel/cavs/platform.c b/src/platform/intel/cavs/platform.c index 14afc8b3ff51..ba46cbad2eb6 100644 --- a/src/platform/intel/cavs/platform.c +++ b/src/platform/intel/cavs/platform.c @@ -68,6 +68,7 @@ static const struct sof_ipc_fw_ready ready #endif .tag = SOF_TAG, .abi_version = SOF_ABI_VERSION, + .src_hash = SOF_SRC_HASH, }, .flags = DEBUG_SET_FW_READY_FLAGS, }; From b89b649778da80bcee1b67a74420fabea5b1a6c2 Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Wed, 8 Jul 2020 13:38:58 +0200 Subject: [PATCH 3/5] logger: Validate by src_hash instead of abi version from fw_ready ABI version saved in fw_ready doesn't match with DBG_ABI version saved in ldc file even for proper pair of fw and ldc file. Moreover ldc file content changes at any log modyfication, what is not related with DBG_ABI change, so this way of solving this problem is incorrect. Introduced src_hash value change solves both problems. Signed-off-by: Karol Trzcinski --- src/include/user/abi_dbg.h | 2 +- tools/logger/convert.c | 29 +++++------------------------ 2 files changed, 6 insertions(+), 25 deletions(-) diff --git a/src/include/user/abi_dbg.h b/src/include/user/abi_dbg.h index 8ea8ef37da2f..16bf77fa9c92 100644 --- a/src/include/user/abi_dbg.h +++ b/src/include/user/abi_dbg.h @@ -19,7 +19,7 @@ #define __USER_ABI_DBG_H__ #define SOF_ABI_DBG_MAJOR 5 -#define SOF_ABI_DBG_MINOR 0 +#define SOF_ABI_DBG_MINOR 1 #define SOF_ABI_DBG_PATCH 0 #define SOF_ABI_DBG_VERSION SOF_ABI_VER(SOF_ABI_DBG_MAJOR, \ diff --git a/tools/logger/convert.c b/tools/logger/convert.c index 1f26e44112df..2fe837a0da4e 100644 --- a/tools/logger/convert.c +++ b/tools/logger/convert.c @@ -660,7 +660,7 @@ static int verify_fw_ver(const struct convert_config *config, const struct snd_sof_logs_header *snd) { struct sof_ipc_fw_version ver; - int count, ret = 0; + int count; if (!config->version_fd) return 0; @@ -673,30 +673,11 @@ static int verify_fw_ver(const struct convert_config *config, return -ferror(config->version_fd); } - ret = memcmp(&ver, &snd->version, sizeof(struct sof_ipc_fw_version)); - if (ret) { - log_err(config->out_fd, - "fw version in %s file does not coincide with fw version in %s file.\n", - config->ldc_file, config->version_file); - return -EINVAL; - } - - /* logger and version_file abi dbg verification */ - if (SOF_ABI_VERSION_INCOMPATIBLE(SOF_ABI_DBG_VERSION, - ver.abi_version)) { - log_err(config->out_fd, - "abi version in %s file does not coincide with abi version used by logger.\n", - config->version_file); - log_err(config->out_fd, - "logger ABI Version is %d:%d:%d\n", - SOF_ABI_VERSION_MAJOR(SOF_ABI_DBG_VERSION), - SOF_ABI_VERSION_MINOR(SOF_ABI_DBG_VERSION), - SOF_ABI_VERSION_PATCH(SOF_ABI_DBG_VERSION)); + /* compare source hash value from version file and ldc file */ + if (ver.src_hash != snd->version.src_hash) { log_err(config->out_fd, - "version_file ABI Version is %d:%d:%d\n", - SOF_ABI_VERSION_MAJOR(ver.abi_version), - SOF_ABI_VERSION_MINOR(ver.abi_version), - SOF_ABI_VERSION_PATCH(ver.abi_version)); + "src hash value from version file (0x%x) differ from src hash version saved in dictionary (0x%x).\n", + ver.src_hash, snd->version.src_hash); return -EINVAL; } return 0; From 1eca05b14c7eca365d936ab403cd7803eb89e96e Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Wed, 29 Jul 2020 09:45:06 +0200 Subject: [PATCH 4/5] cmake: Include GIT_TAG in generated version.h file GIT_TAG is user readable form of used source code version with commit identifier, what is important for bugs reproducibility, so it will be convenient to have this information in output logs. Signed-off-by: Karol Trzcinski --- scripts/cmake/version.cmake | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/cmake/version.cmake b/scripts/cmake/version.cmake index b41980bfdbcd..f806845eaded 100644 --- a/scripts/cmake/version.cmake +++ b/scripts/cmake/version.cmake @@ -97,6 +97,7 @@ function(sof_check_version_h) "#define SOF_MICRO ${SOF_MICRO}\n" "#define SOF_TAG \"${SOF_TAG}\"\n" "#define SOF_BUILD ${SOF_BUILD}\n" + "#define SOF_GIT_TAG \"${GIT_TAG}\"\n" "#define SOF_SRC_HASH 0x${SOF_SRC_HASH}\n" ) From f14274a1a80e578b4482b782f4f0138f63fd12dc Mon Sep 17 00:00:00 2001 From: Karol Trzcinski Date: Fri, 26 Jun 2020 11:04:50 +0200 Subject: [PATCH 5/5] trace: Log FW ABI and hash numbers This log allows to fully identify source code and check ldc compatibility with FW by comparing source hash value saved in ldc file with value received from FW in runtimee. Without this mechanism it's impossible to be sure about right connection without booting firmware (to read fw_ready message content), because value of fw_hash hasn't been provided in output logs file. This is especially important during debugging process based on logs received by client. Signed-off-by: Karol Trzcinski --- src/trace/dma-trace.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/trace/dma-trace.c b/src/trace/dma-trace.c index e3a9fbe07ba4..ec6e5cfd7268 100644 --- a/src/trace/dma-trace.c +++ b/src/trace/dma-trace.c @@ -24,6 +24,9 @@ #include #include #include +#include +#include +#include #include #include @@ -335,6 +338,11 @@ int dma_trace_enable(struct dma_trace_data *d) d->enabled = 1; schedule_task(&d->dmat_work, DMA_TRACE_PERIOD, DMA_TRACE_PERIOD); + /* it should be the very first sent log for easily identification */ + tr_info(&dt_tr, "FW ABI 0x%x DBG ABI 0x%x tag " SOF_GIT_TAG " src hash 0x%x (ldc hash " META_QUOTE(SOF_SRC_HASH) ")", + SOF_ABI_VERSION, SOF_ABI_DBG_VERSION, SOF_SRC_HASH); + trace_flush(); + out: platform_shared_commit(d, sizeof(*d));