ArmVirt: move Defines section to new include file

[kraxel]

The [Defines] section must be processed before including MdeLibs.dsc.inc
so the correct stack cookie library can be picked.

The other sections (especially [LibraryClasses]) should be processed
afterwards so ArmVirt.dsc.inc can override the defaults choosen by
MdeLibs.dsc.inc.

Implement that by splitting the ArmVirt.dsc.inc include file into two,
with the new file carrying the [Defines] section.

[samimujawar]

This code change looks good to me. However, I believe you mean that the ArmTrngLib declaration from MdePkg/MdeLibs.dsc.inc would be used, see https://github.com/tianocore/edk2/blob/master/MdePkg/MdeLibs.dsc.inc#L24

I am not sure if the declaration in SecurityPkg/SecurityPkg.dsc should cause an issue.

Can you confirm, please?

[samimujawar]

The commit message may need updating. Can you check please?

[kraxel]

This code change looks good to me. However, I believe you mean that the ArmTrngLib declaration from MdePkg/MdeLibs.dsc.inc would be used, see https://github.com/tianocore/edk2/blob/master/MdePkg/MdeLibs.dsc.inc#L24

I am not sure if the declaration in SecurityPkg/SecurityPkg.dsc should cause an issue.

Can you confirm, please?

Yes, the MdePkg include causing this makes alot more sense. Somehow missed that when grepping the source tree. Updated PR + commit message accordingly.

[pierregondois]

This issue might come from the patch:

• 1f1182c396466300ad6659c42b517542c61706d9 ("ArmVirtPkg: ArmVirtQemu: Add Custom Stack Cookies")

For ARM platforms, there was this patch:
tianocore/edk2-platforms@8e3342a

Ideally I think it would be better to change back the include order of ArmVirtPkg/ArmVirt.dsc.inc and MdePkg/MdeLibs.dsc.inc (to include MdeLibs.dsc.inc first), but given that it will open another issue with the definition of CUSTOM_STACK_CHECK_LIB, maybe your solution would be easier.

[kraxel]

This issue might come from the patch:

* `1f1182c396466300ad6659c42b517542c61706d9 ("ArmVirtPkg: ArmVirtQemu: Add Custom Stack Cookies")`

For ARM platforms, there was this patch: tianocore/edk2-platforms@8e3342a

Ideally I think it would be better to change back the include order of ArmVirtPkg/ArmVirt.dsc.inc and MdePkg/MdeLibs.dsc.inc (to include MdeLibs.dsc.inc first), but given that it will open another issue with the definition of CUSTOM_STACK_CHECK_LIB, maybe your solution would be easier.

The order isn't the problem, but the different section names.

• MdePkg/MdeLibs.dsc.inc uses [LibraryClasses].
• ArmVirtPkg/ArmVirt.dsc.inc uses [LibraryClasses.common]

Apparently LibraryClasses.common has a lower priority.

[pierregondois]

I think they should have the same priority, cf:
https://tianocore-docs.github.io/edk2-DscSpecification/draft/edk2-DscSpecification-draft.pdf

2.6 [LibraryClasses] Section Processing

[...]

The Library Instances will be selected using the following rules to satisfy a library class for each module
listed in the [Components] section (in order of highest precedence):
1. <LibraryClasses> associated with the INF file in the [Components] section
2. [LibraryClasses.$(Arch).$(MODULE_TYPE), LibraryClasses.$(Arch).$(MODULE_TYPE)]
3. [LibraryClasses.$(Arch).$(MODULE_TYPE)]
4. [LibraryClasses.common.$(MODULE_TYPE)]
5. [LibraryClasses.$(Arch)]
6. [LibraryClasses.common] or [LibraryClasses]

When building ArmVirtQemu.dsc, I m able to switch between the ArmTrngLib|BaseArmTrngLibNull by doing the following:

• including MdePkg/MdeLibs.dsc.inc first so it's definition are overwritten
• defining DEFINE CUSTOM_STACK_CHECK_LIB to solve definitions that are created by the include swaps

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 39470ddd42df..8fb65b3f5734 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -52,9 +52,9 @@ [Defines]
 !include NetworkPkg/NetworkDefines.dsc.inc
 
 # This comes before MdeLibs to ensure stack cookie configuration is chosen
-!include ArmVirtPkg/ArmVirt.dsc.inc
 
 !include MdePkg/MdeLibs.dsc.inc
+!include ArmVirtPkg/ArmVirt.dsc.inc
 
 [LibraryClasses.common]
   ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
diff --git a/MdePkg/MdeLibs.dsc.inc b/MdePkg/MdeLibs.dsc.inc
index 57818e249971..5c926bffe1bb 100644
--- a/MdePkg/MdeLibs.dsc.inc
+++ b/MdePkg/MdeLibs.dsc.inc
@@ -16,7 +16,7 @@ [Defines]
   # The DSC parser will set any unset macros to 0. Then, below when we check for STATIC or DYNAMIC, even if we couch
   # that in a !ifdef CUSTOM_STACK_CHECK_LIB, the parser will issue a warning that we are comparing a boolean (0) against
   # a string, which will always fail. So we set it to a dummy value here.
-  DEFINE CUSTOM_STACK_CHECK_LIB = NONE
+  DEFINE CUSTOM_STACK_CHECK_LIB = DYNAMIC
 !endif
 
 [LibraryClasses]

[kraxel]

When building ArmVirtQemu.dsc, I m able to switch between the ArmTrngLib|BaseArmTrngLibNull by doing the following:

* including `MdePkg/MdeLibs.dsc.inc` first so it's definition are overwritten
* defining `DEFINE CUSTOM_STACK_CHECK_LIB` to solve definitions that are created by the include swaps

Ok. So I guess the options are:

• go with this (slightly hackish) patch.
• split ArmVirt.dsc.inc file into two (separate the [Defines] section), then go include one before and one after MdeLibs.dsc.inc.
• something else?

[pierregondois]

When building ArmVirtQemu.dsc, I m able to switch between the ArmTrngLib|BaseArmTrngLibNull by doing the following:

* including `MdePkg/MdeLibs.dsc.inc` first so it's definition are overwritten
* defining `DEFINE CUSTOM_STACK_CHECK_LIB` to solve definitions that are created by the include swaps

Ok. So I guess the options are:

* go with this (slightly hackish) patch.

* split `ArmVirt.dsc.inc` file into two (separate the `[Defines]` section), then go include one before and one after `MdeLibs.dsc.inc`.

* something else?

I guess so. I think that in each dsc file of ArmVirtPkg impacted by commit 1f1182c, placing:

# Dynamic stack cookies are not supported on ARM
!if $(ARCH) == ARM
  DEFINE CUSTOM_STACK_CHECK_LIB = STATIC
!else
  DEFINE CUSTOM_STACK_CHECK_LIB = DYNAMIC
!endif

before including MdePkg/MdeLibs.dsc.inc should also work and avoid to create another .dsc.inc file.

[kraxel]

I guess so. I think that in each dsc file of ArmVirtPkg impacted by commit 1f1182c, placing:

# Dynamic stack cookies are not supported on ARM
!if $(ARCH) == ARM
  DEFINE CUSTOM_STACK_CHECK_LIB = STATIC
!else
  DEFINE CUSTOM_STACK_CHECK_LIB = DYNAMIC
!endif

before including MdePkg/MdeLibs.dsc.inc should also work and avoid to create another .dsc.inc file.

We have five files including ArmVirt.dsc.inc, so I think I prefer a second include file over duplicating the block above five times.

[ardbiesheuvel]

I guess so. I think that in each dsc file of ArmVirtPkg impacted by commit 1f1182c, placing:

# Dynamic stack cookies are not supported on ARM
!if $(ARCH) == ARM
  DEFINE CUSTOM_STACK_CHECK_LIB = STATIC
!else
  DEFINE CUSTOM_STACK_CHECK_LIB = DYNAMIC
!endif

before including MdePkg/MdeLibs.dsc.inc should also work and avoid to create another .dsc.inc file.

We have five files including ArmVirt.dsc.inc, so I think I prefer a second include file over duplicating the block above five times.

Can we fix the MdeLibs include file instead? Apparently, setting this to DYNAMIC for ARM never makes sense, right?

[kraxel]

I guess so. I think that in each dsc file of ArmVirtPkg impacted by commit 1f1182c, placing:

# Dynamic stack cookies are not supported on ARM
!if $(ARCH) == ARM
  DEFINE CUSTOM_STACK_CHECK_LIB = STATIC
!else
  DEFINE CUSTOM_STACK_CHECK_LIB = DYNAMIC
!endif

before including MdePkg/MdeLibs.dsc.inc should also work and avoid to create another .dsc.inc file.

We have five files including ArmVirt.dsc.inc, so I think I prefer a second include file over duplicating the block above five times.

Can we fix the MdeLibs include file instead? Apparently, setting this to DYNAMIC for ARM never makes sense, right?

Not sure what exactly you have in mind. It's an opt-in feature, in case CUSTOM_STACK_CHECK_LIB is not set MdePkg will use NONE, so platforms have to choose something else before including MdeLibs.

[pierregondois]

I guess so. I think that in each dsc file of ArmVirtPkg impacted by commit 1f1182c, placing:

# Dynamic stack cookies are not supported on ARM
!if $(ARCH) == ARM
  DEFINE CUSTOM_STACK_CHECK_LIB = STATIC
!else
  DEFINE CUSTOM_STACK_CHECK_LIB = DYNAMIC
!endif

before including MdePkg/MdeLibs.dsc.inc should also work and avoid to create another .dsc.inc file.

We have five files including ArmVirt.dsc.inc, so I think I prefer a second include file over duplicating the block above five times.

Can we fix the MdeLibs include file instead? Apparently, setting this to DYNAMIC for ARM never makes sense, right?

I'm not sure I understand either.
Otherwise the PR looks good to me