Skip to content

Latest commit

 

History

History
235 lines (185 loc) · 8.01 KB

code_review_guide.rst

File metadata and controls

235 lines (185 loc) · 8.01 KB
Raw

Code Review Guideline

The purpose of this document is to clarify design items to be reviewed during the code review process.

Please contact :doc:`maintainers </contributing/maintainers>` or write an e-mail thread on the TF-M mailing list for any questions.

List of the guidelines

The prerequisites before going to the review stage:

The review guidelines consist of these items:

Note

Each guideline item is assigned with a unique symbol in the format Rx.y, while x and y are numeric symbols. These symbols are created for easy referencing during the code review process.

Exceptions

Files under listed folders are fully or partially imported from 3rd party projects, these files follow the original project defined guidelines and styles:

Important

  • R1.1 ext and its subfolders.

Non-source items

For all non-source files such as build system files or configuration files:

Important

  • R2.1 Follow the existing style while making changes.

Namespace

TF-M assign the namespace to files and symbols for an easier code reading. The symbol here includes functions, variables, types and MACROs. The prerequisites:

Important

  • R3.1 Follow the documents or specifications if they propose namespaces ('psa_' for PSA defined items E.g.,). Ask the contributor to tell the documents or specifications if the reviewer is not sure about a namespace.
  • R3.2 Assign TF-M specific namespace 'tfm_' or 'TFM_' for symbols implementing TF-M specific features. Ask the contributor to clarify the purpose of the patch if the reviewer is not sure about the namespace.

For the sources out of the above prerequisites (R3.1 and R3.2):

Important

  • R3.3 Do not assign a namespace for source files.
  • R3.4 Assigning a namespace for interface symbols is recommended. The namespace needs to be expressed in one word to be followed by other words into a phrase can represent the functionalities implemented. One word naming is allowed if the namespace can represent the functionality perfectly.
  • R3.5 Assign a namespace for private symbols is NOT recommended.
  • R3.6 Do not assign characters out of alphabets as the leading character.

Examples:

/* R3.1 FILE: s/spm/core/psa_client.c */

/* R3.2 FILE: s/spm/core/tfm_secure_context.c */

/* R3.4 FILE: s/spm/core/spm.c, 'spm\_' as the namespace */
void spm_init(void);

/* R3.5 FILE: s/spm/core/main.c */
static void init_functions(void);

/* R3.6 Not permitted: */
/* static uint32_t __count; */

Assembler code

Important

  • R4.1 Pure assembler sources or inline assembler code are required to be put under the platform-independent or architecture-independent folders. The logic folders should not contain any assembler code, referring to external MACRO wrapped assembler code is allowed. Here is one example of the logic folder:
    • 'secure_fw/spm'.

Examples:

/*
 * R4.1 The following MACRO is allowed to be referenced under
 * 'secure_fw/spm'
 */
#define SVC(code) __asm volatile("svc  %0", ::"I"(code))

Include

This chapter describes the placement of the headers and including. There are two types of headers: The interface headers contain symbols to be shared between modules and the private headers contain symbols only for internal usage.

Important

  • R5.1 Put the interface header of one module in the include folder under the root of this module. Deeper sub-folders can not have include folders, which means only one include is allowed for one module.
  • R5.2 Creating sub-folders under include to represent the more granular scope of the interfaces is allowed.
  • R5.3 private header can be put at the same place with the implementation sources for the private symbols contained in the header. It also can be put at the place where the sources need it. The latter is useful when some "private header" contains abstracted interfaces, but these interfaces are not public interfaces so it won't be put under "include" folder.
  • R5.4 Use <> when including public headers.
  • R5.5 Use "" when including private headers.
  • R5.6 The module's include folder needs to be added into referencing module's header searching path.
  • R5.7 The module's include folder and the root folder needs to be added into its own header searching path and apply a hierarchy including with folder name.
  • R5.8 Path hierarchy including is allowed since there are sub-folders under include folder and the module folder.
  • R5.9 The including statement group order: the beginning group contains toolchain headers, then follows the public headers group and finally the private headers group.
  • R5.10 The including statement order inside a group: Compare the include statement as strings and sort by the string comparison result.
  • R5.11 The header for the referenced symbol or definition must be included even this header is included inside one of the existing included headers. This improves portability in case the existing header implementation changed.

Examples:

/*
 * The structure:
 *   module_a/include/func1.h
 *   module_a/include/func2/fmain.h
 *   module_a/func1.c
 *   module_a/func2/fmain.c
 *   module_b/include/funcx.h
 *   module_b/include/funcy/fmain.h
 *   module_b/funcx.c
 *   module_b/funcxi.h
 *   module_b/funcy/fmain.c
 *   module_b/funcy/fsub.c
 *   module_b/funcy/fsub.h
 * Here takes module_b/funcx.c as example:
 */
#include <func1.h>
#include <func2/fmain.h>
#include <funcx.h>
#include "funcxi.h"
#include "funcy/fsub.h"

Auto-doc

Auto document system such as doxygen is useful for describing interfaces. While it would be a development burden since the details are described in the design documents already. The guidelines for auto-doc:

Important

  • R6.1 Headers and sources under these folders need to apply auto-doc style comments: *include.
  • R6.2 Developers decide the comment style for sources out of listed folders.

Commit Message

TF-M has the requirements on commit message:

Important

  • R7.1 Assign correct topic for a patch. Check the following table.
Topic Justification
Boot bl2/* or bl1/*
Build For build system related purpose.
Docs All *.rst changes.
Dualcpu Dual-cpu related changes.
HAL Generic HAL interface/implementation changes.
Interface Interface changes, either Non-source and secure.
Pack For packing purpose.
Platform Generic platform related changes under platform/*.
Platform Name Specific platform changes.
Partition Multiple partition related changes.
Partition Name Specific partition related changes.
Service Multiple service related changes.
Service Name Specific service related changes.
SPM secure_fw/spm/*
SPRTL secure-fw/partitions/lib/runtime/*
Tool tools folder or tf-m-tools repo

Note

Ideally, one topic should cover one specific type of changes. For crossing topic changes, check the main part of the change and use the main part related topic as patch topic. If there is no suitable topics to cover the change, contact the community for an update.

Copyright (c) 2020-2022, Arm Limited. All rights reserved.