Re: [RFC PATCH 6/6] OvmfPkg: enable AppleSupport library for Ovmf firmware

[Laszlo Ersek <lersek@redhat.com>]

On 03/07/17 04:14, Gabriel L. Somlo wrote:
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de>
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> ---
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c           | 10 ++++++++++
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h           |  1 +
>  .../Library/PlatformBootManagerLib/PlatformBootManagerLib.inf  |  1 +
>  OvmfPkg/OvmfPkgIa32.dsc                                        |  8 ++++++++
>  OvmfPkg/OvmfPkgIa32.fdf                                        |  3 +++
>  OvmfPkg/OvmfPkgIa32X64.dsc                                     |  8 ++++++++
>  OvmfPkg/OvmfPkgIa32X64.fdf                                     |  3 +++
>  OvmfPkg/OvmfPkgX64.dsc                                         |  8 ++++++++
>  OvmfPkg/OvmfPkgX64.fdf                                         |  3 +++

We usually split the DSC/FDF changes from module changes. In some cases
it might mean splitting a patch like this into three: first add the
library resolutions (DSC), then modify the code, then include further
driver modules (DSC/FDF).

>  9 files changed, 45 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index cc35630..9f6be90 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -381,6 +381,11 @@ Returns:
>    }
>  
>    //
> +  // Initialize AppleSupport library
> +  //
> +  InitializeAppleSupport (gImageHandle, gST);
> +
> +  //
>    // Prevent further changes to LockBoxes or SMRAM.
>    //
>    Handle = NULL;
> @@ -1474,6 +1479,11 @@ Routine Description:
>  
>    RemoveStaleFvFileOptions ();
>    SetBootOrderFromQemu ();
> +
> +  //
> +  // Locate and launch Apple's OS X bootloader
> +  //
> +  BdsBootApple ();
>  }

So, points I mentioned earlier:
- not sure why BdsBootApple() is needed,
- whatever calls we add here, they should depend on the new FeaturePCD.

>  
>  /**
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> index ec58efa..3fa7712 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> @@ -49,6 +49,7 @@ Abstract:
>  #include <Library/NvVarsFileLib.h>
>  #include <Library/QemuFwCfgLib.h>
>  #include <Library/QemuBootOrderLib.h>
> +#include <Library/AppleSupportLib.h>
>  
>  #include <Protocol/Decompress.h>
>  #include <Protocol/PciIo.h>
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index f9e35c9..66d31a5 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -40,6 +40,7 @@
>    OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
> +  AppleSupportLib
>    BaseLib
>    MemoryAllocationLib
>    UefiBootServicesTableLib

Please configure your git setup to include the INF/DSC/FDF/DEC section
names in the diff hunk headers. For that, please refer to:

<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05>

(specifically the "xfuncname" setting), and to

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09

> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 0bce56b..da83cba 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -176,6 +176,8 @@
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
>  
> +  AppleSupportLib|OvmfPkg/Library/AppleSupportLib/AppleSupportLib.inf
> +

Any reason to keep an empty line between XenHypercallLib and
AppleSupportLib?

>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>  
> @@ -696,6 +698,12 @@
>    MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>  
>    #
> +  # Apple Support
> +  #
> +  OvmfPkg/FswHfsPlus/FswHfsPlus.inf
> +  IntelFrameworkModulePkg/Universal/DataHubDxe/DataHubDxe.inf
> +

Should depend on the new build flag.

> +  #
>    # Network Support
>    #
>    MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index 09c1658..0e00bd9 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -280,6 +280,9 @@ INF  MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
>  INF  MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
>  INF  MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>  
> +INF  OvmfPkg/FswHfsPlus/FswHfsPlus.inf
> +INF  IntelFrameworkModulePkg/Universal/DataHubDxe/DataHubDxe.inf
> +

Should depend on the new build flag, plus a leading comment (since we're
adding one to the DSC too) would be welcome.

Stopping here.

Thanks
Laszlo

>  INF  FatPkg/EnhancedFatDxe/Fat.inf
>  
>  !ifndef $(USE_OLD_SHELL)
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 56f7ff9..a38dc9b 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -181,6 +181,8 @@
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
>  
> +  AppleSupportLib|OvmfPkg/Library/AppleSupportLib/AppleSupportLib.inf
> +
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>  
> @@ -705,6 +707,12 @@
>    MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>  
>    #
> +  # Apple Support
> +  #
> +  OvmfPkg/FswHfsPlus/FswHfsPlus.inf
> +  IntelFrameworkModulePkg/Universal/DataHubDxe/DataHubDxe.inf
> +
> +  #
>    # Network Support
>    #
>    MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index 5233314..2bd2d77 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -280,6 +280,9 @@ INF  MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
>  INF  MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
>  INF  MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>  
> +INF  OvmfPkg/FswHfsPlus/FswHfsPlus.inf
> +INF  IntelFrameworkModulePkg/Universal/DataHubDxe/DataHubDxe.inf
> +
>  INF  FatPkg/EnhancedFatDxe/Fat.inf
>  
>  !ifndef $(USE_OLD_SHELL)
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index d0b0b0e..2bff68d 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -181,6 +181,8 @@
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
>  
> +  AppleSupportLib|OvmfPkg/Library/AppleSupportLib/AppleSupportLib.inf
> +
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>  
> @@ -703,6 +705,12 @@
>    MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>  
>    #
> +  # Apple Support
> +  #
> +  OvmfPkg/FswHfsPlus/FswHfsPlus.inf
> +  IntelFrameworkModulePkg/Universal/DataHubDxe/DataHubDxe.inf
> +
> +  #
>    # Network Support
>    #
>    MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index 3615010..0165a63 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -280,6 +280,9 @@ INF  MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
>  INF  MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
>  INF  MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>  
> +INF  OvmfPkg/FswHfsPlus/FswHfsPlus.inf
> +INF  IntelFrameworkModulePkg/Universal/DataHubDxe/DataHubDxe.inf
> +
>  INF  FatPkg/EnhancedFatDxe/Fat.inf
>  
>  !ifndef $(USE_OLD_SHELL)
>