[PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[Laszlo Ersek]

(Copying Rich, Xiang and Gabriel for testing requests below.)

Repo:   https://github.com/lersek/edk2.git
Branch: kernel_before_bootdevs

After the recent series "OvmfPkg, ArmVirtQemu: leaner platform BDS
policy for connecting devices", I'm picking up another earlier idea -- a
direct kernel boot does not need devices such as disks and NICs to be
bound by UEFI.

I tested this series extensively on QEMU, in OVMF (IA32X64) and
ArmVirtQemu (AARCH64), both with and without direct kernel boot. I
compared the logs in all sensible relations within a given architecture.

Rich, can you please test this on ARM64, with guestfish/libguestfs?
Please attach a good number of disks at once on the command line, and
compare the appliance's boot time between (e.g.) RHEL7's
"/usr/share/AAVMF/AAVMF_CODE.fd" and the following binary (after
decompression):

  https://people.redhat.com/lersek/kernel_before_bootdevs-991e2f2f-64cf-4566-b933-919928e2aa6b/QEMU_EFI.fd.padded.xz

(That binary corresponds to the branch linked above, cross-built from
x86_64 with "aarch64-linux-gnu-gcc (GCC) 6.1.1 20160621 (Red Hat Cross
6.1.1-2)", using the following options:

  export GCC5_AARCH64_PREFIX=aarch64-linux-gnu-
  build --cmd-len=65536 --hash -t GCC5 -b DEBUG -a AARCH64 \
    -p ArmVirtPkg/ArmVirtQemu.dsc -D DEBUG_PRINT_ERROR_LEVEL=0x80000000

and then it was padded with zeroes to 64MB.)

If you have good results, please respond with your Tested-by (which I'll
apply to patch 1/5, since that's the one that matters for the ARM64
target).

Xiang, if you use guestfish (or else direct kernel boot) occasionally,
then similar testing would be very welcome from your side too.

Gabriel, I'm CC'ing you on patch 4/5, because it affects code that you
had originally written. Can you please regression-test this series with
your usual OVMF environment / guest(s)?

Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: "Richard W.M. Jones" <rjones@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Xiang Zheng <xiang.zheng@linaro.org>

Thanks everyone!
Laszlo

Laszlo Ersek (5):
  ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot
    devices"
  OvmfPkg/PlatformBootManagerLib: wrap overlong lines in "BdsPlatform.c"
  OvmfPkg/PlatformBootManagerLib: rejuvenate old-style function comments
  OvmfPkg/PlatformBootManagerLib: hoist PciAcpiInitialization()
  OvmfPkg/PlatformBootManagerLib: process "-kernel" before boot devices

 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c |  16 +-
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c   | 262 ++++++++++----------
 2 files changed, 136 insertions(+), 142 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 1/5] ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot devices"

[Laszlo Ersek]

Move the TryRunningQemuKernel() call back to its original place. This
improves the UEFI boot time for VMs that have "-kernel", many disks or
NICs, and no "bootindex" properties. A well-known example is
guestfish/libguestfs.

For more info on the TryRunningQemuKernel() location, see the following
commits: 23d04b58e27b, a78c4836ea0b, 158990b941e4.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Xiang Zheng <xiang.zheng@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 36e0eed2384a..5d5e51d8c870 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -667,31 +667,31 @@ PlatformBootManagerAfterConsole (
 
   //
   // Show the splash screen.
   //
   BootLogoEnableLogo ();
 
+  //
+  // Process QEMU's -kernel command line option. The kernel booted this way
+  // will receive ACPI tables: in PlatformBootManagerBeforeConsole(), we
+  // connected any and all PCI root bridges, and then signaled the ACPI
+  // platform driver.
+  //
+  TryRunningQemuKernel ();
+
   //
   // Connect the purported boot devices.
   //
   Status = ConnectDevicesFromQemu ();
   if (RETURN_ERROR (Status)) {
     //
     // Connect the rest of the devices.
     //
     EfiBootManagerConnectAll ();
   }
 
-  //
-  // Process QEMU's -kernel command line option. Note that the kernel booted
-  // this way should receive ACPI tables, which is why we connect all devices
-  // first (see above) -- PCI enumeration blocks ACPI table installation, if
-  // there is a PCI host.
-  //
-  TryRunningQemuKernel ();
-
   //
   // Enumerate all possible boot options, then filter and reorder them based on
   // the QEMU configuration.
   //
   EfiBootManagerRefreshAllBootOption ();
 
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 2/5] OvmfPkg/PlatformBootManagerLib: wrap overlong lines in "BdsPlatform.c"

[Laszlo Ersek]

No functional changes.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 79 +++++++++++++-------
 1 file changed, 51 insertions(+), 28 deletions(-)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 5ef0e828900d..99b7db7cc05a 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -1,17 +1,17 @@
 /** @file
   Platform BDS customizations.
 
   Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.<BR>
-  This program and the accompanying materials
-  are licensed and made available under the terms and conditions of the BSD License
-  which accompanies this distribution.  The full text of the license may be found at
+  This program and the accompanying materials are licensed and made available
+  under the terms and conditions of the BSD License which accompanies this
+  distribution.  The full text of the license may be found at
   http://opensource.org/licenses/bsd-license.php
 
-  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
 **/
 
 #include "BdsPlatform.h"
 #include <Guid/XenInfo.h>
 #include <Guid/RootBridgesConnectedEventGroup.h>
@@ -387,13 +387,14 @@ Returns:
   Status = gBS->InstallProtocolInterface (&Handle,
                   &gEfiDxeSmmReadyToLockProtocolGuid, EFI_NATIVE_INTERFACE,
                   NULL);
   ASSERT_EFI_ERROR (Status);
 
   //
-  // Dispatch deferred images after EndOfDxe event and ReadyToLock installation.
+  // Dispatch deferred images after EndOfDxe event and ReadyToLock
+  // installation.
   //
   EfiBootManagerDispatchDeferredImages ();
 
   PlatformInitializeConsole (gPlatformConsole);
   PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
                 GetFrontPageTimeoutFromQemu ());
@@ -467,25 +468,29 @@ Returns:
   }
   TempDevicePath = DevicePath;
 
   //
   // Register Keyboard
   //
-  DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gPnpPs2KeyboardDeviceNode);
+  DevicePath = AppendDevicePathNode (DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&gPnpPs2KeyboardDeviceNode);
 
   EfiBootManagerUpdateConsoleVariable (ConIn, DevicePath, NULL);
 
   //
   // Register COM1
   //
   DevicePath = TempDevicePath;
   gPnp16550ComPortDeviceNode.UID = 0;
 
-  DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gPnp16550ComPortDeviceNode);
-  DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gUartDeviceNode);
-  DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gTerminalTypeDeviceNode);
+  DevicePath = AppendDevicePathNode (DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&gPnp16550ComPortDeviceNode);
+  DevicePath = AppendDevicePathNode (DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&gUartDeviceNode);
+  DevicePath = AppendDevicePathNode (DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&gTerminalTypeDeviceNode);
 
   //
   // Print Device Path
   //
   DevPathStr = ConvertDevicePathToText (DevicePath, FALSE, FALSE);
   if (DevPathStr != NULL) {
@@ -506,15 +511,18 @@ Returns:
   //
   // Register COM2
   //
   DevicePath = TempDevicePath;
   gPnp16550ComPortDeviceNode.UID = 1;
 
-  DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gPnp16550ComPortDeviceNode);
-  DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gUartDeviceNode);
-  DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gTerminalTypeDeviceNode);
+  DevicePath = AppendDevicePathNode (DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&gPnp16550ComPortDeviceNode);
+  DevicePath = AppendDevicePathNode (DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&gUartDeviceNode);
+  DevicePath = AppendDevicePathNode (DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&gTerminalTypeDeviceNode);
 
   //
   // Print Device Path
   //
   DevPathStr = ConvertDevicePathToText (DevicePath, FALSE, FALSE);
   if (DevPathStr != NULL) {
@@ -585,13 +593,14 @@ GetGopDevicePath (
                   );
   if (!EFI_ERROR (Status)) {
     //
     // Add all the child handles as possible Console Device
     //
     for (Index = 0; Index < GopHandleCount; Index++) {
-      Status = gBS->HandleProtocol (GopHandleBuffer[Index], &gEfiDevicePathProtocolGuid, (VOID*)&TempDevicePath);
+      Status = gBS->HandleProtocol (GopHandleBuffer[Index],
+                      &gEfiDevicePathProtocolGuid, (VOID*)&TempDevicePath);
       if (EFI_ERROR (Status)) {
         continue;
       }
       if (CompareMem (
             PciDevicePath,
             TempDevicePath,
@@ -604,14 +613,14 @@ GetGopDevicePath (
         // In future, we could select all child handles to be console device
         //
 
         *GopDevicePath = TempDevicePath;
 
         //
-        // Delete the PCI device's path that added by GetPlugInPciVgaDevicePath()
-        // Add the integrity GOP device path.
+        // Delete the PCI device's path that added by
+        // GetPlugInPciVgaDevicePath(). Add the integrity GOP device path.
         //
         EfiBootManagerUpdateConsoleVariable (ConOutDev, NULL, PciDevicePath);
         EfiBootManagerUpdateConsoleVariable (ConOutDev, TempDevicePath, NULL);
       }
     }
     gBS->FreePool (GopHandleBuffer);
@@ -697,14 +706,16 @@ Returns:
                   (VOID*)&DevicePath
                   );
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
-  DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gUartDeviceNode);
-  DevicePath = AppendDevicePathNode (DevicePath, (EFI_DEVICE_PATH_PROTOCOL *)&gTerminalTypeDeviceNode);
+  DevicePath = AppendDevicePathNode (DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&gUartDeviceNode);
+  DevicePath = AppendDevicePathNode (DevicePath,
+                 (EFI_DEVICE_PATH_PROTOCOL *)&gTerminalTypeDeviceNode);
 
   EfiBootManagerUpdateConsoleVariable (ConOut, DevicePath, NULL);
   EfiBootManagerUpdateConsoleVariable (ConIn, DevicePath, NULL);
   EfiBootManagerUpdateConsoleVariable (ErrOut, DevicePath, NULL);
 
   return EFI_SUCCESS;
@@ -814,13 +825,14 @@ VisitAllPciInstances (
   ConOut, ConIn, ErrOut.
 
   @param[in]  Handle - Handle of PCI device instance
   @param[in]  PciIo - PCI IO protocol instance
   @param[in]  Pci - PCI Header register block
 
-  @retval EFI_SUCCESS - PCI Device check and Console variable update successfully.
+  @retval EFI_SUCCESS - PCI Device check and Console variable update
+                        successfully.
   @retval EFI_STATUS - PCI Device check or Console variable update fail.
 
 **/
 EFI_STATUS
 EFIAPI
 DetectAndPreparePlatformPciDevicePath (
@@ -888,13 +900,14 @@ DetectAndPreparePlatformPciDevicePath (
 
 /**
   Do platform specific PCI Device check and add them to ConOut, ConIn, ErrOut
 
   @param[in]  DetectVgaOnly - Only detect VGA device if it's TRUE.
 
-  @retval EFI_SUCCESS - PCI Device check and Console variable update successfully.
+  @retval EFI_SUCCESS - PCI Device check and Console variable update
+                        successfully.
   @retval EFI_STATUS - PCI Device check or Console variable update fail.
 
 **/
 EFI_STATUS
 DetectAndPreparePlatformPciDevicePaths (
   BOOLEAN DetectVgaOnly
@@ -925,18 +938,20 @@ Arguments:
   EFI_DEVICE_PATH_PROTOCOL           *VarConout;
   EFI_DEVICE_PATH_PROTOCOL           *VarConin;
 
   //
   // Connect RootBridge
   //
-  GetEfiGlobalVariable2 (EFI_CON_OUT_VARIABLE_NAME, (VOID **) &VarConout, NULL);
+  GetEfiGlobalVariable2 (EFI_CON_OUT_VARIABLE_NAME, (VOID **) &VarConout,
+    NULL);
   GetEfiGlobalVariable2 (EFI_CON_IN_VARIABLE_NAME, (VOID **) &VarConin, NULL);
 
   if (VarConout == NULL || VarConin == NULL) {
     //
-    // Do platform specific PCI Device check and add them to ConOut, ConIn, ErrOut
+    // Do platform specific PCI Device check and add them to ConOut, ConIn,
+    // ErrOut
     //
     DetectAndPreparePlatformPciDevicePaths (FALSE);
 
     //
     // Have chance to connect the platform default console,
     // the platform default console is the minimum device group
@@ -944,19 +959,22 @@ Arguments:
     //
     for (Index = 0; PlatformConsole[Index].DevicePath != NULL; ++Index) {
       //
       // Update the console variable with the connect type
       //
       if ((PlatformConsole[Index].ConnectType & CONSOLE_IN) == CONSOLE_IN) {
-        EfiBootManagerUpdateConsoleVariable (ConIn, PlatformConsole[Index].DevicePath, NULL);
+        EfiBootManagerUpdateConsoleVariable (ConIn,
+          PlatformConsole[Index].DevicePath, NULL);
       }
       if ((PlatformConsole[Index].ConnectType & CONSOLE_OUT) == CONSOLE_OUT) {
-        EfiBootManagerUpdateConsoleVariable (ConOut, PlatformConsole[Index].DevicePath, NULL);
+        EfiBootManagerUpdateConsoleVariable (ConOut,
+          PlatformConsole[Index].DevicePath, NULL);
       }
       if ((PlatformConsole[Index].ConnectType & STD_ERROR) == STD_ERROR) {
-        EfiBootManagerUpdateConsoleVariable (ErrOut, PlatformConsole[Index].DevicePath, NULL);
+        EfiBootManagerUpdateConsoleVariable (ErrOut,
+          PlatformConsole[Index].DevicePath, NULL);
       }
     }
   } else {
     //
     // Only detect VGA device and add them to ConOut
     //
@@ -1234,13 +1252,16 @@ ConnectRecursivelyIfPciMassStorage (
     //
     DevPathStr = ConvertDevicePathToText (DevicePath, FALSE, FALSE);
     if (DevPathStr != NULL) {
       DEBUG((
         EFI_D_INFO,
         "Found %s device: %s\n",
-        IS_CLASS1 (PciHeader, PCI_CLASS_MASS_STORAGE) ? L"Mass Storage" : L"Xen",
+        (IS_CLASS1 (PciHeader, PCI_CLASS_MASS_STORAGE) ?
+         L"Mass Storage" :
+         L"Xen"
+         ),
         DevPathStr
         ));
       FreePool(DevPathStr);
     }
 
     Status = gBS->ConnectController (Handle, NULL, NULL, TRUE);
@@ -1528,13 +1549,14 @@ NotifyDevPath (
       continue;
     }
 
     //
     // Get the DevicePath protocol on that handle
     //
-    Status = gBS->HandleProtocol (Handle, &gEfiDevicePathProtocolGuid, (VOID **)&DevPathNode);
+    Status = gBS->HandleProtocol (Handle, &gEfiDevicePathProtocolGuid,
+                    (VOID **)&DevPathNode);
     ASSERT_EFI_ERROR (Status);
 
     while (!IsDevicePathEnd (DevPathNode)) {
       //
       // Find the handler to dump this device path node
       //
@@ -1578,13 +1600,14 @@ InstallDevicePathCallback (
                           NULL,
                           &mEfiDevPathNotifyReg
                           );
 }
 
 /**
-  This function is called each second during the boot manager waits the timeout.
+  This function is called each second during the boot manager waits the
+  timeout.
 
   @param TimeoutRemain  The remaining timeout.
 **/
 VOID
 EFIAPI
 PlatformBootManagerWaitCallback (
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 3/5] OvmfPkg/PlatformBootManagerLib: rejuvenate old-style function comments

[Laszlo Ersek]

The old-style "Routine Description: ..." comments use the leftmost column
and are placed between the parameter list and the function body. Therefore
they cause git-diff to produce bogus hunk headers that fail to name the
function being patched.

Convert these comment blocks to the current edk2 style. While at it, clean
them up too.

For PlatformBootManagerBeforeConsole() and
PlatformBootManagerAfterConsole(), copy the descriptions from the call
sites in "MdeModulePkg/Universal/BdsDxe/BdsEntry.c". They are more
detailed than the comments in the lib class header
"MdeModulePkg/Include/Library/PlatformBootManagerLib.h"; ArmVirtPkg
already uses these comments.

No functional changes.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 172 ++++++++------------
 1 file changed, 70 insertions(+), 102 deletions(-)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 99b7db7cc05a..b155639f3c97 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -325,31 +325,30 @@ SaveS3BootScript (
   VOID
   );
 
 //
 // BDS Platform Functions
 //
+/**
+  Do the platform init, can be customized by OEM/IBV
+
+  Possible things that can be done in PlatformBootManagerBeforeConsole:
+
+  > Update console variable: 1. include hot-plug devices;
+  >                          2. Clear ConIn and add SOL for AMT
+  > Register new Driver#### or Boot####
+  > Register new Key####: e.g.: F12
+  > Signal ReadyToLock event
+  > Authentication action: 1. connect Auth devices;
+  >                        2. Identify auto logon user.
+**/
 VOID
 EFIAPI
 PlatformBootManagerBeforeConsole (
   VOID
   )
-/*++
-
-Routine Description:
-
-  Platform Bds init. Include the platform firmware vendor, revision
-  and so crc check.
-
-Arguments:
-
-Returns:
-
-  None.
-
---*/
 {
   EFI_HANDLE    Handle;
   EFI_STATUS    Status;
   RETURN_STATUS PcdStatus;
 
   DEBUG ((EFI_D_INFO, "PlatformBootManagerBeforeConsole\n"));
@@ -426,34 +425,27 @@ ConnectRootBridge (
                   FALSE             // Recursive
                   );
   return Status;
 }
 
 
+/**
+  Add IsaKeyboard to ConIn; add IsaSerial to ConOut, ConIn, ErrOut.
+
+  @param[in] DeviceHandle  Handle of the LPC Bridge device.
+
+  @retval EFI_SUCCESS  Console devices on the LPC bridge have been added to
+                       ConOut, ConIn, and ErrOut.
+
+  @return              Error codes, due to EFI_DEVICE_PATH_PROTOCOL missing
+                       from DeviceHandle.
+**/
 EFI_STATUS
 PrepareLpcBridgeDevicePath (
   IN EFI_HANDLE                DeviceHandle
   )
-/*++
-
-Routine Description:
-
-  Add IsaKeyboard to ConIn,
-  add IsaSerial to ConOut, ConIn, ErrOut.
-  LPC Bridge: 06 01 00
-
-Arguments:
-
-  DeviceHandle            - Handle of PCIIO protocol.
-
-Returns:
-
-  EFI_SUCCESS             - LPC bridge is added to ConOut, ConIn, and ErrOut.
-  EFI_STATUS              - No LPC bridge is added.
-
---*/
 {
   EFI_STATUS                Status;
   EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
   EFI_DEVICE_PATH_PROTOCOL  *TempDevicePath;
   CHAR16                    *DevPathStr;
 
@@ -626,33 +618,26 @@ GetGopDevicePath (
     gBS->FreePool (GopHandleBuffer);
   }
 
   return EFI_SUCCESS;
 }
 
+/**
+  Add PCI display to ConOut.
+
+  @param[in] DeviceHandle  Handle of the PCI display device.
+
+  @retval EFI_SUCCESS  The PCI display device has been added to ConOut.
+
+  @return              Error codes, due to EFI_DEVICE_PATH_PROTOCOL missing
+                       from DeviceHandle.
+**/
 EFI_STATUS
 PreparePciDisplayDevicePath (
   IN EFI_HANDLE                DeviceHandle
   )
-/*++
-
-Routine Description:
-
-  Add PCI VGA to ConOut.
-  PCI VGA: 03 00 00
-
-Arguments:
-
-  DeviceHandle            - Handle of PCIIO protocol.
-
-Returns:
-
-  EFI_SUCCESS             - PCI VGA is added to ConOut.
-  EFI_STATUS              - No PCI VGA device is added.
-
---*/
 {
   EFI_STATUS                Status;
   EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
   EFI_DEVICE_PATH_PROTOCOL  *GopDevicePath;
 
   DevicePath    = NULL;
@@ -671,33 +656,27 @@ Returns:
 
   EfiBootManagerUpdateConsoleVariable (ConOut, DevicePath, NULL);
 
   return EFI_SUCCESS;
 }
 
-EFI_STATUS
-PreparePciSerialDevicePath (
-  IN EFI_HANDLE                DeviceHandle
-  )
-/*++
-
-Routine Description:
-
+/**
   Add PCI Serial to ConOut, ConIn, ErrOut.
-  PCI Serial: 07 00 02
 
-Arguments:
+  @param[in] DeviceHandle  Handle of the PCI serial device.
 
-  DeviceHandle            - Handle of PCIIO protocol.
+  @retval EFI_SUCCESS  The PCI serial device has been added to ConOut, ConIn,
+                       ErrOut.
 
-Returns:
-
-  EFI_SUCCESS             - PCI Serial is added to ConOut, ConIn, and ErrOut.
-  EFI_STATUS              - No PCI Serial device is added.
-
---*/
+  @return              Error codes, due to EFI_DEVICE_PATH_PROTOCOL missing
+                       from DeviceHandle.
+**/
+EFI_STATUS
+PreparePciSerialDevicePath (
+  IN EFI_HANDLE                DeviceHandle
+  )
 {
   EFI_STATUS                Status;
   EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
 
   DevicePath = NULL;
   Status = gBS->HandleProtocol (
@@ -915,27 +894,23 @@ DetectAndPreparePlatformPciDevicePaths (
 {
   mDetectVgaOnly = DetectVgaOnly;
   return VisitAllPciInstances (DetectAndPreparePlatformPciDevicePath);
 }
 
 
+/**
+  Connect the predefined platform default console device.
+
+  Always try to find and enable PCI display devices.
+
+  @param[in] PlatformConsole  Predefined platform default console device array.
+**/
 VOID
 PlatformInitializeConsole (
   IN PLATFORM_CONSOLE_CONNECT_ENTRY   *PlatformConsole
   )
-/*++
-
-Routine Description:
-
-  Connect the predefined platform default console device. Always try to find
-  and enable the vga device if have.
-
-Arguments:
-
-  PlatformConsole         - Predefined platform default console device array.
---*/
 {
   UINTN                              Index;
   EFI_DEVICE_PATH_PROTOCOL           *VarConout;
   EFI_DEVICE_PATH_PROTOCOL           *VarConin;
 
   //
@@ -1343,32 +1318,21 @@ PlatformBdsRestoreNvVarsFromHardDisk (
     VisitingFileSystemInstance,
     NULL
     );
 
 }
 
+/**
+  Connect with predefined platform connect sequence.
+
+  The OEM/IBV can customize with their own connect sequence.
+**/
 VOID
 PlatformBdsConnectSequence (
   VOID
   )
-/*++
-
-Routine Description:
-
-  Connect with predefined platform connect sequence,
-  the OEM/IBV can customize with their own connect sequence.
-
-Arguments:
-
-  None.
-
-Returns:
-
-  None.
-
---*/
 {
   UINTN         Index;
   RETURN_STATUS Status;
 
   DEBUG ((EFI_D_INFO, "PlatformBdsConnectSequence\n"));
 
@@ -1428,26 +1392,30 @@ SaveS3BootScript (
                          (UINT32) sizeof Info,
                          (EFI_PHYSICAL_ADDRESS)(UINTN) &Info);
   ASSERT_EFI_ERROR (Status);
 }
 
 
+/**
+  Do the platform specific action after the console is ready
+
+  Possible things that can be done in PlatformBootManagerAfterConsole:
+
+  > Console post action:
+    > Dynamically switch output mode from 100x31 to 80x25 for certain senarino
+    > Signal console ready platform customized event
+  > Run diagnostics like memory testing
+  > Connect certain devices
+  > Dispatch aditional option roms
+  > Special boot: e.g.: USB boot, enter UI
+**/
 VOID
 EFIAPI
 PlatformBootManagerAfterConsole (
   VOID
   )
-/*++
-
-Routine Description:
-
-  The function will execute with as the platform policy, current policy
-  is driven by boot mode. IBV/OEM can customize this code for their specific
-  policy action.
-
---*/
 {
   EFI_BOOT_MODE                      BootMode;
 
   DEBUG ((EFI_D_INFO, "PlatformBootManagerAfterConsole\n"));
 
   if (PcdGetBool (PcdOvmfFlashVariablesEnable)) {
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 4/5] OvmfPkg/PlatformBootManagerLib: hoist PciAcpiInitialization()

[Laszlo Ersek]

  PlatformBootManagerAfterConsole()
                              <--------------------------------+
    PlatformBdsConnectSequence()                               |
      ConnectDevicesFromQemu() / EfiBootManagerConnectAll()    |
      PciAcpiInitialization() ---------------------------------+
    TryRunningQemuKernel()

Functionally this is a no-op:

- PciAcpiInitialization() iterates over PciIo protocol instances, which
  are available just the same at the new call site.

- The PCI interrupt line register exists only to inform system software
  (it doesn't affect hardware) and UEFI drivers don't use PCI interrupts
  anyway.

(More background in commits 2e70cf8ade0d and 5218c27950c4.)

This change will let us move TryRunningQemuKernel() between
PciAcpiInitialization() and PlatformBdsConnectSequence() in the next
patch.

Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index b155639f3c97..b624b8f22535 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -1356,14 +1356,12 @@ PlatformBdsConnectSequence (
     //
     // Just use the simple policy to connect all devices
     //
     DEBUG ((DEBUG_INFO, "EfiBootManagerConnectAll\n"));
     EfiBootManagerConnectAll ();
   }
-
-  PciAcpiInitialization ();
 }
 
 /**
   Save the S3 boot script.
 
   Note that DxeSmmReadyToLock must be signaled after this function returns;
@@ -1443,12 +1441,17 @@ PlatformBootManagerAfterConsole (
 
   //
   // Logo show
   //
   BootLogoEnableLogo ();
 
+  //
+  // Set PCI Interrupt Line registers and ACPI SCI_EN
+  //
+  PciAcpiInitialization ();
+
   //
   // Perform some platform specific connect sequence
   //
   PlatformBdsConnectSequence ();
 
   //
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 5/5] OvmfPkg/PlatformBootManagerLib: process "-kernel" before boot devices

[Laszlo Ersek]

This improves the UEFI boot time for VMs that have "-kernel", many disks
or NICs, and no "bootindex" properties.

(Unlike in ArmVirt commit 23d04b58e27b, in OvmfPkg commit 52fba28994e9 we
introduced TryRunningQemuKernel() right from the start *after*
BdsLibConnectAll(). Therefore, unlike in patch
'ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot
devices"', we adopt the logic as new in this patch.)

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index b624b8f22535..862fa6ebb4e5 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -1446,22 +1446,22 @@ PlatformBootManagerAfterConsole (
 
   //
   // Set PCI Interrupt Line registers and ACPI SCI_EN
   //
   PciAcpiInitialization ();
 
-  //
-  // Perform some platform specific connect sequence
-  //
-  PlatformBdsConnectSequence ();
-
   //
   // Process QEMU's -kernel command line option
   //
   TryRunningQemuKernel ();
 
+  //
+  // Perform some platform specific connect sequence
+  //
+  PlatformBdsConnectSequence ();
+
   EfiBootManagerRefreshAllBootOption ();
 
   //
   // Register UEFI Shell
   //
   PlatformRegisterFvBootOption (
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[Richard W.M. Jones]

On Thu, Mar 15, 2018 at 08:02:53PM +0100, Laszlo Ersek wrote:
> (Copying Rich, Xiang and Gabriel for testing requests below.)
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: kernel_before_bootdevs
> 
> After the recent series "OvmfPkg, ArmVirtQemu: leaner platform BDS
> policy for connecting devices", I'm picking up another earlier idea -- a
> direct kernel boot does not need devices such as disks and NICs to be
> bound by UEFI.
> 
> I tested this series extensively on QEMU, in OVMF (IA32X64) and
> ArmVirtQemu (AARCH64), both with and without direct kernel boot. I
> compared the logs in all sensible relations within a given architecture.
> 
> Rich, can you please test this on ARM64, with guestfish/libguestfs?
> Please attach a good number of disks at once on the command line, and
> compare the appliance's boot time between (e.g.) RHEL7's
> "/usr/share/AAVMF/AAVMF_CODE.fd" and the following binary (after
> decompression):
> 
>   https://people.redhat.com/lersek/kernel_before_bootdevs-991e2f2f-64cf-4566-b933-919928e2aa6b/QEMU_EFI.fd.padded.xz

I tested this on Fedora Rawhide (aarch64) with:

  kernel-core-4.16.0-0.rc5.git1.2.fc29.aarch64 (host & guest)
  qemu-2.11.0-5.fc29.aarch64
  edk2-aarch64-20171011git92d07e4-2.fc28.noarch
  libguestfs-1.39.1-1.fc29.aarch64

I used the /usr/bin/libguestfs-boot-benchmark tool from
libguestfs-benchmarking-1.39.1-1.fc29.aarch64

As a baseline, on my mid-range Intel i7 laptop (note that this number
is NOT comparable to the aarch64 numbers, it's just to give a flavour
of what is possible):

  Result: 1384.5ms ±9.2ms

On aarch64 using edk2-aarch64 from Fedora:

  Result: 8844.0ms ±30.7ms

On aarch64 using your supplied build of AAVMF:

  Result: 4156.6ms ±1.3ms

I also confirmed (using libguestfs-test-tool) that it was working and
using the right AAVMF_CODE.fd file.  Therefore:

  Tested-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [PATCH 1/5] ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot devices"

[Ard Biesheuvel]

On 15 March 2018 at 19:02, Laszlo Ersek <lersek@redhat.com> wrote:
> Move the TryRunningQemuKernel() call back to its original place.

When was it moved and why?

> This
> improves the UEFI boot time for VMs that have "-kernel", many disks or
> NICs, and no "bootindex" properties. A well-known example is
> guestfish/libguestfs.
>
> For more info on the TryRunningQemuKernel() location, see the following
> commits: 23d04b58e27b, a78c4836ea0b, 158990b941e4.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Xiang Zheng <xiang.zheng@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 36e0eed2384a..5d5e51d8c870 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -667,31 +667,31 @@ PlatformBootManagerAfterConsole (
>
>    //
>    // Show the splash screen.
>    //
>    BootLogoEnableLogo ();
>
> +  //
> +  // Process QEMU's -kernel command line option. The kernel booted this way
> +  // will receive ACPI tables: in PlatformBootManagerBeforeConsole(), we
> +  // connected any and all PCI root bridges, and then signaled the ACPI
> +  // platform driver.
> +  //
> +  TryRunningQemuKernel ();
> +
>    //
>    // Connect the purported boot devices.
>    //
>    Status = ConnectDevicesFromQemu ();
>    if (RETURN_ERROR (Status)) {
>      //
>      // Connect the rest of the devices.
>      //
>      EfiBootManagerConnectAll ();
>    }
>
> -  //
> -  // Process QEMU's -kernel command line option. Note that the kernel booted
> -  // this way should receive ACPI tables, which is why we connect all devices
> -  // first (see above) -- PCI enumeration blocks ACPI table installation, if
> -  // there is a PCI host.
> -  //
> -  TryRunningQemuKernel ();
> -
>    //
>    // Enumerate all possible boot options, then filter and reorder them based on
>    // the QEMU configuration.
>    //
>    EfiBootManagerRefreshAllBootOption ();
>
> --
> 2.14.1.3.gb7cf6e02401b
>
>

Re: [PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[Richard W.M. Jones]

On Thu, Mar 15, 2018 at 08:02:53PM +0100, Laszlo Ersek wrote:
> Please attach a good number of disks at once on the command line,

I read this bit then forgot to do it :-/

The test suite has a test for adding a large number of drives:

  https://github.com/libguestfs/libguestfs/tree/master/tests/disks

so it's quite easy for me to test this:

  $ time ./test-add-disks -n 100

Results with Fedora's edk2-aarch64:

  real	8m25.353s
  user	0m2.393s
  sys	0m2.657s

Results with your file:

  real	8m15.285s
  user	0m0.178s
  sys	0m0.381s

So there's not really a great difference here, but boot time is not a
significant factor for this test unlike the boot benchmark tests.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [PATCH 1/5] ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot devices"

[Laszlo Ersek]

On 03/16/18 10:58, Ard Biesheuvel wrote:
> On 15 March 2018 at 19:02, Laszlo Ersek <lersek@redhat.com> wrote:
>> Move the TryRunningQemuKernel() call back to its original place.
> 
> When was it moved and why?

See, I'm at a loss about the detail you expect in commit messages. :) I
had spent 10 minutes on wording this commit message. Near the end of my
struggle, I had exactly one sentence on each of those three commits that
I listed at the bottom (23d04b58e27b, a78c4836ea0b, 158990b941e4). Those
three sentences were going to give the answer to your question. I still
worried you might file them under "personal journey" or "stream of
consciousness", and I cut them; I only left the three commit hashes at
the end as pointers.

So, the executive summary is: "TryRunningQemuKernel() was moved in
a78c4836ea0b, because the guest kernel depended on ACPI tables, which
depended on our ACPI platform driver, which at the time depended on
PciBusDxe itself reporting 'enumeration complete', which at the time
depended on our BdsLibConnectAll() call".

The somewhat expanded answer is, from scratch:

(1) in commit 23d04b58e27b, we introduced TryRunningQemuKernel() in the
    right spot (for boot performance), between
    PlatformBdsConnectConsole() and BdsLibConnectAll();

(2) in commit a78c4836ea0b, we moved TryRunningQemuKernel() after
    BdsLibConnectAll(): the direct-booted kernel needed ACPI tables
    (reflecting PCI resources correctly), and at that time, we only
    connected the root bridges to PciBusDxe as part of
    BdsLibConnectAll() (which signaled the ACPI platform driver via
    "gEfiPciEnumerationCompleteProtocolGuid");

(3) in commit 60dc18a17c516 and (more importantly) 158990b941e4,
    connecting the root bridges and cueing the ACPI platform driver were
    finally separated from BdsLibConnectAll(), but we didn't realize we
    could move TryRunningQemuKernel() back above BdsLibConnectAll().

So, after 158990b941e4, we can do it now.

Thanks,
Laszlo

Re: [PATCH 1/5] ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot devices"

[Ard Biesheuvel]

On 16 March 2018 at 13:40, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/16/18 10:58, Ard Biesheuvel wrote:
>> On 15 March 2018 at 19:02, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Move the TryRunningQemuKernel() call back to its original place.
>>
>> When was it moved and why?
>
> See, I'm at a loss about the detail you expect in commit messages. :)

I see. But in this case, I think it is rather obvious that when you
move something back to where it was moved away from earlier, you have
to justify why the original reason for moving it no longer applies, or
is overruled by something more important.

> I
> had spent 10 minutes on wording this commit message. Near the end of my
> struggle, I had exactly one sentence on each of those three commits that
> I listed at the bottom (23d04b58e27b, a78c4836ea0b, 158990b941e4). Those
> three sentences were going to give the answer to your question. I still
> worried you might file them under "personal journey" or "stream of
> consciousness", and I cut them; I only left the three commit hashes at
> the end as pointers.
>

I looked at those commit logs but it wasn't obvious to me.

> So, the executive summary is: "TryRunningQemuKernel() was moved in
> a78c4836ea0b, because the guest kernel depended on ACPI tables, which
> depended on our ACPI platform driver, which at the time depended on
> PciBusDxe itself reporting 'enumeration complete', which at the time
> depended on our BdsLibConnectAll() call".
>
> The somewhat expanded answer is, from scratch:
>
> (1) in commit 23d04b58e27b, we introduced TryRunningQemuKernel() in the
>     right spot (for boot performance), between
>     PlatformBdsConnectConsole() and BdsLibConnectAll();
>
> (2) in commit a78c4836ea0b, we moved TryRunningQemuKernel() after
>     BdsLibConnectAll(): the direct-booted kernel needed ACPI tables
>     (reflecting PCI resources correctly), and at that time, we only
>     connected the root bridges to PciBusDxe as part of
>     BdsLibConnectAll() (which signaled the ACPI platform driver via
>     "gEfiPciEnumerationCompleteProtocolGuid");
>
> (3) in commit 60dc18a17c516 and (more importantly) 158990b941e4,
>     connecting the root bridges and cueing the ACPI platform driver were
>     finally separated from BdsLibConnectAll(), but we didn't realize we
>     could move TryRunningQemuKernel() back above BdsLibConnectAll().
>
> So, after 158990b941e4, we can do it now.
>

Thanks for clearing that up.

Re: [PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[Laszlo Ersek]

On 03/16/18 10:59, Richard W.M. Jones wrote:
> On Thu, Mar 15, 2018 at 08:02:53PM +0100, Laszlo Ersek wrote:
>> Please attach a good number of disks at once on the command line,
> 
> I read this bit then forgot to do it :-/
> 
> The test suite has a test for adding a large number of drives:
> 
>   https://github.com/libguestfs/libguestfs/tree/master/tests/disks
> 
> so it's quite easy for me to test this:
> 
>   $ time ./test-add-disks -n 100

What kind of disks does this test add, virtio-blk or virtio-scsi?

And I assume PCI, not virtio-mmio?

It's possible that with a hundred disks, you hit a limit in the firmware
before all the time was spent that would have been necessary to bind all
hundred disks. I mean, the point is exactly to prevent the firmware from
spending time on those disks, but it should be due to a controlled
restriction, not resource exhaustion, and the comparison should use a
test case where the firmware does manage to bind them all (without the
patches).

... We could go into the various limits here, regarding PCI bus number
space, virtio-scsi targets and LUNs, but I think it would be premature
before seeing a domain XML or a QEMU command line. (Also, my apologies
for being vague with "good number of ...".)

Furthermore:

> Results with Fedora's edk2-aarch64:
> 
>   real	8m25.353s
>   user	0m2.393s
>   sys	0m2.657s
> 
> Results with your file:
> 
>   real	8m15.285s
>   user	0m0.178s
>   sys	0m0.381s
> 
> So there's not really a great difference here, but boot time is not a
> significant factor for this test unlike the boot benchmark tests.

indeed this patch should only improve boot time. In your other email,
you mention "libguestfs-boot-benchmark". Does that include kernel boot
time as well?

... OTOH, it probably should too. An interactive user definitely cares
about the time between (a) hitting Enter on the "guestfish" command and
(b) getting the guestfish prompt. Where the boot transitions from
firmware to kernel is irrelevant to the user.

Sorry to bother you with another request, but I currently have no access
to my aarch64 hardware (so that I could test on aarch64 KVM); could you
please compare boot benchmark results using, say, eight disks? Something
like:

  time guestfish --ro \
    -a disk1.img \
    ... \
    -a disk8.img \
    launch : quit

(I hope this test case is not totally bogus.)

If you don't have time, I'll try to run this test myself tonight. I'd
trust your results more though!

Thanks!
Laszlo

Re: [PATCH 1/5] ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot devices"

[Laszlo Ersek]

On 03/16/18 14:45, Ard Biesheuvel wrote:
> On 16 March 2018 at 13:40, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/16/18 10:58, Ard Biesheuvel wrote:
>>> On 15 March 2018 at 19:02, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> Move the TryRunningQemuKernel() call back to its original place.
>>>
>>> When was it moved and why?
>>
>> See, I'm at a loss about the detail you expect in commit messages. :)
> 
> I see. But in this case, I think it is rather obvious that when you
> move something back to where it was moved away from earlier, you have
> to justify why the original reason for moving it no longer applies, or
> is overruled by something more important.
> 
>> I
>> had spent 10 minutes on wording this commit message. Near the end of my
>> struggle, I had exactly one sentence on each of those three commits that
>> I listed at the bottom (23d04b58e27b, a78c4836ea0b, 158990b941e4). Those
>> three sentences were going to give the answer to your question. I still
>> worried you might file them under "personal journey" or "stream of
>> consciousness", and I cut them; I only left the three commit hashes at
>> the end as pointers.
>>
> 
> I looked at those commit logs but it wasn't obvious to me.
> 
>> So, the executive summary is: "TryRunningQemuKernel() was moved in
>> a78c4836ea0b, because the guest kernel depended on ACPI tables, which
>> depended on our ACPI platform driver, which at the time depended on
>> PciBusDxe itself reporting 'enumeration complete', which at the time
>> depended on our BdsLibConnectAll() call".
>>
>> The somewhat expanded answer is, from scratch:
>>
>> (1) in commit 23d04b58e27b, we introduced TryRunningQemuKernel() in the
>>     right spot (for boot performance), between
>>     PlatformBdsConnectConsole() and BdsLibConnectAll();
>>
>> (2) in commit a78c4836ea0b, we moved TryRunningQemuKernel() after
>>     BdsLibConnectAll(): the direct-booted kernel needed ACPI tables
>>     (reflecting PCI resources correctly), and at that time, we only
>>     connected the root bridges to PciBusDxe as part of
>>     BdsLibConnectAll() (which signaled the ACPI platform driver via
>>     "gEfiPciEnumerationCompleteProtocolGuid");
>>
>> (3) in commit 60dc18a17c516 and (more importantly) 158990b941e4,
>>     connecting the root bridges and cueing the ACPI platform driver were
>>     finally separated from BdsLibConnectAll(), but we didn't realize we
>>     could move TryRunningQemuKernel() back above BdsLibConnectAll().
>>
>> So, after 158990b941e4, we can do it now.
>>
> 
> Thanks for clearing that up.

Are you OK with the patch with this information available?

Should I include some (or all) of the above in the commit message?

Thanks!
Laszlo

Re: [PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[Ard Biesheuvel]

On 15 March 2018 at 19:02, Laszlo Ersek <lersek@redhat.com> wrote:
> (Copying Rich, Xiang and Gabriel for testing requests below.)
>
> Repo:   https://github.com/lersek/edk2.git
> Branch: kernel_before_bootdevs
>
> After the recent series "OvmfPkg, ArmVirtQemu: leaner platform BDS
> policy for connecting devices", I'm picking up another earlier idea -- a
> direct kernel boot does not need devices such as disks and NICs to be
> bound by UEFI.
>
> I tested this series extensively on QEMU, in OVMF (IA32X64) and
> ArmVirtQemu (AARCH64), both with and without direct kernel boot. I
> compared the logs in all sensible relations within a given architecture.
>
> Rich, can you please test this on ARM64, with guestfish/libguestfs?
> Please attach a good number of disks at once on the command line, and
> compare the appliance's boot time between (e.g.) RHEL7's
> "/usr/share/AAVMF/AAVMF_CODE.fd" and the following binary (after
> decompression):
>
>   https://people.redhat.com/lersek/kernel_before_bootdevs-991e2f2f-64cf-4566-b933-919928e2aa6b/QEMU_EFI.fd.padded.xz
>
> (That binary corresponds to the branch linked above, cross-built from
> x86_64 with "aarch64-linux-gnu-gcc (GCC) 6.1.1 20160621 (Red Hat Cross
> 6.1.1-2)", using the following options:
>
>   export GCC5_AARCH64_PREFIX=aarch64-linux-gnu-
>   build --cmd-len=65536 --hash -t GCC5 -b DEBUG -a AARCH64 \
>     -p ArmVirtPkg/ArmVirtQemu.dsc -D DEBUG_PRINT_ERROR_LEVEL=0x80000000
>
> and then it was padded with zeroes to 64MB.)
>
> If you have good results, please respond with your Tested-by (which I'll
> apply to patch 1/5, since that's the one that matters for the ARM64
> target).
>
> Xiang, if you use guestfish (or else direct kernel boot) occasionally,
> then similar testing would be very welcome from your side too.
>
> Gabriel, I'm CC'ing you on patch 4/5, because it affects code that you
> had originally written. Can you please regression-test this series with
> your usual OVMF environment / guest(s)?
>
> Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
> Cc: "Richard W.M. Jones" <rjones@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Xiang Zheng <xiang.zheng@linaro.org>
>
> Thanks everyone!
> Laszlo
>
> Laszlo Ersek (5):
>   ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot
>     devices"
>   OvmfPkg/PlatformBootManagerLib: wrap overlong lines in "BdsPlatform.c"
>   OvmfPkg/PlatformBootManagerLib: rejuvenate old-style function comments
>   OvmfPkg/PlatformBootManagerLib: hoist PciAcpiInitialization()
>   OvmfPkg/PlatformBootManagerLib: process "-kernel" before boot devices
>
>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c |  16 +-
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c   | 262 ++++++++++----------
>  2 files changed, 136 insertions(+), 142 deletions(-)
>

For the series

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Re: [PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[Richard W.M. Jones]

On Fri, Mar 16, 2018 at 03:02:57PM +0100, Laszlo Ersek wrote:
> On 03/16/18 10:59, Richard W.M. Jones wrote:
> > On Thu, Mar 15, 2018 at 08:02:53PM +0100, Laszlo Ersek wrote:
> >> Please attach a good number of disks at once on the command line,
> > 
> > I read this bit then forgot to do it :-/
> > 
> > The test suite has a test for adding a large number of drives:
> > 
> >   https://github.com/libguestfs/libguestfs/tree/master/tests/disks
> > 
> > so it's quite easy for me to test this:
> > 
> >   $ time ./test-add-disks -n 100
> 
> What kind of disks does this test add, virtio-blk or virtio-scsi?
> 
> And I assume PCI, not virtio-mmio?

virtio-scsi & PCI.

I think we are still using virtio-mmio in RHEL 7?  But anyway the
tests were performed with the latest upstream stuff.

> It's possible that with a hundred disks, you hit a limit in the firmware
> before all the time was spent that would have been necessary to bind all
> hundred disks. I mean, the point is exactly to prevent the firmware from
> spending time on those disks, but it should be due to a controlled
> restriction, not resource exhaustion, and the comparison should use a
> test case where the firmware does manage to bind them all (without the
> patches).

There's actually a load of problems on x86, mainly with the BIOS &
kernel and how long it takes to enumerate the disks.  I didn't look
closely at what it's trying to do on aarch64.

> ... We could go into the various limits here, regarding PCI bus number
> space, virtio-scsi targets and LUNs, but I think it would be premature
> before seeing a domain XML or a QEMU command line. (Also, my apologies
> for being vague with "good number of ...".)

An easy way to reproduce this is:

  # dnf install /usr/bin/virt-rescue
  $ virt-rescue --scratch=100
  ><rescue> ls /dev/sd
  Display all 101 possibilities? (y or n)
  sda   sdah  sdap  sdax  sdbe  sdbm  sdbu  sdcb  sdcj  sdcr  sdf   sdn   sdv
  sdaa  sdai  sdaq  sday  sdbf  sdbn  sdbv  sdcc  sdck  sdcs  sdg   sdo   sdw
  sdab  sdaj  sdar  sdaz  sdbg  sdbo  sdbw  sdcd  sdcl  sdct  sdh   sdp   sdx
  sdac  sdak  sdas  sdb   sdbh  sdbp  sdbx  sdce  sdcm  sdcu  sdi   sdq   sdy
  sdad  sdal  sdat  sdba  sdbi  sdbq  sdby  sdcf  sdcn  sdcv  sdj   sdr   sdz
  sdae  sdam  sdau  sdbb  sdbj  sdbr  sdbz  sdcg  sdco  sdcw  sdk   sds   
  sdaf  sdan  sdav  sdbc  sdbk  sdbs  sdc   sdch  sdcp  sdd   sdl   sdt   
  sdag  sdao  sdaw  sdbd  sdbl  sdbt  sdca  sdci  sdcq  sde   sdm   sdu   

> indeed this patch should only improve boot time. In your other email,
> you mention "libguestfs-boot-benchmark". Does that include kernel boot
> time as well?

libguestfs-boot-benchmark just starts the appliance (and kernel and
userspace) and shuts it down, so it's as close to testing raw boot
performance as you can get.  However it only uses I think 1 or 2
disks, so it's not testing lots of devices.

> ... OTOH, it probably should too. An interactive user definitely cares
> about the time between (a) hitting Enter on the "guestfish" command and
> (b) getting the guestfish prompt. Where the boot transitions from
> firmware to kernel is irrelevant to the user.
> 
> Sorry to bother you with another request, but I currently have no access
> to my aarch64 hardware (so that I could test on aarch64 KVM); could you
> please compare boot benchmark results using, say, eight disks? Something
> like:
> 
>   time guestfish --ro \
>     -a disk1.img \
>     ... \
>     -a disk8.img \
>     launch : quit
> 
> (I hope this test case is not totally bogus.)

An easier way is this as follows.  I missed out the --just-add option
in my previous tests, which causes the test to do a lot more testing.
With the option it just adds them, launches the appliance and shuts
down.

$ time ./test-add-disks --just-add -n 8 

Results:

with Fedora AAVMF:  0m11.197s 0m10.075s 0m10.033s
with your firmware: 0m5.395s 0m5.385s 0m5.354s

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [PATCH 4/5] OvmfPkg/PlatformBootManagerLib: hoist PciAcpiInitialization()

[Gabriel L. Somlo]

On Thu, Mar 15, 2018 at 08:02:57PM +0100, Laszlo Ersek wrote:
>   PlatformBootManagerAfterConsole()
>                               <--------------------------------+
>     PlatformBdsConnectSequence()                               |
>       ConnectDevicesFromQemu() / EfiBootManagerConnectAll()    |
>       PciAcpiInitialization() ---------------------------------+
>     TryRunningQemuKernel()
> 
> Functionally this is a no-op:
> 
> - PciAcpiInitialization() iterates over PciIo protocol instances, which
>   are available just the same at the new call site.
> 
> - The PCI interrupt line register exists only to inform system software
>   (it doesn't affect hardware) and UEFI drivers don't use PCI interrupts
>   anyway.
> 
> (More background in commits 2e70cf8ade0d and 5218c27950c4.)
> 
> This change will let us move TryRunningQemuKernel() between
> PciAcpiInitialization() and PlatformBdsConnectSequence() in the next
> patch.
> 
> Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index b155639f3c97..b624b8f22535 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -1356,14 +1356,12 @@ PlatformBdsConnectSequence (
>      //
>      // Just use the simple policy to connect all devices
>      //
>      DEBUG ((DEBUG_INFO, "EfiBootManagerConnectAll\n"));
>      EfiBootManagerConnectAll ();
>    }
> -
> -  PciAcpiInitialization ();
>  }
>  
>  /**
>    Save the S3 boot script.
>  
>    Note that DxeSmmReadyToLock must be signaled after this function returns;
> @@ -1443,12 +1441,17 @@ PlatformBootManagerAfterConsole (
>  
>    //
>    // Logo show
>    //
>    BootLogoEnableLogo ();
>  
> +  //
> +  // Set PCI Interrupt Line registers and ACPI SCI_EN
> +  //
> +  PciAcpiInitialization ();
> +

Acked-by: Gabriel Somlo <gsomlo@gmail.com>

>    //
>    // Perform some platform specific connect sequence
>    //
>    PlatformBdsConnectSequence ();
>  
>    //
> -- 
> 2.14.1.3.gb7cf6e02401b
> 
> 

Re: [PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[Gabriel L. Somlo]

On Thu, Mar 15, 2018 at 08:02:53PM +0100, Laszlo Ersek wrote:
> (Copying Rich, Xiang and Gabriel for testing requests below.)
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: kernel_before_bootdevs
> 
> After the recent series "OvmfPkg, ArmVirtQemu: leaner platform BDS
> policy for connecting devices", I'm picking up another earlier idea -- a
> direct kernel boot does not need devices such as disks and NICs to be
> bound by UEFI.
> 
> I tested this series extensively on QEMU, in OVMF (IA32X64) and
> ArmVirtQemu (AARCH64), both with and without direct kernel boot. I
> compared the logs in all sensible relations within a given architecture.
> 
> Rich, can you please test this on ARM64, with guestfish/libguestfs?
> Please attach a good number of disks at once on the command line, and
> compare the appliance's boot time between (e.g.) RHEL7's
> "/usr/share/AAVMF/AAVMF_CODE.fd" and the following binary (after
> decompression):
> 
>   https://people.redhat.com/lersek/kernel_before_bootdevs-991e2f2f-64cf-4566-b933-919928e2aa6b/QEMU_EFI.fd.padded.xz
> 
> (That binary corresponds to the branch linked above, cross-built from
> x86_64 with "aarch64-linux-gnu-gcc (GCC) 6.1.1 20160621 (Red Hat Cross
> 6.1.1-2)", using the following options:
> 
>   export GCC5_AARCH64_PREFIX=aarch64-linux-gnu-
>   build --cmd-len=65536 --hash -t GCC5 -b DEBUG -a AARCH64 \
>     -p ArmVirtPkg/ArmVirtQemu.dsc -D DEBUG_PRINT_ERROR_LEVEL=0x80000000
> 
> and then it was padded with zeroes to 64MB.)
> 
> If you have good results, please respond with your Tested-by (which I'll
> apply to patch 1/5, since that's the one that matters for the ARM64
> target).
> 
> Xiang, if you use guestfish (or else direct kernel boot) occasionally,
> then similar testing would be very welcome from your side too.
> 
> Gabriel, I'm CC'ing you on patch 4/5, because it affects code that you
> had originally written. Can you please regression-test this series with
> your usual OVMF environment / guest(s)?

With the series applied on top of all my out-of-tree macboot patches,
OSX (10.12) boots just as well as before, so no regressions as far as
I'm able to tell!

whole series:

Tested-by: Gabriel Somlo <gsomlo@gmail.com>

Thanks,
--G

> Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
> Cc: "Richard W.M. Jones" <rjones@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Xiang Zheng <xiang.zheng@linaro.org>
> 
> Thanks everyone!
> Laszlo
> 
> Laszlo Ersek (5):
>   ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot
>     devices"
>   OvmfPkg/PlatformBootManagerLib: wrap overlong lines in "BdsPlatform.c"
>   OvmfPkg/PlatformBootManagerLib: rejuvenate old-style function comments
>   OvmfPkg/PlatformBootManagerLib: hoist PciAcpiInitialization()
>   OvmfPkg/PlatformBootManagerLib: process "-kernel" before boot devices
> 
>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c |  16 +-
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c   | 262 ++++++++++----------
>  2 files changed, 136 insertions(+), 142 deletions(-)
> 
> -- 
> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[Laszlo Ersek]

On 03/16/18 15:47, Richard W.M. Jones wrote:

>   # dnf install /usr/bin/virt-rescue
>   $ virt-rescue --scratch=100

OK, I had to read up on virt-rescue for this, and I'm (again) impressed
that the virt-* tools are the DeLuxe utilities in this space. :) "You
can get virt-rescue to give you scratch disk(s) to play with.  This is
useful for testing out Linux utilities (see --scratch)". Convenient!

>>   time guestfish --ro \
>>     -a disk1.img \
>>     ... \
>>     -a disk8.img \
>>     launch : quit
>>
>> (I hope this test case is not totally bogus.)
> 
> An easier way is this as follows.  I missed out the --just-add option
> in my previous tests, which causes the test to do a lot more testing.
> With the option it just adds them, launches the appliance and shuts
> down.
> 
> $ time ./test-add-disks --just-add -n 8 
> 
> Results:
> 
> with Fedora AAVMF:  0m11.197s 0m10.075s 0m10.033s
> with your firmware: 0m5.395s 0m5.385s 0m5.354s

That's great; now I feel confident about picking up your T-b! Thank you
for your help!

Laszlo

Re: [PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[Laszlo Ersek]

On 03/16/18 16:29, Gabriel L. Somlo wrote:
> On Thu, Mar 15, 2018 at 08:02:53PM +0100, Laszlo Ersek wrote:

>> Gabriel, I'm CC'ing you on patch 4/5, because it affects code that you
>> had originally written. Can you please regression-test this series with
>> your usual OVMF environment / guest(s)?
> 
> With the series applied on top of all my out-of-tree macboot patches,
> OSX (10.12) boots just as well as before, so no regressions as far as
> I'm able to tell!
> 
> whole series:
> 
> Tested-by: Gabriel Somlo <gsomlo@gmail.com>

Thank you, Gabriel! I'll add that to patches 2 through 5 (the code
subject to patch 1 is not built into the OVMF binary):

>>   ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot
>>     devices"
>>   OvmfPkg/PlatformBootManagerLib: wrap overlong lines in "BdsPlatform.c"
>>   OvmfPkg/PlatformBootManagerLib: rejuvenate old-style function comments
>>   OvmfPkg/PlatformBootManagerLib: hoist PciAcpiInitialization()
>>   OvmfPkg/PlatformBootManagerLib: process "-kernel" before boot devices

Thanks!
Laszlo

Re: [PATCH 0/5] ArmVirtPkg, OvmfPkg: improve firmware duration of direct kernel boot

[Laszlo Ersek]

On 03/15/18 20:02, Laszlo Ersek wrote:
> (Copying Rich, Xiang and Gabriel for testing requests below.)
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: kernel_before_bootdevs
> 
> After the recent series "OvmfPkg, ArmVirtQemu: leaner platform BDS
> policy for connecting devices", I'm picking up another earlier idea -- a
> direct kernel boot does not need devices such as disks and NICs to be
> bound by UEFI.

Thank you all, series pushed: d0976b9acced..a34a88696256.

Laszlo