GK SDK 源码库: XMIPCLinuxV100R005C00SPC030 (kernel/tools/open_source excluded)
This commit is contained in:
+216
@@ -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/
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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/
|
||||
@@ -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
|
||||
@@ -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/
|
||||
@@ -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
|
||||
@@ -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.*
|
||||
Reference in New Issue
Block a user