Global: Fix the issue that Depex sections exist in the INF files of UEFI_DRIVER or UEFI_APPLICATION

[EricGao2015]

Description

Fixes #12276

The INF specification said:

If the module is not a library (no LIBRARY_CLASS in the [Defines] section) and the MODULE_TYPE is SEC, SMM_CORE, DXE_CORE, PEI_CORE, UEFI_DRIVER, UEFI_APPLICATION or HOST_APPLICATION a Depex section is not permitted.

Thus, this series of patches does the following:

• Remove improper Depex section in UEFI_DRIVER and UEFI_APPLICATION. If the driver really depends on certain protocol, use notify function to handle the dependency.

• Adds a check when retrieving dependency expression in BaseTools. If the module has [Depex] section but it's not permitted by INF specification, just throw warning message t instead of break the building proccess.

• Breaking change?

  • Breaking change - Does this PR cause a break in build or boot behavior?
  • Examples: Does it add a new library class or move a module to a different repo.

• Impacts security?

  • Security - Does this PR have a direct security impact?
  • Examples: Crypto algorithm change or buffer overflow fix.

• Includes tests?

  • Tests - Does this PR include any explicit test code?
  • Examples: Unit tests or integration tests.

How This Was Tested

• github CI check is passed.
• Build every platform, check if there is building failure. Run the following commands for instance:
  build -a X64 -t GCC -p RedfishPkg/RedfishPkg.dsc
build -a X64 -t GCC -p NetworkPkg/NetworkPkg.dsc
build -a X64 -t GCC -p MdeModulePkg/MdeModulePkg.dsc
build -a X64 -t GCC -p MdePkg/MdePkg.dsc
build -a X64 -t GCC -p ShellPkg/ShellPkg.dsc
...
• Add [Depex] in UEFI_DRIVER or UEFI_APPLICATION, building process will not be breaken in BaseTools and a warning message will be throwed when build the driver:
  [Depex] section is not permitted for UEFI_DRIVER/UEFI_APPLICATION module

Integration Instructions

Once the PR is merged, all UEFI drivers and uefi applications with [Depex] section will not be blocked by BaseTools when building but only warning message is throwed. If the driver is UEFI driver, remove Depex from INF file and check dependent protocol in driver binding Start() function or notify function if necessary.

[tianocore-assign-reviewers]

⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.

Non-collaborators requested:

• @richardho

Attn Admins:

• @jljusten
• @ardbiesheuvel
• @lgao4
• @mdkinney
• @makubacki
• @jkmathews

---

Admin Instructions:

• Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
• If they are no longer needed as reviewers, remove them from Maintainers.txt

[leiflindholm]

Since it's a breaking change, PR description should have Integration Instructions.

[mdkinney]

I think the intent of all the network stack drivers is to be UEFI Drivers. Converting to DXE_DRIVER does not lool right. I think the bug may be that there is a depex for gEfiHash2ServiceBindingProtocolGuid. There are ways for a UEFI Driver to be notified when a dependent protocol is available or it can fail a driver binding start if its dependent services are not available.

[mdkinney]

Same comment as TcpDxe. Need input from @AbnerChang to know if this is intended to be a UEFI Driver or if it should be a DXE Driver. If UEFI Driver, then Depex need to be removed and instead, the driver implementation needs to handle cases where dependent services are available or not.

[EricGao2015]

Same comment as TcpDxe. Need input from @AbnerChang to know if this is intended to be a UEFI Driver or if it should be a DXE Driver. If UEFI Driver, then Depex need to be removed and instead, the driver implementation needs to handle cases where dependent services are available or not.

@changab Hi Abner, please help to confirm this issue, thanks.

[changab]

This is an UEFI driver, we have to remove the DEPEX. Let me confirm this.

[EricGao2015]

This is an UEFI driver, we have to remove the DEPEX. Let me confirm this.

Hi Anber, I have removed Depex from RedfishConfigHandler driver and use notify function to handle the dependency. Please review it again. Thank you.

[changab]

@EricGao2015, I didn't know you were working on this. Please refer to #12395, I have tested my fix on AMD platform. We don't need the notification for this driver, because the Redfish credential driver is a DXE driver. The Redfish credential must be ready before the ConnectController(). You can update your PR, remove the changes for Redfish, and resend the PR once 12395 gets merged. Thanks.

[EricGao2015]

Since it's a breaking change, PR description should have Integration Instructions.

Updated, please check again. It's my understand for Integration Instructions.

[niruiyu]

I think it may cause combability issue as some downstream usages may already there.

[EricGao2015]

I think it may cause combability issue as some downstream usages may already there.

Yes, this is also the aspect that I conern about. This issue has persisted for more than a decade, the PR will cause breaking change to downstream. So I would like to know the opinions of all stewards and reviewers.

[mdkinney]

I have 2 open comments above that need to be addressed before this PR can be completed.

For the INFs that have a [Depex] of TRUE for module types that do not support [Depex]. I agree those are incorrect, but are effectively harmless. For example a UEFI Driver has an implied depex of all PI Arch Protocols. ANDing in an additional TRUE does not change the depex. I would like to see those cleaned up. So the patch to remove [Depex] TRUE for those cases looks correct.

The BaseTools change to raise this to an error may be good long term. Short term, raising it to a WARNING so downstream consumers can have a chance to clean up the warnings is likely a better first step. The challenge is deciding when it can be promoted from a WARNING to an ERROR.

[EricGao2015]

I have 2 open comments above that need to be addressed before this PR can be completed.

I will handle the 2 comments after getting Anber's ack.

For the INFs that have a [Depex] of TRUE for module types that do not support [Depex]. I agree those are incorrect, but are effectively harmless. For example a UEFI Driver has an implied depex of all PI Arch Protocols. ANDing in an additional TRUE does not change the depex. I would like to see those cleaned up. So the patch to remove [Depex] TRUE for those cases looks correct.

The BaseTools change to raise this to an error may be good long term. Short term, raising it to a WARNING so downstream consumers can have a chance to clean up the warnings is likely a better first step. The challenge is deciding when it can be promoted from a WARNING to an ERROR.

I agree with the gentle approach that raises warning instead of error. I have changed the print level form EdkLogger.error() to EdkLogger.warn(), so that the building will not be breaken.

[felixpgithub]

The PR does not produce warnings on transitive dependencies (UEF driver uses a library instance with [Depex]). Is this intentional?

[EricGao2015]

The PR does not produce warnings on transitive dependencies (UEF driver uses a library instance with [Depex]). Is this intentional?

Yes. This patch only handles the case that Depex in INF file which MODULE_TYPE is UEFI_DRIVER or UEFI_APPLICATION, because this constitutes a clear violation of the INF specification.
I think the case you mentioned is valid because the INF specification does not forbid this behavior. Actually, if transitive dependencies really exist in UEFI_DRIVER, the driver may be treated as a DXE_DRIVER in BaseTools AutoGen code:

edk2/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py

Line 1342 in b7cf7e4

ModuleType = SUP_MODULE_DXE_DRIVER if self.ModuleType == SUP_MODULE_UEFI_DRIVER and self.DepexGenerated else self.ModuleType

One example is: NetworkPkg/Ip4Dxe/Ip4Dxe.inf is a UEFI_DRIVER without Depex, but this driver uses IoLib(NetworkPkg/Library/DxeNetLib/DxeNetLib.inf) which has dependency on gEfiRngProtocolGuid. After processing by AutoGen, the MODULE_TYPE becomes DXE_DRIVER in Build/NetworkPkg/DEBUG_GCC/X64/NetworkPkg/Ip4Dxe/Ip4Dxe/OUTPUT/Ip4Dxe.inf.

[mdkinney]

The PR does not produce warnings on transitive dependencies (UEF driver uses a library instance with [Depex]). Is this intentional?

Yes. This patch only handles the case that Depex in INF file which MODULE_TYPE is UEFI_DRIVER or UEFI_APPLICATION, because this constitutes a clear violation of the INF specification. I think the case you mentioned is valid because the INF specification does not forbid this behavior. Actually, if transitive dependencies really exist in UEFI_DRIVER, the driver may be treated as a DXE_DRIVER in BaseTools AutoGen code:

edk2/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py

Line 1342 in b7cf7e4

ModuleType = SUP_MODULE_DXE_DRIVER if self.ModuleType == SUP_MODULE_UEFI_DRIVER and self.DepexGenerated else self.ModuleType

One example is: NetworkPkg/Ip4Dxe/Ip4Dxe.inf is a UEFI_DRIVER without Depex, but this driver uses IoLib(NetworkPkg/Library/DxeNetLib/DxeNetLib.inf) which has dependency on gEfiRngProtocolGuid. After processing by AutoGen, the MODULE_TYPE becomes DXE_DRIVER in Build/NetworkPkg/DEBUG_GCC/X64/NetworkPkg/Ip4Dxe/Ip4Dxe/OUTPUT/Ip4Dxe.inf.

Library INFs that support multiple module types and Depex need to make sure their [Depex] section is correct. The [Depex] section does allow syntax to provide a module type specific depex such as:

[Depex.common.DXE_DRIVER, Depex.common.DXE_RUNTIME_DRIVER, Depex.common.DXE_SMM_DRIVER]
  gEfiFileExplorerProtocolGuid

The DxeNetLib lists the following module compatibiliy:

  LIBRARY_CLASS                  = NetLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER

But has a depex section that is applied to all module types the library supports

[Depex]
  gEfiRngProtocolGuid

So either DxeNetLib is listing extra module types that need to be removed or the [Depex] statement needs to be updated. I do not think that DXE_CORE or DXE_RUNTIME_DRIVER or DXE_SMM_DRIVER make much sense for this library. It would help to remove ones that can never be used.

Also, the dependency on The RNG Protocol can be handled differently for DXE_DRIVER and UEFI_DRIVER and UEFI_APPLICATION. For UEFI_DRIVER and UEFI_APPLICATION, the depex must be empty and the constructor can look for the RNG Protocol or use a RegisterProtocolNotify(). Only DXE_DRIVER can use a direct [Depex] on RNG Protocol. The most compatible lib would be to remove the [Depex] all together and use RegisterProtocolNotify() and have services that do not work until the RNG Protocol is available.

In general, the mix of UEFI_DRIVER and DXE_DRIVER in the network stack does not make sense. The network stack should be pure UEFI Drivers that can be loaded from Boot Manager or the Shell as well as embedded in platform FW.

[EricGao2015]

The PR does not produce warnings on transitive dependencies (UEF driver uses a library instance with [Depex]). Is this intentional?

Yes. This patch only handles the case that Depex in INF file which MODULE_TYPE is UEFI_DRIVER or UEFI_APPLICATION, because this constitutes a clear violation of the INF specification. I think the case you mentioned is valid because the INF specification does not forbid this behavior. Actually, if transitive dependencies really exist in UEFI_DRIVER, the driver may be treated as a DXE_DRIVER in BaseTools AutoGen code:

edk2/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py

Line 1342 in b7cf7e4

ModuleType = SUP_MODULE_DXE_DRIVER if self.ModuleType == SUP_MODULE_UEFI_DRIVER and self.DepexGenerated else self.ModuleType

One example is: NetworkPkg/Ip4Dxe/Ip4Dxe.inf is a UEFI_DRIVER without Depex, but this driver uses IoLib(NetworkPkg/Library/DxeNetLib/DxeNetLib.inf) which has dependency on gEfiRngProtocolGuid. After processing by AutoGen, the MODULE_TYPE becomes DXE_DRIVER in Build/NetworkPkg/DEBUG_GCC/X64/NetworkPkg/Ip4Dxe/Ip4Dxe/OUTPUT/Ip4Dxe.inf.

Library INFs that support multiple module types and Depex need to make sure their [Depex] section is correct. The [Depex] section does allow syntax to provide a module type specific depex such as:

[Depex.common.DXE_DRIVER, Depex.common.DXE_RUNTIME_DRIVER, Depex.common.DXE_SMM_DRIVER]
  gEfiFileExplorerProtocolGuid

The DxeNetLib lists the following module compatibiliy:

  LIBRARY_CLASS                  = NetLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER

But has a depex section that is applied to all module types the library supports

[Depex]
  gEfiRngProtocolGuid

So either DxeNetLib is listing extra module types that need to be removed or the [Depex] statement needs to be updated. I do not think that DXE_CORE or DXE_RUNTIME_DRIVER or DXE_SMM_DRIVER make much sense for this library. It would help to remove ones that can never be used.

Also, the dependency on The RNG Protocol can be handled differently for DXE_DRIVER and UEFI_DRIVER and UEFI_APPLICATION. For UEFI_DRIVER and UEFI_APPLICATION, the depex must be empty and the constructor can look for the RNG Protocol or use a RegisterProtocolNotify(). Only DXE_DRIVER can use a direct [Depex] on RNG Protocol. The most compatible lib would be to remove the [Depex] all together and use RegisterProtocolNotify() and have services that do not work until the RNG Protocol is available.

In general, the mix of UEFI_DRIVER and DXE_DRIVER in the network stack does not make sense. The network stack should be pure UEFI Drivers that can be loaded from Boot Manager or the Shell as well as embedded in platform FW.

Thanks for your explanation, I understood your thought. The DxeNetLib issue is different with the subject of this PR, so I'll submit another PR to fix the issue later.

[mikebeaton]

In general, the mix of UEFI_DRIVER and DXE_DRIVER in the network stack does not make sense. The network stack should be pure UEFI Drivers that can be loaded from Boot Manager or the Shell as well as embedded in platform FW.

@mdkinney - Hi. I'm not sure what should happen, but if not for your comment I would have said I am almost certain that DXE Drivers can be loaded from the Shell. A quick Google seems to confirm, though I haven't had a chance to retest what I thought I knew.