diff options
| -rw-r--r-- | docs/getting_started/porting-guide.rst | 15 | ||||
| -rw-r--r-- | docs/process/coding-guidelines.rst | 515 | ||||
| -rw-r--r-- | docs/process/coding-style.rst | 468 | ||||
| -rw-r--r-- | docs/process/contributing.rst | 4 | ||||
| -rw-r--r-- | docs/process/index.rst | 1 | ||||
| -rw-r--r-- | docs/process/security-hardening.rst | 26 |
6 files changed, 698 insertions, 331 deletions
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,232 +1,126 @@ -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 +------------------------------ -.. 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 overrides --------------------- - -Some checkpatch warnings in the TF codebase are deliberately ignored. These -include: - -- ``**WARNING: line over 80 characters**``: Although the codebase should - generally conform to the 80 character limit this is overly restrictive in some - cases. - -- ``**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. - -Headers and inclusion ---------------------- - -Header guards -^^^^^^^^^^^^^ - -For a header file called "some_driver.h" the style used by the Trusted Firmware -is: - -.. code:: c - - #ifndef SOME_DRIVER_H - #define SOME_DRIVER_H - - <header content> - - #endif /* SOME_DRIVER_H */ +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. -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 - are *project* includes. - -#. *Platform* includes: Header files relating to a single, specific platform, - and which are located under the ``plat/<platform_name>`` 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. - -.. code:: c - - #include <string.h> - - #include <a_dir/example/a_header.h> - #include <a_dir/example/b_header.h> - #include <a_dir/test/a_header.h> - #include <b_dir/example/a_header.h> - - #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 +Several editors include built-in support for EditorConfig files, and many others +support its functionality through plugins. - #include <assert.h> - #include <errno.h> - #include <string.h> +Use of the EditorConfig file is suggested but is not required. - #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 +Automatic Compliance Checking +----------------------------- -Types and typedefs ------------------- +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. -Use of built-in *C* and *libc* data types -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +.. note:: + Checkpatch errors will gate upstream merging of pull requests. + Checkpatch warnings will not gate merging but should be reviewed and fixed if + possible. -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: +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: -- 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. +.. code:: shell -- 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`_ . + make CHECKPATCH=<path-to-linux>/linux/scripts/checkpatch.pl checkcodebase -- 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``. +To just check the style on the files that differ between your local branch and +the remote master, use: -- 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``. +.. code:: shell -- Use ``char`` for storing text. Use ``uint8_t`` for storing other 8-bit data. + make CHECKPATCH=<path-to-linux>/linux/scripts/checkpatch.pl checkpatch -- 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. +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``. -- For pointer types: +Ignored Checkpatch Warnings +^^^^^^^^^^^^^^^^^^^^^^^^^^^ - - 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 *``). +Some checkpatch warnings in the TF codebase are deliberately ignored. These +include: - - 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. +- ``**WARNING: line over 80 characters**``: Although the codebase should + generally conform to the 80 character limit this is overly restrictive in some + cases. - - 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. +- ``**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 legimate uses of the + volatile keyword within the TF codebase. - - Use ``ptrdiff_t`` to compare the difference between 2 pointers. +Performance considerations +-------------------------- -- Use ``size_t`` when storing the ``sizeof()`` something. +Avoid printf and use logging macros +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -- 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. +``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 ``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: +Each logging macro has a numerical log level: .. 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; + #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 -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`` -Avoid anonymous typedefs of structs/enums in headers -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Use const data where possible +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -For example, the following definition: +For example, the following code: .. 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: - -.. code:: c - - #include <my_struct.h> - 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". + void init(const struct my_struct *ptr); -The Linux coding standards also discourage new typedefs and checkpatch emits -a warning for this. + void main(void) + { + const struct my_struct x = { 1, 2 }; + init(&x); + } -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 ------------------------ - -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 -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Use of built-in *C* and *libc* data types +----------------------------------------- -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. +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: -Performance considerations --------------------------- +- 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 printf and use logging macros -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +- 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. -``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 ``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`_ . -Each logging macro has a numerical log level: +- 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``. -.. code:: c +- 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``. - #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 +- 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. -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: +- For pointer types: -``make PLAT=fvp LOG_LEVEL=50 all`` + - 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 *``). -Use const data where possible -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + - 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 example, the following code: + - 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. -.. code:: c + - 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. - struct my_struct { - int arg1; - int arg2; - }; + - Use ``ptrdiff_t`` to compare the difference between 2 pointers. - void init(struct my_struct *ptr); +- Use ``size_t`` when storing the ``sizeof()`` something. - void main(void) - { - struct my_struct x; - x.arg1 = 1; - x.arg2 = 2; - init(&x); - } +- 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. -is better written as: +- 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 - 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. + 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; -A driver (under ``drivers/`` and ``include/drivers/``) is defined as code that -interfaces with hardware via a memory mapped interface. +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``. -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. +These guidelines should be updated if additional types are needed. -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 + + <header content> + + #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/<platform_name>`` 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 <string.h> + + #include <a_dir/example/a_header.h> + #include <a_dir/example/b_header.h> + #include <a_dir/test/a_header.h> + #include <b_dir/example/a_header.h> + + #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 <assert.h> + #include <errno.h> + #include <string.h> + + #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 <my_struct.h> + 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.* |