[PATCH v2 1/7] OvmfPkg/LegacyBios: set NumberBbsEntries to the size of BbsTable

[PATCH v2 1/7] OvmfPkg/LegacyBios: set NumberBbsEntries to the size of BbsTable

[David Woodhouse]

This is hard-coded in the IntThunk structure, and the additional entries
will be needed for other devices like VirtIO and NVMe disks. So admit
that they exist.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
index 05e3ffd2bb..5e795bfe65 100644
--- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
+++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
@@ -565,12 +565,13 @@ ShadowAndStartLegacy16 (
 
   EfiToLegacy16BootTable->BbsTable  = (UINT32)(UINTN)BbsTable;
   Private->BbsTablePtr              = (VOID *) BbsTable;
+
   //
-  // Skip Floppy and possible onboard IDE drives
+  // Populate entire table with BBS_IGNORE_ENTRY
   //
-  EfiToLegacy16BootTable->NumberBbsEntries = 1 + 2 * MAX_IDE_CONTROLLER;
+  EfiToLegacy16BootTable->NumberBbsEntries = MAX_BBS_ENTRIES;
 
-  for (Index = 0; Index < (sizeof (Private->IntThunk->BbsTable) / sizeof (BBS_TABLE)); Index++) {
+  for (Index = 0; Index < MAX_BBS_ENTRIES; Index++) {
     BbsTable[Index].BootPriority = BBS_IGNORE_ENTRY;
   }
   //
-- 
2.21.0

[PATCH v2 2/7] OvmfPkg/LegacyBbs: Add boot entries for VirtIO and NVME devices

[David Woodhouse]

Iterate over the available block devices in much the same way as
BdsLibEnumerateAllBootOption() does, but limiting to those devices
which are PCI-backed, which can be represented in the BbsTable.

One day we might need to extend the BbsTable to allow us to distinguish
between different NVMe namespaces on a device.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c | 157 +++++++++++++++++++++++++-
 1 file changed, 152 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
index 6b1dd344f3..5e4c7a249e 100644
--- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
+++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
@@ -140,10 +140,14 @@ LegacyBiosBuildBbs (
   IN  BBS_TABLE                 *BbsTable
   )
 {
-  UINTN     BbsIndex;
-  HDD_INFO  *HddInfo;
-  UINTN     HddIndex;
-  UINTN     Index;
+  UINTN       BbsIndex;
+  HDD_INFO    *HddInfo;
+  UINTN       HddIndex;
+  UINTN       Index;
+  EFI_HANDLE  *BlockIoHandles;
+  UINTN       NumberBlockIoHandles;
+  UINTN       BlockIndex;
+  EFI_STATUS  Status;
 
   //
   // First entry is floppy.
@@ -252,8 +256,151 @@ LegacyBiosBuildBbs (
     }
   }
 
-  return EFI_SUCCESS;
+  //
+  // Add non-IDE block devices
+  //
+  BbsIndex = HddIndex * 2 + 1;
+
+  Status = gBS->LocateHandleBuffer (
+                  ByProtocol,
+                  &gEfiBlockIoProtocolGuid,
+                  NULL,
+                  &NumberBlockIoHandles,
+                  &BlockIoHandles
+                  );
+
+  if (!EFI_ERROR (Status)) {
+    UINTN                     Removable;
+    EFI_BLOCK_IO_PROTOCOL     *BlkIo;
+    EFI_PCI_IO_PROTOCOL       *PciIo;
+    EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
+    EFI_DEVICE_PATH_PROTOCOL  *DevicePathNode;
+    EFI_HANDLE                PciHandle;
+    UINTN                     SegNum;
+    UINTN                     BusNum;
+    UINTN                     DevNum;
+    UINTN                     FuncNum;
+
+    for (Removable = 0; Removable < 2; Removable++) {
+      for (BlockIndex = 0; BlockIndex < NumberBlockIoHandles; BlockIndex++) {
+        Status = gBS->HandleProtocol (
+                        BlockIoHandles[BlockIndex],
+                        &gEfiBlockIoProtocolGuid,
+                        (VOID **) &BlkIo
+                        );
+
+        if (EFI_ERROR (Status)) {
+          continue;
+        }
+
+        //
+        // Skip the logical partitions
+        //
+        if (BlkIo->Media->LogicalPartition) {
+          continue;
+        }
+
+        //
+        // Skip the fixed block io then the removable block io
+        //
+        if (BlkIo->Media->RemovableMedia == ((Removable == 0) ? FALSE : TRUE)) {
+          continue;
+        }
 
+        //
+        // Get Device Path
+        //
+        Status = gBS->HandleProtocol (
+                        BlockIoHandles[BlockIndex],
+                        &gEfiDevicePathProtocolGuid,
+                        (VOID **) &DevicePath
+                        );
+        if (EFI_ERROR (Status)) {
+          continue;
+        }
+
+        //
+        // Skip ATA devices as they have already been handled
+        //
+        DevicePathNode = DevicePath;
+        while (!IsDevicePathEnd (DevicePathNode)) {
+          if (DevicePathType (DevicePathNode) == MESSAGING_DEVICE_PATH &&
+              DevicePathSubType (DevicePathNode) == MSG_ATAPI_DP) {
+            break;
+          }
+          DevicePathNode = NextDevicePathNode (DevicePathNode);
+        }
+        if (!IsDevicePathEnd (DevicePathNode)) {
+            continue;
+        }
+
+        //
+        //  Locate which PCI device
+        //
+        Status = gBS->LocateDevicePath (
+                        &gEfiPciIoProtocolGuid,
+                        &DevicePath,
+                        &PciHandle
+                        );
+        if (EFI_ERROR (Status)) {
+          continue;
+        }
+
+        Status = gBS->HandleProtocol (
+                        PciHandle,
+                        &gEfiPciIoProtocolGuid,
+                        (VOID **) &PciIo
+                        );
+        if (EFI_ERROR (Status)) {
+          continue;
+        }
+
+        Status = PciIo->GetLocation (
+                          PciIo,
+                          &SegNum,
+                          &BusNum,
+                          &DevNum,
+                          &FuncNum
+                          );
+        if (EFI_ERROR (Status)) {
+          continue;
+        }
+
+        if (SegNum != 0) {
+          DEBUG ((DEBUG_WARN, "CSM cannot use PCI devices in segment %Lu\n",
+            (UINT64) SegNum));
+          continue;
+        }
+
+        DEBUG ((DEBUG_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d\n",
+          BusNum, DevNum, FuncNum));
+
+        BbsTable[BbsIndex].Bus                      = BusNum;
+        BbsTable[BbsIndex].Device                   = DevNum;
+        BbsTable[BbsIndex].Function                 = FuncNum;
+        BbsTable[BbsIndex].Class                    = 1;
+        BbsTable[BbsIndex].SubClass                 = 0x80;
+        BbsTable[BbsIndex].StatusFlags.OldPosition  = 0;
+        BbsTable[BbsIndex].StatusFlags.Reserved1    = 0;
+        BbsTable[BbsIndex].StatusFlags.Enabled      = 0;
+        BbsTable[BbsIndex].StatusFlags.Failed       = 0;
+        BbsTable[BbsIndex].StatusFlags.MediaPresent = 0;
+        BbsTable[BbsIndex].StatusFlags.Reserved2    = 0;
+        BbsTable[BbsIndex].DeviceType               = BBS_HARDDISK;
+        BbsTable[BbsIndex].BootPriority             = BBS_UNPRIORITIZED_ENTRY;
+        BbsIndex++;
+
+        if (BbsIndex == MAX_BBS_ENTRIES) {
+          Removable = 2;
+          break;
+        }
+      }
+    }
+
+    FreePool (BlockIoHandles);
+  }
+
+  return EFI_SUCCESS;
 }
 
 
-- 
2.21.0

[PATCH v2 3/7] OvmfPkg: Don't build in QemuVideoDxe when we have CSM

[David Woodhouse]

QemuVideoDxe installs its own legacy INT 10h handler for the benefit of
systems like Windows 2008r2 which attempt to use INT 10h even when booted
via EFI.

This interacts extremely badly with a CSM actually attempting to install
a real video BIOS.

The last thing done before invoking a legacy OpROM is to call INT 10h to
set a plain text mode. In the case where it's the video BIOS OpROM being
loaded, INT 10h will normally point to an iret stub in the CSM itself.

Unless QemuVideoDxe has changed INT10h to point to a location in the
0xC0000 segment that it didn't allocate properly, so the real OpROM has
been shadowed over them top of it, and the INT 10h vector now points to
some random place in the middle of the newly-shadowed OpROM.

Don't Do That Then. QemuVideoDxe doesn't do any acceleration and just
sets up a linear framebuffer, so we don't lose much by just
unconditionally using BiosVideoDxe instead when CSM is present.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 2 ++
 OvmfPkg/OvmfPkgIa32.fdf    | 3 ++-
 OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
 OvmfPkg/OvmfPkgIa32X64.fdf | 3 ++-
 OvmfPkg/OvmfPkgX64.dsc     | 2 ++
 OvmfPkg/OvmfPkgX64.fdf     | 3 ++-
 6 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 473eaba246..8771612399 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -738,7 +738,9 @@
   MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
   MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
 
+!ifndef $(CSM_ENABLE)
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+!endif
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 14100356b4..785affeb90 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -305,9 +305,10 @@ INF  MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf
 INF  OvmfPkg/Csm/BiosThunk/VideoDxe/VideoDxe.inf
 INF  OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
 INF  RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf
+!else
+INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
 !endif
 
-INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
 INF  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
 INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 INF  OvmfPkg/PlatformDxe/Platform.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 73f33b7218..639e33cb28 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -747,7 +747,9 @@
   MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
   MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
 
+!ifndef $(CSM_ENABLE)
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+!endif
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 5af3cdc93d..7440707256 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -311,9 +311,10 @@ INF  MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf
 INF  OvmfPkg/Csm/BiosThunk/VideoDxe/VideoDxe.inf
 INF  OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
 INF  RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf
+!else
+INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
 !endif
 
-INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
 INF  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
 INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 INF  OvmfPkg/PlatformDxe/Platform.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 39ac791565..69a3497c2c 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -745,7 +745,9 @@
   MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
   MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
 
+!ifndef $(CSM_ENABLE)
   OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+!endif
   OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
   OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 5af3cdc93d..7440707256 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -311,9 +311,10 @@ INF  MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf
 INF  OvmfPkg/Csm/BiosThunk/VideoDxe/VideoDxe.inf
 INF  OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
 INF  RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf
+!else
+INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
 !endif
 
-INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
 INF  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
 INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 INF  OvmfPkg/PlatformDxe/Platform.inf
-- 
2.21.0

[PATCH v2 4/7] MdeModulePkg/UefiBootManagerLib: export EfiBootManagerGetBootDescription()

[David Woodhouse]

It would be useful for LegacyBiosDxe to be able to get descriptive names
for block devices, for legacy boot options. It gets a bit confusing when
they're all called "Harddisk".

Since we have a collection of the special cases for various types of
device already in BmGetBootDescription(), let's export that with a
minor tweak to let the caller set the "UEFI " vs. "Legacy " prefix.

There's no way we want to reproduce all those device-specific special
cases again in the LegacyBiosDxe. It's bad enough that they exist in
UefiBootManagerLib in the first place, instead of being in a protocol
provided by the individual disk drivers themselves.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 .../Include/Library/UefiBootManagerLib.h      | 16 ++++++++++++
 .../UefiBootManagerLib/BmBootDescription.c    | 26 ++++++++++++++++---
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Include/Library/UefiBootManagerLib.h b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
index 0b69a6021d..a9d6bfda88 100644
--- a/MdeModulePkg/Include/Library/UefiBootManagerLib.h
+++ b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
@@ -249,6 +249,22 @@ EfiBootManagerFindLoadOption (
   IN UINTN                              Count
   );
 
+/**
+  Return the boot description for the controller.
+
+  @param Prefix                String prefix (e.g "UEFI " or "Legacy ").
+  @param Handle                Controller handle.
+
+  @return  The description string.
+**/
+CHAR16 *
+EFIAPI
+EfiBootManagerGetBootDescription (
+  IN CHAR16                     *Prefix,
+  IN EFI_HANDLE                  Handle
+  );
+
+
 //
 // Boot Manager hot key library functions.
 //
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
index aa891feb17..06edba8b4d 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
@@ -758,12 +758,15 @@ BM_GET_BOOT_DESCRIPTION mBmBootDescriptionHandlers[] = {
 /**
   Return the boot description for the controller.
 
+  @param Prefix                String prefix (e.g "UEFI " or "Legacy ").
   @param Handle                Controller handle.
 
   @return  The description string.
 **/
 CHAR16 *
-BmGetBootDescription (
+EFIAPI
+EfiBootManagerGetBootDescription (
+  IN CHAR16                     *Prefix,
   IN EFI_HANDLE                  Handle
   )
 {
@@ -785,10 +788,10 @@ BmGetBootDescription (
       // Avoid description confusion between UEFI & Legacy boot option by adding "UEFI " prefix
       // ONLY for core provided boot description handler.
       //
-      Temp = AllocatePool (StrSize (DefaultDescription) + sizeof (mBmUefiPrefix));
+      Temp = AllocatePool (StrSize (DefaultDescription) + StrSize (Prefix));
       ASSERT (Temp != NULL);
-      StrCpyS (Temp, (StrSize (DefaultDescription) + sizeof (mBmUefiPrefix)) / sizeof (CHAR16), mBmUefiPrefix);
-      StrCatS (Temp, (StrSize (DefaultDescription) + sizeof (mBmUefiPrefix)) / sizeof (CHAR16), DefaultDescription);
+      StrCpyS (Temp, (StrSize (DefaultDescription) + StrSize (Prefix)) / sizeof (CHAR16), Prefix);
+      StrCatS (Temp, (StrSize (DefaultDescription) + StrSize (Prefix)) / sizeof (CHAR16), DefaultDescription);
       FreePool (DefaultDescription);
       DefaultDescription = Temp;
       break;
@@ -814,6 +817,21 @@ BmGetBootDescription (
   return DefaultDescription;
 }
 
+/**
+  Return the boot description for the controller, for UEFI boot.
+
+  @param Handle                Controller handle.
+
+  @return  The description string.
+**/
+CHAR16 *
+BmGetBootDescription (
+  IN EFI_HANDLE                  Handle
+  )
+{
+  return EfiBootManagerGetBootDescription (mBmUefiPrefix, Handle);
+}
+
 /**
   Enumerate all boot option descriptions and append " 2"/" 3"/... to make
   unique description.
-- 
2.21.0

[PATCH v2 5/7] OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription()

[David Woodhouse]

No longer call all NVMe & VirtIO devices just "Harddisk". Admittedly,
VirtIO disks are now just called 'Misc Device' instead, but at least
that is now Someone Else's Problem™.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c       | 47 ++++++++++++++++++++-
 OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf |  1 +
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
index 5e4c7a249e..f3cc64f89d 100644
--- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
+++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
@@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include "LegacyBiosInterface.h"
 #include <IndustryStandard/Pci.h>
+#include <Library/UefiBootManagerLib.h>
 
 // Give floppy 3 states
 // FLOPPY_PRESENT_WITH_MEDIA  = Floppy controller present and media is inserted
@@ -280,6 +281,8 @@ LegacyBiosBuildBbs (
     UINTN                     BusNum;
     UINTN                     DevNum;
     UINTN                     FuncNum;
+    CHAR16                    *Description;
+    CHAR8                     AsciiDescription[32];
 
     for (Removable = 0; Removable < 2; Removable++) {
       for (BlockIndex = 0; BlockIndex < NumberBlockIoHandles; BlockIndex++) {
@@ -372,8 +375,48 @@ LegacyBiosBuildBbs (
           continue;
         }
 
-        DEBUG ((DEBUG_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d\n",
-          BusNum, DevNum, FuncNum));
+        Description = EfiBootManagerGetBootDescription(L"Legacy ", BlockIoHandles[BlockIndex]);
+
+        DEBUG ((DEBUG_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d: %s\n",
+          BusNum, DevNum, FuncNum, Description ? Description : L"<No description>"));
+
+        if (Description != NULL) {
+          VOID *BbsDescription;
+
+          //
+          // Truncate Description and convert to ASCII.
+          //
+          if (StrLen (Description) >= sizeof (AsciiDescription)) {
+                  Description[sizeof (AsciiDescription) - 1] = L'0';
+          }
+          UnicodeStrToAsciiStrS (Description, AsciiDescription, sizeof (AsciiDescription));
+
+          //
+          // Copy to low memory and reference from BbsTable
+          //
+          Status = Private->LegacyBios.GetLegacyRegion(
+                                         &Private->LegacyBios,
+                                         AsciiStrSize(AsciiDescription),
+                                         (UINTN)0, /* Any region */
+                                         (UINTN)1, /* Alignment */
+                                         &BbsDescription
+                                         );
+
+          if (!EFI_ERROR (Status)) {
+            Status = Private->LegacyBios.CopyLegacyRegion (
+                                           &Private->LegacyBios,
+                                           AsciiStrSize(AsciiDescription),
+                                           BbsDescription,
+                                           AsciiDescription
+                                           );
+          }
+          if (!EFI_ERROR (Status)) {
+            BbsTable[BbsIndex].DescStringSegment = (UINT16) (((UINTN) BbsDescription >> 16) << 12);
+            BbsTable[BbsIndex].DescStringOffset  = (UINT16) (UINTN) BbsDescription;
+          }
+
+          FreePool (Description);
+        }
 
         BbsTable[BbsIndex].Bus                      = BusNum;
         BbsTable[BbsIndex].Device                   = DevNum;
diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
index f6379dcc46..0b9fef02dc 100644
--- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
+++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
@@ -55,6 +55,7 @@
 [LibraryClasses]
   DevicePathLib
   UefiBootServicesTableLib
+  UefiBootManagerLib
   MemoryAllocationLib
   UefiDriverEntryPoint
   BaseMemoryLib
-- 
2.21.0

[PATCH v2 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[David Woodhouse]

I know, I said it was Someone Else's Problem. But it annoyed me.

My initial thought was to look for VIRTIO_DEVICE_PROTOCOL on the same
handle but I don't think I can do that if I can't rely on VirtIO being
present in the build. This will do.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 .../UefiBootManagerLib/BmBootDescription.c    | 34 +++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
index 06edba8b4d..95adb9a7d3 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
@@ -661,6 +661,8 @@ BmGetMiscDescription (
   CHAR16                          *Description;
   EFI_BLOCK_IO_PROTOCOL           *BlockIo;
   EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *Fs;
+  EFI_PCI_IO_PROTOCOL             *PciIo;
+  PCI_TYPE00                      Pci;
 
   switch (BmDevicePathType (DevicePathFromHandle (Handle))) {
   case BmAcpiFloppyBoot:
@@ -698,9 +700,37 @@ BmGetMiscDescription (
     Status = gBS->HandleProtocol (Handle, &gEfiSimpleFileSystemProtocolGuid, (VOID **) &Fs);
     if (!EFI_ERROR (Status)) {
       Description = L"Non-Block Boot Device";
-    } else {
-      Description = L"Misc Device";
+      break;
+    }
+    Status = gBS->HandleProtocol (Handle, &gEfiPciIoProtocolGuid, (VOID **) &PciIo);
+    if (!EFI_ERROR (Status)) {
+      Status = PciIo->Pci.Read (
+                            PciIo,                        // (protocol, device)
+                                                          // handle
+                            EfiPciIoWidthUint32,          // access width & copy
+                                                          // mode
+                            0,                            // Offset
+                            sizeof Pci / sizeof (UINT32), // Count
+                            &Pci                          // target buffer
+                            );
+      //
+      // If the same node is a Qumranet/Red Hat PCI device, it's VirtIO.
+      //
+      if (!EFI_ERROR (Status) && (Pci.Hdr.VendorId == 0x1AF4))
+        //
+        // Separate checks for legacy/transitional VirtIO vs. Virtio 1.0+
+        //
+        if (((Pci.Hdr.DeviceId >= 0x1000) && (Pci.Hdr.DeviceId <= 0x103F) &&
+             (Pci.Hdr.RevisionID == 0x00)) ||
+            (Pci.Hdr.DeviceId >= 0x1040 && Pci.Hdr.DeviceId <= 0x107F &&
+             Pci.Hdr.RevisionID >= 0x01 && Pci.Device.SubsystemID >= 0x40 &&
+             (Pci.Hdr.Status & EFI_PCI_STATUS_CAPABILITY) != 0)) {
+        Description = L"VirtIO Device";
+        break;
+      }
     }
+
+    Description = L"Misc Device";
     break;
   }
 
-- 
2.21.0

[PATCH v2 7/7] OvmfPkg: don't assign PCI BARs above 4GiB when CSM enabled

[David Woodhouse]

Mostly, this is only necessary for devices that the CSM might have
native support for, such as VirtIO and NVMe; PciBusDxe will already
degrade devices to 32-bit if they have an OpROM.

However, there doesn't seem to be a generic way of requesting PciBusDxe
to downgrade specific devices.

There's IncompatiblePciDeviceSupportProtocol but that doesn't provide
the PCI class information or a handle to the device itself, so there's
no simple way to just match on all NVMe devices, for example.

Just leave gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size set to zero for
CSM builds, until/unless that can be fixed.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
 OvmfPkg/OvmfPkgX64.dsc     | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 639e33cb28..ad20531ceb 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -543,7 +543,11 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0
+!ifdef $(CSM_ENABLE)
+  gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x0
+!else
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
+!endif
 
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
 
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 69a3497c2c..0542ac2235 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -542,7 +542,11 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0
+!ifdef $(CSM_ENABLE)
+  gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x0
+!else
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
+!endif
 
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
 
-- 
2.21.0

Re: [PATCH v2 5/7] OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription()

[Laszlo Ersek]

On 06/25/19 13:48, David Woodhouse wrote:
> No longer call all NVMe & VirtIO devices just "Harddisk". Admittedly,
> VirtIO disks are now just called 'Misc Device' instead, but at least
> that is now Someone Else's Problem™.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
>  OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c       | 47 ++++++++++++++++++++-
>  OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf |  1 +
>  2 files changed, 46 insertions(+), 2 deletions(-)

Please consider including a patch-level changelog (see "git-notes").

> diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
> index 5e4c7a249e..f3cc64f89d 100644
> --- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
> +++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
> @@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  #include "LegacyBiosInterface.h"
>  #include <IndustryStandard/Pci.h>
> +#include <Library/UefiBootManagerLib.h>
>  
>  // Give floppy 3 states
>  // FLOPPY_PRESENT_WITH_MEDIA  = Floppy controller present and media is inserted
> @@ -280,6 +281,8 @@ LegacyBiosBuildBbs (
>      UINTN                     BusNum;
>      UINTN                     DevNum;
>      UINTN                     FuncNum;
> +    CHAR16                    *Description;
> +    CHAR8                     AsciiDescription[32];
>  
>      for (Removable = 0; Removable < 2; Removable++) {
>        for (BlockIndex = 0; BlockIndex < NumberBlockIoHandles; BlockIndex++) {
> @@ -372,8 +375,48 @@ LegacyBiosBuildBbs (
>            continue;
>          }
>  
> -        DEBUG ((DEBUG_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d\n",
> -          BusNum, DevNum, FuncNum));
> +        Description = EfiBootManagerGetBootDescription(L"Legacy ", BlockIoHandles[BlockIndex]);

(1) Missing space character.

> +
> +        DEBUG ((DEBUG_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d: %s\n",
> +          BusNum, DevNum, FuncNum, Description ? Description : L"<No description>"));
> +
> +        if (Description != NULL) {
> +          VOID *BbsDescription;

(2) You removed the NULL-initialization altogether, and the resultant
code remains correct (and it now conforms to the edk2 coding style).

However, this pattern tends to tickle bugs in compiler data flow
analysis. I expect that at least one toolchain will whine about
"BbsDescription" being used without initialization. The toolchain might
not see that reaching CopyLegacyRegion() implies success of
GetLegacyRegion() implies non-NULL-ity of "BbsDescription".

This occurs so frequently that we have some "recommended" comment
format, to document spurious variable assignments (that we do only for
shutting up compilers). See
<https://bugzilla.tianocore.org/show_bug.cgi?id=607>:

  //
  // set BbsDescription to suppress incorrect compiler/analyzer warnings
  //
  BbsDescription = NULL;

Anyway, I don't insist that you add "BbsDescription = NULL;"
immediately, I'm just highlighting a potential build failure.

> +
> +          //
> +          // Truncate Description and convert to ASCII.
> +          //
> +          if (StrLen (Description) >= sizeof (AsciiDescription)) {
> +                  Description[sizeof (AsciiDescription) - 1] = L'0';

(3) Sneaky typo. You mean (and I requested) L'\0'. L'0' is different. :)

> +          }
> +          UnicodeStrToAsciiStrS (Description, AsciiDescription, sizeof (AsciiDescription));
> +
> +          //
> +          // Copy to low memory and reference from BbsTable
> +          //
> +          Status = Private->LegacyBios.GetLegacyRegion(

(4) missing space

> +                                         &Private->LegacyBios,
> +                                         AsciiStrSize(AsciiDescription),

(5) missing space

> +                                         (UINTN)0, /* Any region */
> +                                         (UINTN)1, /* Alignment */
> +                                         &BbsDescription
> +                                         );
> +
> +          if (!EFI_ERROR (Status)) {
> +            Status = Private->LegacyBios.CopyLegacyRegion (
> +                                           &Private->LegacyBios,
> +                                           AsciiStrSize(AsciiDescription),

(6) missing space

> +                                           BbsDescription,
> +                                           AsciiDescription
> +                                           );
> +          }
> +          if (!EFI_ERROR (Status)) {
> +            BbsTable[BbsIndex].DescStringSegment = (UINT16) (((UINTN) BbsDescription >> 16) << 12);
> +            BbsTable[BbsIndex].DescStringOffset  = (UINT16) (UINTN) BbsDescription;
> +          }
> +
> +          FreePool (Description);
> +        }
>  
>          BbsTable[BbsIndex].Bus                      = BusNum;
>          BbsTable[BbsIndex].Device                   = DevNum;
> diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
> index f6379dcc46..0b9fef02dc 100644
> --- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
> +++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
> @@ -55,6 +55,7 @@
>  [LibraryClasses]
>    DevicePathLib
>    UefiBootServicesTableLib
> +  UefiBootManagerLib
>    MemoryAllocationLib
>    UefiDriverEntryPoint
>    BaseMemoryLib
> 

With (1), and (3) through (6), fixed, and with (2) optionally fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH v2 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[Laszlo Ersek]

On 06/25/19 13:48, David Woodhouse wrote:
> I know, I said it was Someone Else's Problem. But it annoyed me.
> 
> My initial thought was to look for VIRTIO_DEVICE_PROTOCOL on the same
> handle but I don't think I can do that if I can't rely on VirtIO being
> present in the build. This will do.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
>  .../UefiBootManagerLib/BmBootDescription.c    | 34 +++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> index 06edba8b4d..95adb9a7d3 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> @@ -661,6 +661,8 @@ BmGetMiscDescription (
>    CHAR16                          *Description;
>    EFI_BLOCK_IO_PROTOCOL           *BlockIo;
>    EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *Fs;
> +  EFI_PCI_IO_PROTOCOL             *PciIo;
> +  PCI_TYPE00                      Pci;
>  
>    switch (BmDevicePathType (DevicePathFromHandle (Handle))) {
>    case BmAcpiFloppyBoot:
> @@ -698,9 +700,37 @@ BmGetMiscDescription (
>      Status = gBS->HandleProtocol (Handle, &gEfiSimpleFileSystemProtocolGuid, (VOID **) &Fs);
>      if (!EFI_ERROR (Status)) {
>        Description = L"Non-Block Boot Device";
> -    } else {
> -      Description = L"Misc Device";
> +      break;
> +    }
> +    Status = gBS->HandleProtocol (Handle, &gEfiPciIoProtocolGuid, (VOID **) &PciIo);
> +    if (!EFI_ERROR (Status)) {
> +      Status = PciIo->Pci.Read (
> +                            PciIo,                        // (protocol, device)
> +                                                          // handle
> +                            EfiPciIoWidthUint32,          // access width & copy
> +                                                          // mode
> +                            0,                            // Offset
> +                            sizeof Pci / sizeof (UINT32), // Count
> +                            &Pci                          // target buffer
> +                            );
> +      //
> +      // If the same node is a Qumranet/Red Hat PCI device, it's VirtIO.
> +      //
> +      if (!EFI_ERROR (Status) && (Pci.Hdr.VendorId == 0x1AF4))
> +        //
> +        // Separate checks for legacy/transitional VirtIO vs. Virtio 1.0+
> +        //
> +        if (((Pci.Hdr.DeviceId >= 0x1000) && (Pci.Hdr.DeviceId <= 0x103F) &&
> +             (Pci.Hdr.RevisionID == 0x00)) ||
> +            (Pci.Hdr.DeviceId >= 0x1040 && Pci.Hdr.DeviceId <= 0x107F &&
> +             Pci.Hdr.RevisionID >= 0x01 && Pci.Device.SubsystemID >= 0x40 &&
> +             (Pci.Hdr.Status & EFI_PCI_STATUS_CAPABILITY) != 0)) {
> +        Description = L"VirtIO Device";
> +        break;
> +      }

This code is functionally correct, but there are two syntactical issues
with it.

(1) The last three lines -- the assignment to Description, the break
statement, and the closing brace -- should be indented one level more
deeply.

(2) Which in turn shows that the "if" statement that checks "Status" and
"VendorId" lacks an opening brace. The brace is required by the edk2
coding style.

>      }
> +
> +    Description = L"Misc Device";
>      break;
>    }
>  
> 

With those fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH v2 7/7] OvmfPkg: don't assign PCI BARs above 4GiB when CSM enabled

[Laszlo Ersek]

On 06/25/19 13:48, David Woodhouse wrote:
> Mostly, this is only necessary for devices that the CSM might have
> native support for, such as VirtIO and NVMe; PciBusDxe will already
> degrade devices to 32-bit if they have an OpROM.
> 
> However, there doesn't seem to be a generic way of requesting PciBusDxe
> to downgrade specific devices.
> 
> There's IncompatiblePciDeviceSupportProtocol but that doesn't provide
> the PCI class information or a handle to the device itself, so there's
> no simple way to just match on all NVMe devices, for example.
> 
> Just leave gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size set to zero for
> CSM builds, until/unless that can be fixed.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
>  OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
>  OvmfPkg/OvmfPkgX64.dsc     | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 639e33cb28..ad20531ceb 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -543,7 +543,11 @@
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0
> +!ifdef $(CSM_ENABLE)
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x0
> +!else
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
> +!endif
>  
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>  
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 69a3497c2c..0542ac2235 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -542,7 +542,11 @@
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0
> +!ifdef $(CSM_ENABLE)
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x0
> +!else
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
> +!endif
>  
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>  
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

Re: [edk2-devel] [PATCH v2 5/7] OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1622 bytes --]

On Tue, 2019-06-25 at 15:29 +0200, Laszlo Ersek wrote:
> (2) You removed the NULL-initialization altogether, 

Well yeah, you told me the EDK-II coding standards forbid the common
defensive coding technique used in the language called "C", of
initialising the variable where it's declared. I wasn't up for arguing
about it, so I just stopped doing it.

Now you tell me to put it back, just in a more cumbersome way... OK,
I'll do that too.

I'm not sure the answer is going to make me any happier... but *why*
are we not allowed to just initialise variables where they're declared?

> > +
> > +          //
> > +          // Truncate Description and convert to ASCII.
> > +          //
> > +          if (StrLen (Description) >= sizeof (AsciiDescription)) {
> > +                  Description[sizeof (AsciiDescription) - 1] = L'0';
> 
> (3) Sneaky typo. You mean (and I requested) L'\0'. L'0' is different. :)

Oops :)

What was it I said about testing of corner cases when
updating/rebasing? I had explicitly tested this by cutting the size
down to 16 and watching it actually truncate. Obviously I didn't do
*that* again this time round (I have now!)

Well spotted; thanks!

I've pushed all the fixes you've just pointed out (thanks again) to my
'csm' branch. Won't send another series by email today; I'll let the
dust settle and some of the other discussions continue.

When I do repost, I may send the required functional patches 1-3,7 on
their own as I think they're ready to apply as-is, and leave the
cosmetic patches 4-6 in a separate series for further bikeshedding.




[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v2 5/7] OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription()

[Laszlo Ersek]

On 06/25/19 16:10, David Woodhouse wrote:
> On Tue, 2019-06-25 at 15:29 +0200, Laszlo Ersek wrote:
>> (2) You removed the NULL-initialization altogether, 
> 
> Well yeah, you told me the EDK-II coding standards forbid the common
> defensive coding technique used in the language called "C", of
> initialising the variable where it's declared. I wasn't up for arguing
> about it, so I just stopped doing it.
> 
> Now you tell me to put it back, just in a more cumbersome way... OK,
> I'll do that too.

For the record, I wrote:

    The edk2 coding style forbids initialization of locals. Please use a
    separate assignment if necessary.

I guess I didn't foresee that you'd decide it wasn't necessary after all
:) If I had thought of that, I could have dropped "if necessary".

> I'm not sure the answer is going to make me any happier... but *why*
> are we not allowed to just initialise variables where they're declared?

I don't remember. I had simply accepted this guideline years ago and
haven't gone back to question it since. There's no time to fight the
same old battles again and again. :)

>>> +
>>> +          //
>>> +          // Truncate Description and convert to ASCII.
>>> +          //
>>> +          if (StrLen (Description) >= sizeof (AsciiDescription)) {
>>> +                  Description[sizeof (AsciiDescription) - 1] = L'0';
>>
>> (3) Sneaky typo. You mean (and I requested) L'\0'. L'0' is different. :)
> 
> Oops :)
> 
> What was it I said about testing of corner cases when
> updating/rebasing? I had explicitly tested this by cutting the size
> down to 16 and watching it actually truncate. Obviously I didn't do
> *that* again this time round (I have now!)
> 
> Well spotted; thanks!
> 
> I've pushed all the fixes you've just pointed out (thanks again) to my
> 'csm' branch. Won't send another series by email today; I'll let the
> dust settle and some of the other discussions continue.

I agree, let's give Ray and others time to react. My comments have been
mostly cosmetic in this round anyway.

> When I do repost, I may send the required functional patches 1-3,7 on
> their own as I think they're ready to apply as-is, and leave the
> cosmetic patches 4-6 in a separate series for further bikeshedding.

1-3,7 only touch OvmfPkg, so I could push those quickly then.

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v2 4/7] MdeModulePkg/UefiBootManagerLib: export EfiBootManagerGetBootDescription()

[Ni, Ray]

David,
I agree to expose *GetBootDescription() API.
But can you remove the 1st parameter Prefix and let caller to pre-pend it after calling this API?

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> David Woodhouse
> Sent: Tuesday, June 25, 2019 7:49 PM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH v2 4/7] MdeModulePkg/UefiBootManagerLib:
> export EfiBootManagerGetBootDescription()
> 
> It would be useful for LegacyBiosDxe to be able to get descriptive names for
> block devices, for legacy boot options. It gets a bit confusing when they're all
> called "Harddisk".
> 
> Since we have a collection of the special cases for various types of device
> already in BmGetBootDescription(), let's export that with a minor tweak to
> let the caller set the "UEFI " vs. "Legacy " prefix.
> 
> There's no way we want to reproduce all those device-specific special cases
> again in the LegacyBiosDxe. It's bad enough that they exist in
> UefiBootManagerLib in the first place, instead of being in a protocol
> provided by the individual disk drivers themselves.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  .../Include/Library/UefiBootManagerLib.h      | 16 ++++++++++++
>  .../UefiBootManagerLib/BmBootDescription.c    | 26 ++++++++++++++++---
>  2 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Include/Library/UefiBootManagerLib.h
> b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
> index 0b69a6021d..a9d6bfda88 100644
> --- a/MdeModulePkg/Include/Library/UefiBootManagerLib.h
> +++ b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
> @@ -249,6 +249,22 @@ EfiBootManagerFindLoadOption (
>    IN UINTN                              Count
>    );
> 
> +/**
> +  Return the boot description for the controller.
> +
> +  @param Prefix                String prefix (e.g "UEFI " or "Legacy ").
> +  @param Handle                Controller handle.
> +
> +  @return  The description string.
> +**/
> +CHAR16 *
> +EFIAPI
> +EfiBootManagerGetBootDescription (
> +  IN CHAR16                     *Prefix,
> +  IN EFI_HANDLE                  Handle
> +  );
> +
> +
>  //
>  // Boot Manager hot key library functions.
>  //
> diff --git
> a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> index aa891feb17..06edba8b4d 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> @@ -758,12 +758,15 @@ BM_GET_BOOT_DESCRIPTION
> mBmBootDescriptionHandlers[] = {
>  /**
>    Return the boot description for the controller.
> 
> +  @param Prefix                String prefix (e.g "UEFI " or "Legacy ").
>    @param Handle                Controller handle.
> 
>    @return  The description string.
>  **/
>  CHAR16 *
> -BmGetBootDescription (
> +EFIAPI
> +EfiBootManagerGetBootDescription (
> +  IN CHAR16                     *Prefix,
>    IN EFI_HANDLE                  Handle
>    )
>  {
> @@ -785,10 +788,10 @@ BmGetBootDescription (
>        // Avoid description confusion between UEFI & Legacy boot option by
> adding "UEFI " prefix
>        // ONLY for core provided boot description handler.
>        //
> -      Temp = AllocatePool (StrSize (DefaultDescription) + sizeof
> (mBmUefiPrefix));
> +      Temp = AllocatePool (StrSize (DefaultDescription) + StrSize
> + (Prefix));
>        ASSERT (Temp != NULL);
> -      StrCpyS (Temp, (StrSize (DefaultDescription) + sizeof (mBmUefiPrefix)) /
> sizeof (CHAR16), mBmUefiPrefix);
> -      StrCatS (Temp, (StrSize (DefaultDescription) + sizeof (mBmUefiPrefix)) /
> sizeof (CHAR16), DefaultDescription);
> +      StrCpyS (Temp, (StrSize (DefaultDescription) + StrSize (Prefix)) / sizeof
> (CHAR16), Prefix);
> +      StrCatS (Temp, (StrSize (DefaultDescription) + StrSize (Prefix))
> + / sizeof (CHAR16), DefaultDescription);
>        FreePool (DefaultDescription);
>        DefaultDescription = Temp;
>        break;
> @@ -814,6 +817,21 @@ BmGetBootDescription (
>    return DefaultDescription;
>  }
> 
> +/**
> +  Return the boot description for the controller, for UEFI boot.
> +
> +  @param Handle                Controller handle.
> +
> +  @return  The description string.
> +**/
> +CHAR16 *
> +BmGetBootDescription (
> +  IN EFI_HANDLE                  Handle
> +  )
> +{
> +  return EfiBootManagerGetBootDescription (mBmUefiPrefix, Handle); }
> +
>  /**
>    Enumerate all boot option descriptions and append " 2"/" 3"/... to make
>    unique description.
> --
> 2.21.0
> 
> 
>