|
| 1 | +Multi-architecture development coding guideline |
| 2 | +############################################### |
| 3 | + |
| 4 | +This document is intended for guiding initial development of |
| 5 | +ACRN multi-architecture. |
| 6 | + |
| 7 | +This document is NOT a coding style guideline (where one normally expects |
| 8 | +that the document explains things such as indentation, naming convention, |
| 9 | +typedef rules, etc.) |
| 10 | + |
| 11 | +During early development of multi-architecture project, the APIs |
| 12 | +and data structures should be organized in a consistent approach for |
| 13 | +better readability and maintainability. There are multiple ways to organize |
| 14 | +these factors and this documentation lays out a guideline to assist future |
| 15 | +development. |
| 16 | + |
| 17 | +General Assumptions and Practices |
| 18 | +********************************* |
| 19 | + |
| 20 | +Due to FuSa constraints, we have several compiler features or coding styles that |
| 21 | +are NOT encouraged in ``hypervisor/`` code: |
| 22 | + |
| 23 | +* Weak link (``__weak``): There are several MISRA rules that discourage uses of weak |
| 24 | + link (Rule 1.2, 5.3, 5.8, 5.9). See `coding-guidelines`_ for more information. |
| 25 | + |
| 26 | +* Function-like macro: Use inline function whenever possible. Function-like macros |
| 27 | + should be avoided unless absolutely necessary. This also implies that section-based |
| 28 | + variable placement is also discouraged (utilizes linker to group symbols in certain |
| 29 | + binary sections. See ``devicemodel/include/types.h``, macro ``SET_*``). |
| 30 | + |
| 31 | +Practices and general rule of thumb: |
| 32 | + |
| 33 | +* Enable compiler ``-O2`` as default. |
| 34 | + |
| 35 | +* Consistent function storage class: If a common function prototype indicates a non-static |
| 36 | + function, do NOT implement it as ``static inline`` in header. Implement as non-static |
| 37 | + in C file. |
| 38 | + |
| 39 | +* APIs that are public (globally visible) but implemented in arch domains should start |
| 40 | + their name with ``arch_``. If this API is optional, a default/empty ``arch_`` API |
| 41 | + can be implemented in common C/header file. |
| 42 | + |
| 43 | +* Architecture headers should be exposure-controlled: If a structure or an API is used/shared |
| 44 | + only within the module, then do not expose it to public headers, put it to private headers. |
| 45 | + There is also a reverse statement: all arch symbols exposed to common scope should be public. |
| 46 | + This is nearly impossible in practice. So we follow only the former part, and leave the latter |
| 47 | + as future optimization. |
| 48 | + |
| 49 | +.. _coding-guidelines: https://projectacrn.github.io/latest/developer-guides/coding_guidelines.html |
| 50 | + |
| 51 | +Data Structures |
| 52 | +*************** |
| 53 | + |
| 54 | +In multi-architecture project we might have a common data structure that needs |
| 55 | +to carry some architecture specific information (for example, a vCPU object). |
| 56 | + |
| 57 | +For this type of data structures we prefer the following style: |
| 58 | + |
| 59 | +In common header: |
| 60 | + |
| 61 | +.. code-block:: c |
| 62 | +
|
| 63 | + /* File: common.h */ |
| 64 | + #include <arch-public.h> |
| 65 | +
|
| 66 | + struct foo { |
| 67 | + <common member1>; |
| 68 | + <common member2>; |
| 69 | + <common member3>; |
| 70 | + struct foo_arch arch; |
| 71 | + }; |
| 72 | +
|
| 73 | + struct bar { |
| 74 | + <common member1>; |
| 75 | + <common member2>; |
| 76 | + <common member3>; |
| 77 | + } |
| 78 | +
|
| 79 | +And in architecture specific header: |
| 80 | + |
| 81 | +.. code-block:: c |
| 82 | +
|
| 83 | + /* File: arch-public.h */ |
| 84 | +
|
| 85 | + /* do NOT include common.h here */ |
| 86 | +
|
| 87 | + struct foo_arch { |
| 88 | + <arch member1>; |
| 89 | + <arch member2>; |
| 90 | + <arch member3>; |
| 91 | + }; |
| 92 | +
|
| 93 | + /* Forward declaration when the reference of struct is opaque */ |
| 94 | + struct foo; |
| 95 | + struct bar; |
| 96 | +
|
| 97 | + struct baz { |
| 98 | + struct foo *f; |
| 99 | + struct bar *b; |
| 100 | + } |
| 101 | +
|
| 102 | + /* Forward declaration is also needed for function argument pointer */ |
| 103 | + void some_function(struct foo *); |
| 104 | +
|
| 105 | +The architecture public header should NOT in turn include common header |
| 106 | +that already included the public header. |
| 107 | + |
| 108 | +Consider an inclusion graph with nodes being headers, and edges being |
| 109 | +inclusions. If A includes B, there's an edge from A to B. Ideally, the |
| 110 | +header inclusion graph should be **acyclic**. |
| 111 | + |
| 112 | +If a downstream header needs something from upstream headers, (upstream and |
| 113 | +downstream refers to positions relative to inclusion chain) a forward declaration |
| 114 | +SHOULD be sufficient. If not, then there are problems with your organization of headers. |
| 115 | + |
| 116 | +However in practice, people will not spend time following inclusion chain to |
| 117 | +check if it is acyclic, and this issue is often noticable when there's a circular |
| 118 | +dependency compilation error. In most projects (especially large ones), as long |
| 119 | +as it compiles, it is OK. |
| 120 | + |
| 121 | +So in our project, the **acyclic inclusion** is not an enforcement, but a |
| 122 | +practice that is strongly advised. |
| 123 | + |
| 124 | +Interfaces |
| 125 | +********** |
| 126 | + |
| 127 | +Here we consider only common-arch interfaces. Interfaces within each architecture |
| 128 | +are out of scope of this section. |
| 129 | + |
| 130 | +There are several form of interfaces used in different use cases. |
| 131 | + |
| 132 | +Function prototypes |
| 133 | +=================== |
| 134 | + |
| 135 | +This is the most common form of interface where a function is referenced in common scope |
| 136 | +and implemented in arch-specific scope. |
| 137 | + |
| 138 | +We prefer the following style: |
| 139 | + |
| 140 | +In common header: |
| 141 | + |
| 142 | +.. code-block:: c |
| 143 | +
|
| 144 | + /* File: common.h */ |
| 145 | +
|
| 146 | + #include <arch-public.h> |
| 147 | +
|
| 148 | + /* |
| 149 | + * Each architecture MUST implement this API in arch C file. |
| 150 | + * The implementation SHOULD NOT be marked as static inline |
| 151 | + * even if the implementation is small. |
| 152 | + */ |
| 153 | + void arch_api_mandatory(void); |
| 154 | +
|
| 155 | + /* |
| 156 | + * Short helpers that each architecture MAY provide. |
| 157 | + * Due to FuSa constraints described above, |
| 158 | + * Use a static inline function instead of a macro |
| 159 | + * to select arch implementation. |
| 160 | + */ |
| 161 | + static inline void quick_helper(void) { arch_helper(); } |
| 162 | +
|
| 163 | + /* |
| 164 | + * Architecture MAY implement this API. |
| 165 | + * If it does not, an empty stub is provided. |
| 166 | + * Define __ARCH_HAS_SOME_CAPABILITY in arch headers to |
| 167 | + * indicate implementation. |
| 168 | + */ |
| 169 | + #ifdef __ARCH_HAS_SOME_CAPABILITY |
| 170 | + int arch_api_optional(void); |
| 171 | + #else |
| 172 | + static inline int arch_api_optional(void) { return 0; } |
| 173 | + #endif |
| 174 | +
|
| 175 | + /* |
| 176 | + * Architecture MAY implement this API. |
| 177 | + * If it does not, a common default is provided in common C file. |
| 178 | + * Due to FuSa constraint described above, __weak is discouraged. |
| 179 | + * Define __ARCH_HAS_OPTIONAL_WITH_DEFAULT in arch header to |
| 180 | + * indicate implementation instead. |
| 181 | + */ |
| 182 | + void arch_api_optional_with_default(void); |
| 183 | +
|
| 184 | + /* |
| 185 | + * Configuration-based APIs |
| 186 | + * The CONFIG macro is provided by header generated by build-system. |
| 187 | + */ |
| 188 | + #ifdef CONFIG_SWITCH_1 |
| 189 | + void arch_api_conditional(void); |
| 190 | + #endif |
| 191 | +
|
| 192 | +In common C file: |
| 193 | + |
| 194 | +.. code-block:: c |
| 195 | +
|
| 196 | + /* File: common.c */ |
| 197 | +
|
| 198 | + #include <common.h> |
| 199 | +
|
| 200 | + #ifndef __ARCH_HAS_OPTIONAL_WITH_DEFAULT |
| 201 | + void arch_api_optional_with_default(void) { |
| 202 | + /* default implementation */ |
| 203 | + } |
| 204 | + #endif |
| 205 | +
|
| 206 | + void common_logic(void) { |
| 207 | + arch_api_mandatory(); |
| 208 | +
|
| 209 | + arch_api_optional(); |
| 210 | +
|
| 211 | + arch_api_optional_with_default(); |
| 212 | +
|
| 213 | + #ifdef CONFIG_SWITCH_1 |
| 214 | + arch_api_conditional(); |
| 215 | + #endif |
| 216 | + } |
| 217 | +
|
| 218 | +In architecture-specific public header file: |
| 219 | + |
| 220 | +.. code-block:: c |
| 221 | +
|
| 222 | + /* File: arch-public.h */ |
| 223 | +
|
| 224 | + /* Indicates implementation to optional API */ |
| 225 | + #define __ARCH_HAS_SOME_CAPABILITY |
| 226 | +
|
| 227 | + /* Indicates implementation to optional_with_default API */ |
| 228 | + #define __ARCH_HAS_OPTIONAL_WITH_DEFAULT |
| 229 | +
|
| 230 | + static inline void arch_helper(void) { |
| 231 | + /* short implementation */ |
| 232 | + } |
| 233 | +
|
| 234 | +In architecture-specific C file: |
| 235 | + |
| 236 | +.. code-block:: c |
| 237 | +
|
| 238 | + /* File: arch.c */ |
| 239 | + #include <common.h> |
| 240 | + #include <arch-priate.h> |
| 241 | +
|
| 242 | + void arch_api_mandatory(void) { |
| 243 | + /* implementation */ |
| 244 | + } |
| 245 | +
|
| 246 | + /* __ARCH_HAS_SOME_CAPABILITY is defined in arch-public header */ |
| 247 | + void arch_api_optional(void) { |
| 248 | + /* implementation */ |
| 249 | + } |
| 250 | +
|
| 251 | + /* __ARCH_HAS_OPTIONAL_WITH_DEFAULT is defined in arch-public header */ |
| 252 | + void arch_api_optional_with_default(void) { |
| 253 | + /* implementation */ |
| 254 | + } |
| 255 | +
|
| 256 | + #ifdef CONFIG_SWITCH_1 |
| 257 | + void arch_api_conditional(void) { |
| 258 | + /* implementation */ |
| 259 | + } |
| 260 | + #endif |
| 261 | +
|
| 262 | +Register Callbacks |
| 263 | +================== |
| 264 | + |
| 265 | +Callbacks are used when we do NOT know which implementation to be used |
| 266 | +until runtime. To determine implementation we need to do either detection, |
| 267 | +or accepting user input. |
| 268 | + |
| 269 | +With this approach, all implementations will be compiled-in, and each implementation |
| 270 | +exposes a callback data structure typically called ``*_ops`` to common scope. |
| 271 | + |
| 272 | +In common header file: |
| 273 | + |
| 274 | +.. code-block:: c |
| 275 | +
|
| 276 | + /* File: common.h */ |
| 277 | + struct foo_ops { |
| 278 | + void (*op1)(void); |
| 279 | + int (*op2)(int); |
| 280 | + int (*op3)(struct bar *); |
| 281 | + }; |
| 282 | +
|
| 283 | + extern struct foo_ops a_ops; |
| 284 | + extern struct foo_ops b_ops; |
| 285 | +
|
| 286 | +
|
| 287 | +In common C file: |
| 288 | + |
| 289 | +.. code-block:: c |
| 290 | +
|
| 291 | + /* File: common.c */ |
| 292 | +
|
| 293 | + #include <common.h> |
| 294 | +
|
| 295 | + void do_detection() { |
| 296 | + /* detects environment */ |
| 297 | + if (condition_a) { |
| 298 | + register_callback(a_ops); |
| 299 | + } else { |
| 300 | + register_callback(b_ops); |
| 301 | + } |
| 302 | + } |
| 303 | +
|
| 304 | + void actual_call() { |
| 305 | + op->op1(); |
| 306 | + op->op2(1); |
| 307 | + } |
| 308 | +
|
| 309 | +In arch header file: |
| 310 | + |
| 311 | +.. code-block:: c |
| 312 | +
|
| 313 | + /* File: arch-public.h */ |
| 314 | +
|
| 315 | + /* Do nothing */ |
| 316 | +
|
| 317 | +.. code-block:: c |
| 318 | +
|
| 319 | + /* File: arch.c */ |
| 320 | + #include <common.h> |
| 321 | +
|
| 322 | + static void a_op1(void) { |
| 323 | + /* implementation */ |
| 324 | + } |
| 325 | +
|
| 326 | + static int a_op2(void) { |
| 327 | + /* implementation */ |
| 328 | + } |
| 329 | +
|
| 330 | + static int a_op2(struct bar *b) { |
| 331 | + /* implementation */ |
| 332 | + } |
| 333 | +
|
| 334 | + const struct foo_ops a_ops = { |
| 335 | + .op1 = a_op1, |
| 336 | + .op2 = a_op2, |
| 337 | + .op3 = a_op3, |
| 338 | + } |
0 commit comments