Skip to content

Logger abi check #3195

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lgirdwood merged 5 commits into from
Aug 18, 2020
Merged

Logger abi check #3195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5c6a8ae
sof: Calculate source code hash
ktrzcinx Jul 17, 2020
a7cf0fd
sof: Add `src_hash` to `sof_ipc_fw_version` structure
ktrzcinx Jul 17, 2020
b89b649
logger: Validate by src_hash instead of abi version from fw_ready
ktrzcinx Jul 8, 2020
1eca05b
cmake: Include GIT_TAG in generated version.h file
ktrzcinx Jul 29, 2020
f14274a
trace: Log FW ABI and hash numbers
ktrzcinx Jun 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions scripts/cmake/version.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Collaborator

@marc-hb marc-hb Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

message(WARNING "Source content hash not computed without git, _using_ GIT_LOG_HASH" instead)

"Use" is imperative mood.

endif()

Copy link
Member

@plbossart plbossart Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or on a different line: how can one identify a firmware based on a hash value? if I get 0xdeadbeef from a customer log, what's the next step? Can I find out what this firmware was from? Or is this enough to have the last git commit and a + that identifies the presence of local changes?

Copy link

@mmaka1 mmaka1 Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what is the purpose of logging the hash value in dma_trace_buffer_init(). The hash helps to verify integrity of trace content and the ldc file used to decode the traces). SOF_TAG is sufficient for identification if the fw is built directly from a commit w/o local changes. SOF_FW_HASH value reflects local changes so using it for log decoding is more reliable in the development process. I'd prefer to have SOF_TAG logged as the first trace line (while still use HASH for .ldc integrity check).

Copy link
Member

@plbossart plbossart Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what is the purpose of logging the hash value in dma_trace_buffer_init(). The hash helps to verify integrity of trace content and the ldc file used to decode the traces). SOF_TAG is sufficient for identification if the fw is built directly from a commit w/o local changes. SOF_FW_HASH value reflects local changes so using it for log decoding is more reliable in the development process. I'd prefer to have SOF_TAG logged as the first trace line (while still use HASH for .ldc integrity check).

All I care about is to have a well-defined/repeatable pattern that can be used to figure out if the trace works or not.

Copy link
Member Author

@ktrzcinx ktrzcinx Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is this enough to have the last git commit and a + that identifies the presence of local changes?

This one, but git commit hash must be red from extended manifest or fw_ready, so maybe I should add this value to log message, to have everything in one place

Copy link
Member Author

@ktrzcinx ktrzcinx Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what is the purpose of logging the hash value in dma_trace_buffer_init(). The hash helps to verify integrity of trace content and the ldc file used to decode the traces). SOF_TAG is sufficient for identification if the fw is built directly from a commit w/o local changes. SOF_FW_HASH value reflects local changes so using it for log decoding is more reliable in the development process. I'd prefer to have SOF_TAG logged as the first trace line (while still use HASH for .ldc integrity check).

After fw run, we have logger executable, ldc file, fw_ready file and binary trace data data to get user readable form of traces. Compatibility between logger and ldc file is checked by DBG_ABI, between fw_ready and ldc file by source hash, but there wasn't any check (or even saved information) between ldc/fw_ready and binary trace files. Logging FW source code hash value add possibility to check this compatibility without major DBG_ABI bump.

Copy link
Contributor

@jajanusz jajanusz Jul 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktrzcinx please handle tarball case, thanks, also I'm not sure if we should have so many tmp files in root of build folder, but maybe we should start worrying about it when there will be more of them

Copy link
Member Author

@ktrzcinx ktrzcinx Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should have so many tmp files in root of build folder

Ok, so I remove temp files after final hash number extraction

Copy link
Collaborator

@marc-hb marc-hb Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy to change later but you could move all the temp files to some new build_dir/source_hash/ subdirectory instead.

I don't think CMake builds should ever delete any temporary file because:

  1. Temporary files are useful to understand and debug the build. I very often browse or grep -r the build directory for that purpose. Then I can git grep the filenames and find the corresponding CMakeLists.txt source very quickly.
  2. With out-of-source builds there's no "pollution" concern anymore.

# for SOF_BUILD
include(${CMAKE_CURRENT_LIST_DIR}/version-build-counter.cmake)

Expand All @@ -69,6 +97,8 @@ 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"
)

if(EXISTS "${VERSION_H_PATH}")
Expand Down
4 changes: 3 additions & 1 deletion src/include/ipc/info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
2 changes: 1 addition & 1 deletion src/include/user/abi_dbg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, \
Expand Down
1 change: 1 addition & 0 deletions src/init/ext_manifest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down
1 change: 1 addition & 0 deletions src/platform/baytrail/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down
1 change: 1 addition & 0 deletions src/platform/haswell/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down
1 change: 1 addition & 0 deletions src/platform/imx8/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down
1 change: 1 addition & 0 deletions src/platform/imx8m/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down
1 change: 1 addition & 0 deletions src/platform/intel/cavs/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down
8 changes: 8 additions & 0 deletions src/trace/dma-trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
#include <sof/trace/dma-trace.h>
#include <ipc/topology.h>
#include <ipc/trace.h>
#include <kernel/abi.h>
#include <user/abi_dbg.h>
#include <version.h>

#include <errno.h>
#include <stddef.h>
Expand Down Expand Up @@ -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);
Copy link
Collaborator

@marc-hb marc-hb Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktrzcinx do you remember why you put both META_QUOTE(SOF_SRC_HASH) and %x SOF_SRC_HASH here? Do they ever look different from each other?

Copy link
Member Author

@ktrzcinx ktrzcinx Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They differ in case of using incompatible .ldc file, it's because META_QUOTE(SOF_SRC_HASH) comes from used .ldc file and SOF_SRC_HASH is .ri buildin variable.

Copy link
Collaborator

@marc-hb marc-hb Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ktrzcinx !

Do they ever look different from each other?

Answering that part of my own question: they do but only when forcing sof-logger to use an incompatible .ldc file with the -n option. META_QUOTE(SOF_SRC_HASH) is in the format string which is in the .ldc file and not in the firmware binary

trace_flush();

out:
platform_shared_commit(d, sizeof(*d));

Expand Down
29 changes: 5 additions & 24 deletions tools/logger/convert.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down