diff --git a/multi-arch-coding-guideline.rst b/multi-arch-coding-guideline.rst new file mode 100644 index 0000000000..990523e9be --- /dev/null +++ b/multi-arch-coding-guideline.rst @@ -0,0 +1,341 @@ +Multi-architecture development coding guideline +############################################### + +This document is intended for guiding initial development of +ACRN multi-architecture. + +This document is NOT a coding style guideline (where one normally expects +that the document explains things such as indentation, naming convention, +typedef rules, etc.) + +During early development of multi-architecture project, the APIs +and data structures should be organized in a consistent approach for +better readability and maintainability. There are multiple ways to organize +these factors and this documentation lays out a guideline to assist future +development. + +General Assumptions and Practices +********************************* + +Due to FuSa constraints, we have several compiler features or coding styles that +are NOT encouraged in ``hypervisor/`` code: + +* Weak link (``__weak``): There are several MISRA rules that discourage uses of weak + link (Rule 1.2, 5.3, 5.8, 5.9). See `coding-guidelines`_ for more information. + +* Function-like macro: Use inline function whenever possible. Function-like macros + should be avoided unless absolutely necessary. This also implies that section-based + variable placement is also discouraged (utilizes linker to group symbols in certain + binary sections. See ``devicemodel/include/types.h``, macro ``SET_*``). + +Practices and general rule of thumb: + +* Enable compiler ``-O2`` as default. + +* APIs that are public (globally visible) but implemented in arch domains should start + their name with ``arch_``. If this API is optional, a default/empty ``arch_`` API + can be implemented in common C/header file. + +* Architecture headers should be exposure-controlled: If a structure or an API is used/shared + only within the module, then do not expose it to public headers, put it to private headers. + There is also a reverse statement: all arch symbols exposed to common scope should be public. + This is nearly impossible in practice. So we follow only the former part, and leave the latter + as future optimization. + +.. _coding-guidelines: https://projectacrn.github.io/latest/developer-guides/coding_guidelines.html + +Data Structures +*************** + +In multi-architecture project we might have a common data structure that needs +to carry some architecture specific information (for example, a vCPU object). + +For this type of data structures we prefer the following style: + +In common header: + +.. code-block:: c + + /* File: common.h */ + #include + + struct foo { + ; + ; + ; + struct arch_foo arch; + }; + + struct bar { + ; + ; + ; + } + +And in architecture specific header: + +.. code-block:: c + + /* File: arch-public.h */ + + /* do NOT include common.h here */ + + struct arch_foo { + ; + ; + ; + }; + + /* Forward declaration when the reference of struct is opaque */ + struct foo; + struct bar; + + struct baz { + struct foo *f; + struct bar *b; + } + + /* Forward declaration is also needed for function argument pointer */ + void some_function(struct foo *); + +The architecture public header should NOT in turn include common header +that already included the public header. + +Consider an inclusion graph with nodes being headers, and edges being +inclusions. If A includes B, there's an edge from A to B. Ideally, the +header inclusion graph should be **acyclic**. + +If a downstream header needs something from upstream headers, (upstream and +downstream refers to positions relative to inclusion chain) a forward declaration +SHOULD be sufficient. If not, then there are problems with your organization of headers. + +However in practice, people will not spend time following inclusion chain to +check if it is acyclic, and this issue is often noticable when there's a circular +dependency compilation error. In most projects (especially large ones), as long +as it compiles, it is OK. + +So in our project, the **acyclic inclusion** is not an enforcement, but a +practice that is strongly advised. + +Interfaces +********** + +Here we consider only common-arch interfaces. Interfaces within each architecture +are out of scope of this section. + +There are several form of interfaces used in different use cases. + +Function prototypes +=================== + +This is the most common form of interface where a function is referenced in common scope +and implemented in arch-specific scope. + +We prefer the following style: + +In common header: + +.. code-block:: c + + /* File: common.h */ + + #include + + /* + * Each architecture MUST implement this API in arch C file. + * The implementation SHOULD NOT be marked as static inline + * even if the implementation is small. + */ + void arch_api_mandatory(void); + + /* + * Short helpers that each architecture MAY provide. + * Due to FuSa constraints described above, + * Use a static inline function instead of a macro + * to select arch implementation. + */ + static inline void quick_helper(void) { arch_helper(); } + + /* + * For short helpers from each architecture that SHOULD be + * implemented as static inline function (or macro), + * It is also OK to implement them directly in arch-public + * header. + */ + + /* + * Architecture MAY implement this API. + * If it does not, an empty stub is provided. + * Define ARCH_HAS_SOME_CAPABILITY in arch headers to + * indicate implementation. + */ + #ifdef ARCH_HAS_SOME_CAPABILITY + int arch_api_optional(void); + #else + static inline int arch_api_optional(void) { return 0; } + #endif + + /* + * Architecture MAY implement this API. + * If it does not, a common default is provided in common C file. + * Due to FuSa constraint described above, __weak is discouraged. + * Define ARCH_HAS_OPTIONAL_WITH_DEFAULT in arch header to + * indicate implementation instead. + */ + void arch_api_optional_with_default(void); + + /* + * Configuration-based APIs + * The CONFIG macro is provided by header generated by build-system. + */ + #ifdef CONFIG_SWITCH_1 + void arch_api_conditional(void); + #endif + +In common C file: + +.. code-block:: c + + /* File: common.c */ + + #include + + #ifndef ARCH_HAS_OPTIONAL_WITH_DEFAULT + void arch_api_optional_with_default(void) { + /* default implementation */ + } + #endif + + void common_logic(void) { + arch_api_mandatory(); + + arch_api_optional(); + + arch_api_optional_with_default(); + + #ifdef CONFIG_SWITCH_1 + arch_api_conditional(); + #endif + } + +In architecture-specific public header file: + +.. code-block:: c + + /* File: arch-public.h */ + + /* Indicates implementation to optional API */ + #define ARCH_HAS_SOME_CAPABILITY + + /* Indicates implementation to optional_with_default API */ + #define ARCH_HAS_OPTIONAL_WITH_DEFAULT + + static inline void arch_helper(void) { + /* short implementation */ + } + +In architecture-specific C file: + +.. code-block:: c + + /* File: arch.c */ + #include + #include + + void arch_api_mandatory(void) { + /* implementation */ + } + + /* ARCH_HAS_SOME_CAPABILITY is defined in arch-public header */ + void arch_api_optional(void) { + /* implementation */ + } + + /* ARCH_HAS_OPTIONAL_WITH_DEFAULT is defined in arch-public header */ + void arch_api_optional_with_default(void) { + /* implementation */ + } + + #ifdef CONFIG_SWITCH_1 + void arch_api_conditional(void) { + /* implementation */ + } + #endif + +Register Callbacks +================== + +Callbacks are used when we do NOT know which implementation to be used +until runtime. To determine implementation we need to do either detection, +or accepting user input. + +With this approach, all implementations will be compiled-in, and each implementation +exposes a callback data structure typically called ``*_ops`` to common scope. + +In common header file: + +.. code-block:: c + + /* File: common.h */ + struct foo_ops { + void (*op1)(void); + int (*op2)(int); + int (*op3)(struct bar *); + }; + + extern struct foo_ops a_ops; + extern struct foo_ops b_ops; + + +In common C file: + +.. code-block:: c + + /* File: common.c */ + + #include + + void do_detection() { + /* detects environment */ + if (condition_a) { + register_callback(a_ops); + } else { + register_callback(b_ops); + } + } + + void actual_call() { + op->op1(); + op->op2(1); + } + +In arch header file: + +.. code-block:: c + + /* File: arch-public.h */ + + /* Do nothing */ + +.. code-block:: c + + /* File: arch.c */ + #include + + static void a_op1(void) { + /* implementation */ + } + + static int a_op2(void) { + /* implementation */ + } + + static int a_op2(struct bar *b) { + /* implementation */ + } + + const struct foo_ops a_ops = { + .op1 = a_op1, + .op2 = a_op2, + .op3 = a_op3, + }