From e63f5d129fadf520f42110d9a16c4192cba48784 Mon Sep 17 00:00:00 2001 From: Paul Beesley Date: Thu, 16 May 2019 13:33:18 +0100 Subject: [PATCH] doc: Split and expand coding style documentation This patch expands the coding style documentation, splitting it into two documents: the core style rules and extended guidelines. Note that it does not redefine or change the coding style (aside from section 4.6.2) - generally, it is only documenting the existing style in more detail. The aim is for the coding style to be more readable and, in turn, for it to be followed by more people. We can use this as a more concrete reference when discussing the accepted style with external contributors. Change-Id: I87405ace9a879d7f81e6b0b91b93ca69535e50ff Signed-off-by: Paul Beesley Signed-off-by: Petre-Ionut Tudor --- docs/getting_started/porting-guide.rst | 15 +- docs/process/coding-guidelines.rst | 543 ++++++++++--------------- docs/process/coding-style.rst | 468 +++++++++++++++++++++ docs/process/contributing.rst | 4 +- docs/process/index.rst | 1 + docs/process/security-hardening.rst | 26 +- 6 files changed, 712 insertions(+), 345 deletions(-) create mode 100644 docs/process/coding-style.rst diff --git a/docs/getting_started/porting-guide.rst b/docs/getting_started/porting-guide.rst index bb1471752..e8357b385 100644 --- a/docs/getting_started/porting-guide.rst +++ b/docs/getting_started/porting-guide.rst @@ -2763,6 +2763,19 @@ build system. to ``no``. If any of the options ``EL3_PAYLOAD_BASE`` or ``PRELOADED_BL33_BASE`` are used, this flag will be set to ``no`` automatically. +Platform include paths +---------------------- + +Platforms are allowed to add more include paths to be passed to the compiler. +The ``PLAT_INCLUDES`` variable is used for this purpose. This is needed in +particular for the file ``platform_def.h``. + +Example: + +.. code:: c + + PLAT_INCLUDES += -Iinclude/plat/myplat/include + C Library --------- @@ -2844,7 +2857,7 @@ amount of open resources per driver. -------------- -*Copyright (c) 2013-2019, Arm Limited and Contributors. All rights reserved.* +*Copyright (c) 2013-2020, Arm Limited and Contributors. All rights reserved.* .. _PSCI: http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf .. _Arm Generic Interrupt Controller version 2.0 (GICv2): http://infocenter.arm.com/help/topic/com.arm.doc.ihi0048b/index.html diff --git a/docs/process/coding-guidelines.rst b/docs/process/coding-guidelines.rst index cb8b89245..f7d53a97e 100644 --- a/docs/process/coding-guidelines.rst +++ b/docs/process/coding-guidelines.rst @@ -1,18 +1,61 @@ -Coding Style & Guidelines -========================= +Coding Guidelines +================= -The following sections contain TF coding guidelines. They are continually -evolving and should not be considered "set in stone". Feel free to question them -and provide feedback. +This document provides some additional guidelines to consider when writing +|TF-A| code. These are not intended to be strictly-enforced rules like the +contents of the :ref:`Coding Style`. -Some of the guidelines may also apply to other codebases. +Automatic Editor Configuration +------------------------------ + +Many of the rules given below (such as indentation size, use of tabs, and +newlines) can be set automatically using the `EditorConfig`_ configuration file +in the root of the repository: ``.editorconfig``. With a supported editor, the +rules set out in this file can be automatically applied when you are editing +files in the |TF-A| repository. + +Several editors include built-in support for EditorConfig files, and many others +support its functionality through plugins. + +Use of the EditorConfig file is suggested but is not required. + + +Automatic Compliance Checking +----------------------------- + +To assist with coding style compliance, the project Makefile contains two +targets which both utilise the `checkpatch.pl` script that ships with the Linux +source tree. The project also defines certain *checkpatch* options in the +``.checkpatch.conf`` file in the top-level directory. .. note:: - The existing TF codebase does not necessarily comply with all the - below guidelines but the intent is for it to do so eventually. + Checkpatch errors will gate upstream merging of pull requests. + Checkpatch warnings will not gate merging but should be reviewed and fixed if + possible. -Checkpatch overrides --------------------- +To check the entire source tree, you must first download copies of +``checkpatch.pl``, ``spelling.txt`` and ``const_structs.checkpatch`` available +in the `Linux master tree`_ *scripts* directory, then set the ``CHECKPATCH`` +environment variable to point to ``checkpatch.pl`` (with the other 2 files in +the same directory) and build the `checkcodebase` target: + +.. code:: shell + + make CHECKPATCH=/linux/scripts/checkpatch.pl checkcodebase + +To just check the style on the files that differ between your local branch and +the remote master, use: + +.. code:: shell + + make CHECKPATCH=/linux/scripts/checkpatch.pl checkpatch + +If you wish to check your patch against something other than the remote master, +set the ``BASE_COMMIT`` variable to your desired branch. By default, +``BASE_COMMIT`` is set to ``origin/master``. + +Ignored Checkpatch Warnings +^^^^^^^^^^^^^^^^^^^^^^^^^^^ Some checkpatch warnings in the TF codebase are deliberately ignored. These include: @@ -23,210 +66,61 @@ include: - ``**WARNING: Use of volatile is usually wrong``: see `Why the “volatile” type class should not be used`_ . Although this document - contains some very useful information, there are several legitimate uses of - the volatile keyword within the TF codebase. + contains some very useful information, there are several legimate uses of the + volatile keyword within the TF codebase. -Headers and inclusion ---------------------- +Performance considerations +-------------------------- -Header guards -^^^^^^^^^^^^^ +Avoid printf and use logging macros +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -For a header file called "some_driver.h" the style used by the Trusted Firmware -is: +``debug.h`` provides logging macros (for example, ``WARN`` and ``ERROR``) +which wrap ``tf_log`` and which allow the logging call to be compiled-out +depending on the ``make`` command. Use these macros to avoid print statements +being compiled unconditionally into the binary. + +Each logging macro has a numerical log level: .. code:: c - #ifndef SOME_DRIVER_H - #define SOME_DRIVER_H + #define LOG_LEVEL_NONE 0 + #define LOG_LEVEL_ERROR 10 + #define LOG_LEVEL_NOTICE 20 + #define LOG_LEVEL_WARNING 30 + #define LOG_LEVEL_INFO 40 + #define LOG_LEVEL_VERBOSE 50 -
+By default, all logging statements with a log level ``<= LOG_LEVEL_INFO`` will +be compiled into debug builds and all statements with a log level +``<= LOG_LEVEL_NOTICE`` will be compiled into release builds. This can be +overridden from the command line or by the platform makefile (although it may be +necessary to clean the build directory first). For example, to enable +``VERBOSE`` logging on FVP: - #endif /* SOME_DRIVER_H */ +``make PLAT=fvp LOG_LEVEL=50 all`` -Include statement ordering -^^^^^^^^^^^^^^^^^^^^^^^^^^ +Use const data where possible +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -All header files that are included by a source file must use the following, -grouped ordering. This is to improve readability (by making it easier to quickly -read through the list of headers) and maintainability. - -#. *System* includes: Header files from the standard *C* library, such as - ``stddef.h`` and ``string.h``. - -#. *Project* includes: Header files under the ``include/`` directory within TF - are *project* includes. - -#. *Platform* includes: Header files relating to a single, specific platform, - and which are located under the ``plat/`` directory within TF, - are *platform* includes. - -Within each group, ``#include`` statements must be in alphabetical order, -taking both the file and directory names into account. - -Groups must be separated by a single blank line for clarity. - -The example below illustrates the ordering rules using some contrived header -file names; this type of name reuse should be otherwise avoided. +For example, the following code: .. code:: c - #include - - #include - #include - #include - #include - - #include "./a_header.h" - -Include statement variants -^^^^^^^^^^^^^^^^^^^^^^^^^^ - -Two variants of the ``#include`` directive are acceptable in the TF codebase. -Correct use of the two styles improves readability by suggesting the location -of the included header and reducing ambiguity in cases where generic and -platform-specific headers share a name. - -For header files that are in the same directory as the source file that is -including them, use the ``"..."`` variant. - -For header files that are **not** in the same directory as the source file that -is including them, use the ``<...>`` variant. - -Example (bl1_fwu.c): - -.. code:: c - - #include - #include - #include - - #include "bl1_private.h" - -Platform include paths -^^^^^^^^^^^^^^^^^^^^^^ - -Platforms are allowed to add more include paths to be passed to the compiler. -The ``PLAT_INCLUDES`` variable is used for this purpose. This is needed in -particular for the file ``platform_def.h``. - -Example: - -.. code:: c - - PLAT_INCLUDES += -Iinclude/plat/myplat/include - -Types and typedefs ------------------- - -Use of built-in *C* and *libc* data types -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -The TF codebase should be kept as portable as possible, especially since both -64-bit and 32-bit platforms are supported. To help with this, the following data -type usage guidelines should be followed: - -- Where possible, use the built-in *C* data types for variable storage (for - example, ``char``, ``int``, ``long long``, etc) instead of the standard *C99* - types. Most code is typically only concerned with the minimum size of the - data stored, which the built-in *C* types guarantee. - -- Avoid using the exact-size standard *C99* types in general (for example, - ``uint16_t``, ``uint32_t``, ``uint64_t``, etc) since they can prevent the - compiler from making optimizations. There are legitimate uses for them, - for example to represent data of a known structure. When using them in struct - definitions, consider how padding in the struct will work across architectures. - For example, extra padding may be introduced in AArch32 systems if a struct - member crosses a 32-bit boundary. - -- Use ``int`` as the default integer type - it's likely to be the fastest on all - systems. Also this can be assumed to be 32-bit as a consequence of the - `Procedure Call Standard for the Arm Architecture`_ and the `Procedure Call - Standard for the Arm 64-bit Architecture`_ . - -- Avoid use of ``short`` as this may end up being slower than ``int`` in some - systems. If a variable must be exactly 16-bit, use ``int16_t`` or - ``uint16_t``. - -- Avoid use of ``long``. This is guaranteed to be at least 32-bit but, given - that `int` is 32-bit on Arm platforms, there is no use for it. For integers of - at least 64-bit, use ``long long``. - -- Use ``char`` for storing text. Use ``uint8_t`` for storing other 8-bit data. - -- Use ``unsigned`` for integers that can never be negative (counts, - indices, sizes, etc). TF intends to comply with MISRA "essential type" coding - rules (10.X), where signed and unsigned types are considered different - essential types. Choosing the correct type will aid this. MISRA static - analysers will pick up any implicit signed/unsigned conversions that may lead - to unexpected behaviour. - -- For pointer types: - - - If an argument in a function declaration is pointing to a known type then - simply use a pointer to that type (for example: ``struct my_struct *``). - - - If a variable (including an argument in a function declaration) is pointing - to a general, memory-mapped address, an array of pointers or another - structure that is likely to require pointer arithmetic then use - ``uintptr_t``. This will reduce the amount of casting required in the code. - Avoid using ``unsigned long`` or ``unsigned long long`` for this purpose; it - may work but is less portable. - - - For other pointer arguments in a function declaration, use ``void *``. This - includes pointers to types that are abstracted away from the known API and - pointers to arbitrary data. This allows the calling function to pass a - pointer argument to the function without any explicit casting (the cast to - ``void *`` is implicit). The function implementation can then do the - appropriate casting to a specific type. - - - Use ``ptrdiff_t`` to compare the difference between 2 pointers. - -- Use ``size_t`` when storing the ``sizeof()`` something. - -- Use ``ssize_t`` when returning the ``sizeof()`` something from a function that - can also return an error code; the signed type allows for a negative return - code in case of error. This practice should be used sparingly. - -- Use ``u_register_t`` when it's important to store the contents of a register - in its native size (32-bit in AArch32 and 64-bit in AArch64). This is not a - standard *C99* type but is widely available in libc implementations, - including the FreeBSD version included with the TF codebase. Where possible, - cast the variable to a more appropriate type before interpreting the data. For - example, the following struct in ``ep_info.h`` could use this type to minimize - the storage required for the set of registers: - -.. code:: c - - typedef struct aapcs64_params { - u_register_t arg0; - u_register_t arg1; - u_register_t arg2; - u_register_t arg3; - u_register_t arg4; - u_register_t arg5; - u_register_t arg6; - u_register_t arg7; - } aapcs64_params_t; - -If some code wants to operate on ``arg0`` and knows that it represents a 32-bit -unsigned integer on all systems, cast it to ``unsigned int``. - -These guidelines should be updated if additional types are needed. - -Avoid anonymous typedefs of structs/enums in headers -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -For example, the following definition: - -.. code:: c - - typedef struct { + struct my_struct { int arg1; int arg2; - } my_struct_t; + }; + void init(struct my_struct *ptr); + + void main(void) + { + struct my_struct x; + x.arg1 = 1; + x.arg2 = 2; + init(&x); + } is better written as: @@ -237,31 +131,18 @@ is better written as: int arg2; }; -This allows function declarations in other header files that depend on the -struct/enum to forward declare the struct/enum instead of including the -entire header: + void init(const struct my_struct *ptr); -.. code:: c + void main(void) + { + const struct my_struct x = { 1, 2 }; + init(&x); + } - #include - void my_func(my_struct_t *arg); - -instead of: - -.. code:: c - - struct my_struct; - void my_func(struct my_struct *arg); - -Some TF definitions use both a struct/enum name **and** a typedef name. This -is discouraged for new definitions as it makes it difficult for TF to comply -with MISRA rule 8.3, which states that "All declarations of an object or -function shall use the same names and type qualifiers". - -The Linux coding standards also discourage new typedefs and checkpatch emits -a warning for this. - -Existing typedefs will be retained for compatibility. +This allows the linker to put the data in a read-only data section instead of a +writeable data section, which may result in a smaller and faster binary. Note +that this may require dependent functions (``init()`` in the above example) to +have ``const`` arguments, assuming they don't need to modify the data. Libc functions that are banned or to be used with caution --------------------------------------------------------- @@ -410,14 +291,14 @@ error. This situation should be handled in one of the following ways: then emit an ``ERROR`` message and call the platform-specific function ``plat_error_handler()``. -Cases 1 and 2 are subtly different. A platform may implement ``plat_panic_handler`` -and ``plat_error_handler`` in the same way (for example, by waiting for a secure -watchdog to time-out or by invoking an interface on the platform's power -controller to reset the platform). However, ``plat_error_handler`` may take -additional action for some errors (for example, it may set a flag so the -platform resets into a different mode). Also, ``plat_panic_handler()`` may -implement additional debug functionality (for example, invoking a hardware -breakpoint). +Cases 1 and 2 are subtly different. A platform may implement +``plat_panic_handler`` and ``plat_error_handler`` in the same way (for example, +by waiting for a secure watchdog to time-out or by invoking an interface on the +platform's power controller to reset the platform). However, +``plat_error_handler`` may take additional action for some errors (for example, +it may set a flag so the platform resets into a different mode). Also, +``plat_panic_handler()`` may implement additional debug functionality (for +example, invoking a hardware breakpoint). Examples of unexpected unrecoverable errors: @@ -456,131 +337,115 @@ Examples: - Secure world is waiting for a hardware response that is critical for continued operation. -Security considerations ------------------------ +Use of built-in *C* and *libc* data types +----------------------------------------- -Part of the security of a platform is handling errors correctly, as described in -the previous section. There are several other security considerations covered in -this section. +The |TF-A| codebase should be kept as portable as possible, especially since +both 64-bit and 32-bit platforms are supported. To help with this, the following +data type usage guidelines should be followed: -Do not leak secrets to the normal world -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Where possible, use the built-in *C* data types for variable storage (for + example, ``char``, ``int``, ``long long``, etc) instead of the standard *C99* + types. Most code is typically only concerned with the minimum size of the + data stored, which the built-in *C* types guarantee. -The secure world **must not** leak secrets to the normal world, for example in -response to an SMC. +- Avoid using the exact-size standard *C99* types in general (for example, + ``uint16_t``, ``uint32_t``, ``uint64_t``, etc) since they can prevent the + compiler from making optimizations. There are legitimate uses for them, + for example to represent data of a known structure. When using them in struct + definitions, consider how padding in the struct will work across architectures. + For example, extra padding may be introduced in |AArch32| systems if a struct + member crosses a 32-bit boundary. -Handling Denial of Service attacks -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Use ``int`` as the default integer type - it's likely to be the fastest on all + systems. Also this can be assumed to be 32-bit as a consequence of the + `Procedure Call Standard for the Arm Architecture`_ and the `Procedure Call + Standard for the Arm 64-bit Architecture`_ . -The secure world **should never** crash or become unusable due to receiving too -many normal world requests (a *Denial of Service* or *DoS* attack). It should -have a mechanism for throttling or ignoring normal world requests. +- Avoid use of ``short`` as this may end up being slower than ``int`` in some + systems. If a variable must be exactly 16-bit, use ``int16_t`` or + ``uint16_t``. -Performance considerations --------------------------- +- Avoid use of ``long``. This is guaranteed to be at least 32-bit but, given + that `int` is 32-bit on Arm platforms, there is no use for it. For integers of + at least 64-bit, use ``long long``. -Avoid printf and use logging macros -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Use ``char`` for storing text. Use ``uint8_t`` for storing other 8-bit data. -``debug.h`` provides logging macros (for example, ``WARN`` and ``ERROR``) -which wrap ``tf_log`` and which allow the logging call to be compiled-out -depending on the ``make`` command. Use these macros to avoid print statements -being compiled unconditionally into the binary. +- Use ``unsigned`` for integers that can never be negative (counts, + indices, sizes, etc). TF intends to comply with MISRA "essential type" coding + rules (10.X), where signed and unsigned types are considered different + essential types. Choosing the correct type will aid this. MISRA static + analysers will pick up any implicit signed/unsigned conversions that may lead + to unexpected behaviour. -Each logging macro has a numerical log level: +- For pointer types: + + - If an argument in a function declaration is pointing to a known type then + simply use a pointer to that type (for example: ``struct my_struct *``). + + - If a variable (including an argument in a function declaration) is pointing + to a general, memory-mapped address, an array of pointers or another + structure that is likely to require pointer arithmetic then use + ``uintptr_t``. This will reduce the amount of casting required in the code. + Avoid using ``unsigned long`` or ``unsigned long long`` for this purpose; it + may work but is less portable. + + - For other pointer arguments in a function declaration, use ``void *``. This + includes pointers to types that are abstracted away from the known API and + pointers to arbitrary data. This allows the calling function to pass a + pointer argument to the function without any explicit casting (the cast to + ``void *`` is implicit). The function implementation can then do the + appropriate casting to a specific type. + + - Avoid pointer arithmetic generally (as this violates MISRA C 2012 rule + 18.4) and especially on void pointers (as this is only supported via + language extensions and is considered non-standard). In TF-A, setting the + ``W`` build flag to ``W=3`` enables the *-Wpointer-arith* compiler flag and + this will emit warnings where pointer arithmetic is used. + + - Use ``ptrdiff_t`` to compare the difference between 2 pointers. + +- Use ``size_t`` when storing the ``sizeof()`` something. + +- Use ``ssize_t`` when returning the ``sizeof()`` something from a function that + can also return an error code; the signed type allows for a negative return + code in case of error. This practice should be used sparingly. + +- Use ``u_register_t`` when it's important to store the contents of a register + in its native size (32-bit in |AArch32| and 64-bit in |AArch64|). This is not a + standard *C99* type but is widely available in libc implementations, + including the FreeBSD version included with the TF codebase. Where possible, + cast the variable to a more appropriate type before interpreting the data. For + example, the following struct in ``ep_info.h`` could use this type to minimize + the storage required for the set of registers: .. code:: c - #define LOG_LEVEL_NONE 0 - #define LOG_LEVEL_ERROR 10 - #define LOG_LEVEL_NOTICE 20 - #define LOG_LEVEL_WARNING 30 - #define LOG_LEVEL_INFO 40 - #define LOG_LEVEL_VERBOSE 50 + typedef struct aapcs64_params { + u_register_t arg0; + u_register_t arg1; + u_register_t arg2; + u_register_t arg3; + u_register_t arg4; + u_register_t arg5; + u_register_t arg6; + u_register_t arg7; + } aapcs64_params_t; +If some code wants to operate on ``arg0`` and knows that it represents a 32-bit +unsigned integer on all systems, cast it to ``unsigned int``. -By default, all logging statements with a log level ``<= LOG_LEVEL_INFO`` will -be compiled into debug builds and all statements with a log level -``<= LOG_LEVEL_NOTICE`` will be compiled into release builds. This can be -overridden from the command line or by the platform makefile (although it may be -necessary to clean the build directory first). For example, to enable -``VERBOSE`` logging on FVP: +These guidelines should be updated if additional types are needed. -``make PLAT=fvp LOG_LEVEL=50 all`` +-------------- -Use const data where possible -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -For example, the following code: - -.. code:: c - - struct my_struct { - int arg1; - int arg2; - }; - - void init(struct my_struct *ptr); - - void main(void) - { - struct my_struct x; - x.arg1 = 1; - x.arg2 = 2; - init(&x); - } - -is better written as: - -.. code:: c - - struct my_struct { - int arg1; - int arg2; - }; - - void init(const struct my_struct *ptr); - - void main(void) - { - const struct my_struct x = { 1, 2 }; - init(&x); - } - -This allows the linker to put the data in a read-only data section instead of a -writeable data section, which may result in a smaller and faster binary. Note -that this may require dependent functions (``init()`` in the above example) to -have ``const`` arguments, assuming they don't need to modify the data. - -Library and driver code ------------------------ - -TF library code (under ``lib/`` and ``include/lib``) is any code that provides a -reusable interface to other code, potentially even to code outside of TF. - -In some systems drivers must conform to a specific driver framework to provide -services to the rest of the system. TF has no driver framework and the -distinction between a driver and library is somewhat subjective. - -A driver (under ``drivers/`` and ``include/drivers/``) is defined as code that -interfaces with hardware via a memory mapped interface. - -Some drivers (for example, the Arm CCI driver in ``include/drivers/arm/cci.h``) -provide a general purpose API to that specific hardware. Other drivers (for -example, the Arm PL011 console driver in ``drivers/arm/pl011/pl011_console.S``) -provide a specific hardware implementation of a more abstract library API. In -the latter case there may potentially be multiple drivers for the same hardware -device. - -Neither libraries nor drivers should depend on platform-specific code. If they -require platform-specific data (for example, a base address) to operate then -they should provide an initialization function that takes the platform-specific -data as arguments. - -TF common code (under ``common/`` and ``include/common/``) is code that is re-used -by other generic (non-platform-specific) TF code. It is effectively internal -library code. +*Copyright (c) 2020, Arm Limited and Contributors. All rights reserved.* +.. _`Linux master tree`: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/ +.. _`Procedure Call Standard for the Arm Architecture`: https://developer.arm.com/docs/ihi0042/latest/ +.. _`Procedure Call Standard for the Arm 64-bit Architecture`: https://developer.arm.com/docs/ihi0055/latest/ +.. _`EditorConfig`: http://editorconfig.org/ .. _`Why the “volatile” type class should not be used`: https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html -.. _`Procedure Call Standard for the Arm Architecture`: http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf -.. _`Procedure Call Standard for the Arm 64-bit Architecture`: http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf +.. _`MISRA C:2012 Guidelines`: https://www.misra.org.uk/Activities/MISRAC/tabid/160/Default.aspx +.. _`a spreadsheet`: https://developer.trustedfirmware.org/file/download/lamajxif3w7c4mpjeoo5/PHID-FILE-fp7c7acszn6vliqomyhn/MISRA-and-TF-Analysis-v1.3.ods diff --git a/docs/process/coding-style.rst b/docs/process/coding-style.rst new file mode 100644 index 000000000..fd1984d30 --- /dev/null +++ b/docs/process/coding-style.rst @@ -0,0 +1,468 @@ +Coding Style +============ + +The following sections outline the |TF-A| coding style for *C* code. The style +is based on the `Linux kernel coding style`_, with a few modifications. + +The style should not be considered *set in stone*. Feel free to provide feedback +and suggestions. + +.. note:: + You will almost certainly find code in the |TF-A| repository that does not + follow the style. The intent is for all code to do so eventually. + +File Encoding +------------- + +The source code must use the **UTF-8** character encoding. Comments and +documentation may use non-ASCII characters when required (e.g. Greek letters +used for units) but code itself is still limited to ASCII characters. + +Newlines must be in **Unix** style, which means that only the Line Feed (``LF``) +character is used to break a line and reset to the first column. + +Language +-------- + +The primary language for comments and naming must be International English. In +cases where there is a conflict between the American English and British English +spellings of a word, the American English spelling is used. + +Exceptions are made when referring directly to something that does not use +international style, such as the name of a company. In these cases the existing +name should be used as-is. + +C Language Standard +------------------- + +The C language mode used for TF-A is *GNU99*. This is the "GNU dialect of ISO +C99", which implies the *ISO C99* standard with GNU extensions. + +Both GCC and Clang compiler toolchains have support for *GNU99* mode, though +Clang does lack support for a small number of GNU extensions. These +missing extensions are rarely used, however, and should not pose a problem. + +MISRA Compliance +---------------- + +TF-A attempts to comply with the `MISRA C:2012 Guidelines`_. Coverity +Static Analysis is used to regularly generate a report of current MISRA defects +and to prevent the addition of new ones. + +It is not possible for the project to follow all MISRA guidelines. We maintain +`a spreadsheet`_ that lists all rules and directives and whether we aim to +comply with them or not. A rationale is given for each deviation. + +.. note:: + Enforcing a rule does not mean that the codebase is free of defects + of that rule, only that they would ideally be removed. + +.. note:: + Third-party libraries are not considered in our MISRA analysis and we do not + intend to modify them to make them MISRA compliant. + +Indentation +----------- + +Use **tabs** for indentation. The use of spaces for indentation is forbidden +except in the case where a term is being indented to a boundary that cannot be +achieved using tabs alone. + +Tab spacing should be set to **8 characters**. + +Trailing whitespace is not allowed and must be trimmed. + +Spacing +------- + +Single spacing should be used around most operators, including: + +- Arithmetic operators (``+``, ``-``, ``/``, ``*``) +- Assignment operators (``=``, ``+=``, etc) +- Boolean operators (``&&``, ``||``) +- Comparison operators (``<``, ``>``, ``==``, etc) + +A space should also be used to separate parentheses and braces when they are not +already separated by a newline, such as for the ``if`` statement in the +following example: + +.. code:: c + + int function_foo(bool bar) + { + if (bar) { + function_baz(); + } + } + +Note that there is no space between the name of a function and the following +parentheses. + +Control statements (``if``, ``for``, ``switch``, ``while``, etc) must be +separated from the following open paranthesis by a single space. The previous +example illustrates this for an ``if`` statement. + +Line Length +----------- + +Line length *should* be at most **80 characters**. This limit does not include +non-printing characters such as the line feed. + +This rule is a *should*, not a must, and it is acceptable to exceed the limit +**slightly** where the readability of the code would otherwise be significantly +reduced. Use your judgement in these cases. + +Blank Lines +----------- + +Functions are usually separated by a single blank line. In certain cases it is +acceptable to use additional blank lines for clarity, if required. + +The file must end with a single newline character. Many editors have the option +to insert this automatically and to trim multiple blank lines at the end of the +file. + +Braces +------ + +Opening Brace Placement +^^^^^^^^^^^^^^^^^^^^^^^ + +Braces follow the **Kernighan and Ritchie (K&R)** style, where the opening brace +is **not** placed on a new line. + +Example for a ``while`` loop: + +.. code:: c + + while (condition) { + foo(); + bar(); + } + +This style applies to all blocks except for functions which, following the Linux +style, **do** place the opening brace on a new line. + +Example for a function: + +.. code:: c + + int my_function(void) + { + int a; + + a = 1; + return a; + } + +Conditional Statement Bodies +^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Where conditional statements (such as ``if``, ``for``, ``while`` and ``do``) are +used, braces must be placed around the statements that form the body of the +conditional. This is the case regardless of the number of statements in the +body. + +.. note:: + This is a notable departure from the Linux coding style that has been + adopted to follow MISRA guidelines more closely and to help prevent errors. + +For example, use the following style: + +.. code:: c + + if (condition) { + foo++; + } + +instead of omitting the optional braces around a single statement: + +.. code:: c + + /* This is violating MISRA C 2012: Rule 15.6 */ + if (condition) + foo++; + +The reason for this is to prevent accidental changes to control flow when +modifying the body of the conditional. For example, at a quick glance it is easy +to think that the value of ``bar`` is only incremented if ``condition`` +evaluates to ``true`` but this is not the case - ``bar`` will always be +incremented regardless of the condition evaluation. If the developer forgets to +add braces around the conditional body when adding the ``bar++;`` statement then +the program execution will not proceed as intended. + +.. code:: c + + /* This is violating MISRA C 2012: Rule 15.6 */ + if (condition) + foo++; + bar++; + +Naming +------ + +Functions +^^^^^^^^^ + +Use lowercase for function names, separating multiple words with an underscore +character (``_``). This is sometimes referred to as *Snake Case*. An example is +given below: + +.. code:: c + + void bl2_arch_setup(void) + { + ... + } + +Local Variables and Parameters +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Local variables and function parameters use the same format as function names: +lowercase with underscore separation between multiple words. An example is +given below: + +.. code:: c + + static void set_scr_el3_from_rm(uint32_t type, + uint32_t interrupt_type_flags, + uint32_t security_state) + { + uint32_t flag, bit_pos; + + ... + + } + +Preprocessor Macros +^^^^^^^^^^^^^^^^^^^ + +Identifiers that are defined using preprocessor macros are written in all +uppercase text. + +.. code:: c + + #define BUFFER_SIZE_BYTES 64 + +Function Attributes +------------------- + +Place any function attributes after the function type and before the function +name. + +.. code:: c + + void __init plat_arm_interconnect_init(void); + +Alignment +--------- + +Alignment should be performed primarily with tabs, adding spaces if required to +achieve a granularity that is smaller than the tab size. For example, with a tab +size of eight columns it would be necessary to use one tab character and two +spaces to indent text by ten columns. + +Switch Statement Alignment +^^^^^^^^^^^^^^^^^^^^^^^^^^ + +When using ``switch`` statements, align each ``case`` statement with the +``switch`` so that they are in the same column. + +.. code:: c + + switch (condition) { + case A: + foo(); + case B: + bar(); + default: + baz(); + } + +Pointer Alignment +^^^^^^^^^^^^^^^^^ + +The reference and dereference operators (ampersand and *pointer star*) must be +aligned with the name of the object on which they are operating, as opposed to +the type of the object. + +.. code:: c + + uint8_t *foo; + + foo = &bar; + + +Comments +-------- + +The general rule for comments is that the double-slash style of comment (``//``) +is not allowed. Examples of the allowed comment formats are shown below: + +.. code:: c + + /* + * This example illustrates the first allowed style for multi-line comments. + * + * Blank lines within multi-lines are allowed when they add clarity or when + * they separate multiple contexts. + * + */ + +.. code:: c + + /************************************************************************** + * This is the second allowed style for multi-line comments. + * + * In this style, the first and last lines use asterisks that run the full + * width of the comment at its widest point. + * + * This style can be used for additional emphasis. + * + *************************************************************************/ + +.. code:: c + + /* Single line comments can use this format */ + +.. code:: c + + /*************************************************************************** + * This alternative single-line comment style can also be used for emphasis. + **************************************************************************/ + +Headers and inclusion +--------------------- + +Header guards +^^^^^^^^^^^^^ + +For a header file called "some_driver.h" the style used by |TF-A| is: + +.. code:: c + + #ifndef SOME_DRIVER_H + #define SOME_DRIVER_H + +
+ + #endif /* SOME_DRIVER_H */ + +Include statement ordering +^^^^^^^^^^^^^^^^^^^^^^^^^^ + +All header files that are included by a source file must use the following, +grouped ordering. This is to improve readability (by making it easier to quickly +read through the list of headers) and maintainability. + +#. *System* includes: Header files from the standard *C* library, such as + ``stddef.h`` and ``string.h``. + +#. *Project* includes: Header files under the ``include/`` directory within + |TF-A| are *project* includes. + +#. *Platform* includes: Header files relating to a single, specific platform, + and which are located under the ``plat/`` directory within + |TF-A|, are *platform* includes. + +Within each group, ``#include`` statements must be in alphabetical order, +taking both the file and directory names into account. + +Groups must be separated by a single blank line for clarity. + +The example below illustrates the ordering rules using some contrived header +file names; this type of name reuse should be otherwise avoided. + +.. code:: c + + #include + + #include + #include + #include + #include + + #include "a_header.h" + +Include statement variants +^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Two variants of the ``#include`` directive are acceptable in the |TF-A| +codebase. Correct use of the two styles improves readability by suggesting the +location of the included header and reducing ambiguity in cases where generic +and platform-specific headers share a name. + +For header files that are in the same directory as the source file that is +including them, use the ``"..."`` variant. + +For header files that are **not** in the same directory as the source file that +is including them, use the ``<...>`` variant. + +Example (bl1_fwu.c): + +.. code:: c + + #include + #include + #include + + #include "bl1_private.h" + +Typedefs +-------- + +Avoid anonymous typedefs of structs/enums in headers +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +For example, the following definition: + +.. code:: c + + typedef struct { + int arg1; + int arg2; + } my_struct_t; + + +is better written as: + +.. code:: c + + struct my_struct { + int arg1; + int arg2; + }; + +This allows function declarations in other header files that depend on the +struct/enum to forward declare the struct/enum instead of including the +entire header: + +.. code:: c + + struct my_struct; + void my_func(struct my_struct *arg); + +instead of: + +.. code:: c + + #include + void my_func(my_struct_t *arg); + +Some TF definitions use both a struct/enum name **and** a typedef name. This +is discouraged for new definitions as it makes it difficult for TF to comply +with MISRA rule 8.3, which states that "All declarations of an object or +function shall use the same names and type qualifiers". + +The Linux coding standards also discourage new typedefs and checkpatch emits +a warning for this. + +Existing typedefs will be retained for compatibility. + +-------------- + +*Copyright (c) 2020, Arm Limited. All rights reserved.* + +.. _`Linux kernel coding style`: https://www.kernel.org/doc/html/latest/process/coding-style.html +.. _`MISRA C:2012 Guidelines`: https://www.misra.org.uk/Activities/MISRAC/tabid/160/Default.aspx +.. _`a spreadsheet`: https://developer.trustedfirmware.org/file/download/lamajxif3w7c4mpjeoo5/PHID-FILE-fp7c7acszn6vliqomyhn/MISRA-and-TF-Analysis-v1.3.ods diff --git a/docs/process/contributing.rst b/docs/process/contributing.rst index f569fcbe7..68c494baa 100644 --- a/docs/process/contributing.rst +++ b/docs/process/contributing.rst @@ -23,7 +23,7 @@ Making Changes - Make commits of logical units. See these general `Git guidelines`_ for contributing to a project. -- Follow the :ref:`Coding Style & Guidelines`. +- Follow the :ref:`Coding Style` and :ref:`Coding Guidelines`. - Use the checkpatch.pl script provided with the Linux source tree. A Makefile target is provided for convenience. @@ -128,7 +128,7 @@ Binary Components -------------- -*Copyright (c) 2013-2019, Arm Limited and Contributors. All rights reserved.* +*Copyright (c) 2013-2020, Arm Limited and Contributors. All rights reserved.* .. _developer.trustedfirmware.org: https://developer.trustedfirmware.org .. _issue: https://developer.trustedfirmware.org/project/board/1/ diff --git a/docs/process/index.rst b/docs/process/index.rst index 9c12de82f..1cb535405 100644 --- a/docs/process/index.rst +++ b/docs/process/index.rst @@ -8,6 +8,7 @@ Processes & Policies security platform-compatibility-policy + coding-style coding-guidelines contributing faq diff --git a/docs/process/security-hardening.rst b/docs/process/security-hardening.rst index a18a79203..43a572125 100644 --- a/docs/process/security-hardening.rst +++ b/docs/process/security-hardening.rst @@ -1,10 +1,30 @@ -Security hardening -================== +Secure Development Guidelines +============================= This page contains guidance on what to check for additional security measures, including build options that can be modified to improve security or catch issues early in development. +Security considerations +----------------------- + +Part of the security of a platform is handling errors correctly, as described in +the previous section. There are several other security considerations covered in +this section. + +Do not leak secrets to the normal world +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The secure world **must not** leak secrets to the normal world, for example in +response to an SMC. + +Handling Denial of Service attacks +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The secure world **should never** crash or become unusable due to receiving too +many normal world requests (a *Denial of Service* or *DoS* attack). It should +have a mechanism for throttling or ignoring normal world requests. + Build options ------------- @@ -53,4 +73,4 @@ Several build options can be used to check for security issues. Refer to the -------------- -*Copyright (c) 2019, Arm Limited. All rights reserved.* +*Copyright (c) 2019-2020, Arm Limited. All rights reserved.*