diff options
Diffstat (limited to '')
| -rw-r--r-- | docs/process/code-review-guidelines.rst | 216 | ||||
| -rw-r--r-- | docs/process/coding-guidelines.rst | 474 | ||||
| -rw-r--r-- | docs/process/coding-style.rst | 470 | ||||
| -rw-r--r-- | docs/process/commit-style.rst | 153 | ||||
| -rw-r--r-- | docs/process/contributing.rst | 304 | ||||
| -rw-r--r-- | docs/process/faq.rst | 80 | ||||
| -rw-r--r-- | docs/process/index.rst | 16 | ||||
| -rw-r--r-- | docs/process/platform-ports-policy.rst | 51 | ||||
| -rw-r--r-- | docs/process/security-hardening.rst | 175 | ||||
| -rw-r--r-- | docs/process/security.rst | 89 |
10 files changed, 2028 insertions, 0 deletions
diff --git a/docs/process/code-review-guidelines.rst b/docs/process/code-review-guidelines.rst new file mode 100644 index 0000000..67a211f --- /dev/null +++ b/docs/process/code-review-guidelines.rst @@ -0,0 +1,216 @@ +Code Review Guidelines +====================== + +This document provides TF-A specific details about the project's code review +process. It should be read in conjunction with the `Project Maintenance +Process`_, which it supplements. + + +Why do we do code reviews? +-------------------------- + +The main goal of code reviews is to improve the code quality. By reviewing each +other's code, we can help catch issues that were missed by the author +before they are integrated in the source tree. Different people bring different +perspectives, depending on their past work, experiences and their current use +cases of TF-A in their products. + +Code reviews also play a key role in sharing knowledge within the +community. People with more expertise in one area of the code base can +help those that are less familiar with it. + +Code reviews are meant to benefit everyone through team work. It is not about +unfairly criticizing or belittling the work of any contributor. + + +Good practices +-------------- + +To ensure the code review gives the greatest possible benefit, participants in +the project should: + +- Be considerate of other people and their needs. Participants may be working + to different timescales, and have different priorities. Keep this in + mind - be gracious while waiting for action from others, and timely in your + actions when others are waiting for you. + +- Review other people's patches where possible. The more active reviewers there + are, the more quickly new patches can be reviewed and merged. Contributing to + code review helps everyone in the long run, as it creates a culture of + participation which serves everyone's interests. + + +Guidelines for patch contributors +--------------------------------- + +In addition to the rules outlined in the :ref:`Contributor's Guide`, as a patch +contributor you are expected to: + +- Answer all comments from people who took the time to review your + patches. + +- Be patient and resilient. It is quite common for patches to go through + several rounds of reviews and rework before they get approved, especially + for larger features. + + In the event that a code review takes longer than you would hope for, you + may try the following actions to speed it up: + + - Ping the reviewers on Gerrit or on the mailing list. If it is urgent, + explain why. Please remain courteous and do not abuse this. + + - If one code owner has become unresponsive, ask the other code owners for + help progressing the patch. + + - If there is only one code owner and they have become unresponsive, ask one + of the project maintainers for help. + +- Do the right thing for the project, not the fastest thing to get code merged. + + For example, if some existing piece of code - say a driver - does not quite + meet your exact needs, go the extra mile and extend the code with the missing + functionality you require - as opposed to copying the code into some other + directory to have the freedom to change it in any way. This way, your changes + benefit everyone and will be maintained over time. + + +Guidelines for all reviewers +---------------------------- + +There are no good or bad review comments. If you have any doubt about a patch or +need some clarifications, it's better to ask rather than letting a potential +issue slip. Examples of review comments could be: + +- Questions ("Why do you need to do this?", "What if X happens?") +- Bugs ("I think you need a logical \|\| rather than a bitwise \|.") +- Design issues ("This won't scale well when we introduce feature X.") +- Improvements ("Would it be better if we did Y instead?") + + +Guidelines for code owners +-------------------------- + +Code owners are listed on the :ref:`Project Maintenance<code owners>` page, +along with the module(s) they look after. + +When reviewing a patch, code owners are expected to check the following: + +- The patch looks good from a technical point of view. For example: + + - The structure of the code is clear. + + - It complies with the relevant standards or technical documentation (where + applicable). + + - It leverages existing interfaces rather than introducing new ones + unnecessarily. + + - It fits well in the design of the module. + + - It adheres to the security model of the project. In particular, it does not + increase the attack surface (e.g. new SMCs) without justification. + +- The patch adheres to the TF-A :ref:`Coding Style`. The CI system should help + catch coding style violations. + +- (Only applicable to generic code) The code is MISRA-compliant (see + :ref:`misra-compliance`). The CI system should help catch violations. + +- Documentation is provided/updated (where applicable). + +- The patch has had an appropriate level of testing. Testing details are + expected to be provided by the patch author. If they are not, do not hesitate + to request this information. + +- All CI automated tests pass. + +If a code owner is happy with a patch, they should give their approval +through the ``Code-Owner-Review+1`` label in Gerrit. If instead, they have +concerns, questions, or any other type of blocking comment, they should set +``Code-Owner-Review-1``. + +Code owners are expected to behave professionally and responsibly. Here are some +guidelines for them: + +- Once you are engaged in a review, make sure you stay involved until the patch + is merged. Rejecting a patch and going away is not very helpful. You are + expected to monitor the patch author's answers to your review comments, + answer back if needed and review new revisions of their patch. + +- Provide constructive feedback. Just saying, "This is wrong, you should do X + instead." is usually not very helpful. The patch author is unlikely to + understand why you are requesting this change and might feel personally + attacked. + +- Be mindful when reviewing a patch. As a code owner, you are viewed as + the expert for the relevant module. By approving a patch, you are partially + responsible for its quality and the effects it has for all TF-A users. Make + sure you fully understand what the implications of a patch might be. + + +Guidelines for maintainers +-------------------------- + +Maintainers are listed on the :ref:`Project Maintenance<maintainers>` page. + +When reviewing a patch, maintainers are expected to check the following: + +- The general structure of the patch looks good. This covers things like: + + - Code organization. + + - Files and directories, names and locations. + + For example, platform code should be added under the ``plat/`` directory. + + - Naming conventions. + + For example, platform identifiers should be properly namespaced to avoid + name clashes with generic code. + + - API design. + +- Interaction of the patch with other modules in the code base. + +- The patch aims at complying with any standard or technical documentation + that applies. + +- New files must have the correct license and copyright headers. See :ref:`this + paragraph<copyright-license-guidance>` for more information. The CI system + should help catch files with incorrect or no copyright/license headers. + +- There is no third party code or binary blobs with potential IP concerns. + Maintainers should look for copyright or license notices in code, and use + their best judgement. If they are unsure about a patch, they should ask + other maintainers for help. + +- Generally speaking, new driver code should be placed in the generic + layer. There are cases where a driver has to stay into the platform layer but + this should be the exception, rather than the rule. + +- Existing common drivers (in particular for Arm IPs like the GIC driver) should + not be copied into the platform layer to cater for platform quirks. This + type of code duplication hurts the maintainability of the project. The + duplicate driver is less likely to benefit from bug fixes and future + enhancements. In most cases, it is possible to rework a generic driver to + make it more flexible and fit slightly different use cases. That way, these + enhancements benefit everyone. + +- When a platform specific driver really is required, the burden lies with the + patch author to prove the need for it. A detailed justification should be + posted via the commit message or on the mailing list. + +- Before merging a patch, verify that all review comments have been addressed. + If this is not the case, encourage the patch author and the relevant + reviewers to resolve these together. + +If a maintainer is happy with a patch, they should give their approval +through the ``Maintainer-Review+1`` label in Gerrit. If instead, they have +concerns, questions, or any other type of blocking comment, they should set +``Maintainer-Review-1``. + +-------------- + +*Copyright (c) 2020, Arm Limited. All rights reserved.* + +.. _Project Maintenance Process: https://developer.trustedfirmware.org/w/collaboration/project-maintenance-process/ diff --git a/docs/process/coding-guidelines.rst b/docs/process/coding-guidelines.rst new file mode 100644 index 0000000..26c272d --- /dev/null +++ b/docs/process/coding-guidelines.rst @@ -0,0 +1,474 @@ +Coding Guidelines +================= + +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`. + +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: + +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:: + Checkpatch errors will gate upstream merging of pull requests. + Checkpatch warnings will not gate merging but should be reviewed and fixed if + possible. + +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=<path-to-linux>/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=<path-to-linux>/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: + +- ``**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 legimate uses of the + volatile keyword within the TF codebase. + +Performance considerations +-------------------------- + +Avoid printf and use logging macros +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +``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 + + #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: + +.. code:: shell + + 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. + +Libc functions that are banned or to be used with caution +--------------------------------------------------------- + +Below is a list of functions that present security risks and either must not be +used (Banned) or are discouraged from use and must be used with care (Caution). + ++------------------------+-----------+--------------------------------------+ +| libc function | Status | Comments | ++========================+===========+======================================+ +| ``strcpy, wcscpy``, | Banned | use strlcpy instead | +| ``strncpy`` | | | ++------------------------+-----------+--------------------------------------+ +| ``strcat, wcscat``, | Banned | use strlcat instead | +| ``strncat`` | | | ++------------------------+-----------+--------------------------------------+ +| ``sprintf, vsprintf`` | Banned | use snprintf, vsnprintf | +| | | instead | ++------------------------+-----------+--------------------------------------+ +| ``snprintf`` | Caution | ensure result fits in buffer | +| | | i.e : snprintf(buf,size...) < size | ++------------------------+-----------+--------------------------------------+ +| ``vsnprintf`` | Caution | inspect va_list match types | +| | | specified in format string | ++------------------------+-----------+--------------------------------------+ +| ``strtok`` | Banned | use strtok_r or strsep instead | ++------------------------+-----------+--------------------------------------+ +| ``strtok_r, strsep`` | Caution | inspect for terminated input buffer | ++------------------------+-----------+--------------------------------------+ +| ``ato*`` | Banned | use equivalent strto* functions | ++------------------------+-----------+--------------------------------------+ +| ``*toa`` | Banned | Use snprintf instead | ++------------------------+-----------+--------------------------------------+ + +The `libc` component in the codebase will not add support for the banned APIs. + +Error handling and robustness +----------------------------- + +Using CASSERT to check for compile time data errors +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Where possible, use the ``CASSERT`` macro to check the validity of data known at +compile time instead of checking validity at runtime, to avoid unnecessary +runtime code. + +For example, this can be used to check that the assembler's and compiler's views +of the size of an array is the same. + +.. code:: c + + #include <cassert.h> + + define MY_STRUCT_SIZE 8 /* Used by assembler source files */ + + struct my_struct { + uint32_t arg1; + uint32_t arg2; + }; + + CASSERT(MY_STRUCT_SIZE == sizeof(struct my_struct), assert_my_struct_size_mismatch); + + +If ``MY_STRUCT_SIZE`` in the above example were wrong then the compiler would +emit an error like this: + +:: + + my_struct.h:10:1: error: size of array ‘assert_my_struct_size_mismatch’ is negative + + +Using assert() to check for programming errors +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +In general, each secure world TF image (BL1, BL2, BL31 and BL32) should be +treated as a tightly integrated package; the image builder should be aware of +and responsible for all functionality within the image, even if code within that +image is provided by multiple entities. This allows us to be more aggressive in +interpreting invalid state or bad function arguments as programming errors using +``assert()``, including arguments passed across platform porting interfaces. +This is in contrast to code in a Linux environment, which is less tightly +integrated and may attempt to be more defensive by passing the error back up the +call stack. + +Where possible, badly written TF code should fail early using ``assert()``. This +helps reduce the amount of untested conditional code. By default these +statements are not compiled into release builds, although this can be overridden +using the ``ENABLE_ASSERTIONS`` build flag. + +Examples: + +- Bad argument supplied to library function +- Bad argument provided by platform porting function +- Internal secure world image state is inconsistent + + +Handling integration errors +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Each secure world image may be provided by a different entity (for example, a +Trusted Boot vendor may provide the BL2 image, a TEE vendor may provide the BL32 +image and the OEM/SoC vendor may provide the other images). + +An image may contain bugs that are only visible when the images are integrated. +The system integrator may not even have access to the debug variants of all the +images in order to check if asserts are firing. For example, the release variant +of BL1 may have already been burnt into the SoC. Therefore, TF code that detects +an integration error should _not_ consider this a programming error, and should +always take action, even in release builds. + +If an integration error is considered non-critical it should be treated as a +recoverable error. If the error is considered critical it should be treated as +an unexpected unrecoverable error. + +Handling recoverable errors +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The secure world **must not** crash when supplied with bad data from an external +source. For example, data from the normal world or a hardware device. Similarly, +the secure world **must not** crash if it detects a non-critical problem within +itself or the system. It must make every effort to recover from the problem by +emitting a ``WARN`` message, performing any necessary error handling and +continuing. + +Examples: + +- Secure world receives SMC from normal world with bad arguments. +- Secure world receives SMC from normal world at an unexpected time. +- BL31 receives SMC from BL32 with bad arguments. +- BL31 receives SMC from BL32 at unexpected time. +- Secure world receives recoverable error from hardware device. Retrying the + operation may help here. +- Non-critical secure world service is not functioning correctly. +- BL31 SPD discovers minor configuration problem with corresponding SP. + +Handling unrecoverable errors +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +In some cases it may not be possible for the secure world to recover from an +error. This situation should be handled in one of the following ways: + +1. If the unrecoverable error is unexpected then emit an ``ERROR`` message and + call ``panic()``. This will end up calling the platform-specific function + ``plat_panic_handler()``. +2. If the unrecoverable error is expected to occur in certain circumstances, + 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). + +Examples of unexpected unrecoverable errors: + +- BL32 receives an unexpected SMC response from BL31 that it is unable to + recover from. +- BL31 Trusted OS SPD code discovers that BL2 has not loaded the corresponding + Trusted OS, which is critical for platform operation. +- Secure world discovers that a critical hardware device is an unexpected and + unrecoverable state. +- Secure world receives an unexpected and unrecoverable error from a critical + hardware device. +- Secure world discovers that it is running on unsupported hardware. + +Examples of expected unrecoverable errors: + +- BL1/BL2 fails to load the next image due to missing/corrupt firmware on disk. +- BL1/BL2 fails to authenticate the next image due to an invalid certificate. +- Secure world continuously receives recoverable errors from a hardware device + but is unable to proceed without a valid response. + +Handling critical unresponsiveness +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +If the secure world is waiting for a response from an external source (for +example, the normal world or a hardware device) which is critical for continued +operation, it must not wait indefinitely. It must have a mechanism (for example, +a secure watchdog) for resetting itself and/or the external source to prevent +the system from executing in this state indefinitely. + +Examples: + +- BL1 is waiting for the normal world to raise an SMC to proceed to the next + stage of the secure firmware update process. +- A Trusted OS is waiting for a response from a proxy in the normal world that + is critical for continued operation. +- Secure world is waiting for a hardware response that is critical for continued + operation. + +Use of built-in *C* and *libc* data types +----------------------------------------- + +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: + +- 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. + + - 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 + + 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. + +Favor C language over assembly language +--------------------------------------- + +Generally, prefer code written in C over assembly. Assembly code is less +portable, harder to understand, maintain and audit security wise. Also, static +analysis tools generally don't analyze assembly code. + +There are, however, legitimate uses of assembly language. These include: + + - Early boot code executed before the C runtime environment is setup. + + - Exception handling code. + + - Low-level code where the exact sequence of instructions executed on the CPU + matters, such as CPU reset sequences. + + - Low-level code where specific system-level instructions must be used, such + as cache maintenance operations. + +-------------- + +*Copyright (c) 2020, 2022, 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://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst +.. _`Procedure Call Standard for the Arm 64-bit Architecture`: https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst +.. _`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 +.. _`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 0000000..be13b14 --- /dev/null +++ b/docs/process/coding-style.rst @@ -0,0 +1,470 @@ +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: + +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 parenthesis 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/commit-style.rst b/docs/process/commit-style.rst new file mode 100644 index 0000000..d7e937b --- /dev/null +++ b/docs/process/commit-style.rst @@ -0,0 +1,153 @@ +Commit Style +============ + +When writing commit messages, please think carefully about the purpose and scope +of the change you are making: describe briefly what the change does, and +describe in detail why it does it. This helps to ensure that changes to the +code-base are transparent and approachable to reviewers, and it allows us to +keep a more accurate changelog. You may use Markdown in commit messages. + +A good commit message provides all the background information needed for +reviewers to understand the intent and rationale of the patch. This information +is also useful for future reference. + +For example: + +- What does the patch do? +- What motivated it? +- What impact does it have? +- How was it tested? +- Have alternatives been considered? Why did you choose this approach over + another one? +- If it fixes an `issue`_, include a reference. + +|TF-A| follows the `Conventional Commits`_ specification. All commits to the +main repository are expected to adhere to these guidelines, so it is +**strongly** recommended that you read at least the `quick summary`_ of the +specification. + +To briefly summarize, commit messages are expected to be of the form: + +.. code:: + + <type>[optional scope]: <description> + + [optional body] + + [optional footer(s)] + +The following example commit message demonstrates the use of the +``refactor`` type and the ``amu`` scope: + +.. code:: + + refactor(amu): factor out register accesses + + This change introduces a small set of register getters and setters to + avoid having to repeatedly mask and shift in complex code. + + Change-Id: Ia372f60c5efb924cd6eeceb75112e635ad13d942 + Signed-off-by: Chris Kay <chris.kay@arm.com> + +The following `types` are permissible and are strictly enforced: + ++--------------+---------------------------------------------------------------+ +| Scope | Description | ++==============+===============================================================+ +| ``feat`` | A new feature | ++--------------+---------------------------------------------------------------+ +| ``fix`` | A bug fix | ++--------------+---------------------------------------------------------------+ +| ``build`` | Changes that affect the build system or external dependencies | ++--------------+---------------------------------------------------------------+ +| ``ci`` | Changes to our CI configuration files and scripts | ++--------------+---------------------------------------------------------------+ +| ``docs`` | Documentation-only changes | ++--------------+---------------------------------------------------------------+ +| ``perf`` | A code change that improves performance | ++--------------+---------------------------------------------------------------+ +| ``refactor`` | A code change that neither fixes a bug nor adds a feature | ++--------------+---------------------------------------------------------------+ +| ``revert`` | Changes that revert a previous change | ++--------------+---------------------------------------------------------------+ +| ``style`` | Changes that do not affect the meaning of the code | +| | (white-space, formatting, missing semi-colons, etc.) | ++--------------+---------------------------------------------------------------+ +| ``test`` | Adding missing tests or correcting existing tests | ++--------------+---------------------------------------------------------------+ +| ``chore`` | Any other change | ++--------------+---------------------------------------------------------------+ + +The permissible `scopes` are more flexible, and we maintain a list of them in +our :download:`changelog configuration file <../../changelog.yaml>`. Scopes in +this file are organized by their changelog section, where each changelog section +has a single scope that is considered to be blessed, and possibly several +deprecated scopes. Please avoid using deprecated scopes. + +While we don't enforce scopes strictly, we do ask that commits use these if they +can, or add their own if no appropriate one exists (see :ref:`Adding Scopes`). + +It's highly recommended that you use the tooling installed by the optional steps +in the :ref:`prerequisites <Prerequisites>` guide to validate commit messages +locally, as commitlint reports a live list of the acceptable scopes. + +.. _Adding Scopes: + +Adding Scopes +------------- + +Scopes that are not present in the changelog configuration file are considered +to be deprecated, and should be avoided. If you are adding a new component that +does not yet have a designated scope, please add one. + +For example, if you are adding or making modifications to `Foo`'s latest and +greatest new platform `Bar` then you would add it to the `Platforms` changelog +sub-section, and the hierarchy should look something like this: + +.. code:: yaml + + - title: Platforms + + subsections: + - title: Foo + scope: foo + + subsections: + - title: Bar + scope: bar + +When creating new scopes, try to keep them short and succinct, and use kebab +case (``this-is-kebab-case``). Components with a product name (i.e. most +platforms and some drivers) should use that name (e.g. ``gic600ae``, +``flexspi``, ``stpmic1``), otherwise use a name that uniquely represents the +component (e.g. ``marvell-comphy-3700``, ``rcar3-drivers``, ``a3720-uart``). + +Mandated Trailers +----------------- + +Commits are expected to be signed off with the ``Signed-off-by:`` trailer using +your real name and email address. You can do this automatically by committing +with Git's ``-s`` flag. By adding this line the contributor certifies the +contribution is made under the terms of the :download:`Developer Certificate of +Origin <../../dco.txt>`. + +There may be multiple ``Signed-off-by:`` lines depending on the history of the +patch, but one **must** be the committer. More details may be found in the +`Gerrit Signed-off-by Lines guidelines`_. + +Ensure that each commit also has a unique ``Change-Id:`` line. If you have +followed optional steps in the prerequisites to either install the Node.js tools +or clone the repository using the "`Clone with commit-msg hook`" clone method, +then this should be done automatically for you. + +More details may be found in the `Gerrit Change-Ids documentation`_. + +-------------- + +*Copyright (c) 2021, Arm Limited and Contributors. All rights reserved.* + +.. _Conventional Commits: https://www.conventionalcommits.org/en/v1.0.0 +.. _Gerrit Change-Ids documentation: https://review.trustedfirmware.org/Documentation/user-changeid.html +.. _Gerrit Signed-off-by Lines guidelines: https://review.trustedfirmware.org/Documentation/user-signedoffby.html +.. _issue: https://developer.trustedfirmware.org/project/board/1/ +.. _quick summary: https://www.conventionalcommits.org/en/v1.0.0/#summary diff --git a/docs/process/contributing.rst b/docs/process/contributing.rst new file mode 100644 index 0000000..ef9ebd3 --- /dev/null +++ b/docs/process/contributing.rst @@ -0,0 +1,304 @@ +Contributor's Guide +******************* + +Getting Started +=============== + +- Make sure you have a Github account and you are logged on both + `developer.trustedfirmware.org`_ and `review.trustedfirmware.org`_. + +- If you plan to contribute a major piece of work, it is usually a good idea to + start a discussion around it on the mailing list. This gives everyone + visibility of what is coming up, you might learn that somebody else is + already working on something similar or the community might be able to + provide some early input to help shaping the design of the feature. + + If you intend to include Third Party IP in your contribution, please mention + it explicitly in the email thread and ensure that the changes that include + Third Party IP are made in a separate patch (or patch series). + +- Clone `Trusted Firmware-A`_ on your own machine as described in + :ref:`prerequisites_get_source`. + +- Create a local topic branch based on the `Trusted Firmware-A`_ ``master`` + branch. + +Making Changes +============== + +- Ensure commits adhere to the the project's :ref:`Commit Style`. + +- Make commits of logical units. See these general `Git guidelines`_ for + contributing to a project. + +- Keep the commits on topic. If you need to fix another bug or make another + enhancement, please address it on a separate topic branch. + +- Split the patch in manageable units. Small patches are usually easier to + review so this will speed up the review process. + +- Avoid long commit series. If you do have a long series, consider whether + some commits should be squashed together or addressed in a separate topic. + +- 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, see :ref:`this + section<automatic-compliance-checking>` for more details. + +- Where appropriate, please update the documentation. + + - Consider whether the :ref:`Porting Guide`, :ref:`Firmware Design` document + or other in-source documentation needs updating. + + - If you are submitting new files that you intend to be the code owner for + (for example, a new platform port), then also update the + :ref:`code owners` file. + + - For topics with multiple commits, you should make all documentation changes + (and nothing else) in the last commit of the series. Otherwise, include + the documentation changes within the single commit. + +.. _copyright-license-guidance: + +- Ensure that each changed file has the correct copyright and license + information. Files that entirely consist of contributions to this project + should have a copyright notice and BSD-3-Clause SPDX license identifier of + the form as shown in :ref:`license`. Files that contain changes to imported + Third Party IP files should retain their original copyright and license + notices. + + For significant contributions you may add your own copyright notice in the + following format: + + :: + + Portions copyright (c) [XXXX-]YYYY, <OWNER>. All rights reserved. + + where XXXX is the year of first contribution (if different to YYYY) and YYYY + is the year of most recent contribution. <OWNER> is your name or your company + name. + +- Ensure that each patch in the patch series compiles in all supported + configurations. Patches which do not compile will not be merged. + +- Please test your changes. As a minimum, ensure that Linux boots on the + Foundation FVP. See :ref:`Arm Fixed Virtual Platforms (FVP)` for more + information. For more extensive testing, consider running the `TF-A Tests`_ + against your patches. + +- Ensure that all CI automated tests pass. Failures should be fixed. They might + block a patch, depending on how critical they are. + +Submitting Changes +================== + +- Submit your changes for review at https://review.trustedfirmware.org + targeting the ``integration`` branch. + +- Add reviewers for your patch: + + - At least one code owner for each module modified by the patch. See the list + of modules and their :ref:`code owners`. + + - At least one maintainer. See the list of :ref:`maintainers`. + + - If some module has no code owner, try to identify a suitable (non-code + owner) reviewer. Running ``git blame`` on the module's source code can + help, as it shows who has been working the most recently on this area of + the code. + + Alternatively, if it is impractical to identify such a reviewer, you might + send an email to the `TF-A mailing list`_ to broadcast your review request + to the community. + + Note that self-reviewing a patch is prohibited, even if the patch author is + the only code owner of a module modified by the patch. Getting a second pair + of eyes on the code is essential to keep up with the quality standards the + project aspires to. + +- The changes will then undergo further review by the designated people. Any + review comments will be made directly on your patch. This may require you to + do some rework. For controversial changes, the discussion might be moved to + the `TF-A mailing list`_ to involve more of the community. + + Refer to the `Gerrit Uploading Changes documentation`_ for more details. + +- The patch submission rules are the following. For a patch to be approved + and merged in the tree, it must get: + + - One ``Code-Owner-Review+1`` for each of the modules modified by the patch. + - A ``Maintainer-Review+1``. + + In the case where a code owner could not be found for a given module, + ``Code-Owner-Review+1`` is substituted by ``Code-Review+1``. + + In addition to these various code review labels, the patch must also get a + ``Verified+1``. This is usually set by the Continuous Integration (CI) bot + when all automated tests passed on the patch. Sometimes, some of these + automated tests may fail for reasons unrelated to the patch. In this case, + the maintainers might (after analysis of the failures) override the CI bot + score to certify that the patch has been correctly tested. + + In the event where the CI system lacks proper tests for a patch, the patch + author or a reviewer might agree to perform additional manual tests + in their review and the reviewer incorporates the review of the additional + testing in the ``Code-Review+1`` or ``Code-Owner-Review+1`` as applicable to + attest that the patch works as expected. Where possible additional tests should + be added to the CI system as a follow up task. For example, for a + platform-dependent patch where the said platform is not available in the CI + system's board farm. + +- When the changes are accepted, the :ref:`maintainers` will integrate them. + + - Typically, the :ref:`maintainers` will merge the changes into the + ``integration`` branch. + + - If the changes are not based on a sufficiently-recent commit, or if they + cannot be automatically rebased, then the :ref:`maintainers` may rebase it + on the ``integration`` branch or ask you to do so. + + - After final integration testing, the changes will make their way into the + ``master`` branch. If a problem is found during integration, the + :ref:`maintainers` will request your help to solve the issue. They may + revert your patches and ask you to resubmit a reworked version of them or + they may ask you to provide a fix-up patch. + +Add CI Configurations +===================== + +- TF-A uses Jenkins tool for Continuous Integration and testing activities. + Various CI Jobs are deployed which run tests on every patch before being + merged. So each of your patches go through a series of checks before they + get merged on to the master branch. Kindly ensure, that everytime you add + new files under your platform, they are covered under the following two sections: + +Coverity Scan +------------- + +- ``Coverity Scan analysis`` is one of the tests we perform on our source code + at regular intervals. We maintain a build script ``tf-cov-make`` which contains the + build configurations of various platforms in order to cover the entire source + code being analysed by Coverity. + +- When you submit your patches for review containing new source files, please + ensure to include them for the ``Coverity Scan analysis`` by adding the + respective build configurations in the ``tf-cov-make`` build script. + +- In this section you find the details on how to append your new build + configurations for Coverity scan analysis illustrated with examples: + +#. We maintain a separate repository named `tf-a-ci-scripts repository`_ + for placing all the test scripts which will be executed by the CI Jobs. + +#. In this repository, ``tf-cov-make`` script is located at + ``tf-a-ci-scripts/script/tf-coverity/tf-cov-make`` + +#. Edit `tf-cov-make`_ script by appending all the possible build configurations with + the specific ``build-flags`` relevant to your platform, so that newly added + source files get built and analysed by Coverity. + +#. For better understanding follow the below specified examples listed in the + ``tf-cov-make`` script. + +.. code:: shell + + Example 1: + #Intel + make PLAT=stratix10 $(common_flags) all + make PLAT=agilex $(common_flags) all + +- In the above example there are two different SoCs ``stratix`` and ``agilex`` + under the Intel platform and the build configurations has been added suitably + to include most of their source files. + +.. code:: shell + + Example 2: + #Hikey + make PLAT=hikey $(common_flags) ${TBB_OPTIONS} ENABLE_PMF=1 all + make PLAT=hikey960 $(common_flags) ${TBB_OPTIONS} all + make PLAT=poplar $(common_flags) all + +- In this case for ``Hikey`` boards additional ``build-flags`` has been included + along with the ``commom_flags`` to cover most of the files relevant to it. + +- Similar to this you can still find many other different build configurations + of various other platforms listed in the ``tf-cov-make`` script. Kindly refer + them and append your build configurations respectively. + +Test Build Configuration (``tf-l1-build-plat``) +----------------------------------------------- + +- Coverity Scan analysis, runs on a daily basis and will not be triggered for + every individual trusted-firmware patch. + +- Considering this, we have other distinguished CI jobs which run a set of test + configurations on every patch, before they are being passed to ``Coverity scan analysis``. + +- ``tf-l1-build-plat`` is the test group, which holds the test configurations + to build all the platforms. So be kind enough to verify that your newly added + files are built as part of one of the existing platform configurations present + in ``tf-l1-build-plat`` test group. + +- In this section you find the details on how to add the appropriate files, + needed to build your newly introduced platform as part of ``tf-l1-build-plat`` + test group, illustrated with an example: + +- Lets consider ``Hikey`` platform: + In the `tf-a-ci-scripts repository`_ we need to add a build configuration file ``hikey-default`` + under tf_config folder, ``tf_config/hikey-default`` listing all the build parameters + relevant to it. + +.. code:: shell + + #Hikey Build Parameters + CROSS_COMPILE=aarch64-none-elf- + PLAT=hikey + +- Further a test-configuration file ``hikey-default:nil`` need to be added under the + test group, ``tf-l1-build-plat`` located at ``tf-a-ci-scripts/group/tf-l1-build-plat``, + to allow the platform to be built as part of this group. + +.. code:: shell + + # + # Copyright (c) 2019-2022 Arm Limited. All rights reserved. + # + # SPDX-License-Identifier: BSD-3-Clause + # + +- As illustrated above, you need to add the similar files supporting your platform. + +Binary Components +================= + +- Platforms may depend on binary components submitted to the `Trusted Firmware + binary repository`_ if they require code that the contributor is unable or + unwilling to open-source. This should be used as a rare exception. +- All binary components must follow the contribution guidelines (in particular + licensing rules) outlined in the `readme.rst <tf-binaries-readme_>`_ file of + the binary repository. +- Binary components must be restricted to only the specific functionality that + cannot be open-sourced and must be linked into a larger open-source platform + port. The majority of the platform port must still be implemented in open + source. Platform ports that are merely a thin wrapper around a binary + component that contains all the actual code will not be accepted. +- Only platform port code (i.e. in the ``plat/<vendor>`` directory) may rely on + binary components. Generic code must always be fully open-source. + +-------------- + +*Copyright (c) 2013-2022, Arm Limited and Contributors. All rights reserved.* + +.. _developer.trustedfirmware.org: https://developer.trustedfirmware.org +.. _review.trustedfirmware.org: https://review.trustedfirmware.org +.. _Trusted Firmware-A: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git +.. _Git guidelines: http://git-scm.com/book/ch5-2.html +.. _Gerrit Uploading Changes documentation: https://review.trustedfirmware.org/Documentation/user-upload.html +.. _TF-A Tests: https://trustedfirmware-a-tests.readthedocs.io +.. _Trusted Firmware binary repository: https://review.trustedfirmware.org/admin/repos/tf-binaries +.. _tf-binaries-readme: https://git.trustedfirmware.org/tf-binaries.git/tree/readme.rst +.. _TF-A mailing list: https://lists.trustedfirmware.org/mailman3/lists/tf-a.lists.trustedfirmware.org/ +.. _tf-a-ci-scripts repository: https://git.trustedfirmware.org/ci/tf-a-ci-scripts.git/ +.. _tf-cov-make: https://git.trustedfirmware.org/ci/tf-a-ci-scripts.git/tree/script/tf-coverity/tf-cov-make diff --git a/docs/process/faq.rst b/docs/process/faq.rst new file mode 100644 index 0000000..daab198 --- /dev/null +++ b/docs/process/faq.rst @@ -0,0 +1,80 @@ +Frequently-Asked Questions (FAQ) +================================ + +How do I update my changes? +--------------------------- + +Often it is necessary to update your patch set before it is merged. Refer to the +`Gerrit Upload Patch Set documentation`_ on how to do so. + +If you need to modify an existing patch set with multiple commits, refer to the +`Gerrit Replace Changes documentation`_. + +How long will my changes take to merge into ``integration``? +------------------------------------------------------------ + +This can vary a lot, depending on: + +* How important the patch set is considered by the TF maintainers. Where + possible, you should indicate the required timescales for merging the patch + set and the impact of any delay. Feel free to add a comment to your patch set + to get an estimate of when it will be merged. + +* The quality of the patch set. Patches are likely to be merged more quickly if + they follow the coding guidelines, have already had some code review, and have + been appropriately tested. + +* The impact of the patch set. For example, a patch that changes a key generic + API is likely to receive much greater scrutiny than a local change to a + specific platform port. + +* How much opportunity for external review is required. For example, the TF + maintainers may not wait for external review comments to merge trivial + bug-fixes but may wait up to a week to merge major changes, or ones requiring + feedback from specific parties. + +* How many other patch sets are waiting to be integrated and the risk of + conflict between the topics. + +* If there is a code freeze in place in preparation for the release. Please + refer the :ref:`Release Processes` document for more details. + +* The workload of the TF maintainers. + +How long will it take for my changes to go from ``integration`` to ``master``? +------------------------------------------------------------------------------ + +This depends on how many concurrent patches are being processed at the same +time. In simple cases where all potential regressions have already been tested, +the delay will be less than 1 day. If the TF maintainers are trying to merge +several things over the course of a few days, it might take up to a week. +Typically, it will be 1-2 days. + +The worst case is if the TF maintainers are trying to make a release while also +receiving patches that will not be merged into the release. In this case, the +patches will be merged onto ``integration``, which will temporarily diverge from +the release branch. The ``integration`` branch will be rebased onto ``master`` +after the release, and then ``master`` will be fast-forwarded to ``integration`` +1-2 days later. This whole process could take up 4 weeks. Please refer to the +:ref:`Release Processes` document for code freeze dates. The TF maintainers +will inform the patch owner if this is going to happen. + +It is OK to create a patch based on commits that are only available in +``integration`` or another patch set, rather than ``master``. There is a risk +that the dependency commits will change (for example due to patch set rework or +integration problems). If this happens, the dependent patch will need reworking. + +What are these strange comments in my changes? +---------------------------------------------- + +All the comments from ``ci-bot-user`` are associated with Continuous Integration +infrastructure. The links published on the comment are not currently accessible, +but would be after the CI has been transitioned to `trustedfirmware.org`_. + +-------------- + +*Copyright (c) 2019-2020, Arm Limited. All rights reserved.* + +.. _Gerrit Upload Patch Set documentation: https://review.trustedfirmware.org/Documentation/intro-user.html#upload-patch-set +.. _Gerrit Replace Changes documentation: https://review.trustedfirmware.org/Documentation/user-upload.html#push_replace +.. _trustedfirmware.org: https://www.trustedfirmware.org/ diff --git a/docs/process/index.rst b/docs/process/index.rst new file mode 100644 index 0000000..7914a4e --- /dev/null +++ b/docs/process/index.rst @@ -0,0 +1,16 @@ +Processes & Policies +==================== + +.. toctree:: + :maxdepth: 1 + :caption: Contents + + security + platform-ports-policy + commit-style + coding-style + coding-guidelines + contributing + code-review-guidelines + faq + security-hardening diff --git a/docs/process/platform-ports-policy.rst b/docs/process/platform-ports-policy.rst new file mode 100644 index 0000000..7983749 --- /dev/null +++ b/docs/process/platform-ports-policy.rst @@ -0,0 +1,51 @@ +Platform Ports Policy +===================== + +This document clarifies a couple of policy points around platform ports +management. + +Platform compatibility policy +----------------------------- + +Platform compatibility is mainly affected by changes to Platform APIs (as +documented in the :ref:`Porting Guide`), driver APIs (like the GICv3 drivers) or +library interfaces (like xlat_table library). The project will try to maintain +compatibility for upstream platforms. Due to evolving requirements and +enhancements, there might be changes affecting platform compatibility which +means the previous interface needs to be deprecated and a new interface +introduced to replace it. In case the migration to the new interface is trivial, +the contributor of the change is expected to make good effort to migrate the +upstream platforms to the new interface. + +The deprecated interfaces are listed inside :ref:`Release Processes` as well as +the release after which each one will be removed. When an interface is +deprecated, the page must be updated to indicate the release after which the +interface will be removed. This must be at least 1 full release cycle in future. +For non-trivial interface changes, an email should be sent out to the `TF-A +public mailing list`_ to notify platforms that they should migrate away from the +deprecated interfaces. Platforms are expected to migrate before the removal of +the deprecated interface. + +Platform deprecation policy +--------------------------- + +If a platform is no longer maintained, it is best to deprecate it to keep the +projects' source tree clean and healthy. Deprecation can be a 1-stage or 2-stage +process (up to the platform maintainers). + + - *2-stage*: The platform's source code can be kept in the repository for a + cooling off period before deleting it (typically 2 release cycles). In this + case, we keep track ot the *Deprecated* version separately from the *Deleted* + version. + + - *1-stage*: The platform's source code can be deleted straight away. In this + case, both versions are the same. + +The :ref:`Platform Ports` page provides a list of all deprecated/deleted +platform ports (or soon to be) to this day. + +-------------- + +*Copyright (c) 2018-2022, Arm Limited and Contributors. All rights reserved.* + +.. _TF-A public mailing list: https://lists.trustedfirmware.org/mailman3/lists/tf-a.lists.trustedfirmware.org/ diff --git a/docs/process/security-hardening.rst b/docs/process/security-hardening.rst new file mode 100644 index 0000000..507046f --- /dev/null +++ b/docs/process/security-hardening.rst @@ -0,0 +1,175 @@ +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. + +Preventing Secure-world timing information leakage via PMU counters +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The Secure world needs to implement some defenses to prevent the Non-secure +world from making it leak timing information. In general, higher privilege +levels must defend from those below when the PMU is treated as an attack +vector. + +Refer to the :ref:`Performance Monitoring Unit` guide for detailed information +on the PMU registers. + +Timing leakage attacks from the Non-secure world +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Since the Non-secure world has access to the ``PMCR`` register, it can +configure the PMU to increment counters at any exception level and in both +Secure and Non-secure state. Thus, it attempts to leak timing information from +the Secure world. + +Shown below is an example of such a configuration: + +- ``PMEVTYPER0_EL0`` and ``PMCCFILTR_EL0``: + + - Set ``P`` to ``0``. + - Set ``NSK`` to ``1``. + - Set ``M`` to ``0``. + - Set ``NSH`` to ``0``. + - Set ``SH`` to ``1``. + +- ``PMCNTENSET_EL0``: + + - Set ``P[0]`` to ``1``. + - Set ``C`` to ``1``. + +- ``PMCR_EL0``: + + - Set ``DP`` to ``0``. + - Set ``E`` to ``1``. + +This configuration instructs ``PMEVCNTR0_EL0`` and ``PMCCNTR_EL0`` to increment +at Secure EL1, Secure EL2 (if implemented) and EL3. + +Since the Non-secure world has fine-grained control over where (at which +exception levels) it instructs counters to increment, obtaining event counts +would allow it to carry out side-channel timing attacks against the Secure +world. Examples include Spectre, Meltdown, as well as extracting secrets from +cryptographic algorithms with data-dependent variations in their execution +time. + +Secure world mitigation strategies +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ``MDCR_EL3`` register allows EL3 to configure the PMU (among other things). +The `Arm ARM`_ details all of the bit fields in this register, but for the PMU +there are two bits which determine the permissions of the counters: + +- ``SPME`` for the programmable counters. +- ``SCCD`` for the cycle counter. + +Depending on the implemented features, the Secure world can prohibit counting +in AArch64 state via the following: + +- ARMv8.2-Debug not implemented: + + - Prohibit general event counters and the cycle counter: + ``MDCR_EL3.SPME == 0 && PMCR_EL0.DP == 1 && !ExternalSecureNoninvasiveDebugEnabled()``. + + - ``MDCR_EL3.SPME`` resets to ``0``, so by default general events should + not be counted in the Secure world. + - The ``PMCR_EL0.DP`` bit therefore needs to be set to ``1`` when EL3 is + entered and ``PMCR_EL0`` needs to be saved and restored in EL3. + - ``ExternalSecureNoninvasiveDebugEnabled()`` is an authentication + interface which is implementation-defined unless ARMv8.4-Debug is + implemented. The `Arm ARM`_ has detailed information on this topic. + + - The only other way is to disable the ``PMCR_EL0.E`` bit upon entering + EL3, which disables counting altogether. + +- ARMv8.2-Debug implemented: + + - Prohibit general event counters: ``MDCR_EL3.SPME == 0``. + - Prohibit cycle counter: ``MDCR_EL3.SPME == 0 && PMCR_EL0.DP == 1``. + ``PMCR_EL0`` therefore needs to be saved and restored in EL3. + +- ARMv8.5-PMU implemented: + + - Prohibit general event counters: as in ARMv8.2-Debug. + - Prohibit cycle counter: ``MDCR_EL3.SCCD == 1`` + +In Aarch32 execution state the ``MDCR_EL3`` alias is the ``SDCR`` register, +which has some of the bit fields of ``MDCR_EL3``, most importantly the ``SPME`` +and ``SCCD`` bits. + +Build options +------------- + +Several build options can be used to check for security issues. Refer to the +:ref:`Build Options` for detailed information on these. + +- The ``BRANCH_PROTECTION`` build flag can be used to enable Pointer + Authentication and Branch Target Identification. + +- The ``ENABLE_STACK_PROTECTOR`` build flag can be used to identify buffer + overflows. + +- The ``W`` build flag can be used to enable a number of compiler warning + options to detect potentially incorrect code. + + - W=0 (default value) + + The ``Wunused`` with ``Wno-unused-parameter``, ``Wdisabled-optimization`` + and ``Wvla`` flags are enabled. + + The ``Wunused-but-set-variable``, ``Wmaybe-uninitialized`` and + ``Wpacked-bitfield-compat`` are GCC specific flags that are also enabled. + + - W=1 + + Adds ``Wextra``, ``Wmissing-format-attribute``, ``Wmissing-prototypes``, + ``Wold-style-definition`` and ``Wunused-const-variable``. + + - W=2 + + Adds ``Waggregate-return``, ``Wcast-align``, ``Wnested-externs``, + ``Wshadow``, ``Wlogical-op``. + + - W=3 + + Adds ``Wbad-function-cast``, ``Wcast-qual``, ``Wconversion``, ``Wpacked``, + ``Wpointer-arith``, ``Wredundant-decls`` and + ``Wswitch-default``. + + Refer to the GCC or Clang documentation for more information on the individual + options: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html and + https://clang.llvm.org/docs/DiagnosticsReference.html. + + NB: The ``Werror`` flag is enabled by default in TF-A and can be disabled by + setting the ``E`` build flag to 0. + +.. rubric:: References + +- `Arm ARM`_ + +-------------- + +*Copyright (c) 2019-2020, Arm Limited. All rights reserved.* + +.. _Arm ARM: https://developer.arm.com/docs/ddi0487/latest diff --git a/docs/process/security.rst b/docs/process/security.rst new file mode 100644 index 0000000..e15783b --- /dev/null +++ b/docs/process/security.rst @@ -0,0 +1,89 @@ +Security Handling +================= + +Security Disclosures +-------------------- + +We disclose all security vulnerabilities we find, or are advised about, that are +relevant to Trusted Firmware-A. We encourage responsible disclosure of +vulnerabilities and inform users as best we can about all possible issues. + +We disclose TF-A vulnerabilities as Security Advisories, all of which are listed +at the bottom of this page. Any new ones will, additionally, be announced as +issues in the project's `issue tracker`_ with the ``security-advisory`` tag. You +can receive notification emails for these by watching the "Trusted Firmware-A" +project at https://developer.trustedfirmware.org/. + +Found a Security Issue? +----------------------- + +Although we try to keep TF-A secure, we can only do so with the help of the +community of developers and security researchers. + +.. warning:: + If you think you have found a security vulnerability, please **do not** + report it in the `issue tracker`_ or on the `mailing list`_. Instead, please + follow the `TrustedFirmware.org security incident process`_. + +One of the goals of this process is to ensure providers of products that use +TF-A have a chance to consider the implications of the vulnerability and its +remedy before it is made public. As such, please follow the disclosure plan +outlined in the process. We do our best to respond and fix any issues quickly. + +Afterwards, we encourage you to write-up your findings about the TF-A source +code. + +Attribution +----------- + +We will name and thank you in the :ref:`Change Log & Release Notes` distributed +with the source code and in any published security advisory. + +Security Advisories +------------------- + ++-----------+------------------------------------------------------------------+ +| ID | Title | ++===========+==================================================================+ +| |TFV-1| | Malformed Firmware Update SMC can result in copy of unexpectedly | +| | large data into secure memory | ++-----------+------------------------------------------------------------------+ +| |TFV-2| | Enabled secure self-hosted invasive debug interface can allow | +| | normal world to panic secure world | ++-----------+------------------------------------------------------------------+ +| |TFV-3| | RO memory is always executable at AArch64 Secure EL1 | ++-----------+------------------------------------------------------------------+ +| |TFV-4| | Malformed Firmware Update SMC can result in copy or | +| | authentication of unexpected data in secure memory in AArch32 | +| | state | ++-----------+------------------------------------------------------------------+ +| |TFV-5| | Not initializing or saving/restoring PMCR_EL0 can leak secure | +| | world timing information | ++-----------+------------------------------------------------------------------+ +| |TFV-6| | Trusted Firmware-A exposure to speculative processor | +| | vulnerabilities using cache timing side-channels | ++-----------+------------------------------------------------------------------+ +| |TFV-7| | Trusted Firmware-A exposure to cache speculation vulnerability | +| | Variant 4 | ++-----------+------------------------------------------------------------------+ +| |TFV-8| | Not saving x0 to x3 registers can leak information from one | +| | Normal World SMC client to another | ++-----------+------------------------------------------------------------------+ + +.. _issue tracker: https://developer.trustedfirmware.org/project/board/1/ +.. _mailing list: https://lists.trustedfirmware.org/mailman3/lists/tf-a.lists.trustedfirmware.org/ + +.. |TFV-1| replace:: :ref:`Advisory TFV-1 (CVE-2016-10319)` +.. |TFV-2| replace:: :ref:`Advisory TFV-2 (CVE-2017-7564)` +.. |TFV-3| replace:: :ref:`Advisory TFV-3 (CVE-2017-7563)` +.. |TFV-4| replace:: :ref:`Advisory TFV-4 (CVE-2017-9607)` +.. |TFV-5| replace:: :ref:`Advisory TFV-5 (CVE-2017-15031)` +.. |TFV-6| replace:: :ref:`Advisory TFV-6 (CVE-2017-5753, CVE-2017-5715, CVE-2017-5754)` +.. |TFV-7| replace:: :ref:`Advisory TFV-7 (CVE-2018-3639)` +.. |TFV-8| replace:: :ref:`Advisory TFV-8 (CVE-2018-19440)` + +.. _TrustedFirmware.org security incident process: https://developer.trustedfirmware.org/w/collaboration/security_center/ + +-------------- + +*Copyright (c) 2019-2022, Arm Limited. All rights reserved.* |