Re: [edk2] [Patch v3 23/23] OvmfPkg: Use MdeModulePkg/BDS

2016-04-28 Thread Laszlo Ersek 
On 04/21/16 08:58, Ruiyu Ni wrote:
> By default the new MdeModulePkg/BDS is used.
> If USE_OLD_BDS is defined to TRUE, IntelFrameworkModulePkg/BDS
> is used.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni 
> Cc: Jordan Justen 
> Cc: Laszlo Ersek 
> ---
>  OvmfPkg/OvmfPkgIa32.dsc| 45 ++--
>  OvmfPkg/OvmfPkgIa32.fdf|  5 +
>  OvmfPkg/OvmfPkgIa32X64.dsc | 47 
> +++---
>  OvmfPkg/OvmfPkgIa32X64.fdf |  5 +
>  OvmfPkg/OvmfPkgX64.dsc | 45 ++--
>  OvmfPkg/OvmfPkgX64.fdf |  5 +
>  6 files changed, 133 insertions(+), 19 deletions(-)

This patch looks good. It is complex, but it can be followed.

I'm requesting a few (cosmetic) changes:

> @@ -551,13 +566,32 @@ [Components]
>PcAtChipsetPkg/KbcResetDxe/Reset.inf
>MdeModulePkg/Universal/Metronome/Metronome.inf
>PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf
> +!if $(USE_OLD_BDS) == TRUE
>IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf {
>  
> -!ifdef $(CSM_ENABLE)
> +  !ifdef $(CSM_ENABLE)
>NULL|OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf
> -!endif
> +  !endif
>}

Looking at the DSC files, it seems we don't indent nested "!if"
directives at all. For consistency with the rest of the DSC files, can
we please drop the re-indentation? (For all three DSC files.)

> -
> +!else
> +  MdeModulePkg/Universal/BdsDxe/BdsDxe.inf {
> +
> +  !ifdef $(CSM_ENABLE)

Same here -- please don't indent the !ifdef.

> +  NULL|OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf
> +  
> NULL|IntelFrameworkModulePkg/Library/LegacyBootManagerLib/LegacyBootManagerLib.inf
> +  !endif
> +  }
> +  MdeModulePkg/Application/UiApp/UiApp.inf {
> +
> +  NULL|MdeModulePkg/Library/DeviceManagerUiLib/DeviceManagerUiLib.inf
> +  NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf
> +  
> NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceManagerUiLib.inf
> +  !ifdef $(CSM_ENABLE)

Ditto.

> +  
> NULL|IntelFrameworkModulePkg/Library/LegacyBootManagerLib/LegacyBootManagerLib.inf
> +  
> NULL|IntelFrameworkModulePkg/Library/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf
> +  !endif
> +  }
> +!endif
>OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf
>OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
>OvmfPkg/Virtio10Dxe/Virtio10.inf

[snip]

For the OvmfPkgIa32X64.dsc file specifically:

> @@ -417,11 +429,11 @@ [PcdsFixedAtBuild]
>gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>  !endif
>  
> +[PcdsFixedAtBuild.X64]
>  !ifndef $(USE_OLD_SHELL)
>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 
> 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
>  !endif
>  
> -[PcdsFixedAtBuild.X64]
>  !if $(SMM_REQUIRE) == TRUE
>gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
>gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|10

I agree that PcdShellFile should be set in the [PcdsFixedAtBuild.X64]
section, and not the [PcdsFixedAtBuild] section.

Can you please factor out this change to a separate patch?


Thanks a lot for this work, Ray; I think it's really good, and I look
forward to using the new BDS driver + UI app.

I think this nice separation of patches will also help us much in
porting ArmVirtPkg too.


As discussed before (under patch v3 11/23), please wait with version 4,
until my short series

  [edk2] [PATCH 0/3] OvmfPkg: tighten the SMM lockdown

is committed; your v4 should be based upon it.

Cheers!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch v3 23/23] OvmfPkg: Use MdeModulePkg/BDS

2016-04-21 Thread Ruiyu Ni 
By default the new MdeModulePkg/BDS is used.
If USE_OLD_BDS is defined to TRUE, IntelFrameworkModulePkg/BDS
is used.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Cc: Jordan Justen 
Cc: Laszlo Ersek 
---
 OvmfPkg/OvmfPkgIa32.dsc| 45 ++--
 OvmfPkg/OvmfPkgIa32.fdf|  5 +
 OvmfPkg/OvmfPkgIa32X64.dsc | 47 +++---
 OvmfPkg/OvmfPkgIa32X64.fdf |  5 +
 OvmfPkg/OvmfPkgX64.dsc | 45 ++--
 OvmfPkg/OvmfPkgX64.fdf |  5 +
 6 files changed, 133 insertions(+), 19 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 0206dda..d9e9321 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -37,6 +37,7 @@ [Defines]
   DEFINE NETWORK_IP6_ENABLE  = FALSE
   DEFINE HTTP_BOOT_ENABLE= FALSE
   DEFINE SMM_REQUIRE = FALSE
+  DEFINE USE_OLD_BDS = FALSE
 
 [BuildOptions]
   GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
@@ -75,7 +76,13 @@ [LibraryClasses]
   
UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
   
UefiHiiServicesLib|MdeModulePkg/Library/UefiHiiServicesLib/UefiHiiServicesLib.inf
   HiiLib|MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf
+  SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
+!if $(USE_OLD_BDS) == TRUE
   GenericBdsLib|IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
+!else
+  
UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
+!endif
+  FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
   CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
   DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
   
DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
@@ -275,7 +282,13 @@ [LibraryClasses.common.DXE_DRIVER]
   IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf
   UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf
   DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf
+!if $(USE_OLD_BDS) == TRUE
   PlatformBdsLib|OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf
+  QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
+!else
+  
PlatformBootManagerLib|OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+  QemuBootOrderLib|OvmfPkg/Library/QemuNewBootOrderLib/QemuBootOrderLib.inf
+!endif
   
CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
 !if $(SMM_REQUIRE) == TRUE
   LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf
@@ -285,8 +298,6 @@ [LibraryClasses.common.DXE_DRIVER]
 !ifdef $(SOURCE_DEBUG_ENABLE)
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
 !endif
-  QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
-  FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
 
 [LibraryClasses.common.UEFI_APPLICATION]
@@ -294,6 +305,7 @@ [LibraryClasses.common.UEFI_APPLICATION]
   TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
+  
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
 !ifdef $(DEBUG_ON_SERIAL_PORT)
   DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
 !else
@@ -428,6 +440,9 @@ [PcdsFixedAtBuild]
   # IRQs 5, 9, 10, 11 are level-triggered
   gPcAtChipsetPkgTokenSpaceGuid.Pcd8259LegacyModeEdgeLevel|0x0E20
 
+  # Point to the MdeModulePkg/Application/UiApp/UiApp.inf
+  gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 
0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }
+
 

 #
 # Pcd Dynamic Section - list of all EDK II PCD Entries defined by this Platform
@@ -551,13 +566,32 @@ [Components]
   PcAtChipsetPkg/KbcResetDxe/Reset.inf
   MdeModulePkg/Universal/Metronome/Metronome.inf
   PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf
+!if $(USE_OLD_BDS) == TRUE
   IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf {
 
-!ifdef $(CSM_ENABLE)
+  !ifdef $(CSM_ENABLE)
   NULL|OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf
-!endif
+  !endif
   }
-
+!else
+  MdeModulePkg/Universal/BdsDxe/BdsDxe.inf {
+
+  !ifdef $(CSM_ENABLE)
+  NULL|OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf
+  
NULL|IntelFrameworkModulePkg/Library/LegacyBootManagerLib/LegacyBootManagerLib.inf
+  !endif
+  }
+  MdeModulePkg/Application/UiApp/UiApp.inf {
+
+  NULL|MdeModulePkg/Library/DeviceManagerUiLib/DeviceManagerUiLib.inf
+  NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf
+