Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

headers: Refactor kernel and arch headers. #20119

Merged

Conversation

stephanosio
Copy link
Member

@stephanosio stephanosio commented Oct 24, 2019

This commit refactors kernel and arch headers to establish a boundary
between private and public interface headers.

The refactoring strategy used in this commit is detailed in the issue

This commit introduces the following major changes:

1. Establish a clear boundary between private and public headers by
  removing "kernel/include" and "arch/*/include" from the global
  include paths. Ideally, only kernel/ and arch/*/ source files should
  reference the headers in these directories. If these headers must be
  used by a component, these include paths shall be manually added to
  the CMakeLists.txt file of the component. This is intended to
  discourage applications from including private kernel and arch
  headers either knowingly and unknowingly.

  - kernel/include/ (PRIVATE)
    This directory contains the private headers that provide private
   kernel definitions which should not be visible outside the kernel
   and arch source code. All public kernel definitions must be added
   to an appropriate header located under include/.

  - arch/*/include/ (PRIVATE)
    This directory contains the private headers that provide private
   architecture-specific definitions which should not be visible
   outside the arch and kernel source code. All public architecture-
   specific definitions must be added to an appropriate header located
   under include/arch/*/.

  - include/ AND include/sys/ (PUBLIC)
    This directory contains the public headers that provide public
   kernel definitions which can be referenced by both kernel and
   application code.

  - include/arch/*/ (PUBLIC)
    This directory contains the public headers that provide public
   architecture-specific definitions which can be referenced by both
   kernel and application code.

2. Split arch_interface.h into "kernel-to-arch interface" and "public
  arch interface" divisions.

  - kernel/include/kernel_arch_interface.h
    * provides private "kernel-to-arch interface" definition.
    * includes arch/*/include/kernel_arch_func.h to ensure that the
     interface function implementations are always available.
    * includes sys/arch_interface.h so that public arch interface
     definitions are automatically included when including this file.

  - arch/*/include/kernel_arch_func.h
    * provides architecture-specific "kernel-to-arch interface"
     implementation.
    * only the functions that will be used in kernel and arch source
     files are defined here.

  - include/sys/arch_interface.h
    * provides "public arch interface" definition.
    * includes include/arch/arch_inlines.h to ensure that the
     architecture-specific public inline interface function
     implementations are always available.

  - include/arch/arch_inlines.h
    * includes architecture-specific arch_inlines.h in
     include/arch/*/arch_inline.h.

  - include/arch/*/arch_inline.h
    * provides architecture-specific "public arch interface" inline
     function implementation.
    * supersedes include/sys/arch_inline.h.

3. Refactor kernel and the existing architecture implementations.

  - Remove circular dependency of kernel and arch headers. The
   following general rules should be observed:

    * Never include any private headers from public headers
    * Never include kernel_internal.h in kernel_arch_data.h
    * Always include kernel_arch_data.h from kernel_arch_func.h
    * Never include kernel.h from kernel_struct.h either directly or
     indirectly. Only add the kernel structures that must be referenced
     from public arch headers in this file.

  - Relocate syscall_handler.h to include/ so it can be used in the
   public code. This is necessary because many user-mode public codes
   reference the functions defined in this header.

  - Relocate kernel_arch_thread.h to include/arch/*/thread.h. This is
   necessary to provide architecture-specific thread definition for
   'struct k_thread' in kernel.h.

  - Remove any private header dependencies from public headers using
   the following methods:

    * If dependency is not required, simply omit
    * If dependency is required,
      - Relocate a portion of the required dependencies from the
       private header to an appropriate public header OR
      - Relocate the required private header to make it public.

This commit supersedes #20047, addresses #19666, and fixes #3056.

Signed-off-by: Stephanos Ioannidis <[email protected]>

@stephanosio stephanosio added the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label Oct 24, 2019
@zephyrbot
Copy link
Collaborator

zephyrbot commented Oct 24, 2019

All checks are passing now.

checkpatch (informational only, not a failure)

-:2355: WARNING:NEW_TYPEDEFS: do not add new typedefs
#2355: FILE: include/arch/x86/intel64/thread.h:79:
+typedef struct x86_tss64 x86_tss64_t;

-:2376: WARNING:NEW_TYPEDEFS: do not add new typedefs
#2376: FILE: include/arch/x86/intel64/thread.h:100:
+typedef struct _callee_saved _callee_saved_t;

-:2390: WARNING:LINE_SPACING: Missing a blank line after declarations
#2390: FILE: include/arch/x86/intel64/thread.h:114:
+	u64_t r11;
+	char __aligned(X86_FXSAVE_ALIGN) sse[X86_FXSAVE_SIZE];

-:2393: WARNING:NEW_TYPEDEFS: do not add new typedefs
#2393: FILE: include/arch/x86/intel64/thread.h:117:
+typedef struct _thread_arch _thread_arch_t;

-:3207: WARNING:NEW_TYPEDEFS: do not add new typedefs
#3207: FILE: include/sys/arch_interface.h:43:
+typedef struct _k_thread_stack_element k_thread_stack_t;

-:5385: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5385: FILE: subsys/testsuite/ztest/include/arch/cpu.h:13:
+typedef struct _callee_saved _callee_saved_t;

-:5390: WARNING:NEW_TYPEDEFS: do not add new typedefs
#5390: FILE: subsys/testsuite/ztest/include/arch/cpu.h:18:
+typedef struct _thread_arch _thread_arch_t;

- total: 0 errors, 7 warnings, 3850 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@stephanosio stephanosio force-pushed the refactor_kernel_arch_h branch 3 times, most recently from 0cf557d to 94c7ac0 Compare October 25, 2019 01:55
@stephanosio stephanosio marked this pull request as ready for review October 25, 2019 03:32
@stephanosio stephanosio removed the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label Oct 25, 2019
@stephanosio
Copy link
Member Author

stephanosio commented Oct 25, 2019

This PR supersedes the previous PR #20047, which was an attempt at fixing #3056.

The changes in this PR fix #3056, while addressing all the essential changes detailed in the issue #19666 as well.

Once this gets merged, the following hacks in arch/arm and arch/x86 should be fixed:

/* FIXME prefer these inline, but see GH-3056 */

/* FIXME: IRQ direct inline functions have to be placed here and not in
* arch/cpu.h as inline functions due to nasty circular dependency between
* arch/cpu.h and kernel_structs.h; the inline functions typically need to
* perform operations on _kernel. For now, leave as regular functions, a
* future iteration will resolve this.
*
* See https://github.com/zephyrproject-rtos/zephyr/issues/3056
*/

/* FIXME prefer these inline, but see GH-3056 */

/* FIXME: IRQ direct inline functions have to be placed here and not in
* arch/cpu.h as inline functions due to nasty circular dependency between
* arch/cpu.h and kernel_structs.h; the inline functions typically need to
* perform operations on _kernel. For now, leave as regular functions, a
* future iteration will resolve this.
*
* See https://github.com/zephyrproject-rtos/zephyr/issues/3056
*/

@aescolar
Copy link
Member

aescolar commented Nov 6, 2019

@aescolar What do you think about adding a dummy dts

@stephanosio I presume you are seeing something broken in master, and I want to look into it asap. If that is the case, let's discuss it outside of this PR to not create confusion. If something is broken in master it should be fixed on its own, not in an unrelated PR.

@stephanosio
Copy link
Member Author

Force pushing to address the change requested by @SebastianBoe and update hal_nxp revision from PR ref to merged commit hash in west.yml.

@ioannisg
Copy link
Member

ioannisg commented Nov 6, 2019

Last merge conflicts are due to the 64-bit updates on the syscalls, am I correct?

@nashif
Copy link
Member

nashif commented Nov 6, 2019

Last merge conflicts are due to the 64-bit updates on the syscalls, am I correct?

grrrrrrr

@nashif
Copy link
Member

nashif commented Nov 6, 2019

@stephanosio last rebase maybe? :) thanks.

@andrewboie
Copy link
Contributor

@stephanosio last rebase maybe? :) thanks.

@stephanosio sorry that was my fault

This commit refactors kernel and arch headers to establish a boundary
between private and public interface headers.

The refactoring strategy used in this commit is detailed in the issue

This commit introduces the following major changes:

1. Establish a clear boundary between private and public headers by
  removing "kernel/include" and "arch/*/include" from the global
  include paths. Ideally, only kernel/ and arch/*/ source files should
  reference the headers in these directories. If these headers must be
  used by a component, these include paths shall be manually added to
  the CMakeLists.txt file of the component. This is intended to
  discourage applications from including private kernel and arch
  headers either knowingly and unknowingly.

  - kernel/include/ (PRIVATE)
    This directory contains the private headers that provide private
   kernel definitions which should not be visible outside the kernel
   and arch source code. All public kernel definitions must be added
   to an appropriate header located under include/.

  - arch/*/include/ (PRIVATE)
    This directory contains the private headers that provide private
   architecture-specific definitions which should not be visible
   outside the arch and kernel source code. All public architecture-
   specific definitions must be added to an appropriate header located
   under include/arch/*/.

  - include/ AND include/sys/ (PUBLIC)
    This directory contains the public headers that provide public
   kernel definitions which can be referenced by both kernel and
   application code.

  - include/arch/*/ (PUBLIC)
    This directory contains the public headers that provide public
   architecture-specific definitions which can be referenced by both
   kernel and application code.

2. Split arch_interface.h into "kernel-to-arch interface" and "public
  arch interface" divisions.

  - kernel/include/kernel_arch_interface.h
    * provides private "kernel-to-arch interface" definition.
    * includes arch/*/include/kernel_arch_func.h to ensure that the
     interface function implementations are always available.
    * includes sys/arch_interface.h so that public arch interface
     definitions are automatically included when including this file.

  - arch/*/include/kernel_arch_func.h
    * provides architecture-specific "kernel-to-arch interface"
     implementation.
    * only the functions that will be used in kernel and arch source
     files are defined here.

  - include/sys/arch_interface.h
    * provides "public arch interface" definition.
    * includes include/arch/arch_inlines.h to ensure that the
     architecture-specific public inline interface function
     implementations are always available.

  - include/arch/arch_inlines.h
    * includes architecture-specific arch_inlines.h in
     include/arch/*/arch_inline.h.

  - include/arch/*/arch_inline.h
    * provides architecture-specific "public arch interface" inline
     function implementation.
    * supersedes include/sys/arch_inline.h.

3. Refactor kernel and the existing architecture implementations.

  - Remove circular dependency of kernel and arch headers. The
   following general rules should be observed:

    * Never include any private headers from public headers
    * Never include kernel_internal.h in kernel_arch_data.h
    * Always include kernel_arch_data.h from kernel_arch_func.h
    * Never include kernel.h from kernel_struct.h either directly or
     indirectly. Only add the kernel structures that must be referenced
     from public arch headers in this file.

  - Relocate syscall_handler.h to include/ so it can be used in the
   public code. This is necessary because many user-mode public codes
   reference the functions defined in this header.

  - Relocate kernel_arch_thread.h to include/arch/*/thread.h. This is
   necessary to provide architecture-specific thread definition for
   'struct k_thread' in kernel.h.

  - Remove any private header dependencies from public headers using
   the following methods:

    * If dependency is not required, simply omit
    * If dependency is required,
      - Relocate a portion of the required dependencies from the
       private header to an appropriate public header OR
      - Relocate the required private header to make it public.

This commit supersedes zephyrproject-rtos#20047, addresses zephyrproject-rtos#19666, and fixes zephyrproject-rtos#3056.

Signed-off-by: Stephanos Ioannidis <[email protected]>
This commit modifies the z_new_thread_init function, that was
previously declared as ALWAYS_INLINE to be a normal function.

z_new_thread_init function is only called by the z_arch_new_thread
function and, since this is not a performance-critical function, there
is no good justification for inlining it.

Signed-off-by: Stephanos Ioannidis <[email protected]>
When compiling the components under the arch directory, the compiler
include paths for arch and kernel private headers need to be specified.

This was previously done by adding 'zephyr_library_include_directories'
to CMakeLists.txt file for every component under the arch directory,
and this resulted in a significant amount of duplicate code.

This commit uses the CMake 'include_directories' command in the root
CMakeLists.txt to simplify specification of the private header include
paths for all the arch components.

Signed-off-by: Stephanos Ioannidis <[email protected]>
Remove unnecessary inclusion of offsets_short.h in the LPC54114
start-up code.

See zephyrproject-rtos/hal_nxp#17.

Signed-off-by: Stephanos Ioannidis <[email protected]>
@stephanosio
Copy link
Member Author

Rebased onto the latest master.

@andrewboie andrewboie merged commit afe6dab into zephyrproject-rtos:master Nov 7, 2019
@stephanosio stephanosio deleted the refactor_kernel_arch_h branch November 7, 2019 00:20
stephanosio added a commit to stephanosio/zephyr that referenced this pull request Nov 8, 2019
This commit inlines arch_isr_direct_header function that was previously
placed in irq_manage.c for no good reason (possibly in relation to the
FIXME for zephyrproject-rtos#3056).

In addition, since the PR zephyrproject-rtos#20119 resolved the header circular
dependency issue described in the issue zephyrproject-rtos#3056, this commit removes the
references to it in the code.

The reason for not inlining _arch_is_direct_pm as the zephyrproject-rtos#3056 FIXME
suggests is that there is little to gain from doing so and there still
exists circular dependency for the headers required by this function
(zephyrproject-rtos#20119 only addresses kernel_structs.h, which is required for _current
and _kernel, which, in turn, is required for handling interrupt nesting
in many architectures; in fact, Cortex-A and Cortex-R port will require
it as well).

Signed-off-by: Stephanos Ioannidis <[email protected]>
stephanosio added a commit to stephanosio/zephyr that referenced this pull request Nov 8, 2019
This commit inlines the direct ISR functions that were previously
implemented in irq_manage.c, since the PR zephyrproject-rtos#20119 resolved the circular
dependency between arch.h and kernel_structs.h described in the issue
zephyrproject-rtos#3056.

Signed-off-by: Stephanos Ioannidis <[email protected]>
ioannisg pushed a commit that referenced this pull request Nov 8, 2019
This commit inlines arch_isr_direct_header function that was previously
placed in irq_manage.c for no good reason (possibly in relation to the
FIXME for #3056).

In addition, since the PR #20119 resolved the header circular
dependency issue described in the issue #3056, this commit removes the
references to it in the code.

The reason for not inlining _arch_is_direct_pm as the #3056 FIXME
suggests is that there is little to gain from doing so and there still
exists circular dependency for the headers required by this function
(#20119 only addresses kernel_structs.h, which is required for _current
and _kernel, which, in turn, is required for handling interrupt nesting
in many architectures; in fact, Cortex-A and Cortex-R port will require
it as well).

Signed-off-by: Stephanos Ioannidis <[email protected]>
carlescufi pushed a commit that referenced this pull request Nov 8, 2019
This commit inlines the direct ISR functions that were previously
implemented in irq_manage.c, since the PR #20119 resolved the circular
dependency between arch.h and kernel_structs.h described in the issue
#3056.

Signed-off-by: Stephanos Ioannidis <[email protected]>
@wentongwu
Copy link
Contributor

@stephanosio sorry, just see the PR, why we re-write subsys/debug/tracing/CMakeLists.txt?

@stephanosio
Copy link
Member Author

@stephanosio sorry, just see the PR, why we re-write subsys/debug/tracing/CMakeLists.txt?

Somehow missed this comment.

Because zephyr_library_include_directories must be used to prevent global inclusion of the private header directories and this requires the use of zephyr_library_* functions.

amr-sc added a commit to syntacore/zephyr that referenced this pull request Dec 14, 2022
Zephyr release v.3.2.0 has no EARLY initialization level.
So the MPU and L1/L2 initialization is moved to PRE_KERNEL_1 level
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Architectures area: Kernel Maintainer At least 2 days before merge, maintainer review required Needs review This PR needs attention from Zephyr's maintainers Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants