Skip to content
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.

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

Add processing of code coverage for optee core #4192

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
12 changes: 12 additions & 0 deletions core/arch/arm/plat-imx/conf.mk
Original file line number Diff line number Diff line change
Expand Up @@ -425,5 +425,17 @@ CFG_IMX_CAAM ?= y
endif
endif

ifeq ($(CFG_CORE_GCOV_SUPPORT), y)
ifneq ($(CFG_WITH_LPAE), y)
# Default:
# TEE_RAM_VA_SIZE = CORE_MMU_PGDIR_SIZE
# = BIT(CORE_MMU_PGDIR_SHIFT)
# with LPAE disabled: #define CORE_MMU_PGDIR_SHIFT 20
# = BIT(20)
# = 0x80000 (512 Ko)
Copy link
Contributor

Choose a reason for hiding this comment

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

That should be 0x100000 (1MB)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In effect, my bad

CFG_TEE_RAM_VA_SIZE := 0x200000 # 2MB
endif
endif

# Cryptographic configuration
include core/arch/arm/plat-imx/crypto_conf.mk
6 changes: 6 additions & 0 deletions core/core.mk
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,12 @@ include mk/lib.mk
base-prefix := $(sm)-
endif

ifeq ($(CFG_GCOV_SUPPORT),y)
libname = gcov
libdir = lib/libgcov
include mk/lib.mk
endif

ifeq ($(firstword $(subst /, ,$(CFG_CRYPTOLIB_DIR))),core)
# If a library can be compiled for both core and user space a base-prefix
# is needed in order to avoid conflicts in the output. However, if the
Expand Down
61 changes: 61 additions & 0 deletions lib/libgcov/core_dump_coverage.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/* SPDX-License-Identifier: BSD-2-Clause */
Copy link
Contributor

Choose a reason for hiding this comment

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

// SPDX-License-...

/*
* Copyright 2020 NXP
*/

#include <stdio.h>
#include <string.h>
#include <trace.h>

#include <gcov.h>
#include <optee_rpc_cmd.h>
#include <tee/tee_fs_rpc.h>
#include <tee_api_types.h>

#include "int_gcov.h"

/*
* Dump coverage data to REE FS
*
* The description is prepended to the filepath
* Sends the coverage data to the writer
*
* @desc Description for storage, null terminated string
* @filepath Name of the coverage file
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's the name of the file wouldn't fname be more appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The filepath passed to the function is a patch corresponding to the absolute path of the source file used at compilation. I will replace the "Name" by "Path".

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by absolute path? Is that the dirname() part of the absolute file name?

* @cov_data The coverage data to dump
* @cov_data_size Size of the coverage data
*/
static TEE_Result core_direct_dump_data(const char *desc, const char *filepath,
void *cov_data, uint32_t cov_data_size)
{
const char *core = "core/";
char new_filepath[COVERAGE_PATH_MAX];
uint32_t filepath_size = 0;

if (!desc || !filepath || !cov_data) {
EMSG("Wrong parameters");
Copy link
Contributor

Choose a reason for hiding this comment

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

DMSG(), if we need this at all.
Perhaps a plain assert(desc && fname && cov_data); instead of the if and everything would be more useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

return TEE_ERROR_BAD_PARAMETERS;
}

filepath_size = strlen(core) + strlen(desc) + strlen(filepath) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer something based on snprintf() for simplicity, perhaps like:

if (snprintf(buf, sizeof(buf), "core/%s/%s", desc, fname) >= sizeof(buf))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will use snprintf to simplify the code however not this exact line as I want to check I handle the NULL teminating character properly.

if (filepath_size >= COVERAGE_PATH_MAX) {
EMSG("Not enough space to store new filepath");
Copy link
Contributor

Choose a reason for hiding this comment

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

DMSG(), if we need this at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

return TEE_ERROR_SHORT_BUFFER;
}

filepath_size = 0;
strcpy(new_filepath + filepath_size, core);
filepath_size += strlen(core);

strcpy(new_filepath + filepath_size, desc);
filepath_size += strlen(desc);

strcpy(new_filepath + filepath_size, filepath);
filepath_size += strlen(filepath);

filepath_size++;

return gcov_dump_coverage_data(new_filepath, cov_data, cov_data_size);
}

gcov_dump_file_fn g_gcov_dump_fn = core_direct_dump_data;
141 changes: 141 additions & 0 deletions lib/libgcov/gcov.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
/* SPDX-License-Identifier: BSD-2-Clause */
Copy link
Contributor

Choose a reason for hiding this comment

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

// SPDX-...

/*
* Copyright 2020 NXP
*/

#include <string.h>
#include <trace.h>

#include <gcov.h>

#include "int_gcov.h"

static gcov_dump_writer s_writer_fn;

static bool isnullterm(const char *string, size_t maxlen, size_t *len)
{
size_t s_len = strnlen(string, maxlen);

if (s_len == maxlen)
return false;

if (len)
*len = s_len;

return true;
}

void __gcov_init(void *cov_unit_info)
{
if (!g_gcov_impl || !g_gcov_impl->init)
EMSG("No implementation");
else
g_gcov_impl->init(cov_unit_info);
}

void __gcov_merge_add(void *ctrs, uint32_t nb_ctrs)
{
if (!g_gcov_impl || !g_gcov_impl->merge_add)
EMSG("No implementation");
else
g_gcov_impl->merge_add(ctrs, nb_ctrs);
}

void __gcov_exit(void)
{
if (!g_gcov_impl || !g_gcov_impl->exit)
EMSG("No implementation");
else
g_gcov_impl->exit();
}

TEE_Result gcov_get_version(uint32_t *version)
{
/* We test if the implementation has implementeed the function */
if (!g_gcov_impl || !g_gcov_impl->get_version) {
EMSG("No implementation");
return TEE_ERROR_NOT_IMPLEMENTED;
}

if (version)
*version = g_gcov_impl->get_version();

return TEE_SUCCESS;
}

TEE_Result gcov_reset_coverage_data(void)
{
TEE_Result res = TEE_SUCCESS;

/* We test if the implementation has implementeed the function */
if (!g_gcov_impl || !g_gcov_impl->reset_all_coverage_data) {
EMSG("No implementation");
res = TEE_ERROR_NOT_IMPLEMENTED;
} else {
res = g_gcov_impl->reset_all_coverage_data();
}

return res;
}

TEE_Result register_gcov_dump_writer(gcov_dump_writer writer_fn)
{
s_writer_fn = writer_fn;

return TEE_SUCCESS;
}

TEE_Result gcov_dump_coverage_data(const char *filepath, char *cov_data,
uint32_t cov_data_size)
{
if (!filepath || !cov_data || !cov_data_size)
return TEE_ERROR_BAD_PARAMETERS;

if (!s_writer_fn) {
EMSG("Writer function not registered");
return TEE_ERROR_NOT_IMPLEMENTED;
}

if (!isnullterm(filepath, COVERAGE_PATH_MAX, NULL)) {
EMSG("filepath is not null terminated");
return TEE_ERROR_BAD_PARAMETERS;
}

return s_writer_fn(filepath, cov_data, cov_data_size);
}

TEE_Result gcov_dump_all_coverage_data(const char *desc)
{
TEE_Result res = TEE_SUCCESS;
const char *slash = "/";

if (!desc) {
desc = slash;
} else if (!isnullterm(desc, COVERAGE_PATH_MAX, NULL)) {
EMSG("description is not null terminated");
return TEE_ERROR_BAD_PARAMETERS;
}

/* We test if the implementation has implementeed the function */
if (!g_gcov_impl || !g_gcov_impl->dump_all_coverage_data ||
!g_gcov_dump_fn) {
EMSG("No implementation");
return TEE_ERROR_NOT_IMPLEMENTED;
}

if (g_gcov_impl->start_dump_all_coverage_data) {
res = g_gcov_impl->start_dump_all_coverage_data();
if (res) {
EMSG("Failed to start dump of coverage data");
return res;
}
}

res = g_gcov_impl->dump_all_coverage_data(g_gcov_dump_fn, desc);

if (g_gcov_impl->start_dump_all_coverage_data &&
g_gcov_impl->end_dump_all_coverage_data)
g_gcov_impl->end_dump_all_coverage_data();

return res;
}