Hi Everyone, This is regarding the header file re-organization patch that was submitted by Julius https://review.trustedfirmware.org/#/c/TF-A/trusted-firmware-a/+/1207/.
It is necessary for the headers which form the ABI/handover interface for BL31 to be able to copied separately and included in other projects. The current approach taken in the patch is to define a "raw" version of such headers and have the original header include them. This certainly is the easiest way to solve the problem. But if it possible to have a more refined solution, that would be preferable. For that I have the following questions :
1. Should the project recognize these special headers and have them organized together in a folder ? It is important to recognize that the ABI can be extended by the platform. I would expect even if these "common" headers are organized into a folder, the platform specific ones need not go together with them. 2. Should the header be restricted from including standard C library headers ? 3. Should these ABI headers be allowed to include each other ? Forward declaration might be able to solve some of the issues, but good to have a policy on this.
The current patch as such can be treated as step towards the ideal solution, if that solution needs more work/churn in the code base.
Comments welcome.
Best Regards Soby Mathew
Should the project recognize these special headers and have them organized together in a folder ? It is important to recognize that the ABI can be extended by the platform. I would expect even if these “common” headers are organized into a folder, the platform specific ones need not go together with them. Should the header be restricted from including standard C library headers ? Should these ABI headers be allowed to include each other ? Forward declaration might be able to solve some of the issues, but good to have a policy on this.
Just to explain my reasoning for the proposal I uploaded: I think in order for this to be really usable, you cannot expect other projects to provide C library headers according to any specific standard, because in practice most of them don't (and in fact neither does Trusted Firmware... we have an include/lib/libc/, but it is missing plenty of headers required by ISO C and commonly available in other projects, like <ctype.h>). My intention was to require the absolute minimum that is needed to define handoff structures at all, namely fixed-width integer types like uint32_t. Even if another project doesn't use those, it's relatively easy for them to just write a wrapper header that defines those types before including the raw headers from TF. (They also need to pre-define the U(), __ASSEMBLY__ and AARCH32/AARCH64 macros because it would have been very hard to split out the code needed for this without those. I think the best way to do this is to just define a minimal list of types and macros that need to be pre-defined before one of these headers can be included, and try to not make any future additions to that list unless really necessary.)
Allowing these headers to include each other could be done as a convenience, but only if using "../../double/quote/relative/path.h" notation. Using <abosolute/path/notation.h> like normal TF headers wouldn't work because then the other project would have to add the Trusted Firmware toplevel include/ directory to their own include path, which they can't because it may cause header clashes with their own headers, or at least it would depend on include order and create a big risk of accidentally including the wrong thing and be overall far too messy. In my proposal, since some headers (like <stdint.h>) always need to be pre-included by the wrapper anyway, I opted to just pre-include everything to keep things consistent.
I'm open to putting them in a separate folder if we prefer that. My current proposal just sets them apart by having them all use the "_raw.h" suffix, but we could also make it a prefix instead or create a raw/ subfolder to any folder that has such headers or anything like that. I don't think it makes a big difference either way.
-----Original Message----- From: Julius Werner jwerner@google.com Sent: 24 June 2019 23:46 To: Soby Mathew Soby.Mathew@arm.com Cc: tf-a@lists.trustedfirmware.org; nd nd@arm.com Subject: Re: Header file re-org
Should the project recognize these special headers and have them organized
together in a folder ? It is important to recognize that the ABI can be extended by the platform. I would expect even if these “common” headers are organized into a folder, the platform specific ones need not go together with them.
Should the header be restricted from including standard C library headers ? Should these ABI headers be allowed to include each other ? Forward
declaration might be able to solve some of the issues, but good to have a policy on this.
Just to explain my reasoning for the proposal I uploaded: I think in order for this to be really usable, you cannot expect other projects to provide C library headers according to any specific standard, because in practice most of them don't (and in fact neither does Trusted Firmware... we have an include/lib/libc/, but it is missing plenty of headers required by ISO C and commonly available in other projects, like <ctype.h>). My intention was to require the absolute minimum that is needed to define handoff structures at all, namely fixed-width integer types like uint32_t. Even if another project doesn't use those, it's relatively easy for them to just write a wrapper header that defines those types before including the raw headers from TF. (They also need to pre-define the U(), __ASSEMBLY__ and AARCH32/AARCH64 macros because it would have been very hard to split out the code needed for this without those. I think the best way to do this is to just define a minimal list of types and macros that need to be pre-defined before one of these headers can be included, and try to not make any future additions to that list unless really necessary.)
Thanks Julius for the inputs. That makes sense. We already have a precedence for sharing headers between the cross compiler and the host toolchain for the host tools via `tools_share` directory. Similarly we can have a `xxx_share` directory in /include for these headers. I think it is worth factoring out the U() macro to this folder. Regarding the __ASSEMBLY__ and AARCH32/AARCH64 macros, we could split the header out for asm, aarch32, aarch64 variants of the file thus removing the need to define these macros.
Allowing these headers to include each other could be done as a convenience, but only if using "../../double/quote/relative/path.h" notation. Using <abosolute/path/notation.h> like normal TF headers wouldn't work because then the other project would have to add the Trusted Firmware toplevel include/ directory to their own include path, which they can't because it may cause header clashes with their own headers, or at least it would depend on include order and create a big risk of accidentally including the wrong thing and be overall far too messy. In my proposal, since some headers (like <stdint.h>) always need to be pre-included by the wrapper anyway, I opted to just pre-include everything to keep things consistent.
OK , so we can say that these headers can include others such "raw" headers as long as they stick to the relative path. This also provides another reason to group all such "raw" headers together in a folder as it becomes easier to include and export them.
I'm open to putting them in a separate folder if we prefer that. My current proposal just sets them apart by having them all use the "_raw.h" suffix, but we could also make it a prefix instead or create a raw/ subfolder to any folder that has such headers or anything like that. I don't think it makes a big difference either way.
I think these raw header are worth grouping together as it makes it clear the need to handle them separately and makes it easier to export these headers. A strawman proposal for the header hierarchy :
/include/tf_share --> the ep_info, param_header and other such common header could go in there /include/tf_share/ bl_aux_params --> folder for the new bl_aux param header /include/tf_share/bl_aux_params/vendor/rockchip --> folder for vendor extensions for the rockchip platform
Does that sound sensible ?
Best Regards Soby Mathew
Thanks Julius for the inputs. That makes sense. We already have a precedence for sharing headers between the cross compiler and the host toolchain for the host tools via `tools_share` directory. Similarly we can have a `xxx_share` directory in /include for these headers. I think it is worth factoring out the U() macro to this folder. Regarding the __ASSEMBLY__ and AARCH32/AARCH64 macros, we could split the header out for asm, aarch32, aarch64 variants of the file thus removing the need to define these macros.
I tried factoring them out into different headers and have them include each other at first, but it didn't really work out and became a huge mess because they're so interdependent. entry_point_info_t is an architecture-dependent type that depends on param_header_t (an architecture-independent type), and they both have assembly-only constants associated with them. So in the end you would have to include bl_common_raw.h, bl_common_asm_raw.h, aarch64/ep_info_raw.h, aarch64/ep_info_asm_raw.h, param_header_raw.h and param_header_asm_raw.h all independently based on what macros are set. Maybe if I only split them up by architecture but not by asm/non-asm, that could work better.
For the assembly, btw, another option would be to use the macro __ASSEMBLER__ instead of __ASSEMBLY__. That is predefined by GCC and most other compilers (according to https://gcc.gnu.org/onlinedocs/cpp/Standard-Predefined-Macros.html#Standard-... it's even part of a standard, although I can't find another source for that), so we could just assume that it will work in third-party projects as well and wouldn't need to do anything special for it. (If we want to do that, I think it would make sense to switch all TF-A code from __ASSEMBLY__ to __ASSEMBLER__ first. Do you know if there was a good reason you went with a custom-defined __ASSEMBLY__ in the first place?)
I think these raw header are worth grouping together as it makes it clear the need to handle them separately and makes it easier to export these headers. A strawman proposal for the header hierarchy :
/include/tf_share --> the ep_info, param_header and other such common header could go in there /include/tf_share/ bl_aux_params --> folder for the new bl_aux param header /include/tf_share/bl_aux_params/vendor/rockchip --> folder for vendor extensions for the rockchip platform
Does that sound sensible ?
Yes, that sounds good. I would maybe suggest using "plat" instead of "vendor" to mirror the way the toplevel directories are structured better, so <share/bl_aux_params/plat/rockchip/plat_params.h> or maybe just <share/plat/rockchip/plat_params.h> instead. But I'm happy to follow whatever naming you prefer.
-----Original Message----- From: Julius Werner jwerner@chromium.org Sent: 28 June 2019 02:01 To: Soby Mathew Soby.Mathew@arm.com Cc: tf-a@lists.trustedfirmware.org; nd nd@arm.com Subject: Re: Header file re-org
Thanks Julius for the inputs. That makes sense. We already have a
precedence for sharing headers between the cross compiler and the host toolchain for the host tools via `tools_share` directory. Similarly we can have a `xxx_share` directory in /include for these headers. I think it is worth factoring out the U() macro to this folder. Regarding the __ASSEMBLY__ and AARCH32/AARCH64 macros, we could split the header out for asm, aarch32, aarch64 variants of the file thus removing the need to define these macros.
I tried factoring them out into different headers and have them include each other at first, but it didn't really work out and became a huge mess because they're so interdependent. entry_point_info_t is an architecture-dependent type that depends on param_header_t (an architecture-independent type), and they both have assembly-only constants associated with them. So in the end you would have to include bl_common_raw.h, bl_common_asm_raw.h, aarch64/ep_info_raw.h, aarch64/ep_info_asm_raw.h, param_header_raw.h and param_header_asm_raw.h all independently based on what macros are set. Maybe if I only split them up by architecture but not by asm/non-asm, that could work better.
Hi Julius Sorry for the delay in response. Yes, that would be good to separate atleast by Architecture but if it turns out to be messy we could keep the headers as they are.
For the assembly, btw, another option would be to use the macro __ASSEMBLER__ instead of __ASSEMBLY__. That is predefined by GCC and most other compilers (according to https://gcc.gnu.org/onlinedocs/cpp/Standard-Predefined- Macros.html#Standard-Predefined-Macros it's even part of a standard, although I can't find another source for that), so we could just assume that it will work in third-party projects as well and wouldn't need to do anything special for it. (If we want to do that, I think it would make sense to switch all TF-A code from __ASSEMBLY__ to __ASSEMBLER__ first. Do you know if there was a good reason you went with a custom-defined __ASSEMBLY__ in the first place?)
Hmm, I not sure for the reason for __ASSEMBLY__ instead of __ASSEMBLER__. I don’t see any reason why we shouldn't migrate to the standard macro. This can be done later.
I think these raw header are worth grouping together as it makes it clear the
need to handle them separately and makes it easier to export these headers. A strawman proposal for the header hierarchy :
/include/tf_share --> the ep_info, param_header and other such common
header could go in there
/include/tf_share/ bl_aux_params --> folder for the new bl_aux param
header
/include/tf_share/bl_aux_params/vendor/rockchip --> folder for vendor
extensions for the rockchip platform
Does that sound sensible ?
Yes, that sounds good. I would maybe suggest using "plat" instead of "vendor" to mirror the way the toplevel directories are structured better, so <share/bl_aux_params/plat/rockchip/plat_params.h> or maybe just <share/plat/rockchip/plat_params.h> instead. But I'm happy to follow whatever naming you prefer.
The most difficult part is finding suitable names 😊. How about `include/bl_handoff/plat` and `include/bl_handoff/bl_aux_params/plat` as the folders ?
Best Regards Soby Mathew
Hmm, I not sure for the reason for __ASSEMBLY__ instead of __ASSEMBLER__. I don’t see any reason why we shouldn't migrate to the standard macro. This can be done later.
Sounds good, will do that (some time next week).
Yes, that sounds good. I would maybe suggest using "plat" instead of "vendor"
to mirror the way the toplevel directories are structured better, so <share/bl_aux_params/plat/rockchip/plat_params.h> or maybe just <share/plat/rockchip/plat_params.h> instead. But I'm happy to follow
whatever
naming you prefer.
The most difficult part is finding suitable names 😊. How about `include/bl_handoff/plat` and `include/bl_handoff/bl_aux_params/plat` as the folders ?
I think it may be better to branch out the platforms right at the top level of the new namespace this is creating (e.g. include/<...>/plat/bl_aux_params, rather than include/<...>/bl_aux_params/plat), since that mirrors what the repository as a whole does more closely. Also, even if the current use case has both common and platform-specific headers, this may be useful for future use cases that are completely platform-specific, and for those having platform-specific top-level directories would work better.
I also think something generic (e.g. like you earlier suggested 'share') rather than something specific to this use case like 'bl_handoff' would be better, because it's more flexible for future use cases that may also want to do something similar. For example, imagine a platform that writes a data structure to memory or flash which is later parsed by a user space utility, and you want the user space utility to include TF's struct definitions for that rather than duplicate them... this has nothing to do with "bl_handoff", but it would still benefit from shareable headers like this.
That's why I suggested to structure it as 'include/share/plat/rockchip/...' and 'include/share/bl_aux_params/...'. Still, happy to use your scheme if you prefer.
Hi Julius OK, I agree using a more generic folder name is better. The suggested folder hierarchy is good. Regarding the name, `share` is a bit too generic IMO. How about the name `export` which brings in a bit more context regarding the contents in the folder ? Best Regards Soby Mathew
From: Julius Werner jwerner@chromium.org Sent: 03 July 2019 00:58 To: Soby Mathew Soby.Mathew@arm.com Cc: Julius Werner jwerner@chromium.org; tf-a@lists.trustedfirmware.org; nd nd@arm.com Subject: Re: Header file re-org
Hmm, I not sure for the reason for __ASSEMBLY__ instead of __ASSEMBLER__. I don’t see any reason why we shouldn't migrate to the standard macro. This can be done later.
Sounds good, will do that (some time next week).
Yes, that sounds good. I would maybe suggest using "plat" instead of "vendor" to mirror the way the toplevel directories are structured better, so <share/bl_aux_params/plat/rockchip/plat_params.h> or maybe just <share/plat/rockchip/plat_params.h> instead. But I'm happy to follow whatever naming you prefer.
The most difficult part is finding suitable names 😊. How about `include/bl_handoff/plat` and `include/bl_handoff/bl_aux_params/plat` as the folders ?
I think it may be better to branch out the platforms right at the top level of the new namespace this is creating (e.g. include/<...>/plat/bl_aux_params, rather than include/<...>/bl_aux_params/plat), since that mirrors what the repository as a whole does more closely. Also, even if the current use case has both common and platform-specific headers, this may be useful for future use cases that are completely platform-specific, and for those having platform-specific top-level directories would work better.
I also think something generic (e.g. like you earlier suggested 'share') rather than something specific to this use case like 'bl_handoff' would be better, because it's more flexible for future use cases that may also want to do something similar. For example, imagine a platform that writes a data structure to memory or flash which is later parsed by a user space utility, and you want the user space utility to include TF's struct definitions for that rather than duplicate them... this has nothing to do with "bl_handoff", but it would still benefit from shareable headers like this.
That's why I suggested to structure it as 'include/share/plat/rockchip/...' and 'include/share/bl_aux_params/...'. Still, happy to use your scheme if you prefer.
Regarding the name, `share` is a bit too generic IMO. How about the name `export` which brings in a bit more context regarding the contents in the folder ?
Yes, `export` is good as well. Uploaded a new version with that: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/1207
I realized that the AARCH32 problem can be solved the same way as the __ASSEMBLY__ problem, because there are also pre-defined macros for architectures (e.g. __aarch64__). I think doing it that way is easier that splitting it up into separate files for a one-line difference. Let me know what you think!
On 10/07/2019 00:15, Julius Werner wrote:
Regarding the name, `share` is a bit too generic IMO. How about the name `export` which brings in a bit more context regarding the contents in the folder ?
Yes, `export` is good as well. Uploaded a new version with that: https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/1207
I realized that the AARCH32 problem can be solved the same way as the __ASSEMBLY__ problem, because there are also pre-defined macros for architectures (e.g. __aarch64__). I think doing it that way is easier that splitting it up into separate files for a one-line difference. Let me know what you think!
Thanks Julius for the patches. When I had checked this sometime ago, there was this confusion whether arm64 or aarch64 will defined various toolchain and the reasoning for a custom one. It seems now that __aarch64__ is the standardized macro for both GCC and LLVM (and armclang as well) which is pretty much all of the toolchains we care about currently. Not sure about the MSFT compiler but TF-A doesn't build using that compiler.
I cannot forsee any potential problem with making the change but would like more feedback regarding this (especially from the platform maintainers). I will try reviewing the patch series during the week.
Best Regards Soby Mathew
tf-a@lists.trustedfirmware.org
-
Julius Werner -
Julius Werner -
Soby Mathew