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

[PATCH 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
index 05e3ffd2bb..69abd06c40 100644
--- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
+++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c
@@ -568,7 +568,7 @@ ShadowAndStartLegacy16 (
   //
   // Skip Floppy and possible onboard IDE drives
   //
-  EfiToLegacy16BootTable->NumberBbsEntries = 1 + 2 * MAX_IDE_CONTROLLER;
+  EfiToLegacy16BootTable->NumberBbsEntries = sizeof(Private->IntThunk->BbsTable) / sizeof(BBS_TABLE);
 
   for (Index = 0; Index < (sizeof (Private->IntThunk->BbsTable) / sizeof (BBS_TABLE)); Index++) {
     BbsTable[Index].BootPriority = BBS_IGNORE_ENTRY;
-- 
2.21.0

[PATCH 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 | 162 +++++++++++++++++++++++++-
 1 file changed, 157 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
index 6b1dd344f3..cc84712d25 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,156 @@ 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) {
+          DEBUG((EFI_D_INFO, "Partition\n"));
+          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((EFI_D_INFO, "CSM cannot use PCI devices in segment %d\n", SegNum));
+          continue;
+        }
+
+        DEBUG_CODE (
+          CHAR16 *PathText;
+
+          PathText = ConvertDevicePathToText(DevicePath, FALSE, FALSE);
+
+          DEBUG((EFI_D_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d: %s\n",
+                 BusNum, DevNum, FuncNum, PathText));
+          FreePool(PathText);
+        );
+
+        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 == sizeof(Private->IntThunk->BbsTable) / sizeof(BBS_TABLE)) {
+          Removable = 2;
+          break;
+        }
+      }
+    }
+
+    FreePool (BlockIoHandles);
+  }
+
+  return EFI_SUCCESS;
 }
 
 
-- 
2.21.0

[PATCH 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 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>
---
 .../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..dd4d160f31 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 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       | 46 ++++++++++++++++++++-
 OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf |  1 +
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
index cc84712d25..0e9896e102 100644
--- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
+++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
@@ -8,6 +8,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include "LegacyBiosInterface.h"
 #include <IndustryStandard/Pci.h>
+#include <Library/UefiBootManagerLib.h>
+
+#define  LEGACY_BBS_BOOT_DESCRIPTION_LENGTH  32
 
 // Give floppy 3 states
 // FLOPPY_PRESENT_WITH_MEDIA  = Floppy controller present and media is inserted
@@ -279,6 +282,8 @@ LegacyBiosBuildBbs (
     UINTN                     BusNum;
     UINTN                     DevNum;
     UINTN                     FuncNum;
+    CHAR16                    *Description;
+    CHAR8                     DescriptionA[LEGACY_BBS_BOOT_DESCRIPTION_LENGTH];
 
     for (Removable = 0; Removable < 2; Removable++) {
       for (BlockIndex = 0; BlockIndex < NumberBlockIoHandles; BlockIndex++) {
@@ -370,16 +375,55 @@ LegacyBiosBuildBbs (
           continue;
         }
 
+        Description = EfiBootManagerGetBootDescription(L"Legacy ", BlockIoHandles[BlockIndex]);
+
         DEBUG_CODE (
           CHAR16 *PathText;
 
           PathText = ConvertDevicePathToText(DevicePath, FALSE, FALSE);
 
           DEBUG((EFI_D_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d: %s\n",
-                 BusNum, DevNum, FuncNum, PathText));
+                 BusNum, DevNum, FuncNum, Description ? Description : L"<No description>"));
           FreePool(PathText);
         );
 
+        if (Description != NULL) {
+          VOID *BbsDescription = NULL;
+	  //
+	  // Truncate Description and convert to ASCII.
+	  //
+	  if (StrLen (Description) >= LEGACY_BBS_BOOT_DESCRIPTION_LENGTH) {
+		  Description[LEGACY_BBS_BOOT_DESCRIPTION_LENGTH - 1] = 0;
+	  }
+          UnicodeStrToAsciiStrS (Description, DescriptionA, sizeof (DescriptionA));
+
+	  //
+	  // Copy to low memory and reference from BbsTable
+	  //
+          Status = Private->LegacyBios.GetLegacyRegion(
+                                         &Private->LegacyBios,
+                                         AsciiStrSize(DescriptionA),
+                                         (UINTN)0, /* Any region */
+                                         (UINTN)1, /* Alignment */
+                                         &BbsDescription
+                                         );
+
+          if (!EFI_ERROR (Status)) {
+            Status = Private->LegacyBios.CopyLegacyRegion (
+                                           &Private->LegacyBios,
+                                           AsciiStrSize(DescriptionA),
+                                           BbsDescription,
+                                           DescriptionA
+                                           );
+          }
+          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;
         BbsTable[BbsIndex].Function                 = FuncNum;
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 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    | 30 +++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
index dd4d160f31..b7d9e98790 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,33 @@ 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) &&
+          (Pci.Hdr.DeviceId >= 0x1000) &&
+          (Pci.Hdr.DeviceId <= 0x103F) &&
+          (Pci.Hdr.RevisionID == 0x00)) {
+        Description = L"VirtIO Device";
+        break;
+      }
     }
+
+    Description = L"Misc Device";
     break;
   }
 
-- 
2.21.0

[PATCH 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 | 2 ++
 OvmfPkg/OvmfPkgX64.dsc     | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 639e33cb28..8fbe601386 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -542,8 +542,10 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0
+!ifndef $(CSM_ENABLE)
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
+!endif
 
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
 
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 69a3497c2c..6f15f11220 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -541,8 +541,10 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0
+!ifndef $(CSM_ENABLE)
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
+!endif
 
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
 
-- 
2.21.0

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

[Laszlo Ersek]

On 06/22/19 00:31, David Woodhouse wrote:
> 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>
> ---
>  .../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..dd4d160f31 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;

I first thought this would be redundant wrt. platform description
handlers registered with EfiBootManagerRegisterBootDescriptionHandler(),
which BmGetBootDescription() asks for "better" descriptions.

But, in such a platform callback, we'd still only want to rely on
"mBmBootDescriptionHandlers", just with a different prefix. So this idea
looks fine to me, after all.

> @@ -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);

I noticed this elsewhere (in earlier patches in this series) but I
didn't want to annoy you with obsessing about it :)

(1) Please insert a space character before the opening paren, after the
function designator. (Applies to the sizeof operator, and to
function-like macro invocations, too.)

With this fixed:

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

Thanks

> +}
> +
>  /**
>    Enumerate all boot option descriptions and append " 2"/" 3"/... to make
>    unique description.
> 

Re: [edk2-devel] [PATCH 2/7] OvmfPkg/LegacyBbs: Add boot entries for VirtIO and NVME devices

[Laszlo Ersek]

On 06/22/19 00:31, David Woodhouse wrote:
> 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 | 162 +++++++++++++++++++++++++-
>  1 file changed, 157 insertions(+), 5 deletions(-)

Some second-wave comments on this. Feel free to ignore, given that most
of this code is being copied (as the commit message states). But, I'll
feel better after having them pointed out :)

> diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
> index 6b1dd344f3..cc84712d25 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,156 @@ 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)) {

(1) Whitespace between function designator / macro name / sizeof
operator, and opening paren, please. (Applies elsewhere too.)

> +    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) {
> +          DEBUG((EFI_D_INFO, "Partition\n"));

(2) In new code, we use DEBUG_, not EFI_D_.

> +          continue;
> +        }
> +
> +        //
> +        // Skip the fixed block io then the removable block io
> +        //
> +        if (BlkIo->Media->RemovableMedia == ((Removable == 0) ? FALSE : TRUE)) {

(3) Could be simplified as

  BlkIo->Media->RemovableMedia == (Removable != 0)

> +          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((EFI_D_INFO, "CSM cannot use PCI devices in segment %d\n", SegNum));

(4) Might deserve DEBUG_WARN

> +          continue;
> +        }
> +
> +        DEBUG_CODE (
> +          CHAR16 *PathText;
> +
> +          PathText = ConvertDevicePathToText(DevicePath, FALSE, FALSE);
> +
> +          DEBUG((EFI_D_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d: %s\n",
> +                 BusNum, DevNum, FuncNum, PathText));

(5) This doesn't handle (PathText == NULL).

(6) DEBUG_VERBOSE might be more appropriate.

(7) UINTN values shouldn't be printed with %d; they should be cast to
UINT64, and printed with %Lu.

Feel free to address any number of these (including zero); and keep the ACK.

Thanks
Laszlo

> +          FreePool(PathText);
> +        );
> +
> +        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 == sizeof(Private->IntThunk->BbsTable) / sizeof(BBS_TABLE)) {
> +          Removable = 2;
> +          break;
> +        }
> +      }
> +    }
> +
> +    FreePool (BlockIoHandles);
> +  }
> +
> +  return EFI_SUCCESS;
>  }
>  
>  
> 

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

[Laszlo Ersek]

On 06/22/19 00:31, 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       | 46 ++++++++++++++++++++-
>  OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf |  1 +
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
> index cc84712d25..0e9896e102 100644
> --- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
> +++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c
> @@ -8,6 +8,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  #include "LegacyBiosInterface.h"
>  #include <IndustryStandard/Pci.h>
> +#include <Library/UefiBootManagerLib.h>
> +
> +#define  LEGACY_BBS_BOOT_DESCRIPTION_LENGTH  32

(1) If you mean to include the terminating NUL, it's likely better to
call this _SIZE, not _LENGTH.

... in fact, could we drop LEGACY_BBS_BOOT_DESCRIPTION_LENGTH
altogether, open-code 32 in the definition of "DescriptionA", and use
"sizeof DescriptionA" everywhere? Just an idea.

>  
>  // Give floppy 3 states
>  // FLOPPY_PRESENT_WITH_MEDIA  = Floppy controller present and media is inserted
> @@ -279,6 +282,8 @@ LegacyBiosBuildBbs (
>      UINTN                     BusNum;
>      UINTN                     DevNum;
>      UINTN                     FuncNum;
> +    CHAR16                    *Description;
> +    CHAR8                     DescriptionA[LEGACY_BBS_BOOT_DESCRIPTION_LENGTH];

(2) I think the edk2 coding style suggests "AsciiDescription" for this.

>  
>      for (Removable = 0; Removable < 2; Removable++) {
>        for (BlockIndex = 0; BlockIndex < NumberBlockIoHandles; BlockIndex++) {
> @@ -370,16 +375,55 @@ LegacyBiosBuildBbs (
>            continue;
>          }
>  
> +        Description = EfiBootManagerGetBootDescription(L"Legacy ", BlockIoHandles[BlockIndex]);
> +
>          DEBUG_CODE (
>            CHAR16 *PathText;
>  
>            PathText = ConvertDevicePathToText(DevicePath, FALSE, FALSE);
>  
>            DEBUG((EFI_D_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d: %s\n",
> -                 BusNum, DevNum, FuncNum, PathText));
> +                 BusNum, DevNum, FuncNum, Description ? Description : L"<No description>"));

(3) EfiBootManagerGetBootDescription() currently guarantees that NULL is
never returned. Should we make that part of the interface contract? And
simplify the code here?

>            FreePool(PathText);
>          );
>  
> +        if (Description != NULL) {
> +          VOID *BbsDescription = NULL;

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

> +	  //

(5) This line seems to use a hardware tab. Please use spaces only.
(Please check all the patches.)

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

(6) For readability, I'd suggest L'\0'.

> +	  }
> +          UnicodeStrToAsciiStrS (Description, DescriptionA, sizeof (DescriptionA));
> +
> +	  //
> +	  // Copy to low memory and reference from BbsTable
> +	  //



> +          Status = Private->LegacyBios.GetLegacyRegion(
> +                                         &Private->LegacyBios,
> +                                         AsciiStrSize(DescriptionA),
> +                                         (UINTN)0, /* Any region */
> +                                         (UINTN)1, /* Alignment */
> +                                         &BbsDescription
> +                                         );
> +
> +          if (!EFI_ERROR (Status)) {
> +            Status = Private->LegacyBios.CopyLegacyRegion (
> +                                           &Private->LegacyBios,
> +                                           AsciiStrSize(DescriptionA),
> +                                           BbsDescription,
> +                                           DescriptionA
> +                                           );
> +          }
> +          if (!EFI_ERROR (Status)) {
> +            BbsTable[BbsIndex].DescStringSegment = (UINT16) (((UINTN) BbsDescription >> 16) << 12);
> +            BbsTable[BbsIndex].DescStringOffset  = (UINT16) (UINTN) BbsDescription;
> +          }

Hmmm OK there is no way to release memory allocated by
GetLegacyRegion(), in the (theoretical) case when CopyLegacyRegion() fails.

> +
> +          FreePool (Description);
> +        }
> +
>          BbsTable[BbsIndex].Bus                      = BusNum;
>          BbsTable[BbsIndex].Device                   = DevNum;
>          BbsTable[BbsIndex].Function                 = FuncNum;
> 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
> 

Thanks
Laszlo

Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[Laszlo Ersek]

On 06/22/19 00:31, 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    | 30 +++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> index dd4d160f31..b7d9e98790 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,33 @@ 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
> +                            );

(1) Can we save some cycles by reading just
PCI_DEVICE_INDEPENDENT_REGION, rather than PCI_TYPE00? It would not
complicate the code either.

> +      //
> +      // If the same node is a Qumranet/Red Hat PCI device, it's VirtIO.
> +      //
> +      if (!EFI_ERROR (Status) &&
> +          (Pci.Hdr.VendorId == 0x1AF4) &&
> +          (Pci.Hdr.DeviceId >= 0x1000) &&
> +          (Pci.Hdr.DeviceId <= 0x103F) &&
> +          (Pci.Hdr.RevisionID == 0x00)) {

(2) This will match legacy and transitional virtio devices, but not
modern-only virtio devices. (SeaBIOS supports modern-only devices too.)
Please see the check for the latter in Virtio10BindingSupported().

(Admittedly, "Pci.Device.SubsystemID" cannot be determined from
PCI_DEVICE_INDEPENDENT_REGION...)


In general I think this approach is viable; at the worst we might have
to gate the code with a Feature PCD. Let's see what Ray says.

Thanks
Laszlo

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

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

[Laszlo Ersek]

On 06/22/19 00:31, 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 | 2 ++
>  OvmfPkg/OvmfPkgX64.dsc     | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 639e33cb28..8fbe601386 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -542,8 +542,10 @@
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0
> +!ifndef $(CSM_ENABLE)
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
> +!endif
>  
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>  
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 69a3497c2c..6f15f11220 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -541,8 +541,10 @@
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size|0x0
> +!ifndef $(CSM_ENABLE)
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
> +!endif
>  
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>  
> 

(1) Side request -- can you please set the "xfuncname" knob so that the
diff hunk headers (@@) reflect the section name being modified?

It's explained at
<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>,
and it can be automated by "BaseTools/Scripts/SetupGit.py".

(2) For the patch. I'd prefer
- setting PcdPciMmio64Base to zero unconditionally, and
- setting PcdPciMmio64Size unconditionally as well, just to different
values, dependent on CSM_ENABLE.

The reason is that there is a difference between setting a default for a
dynamic PCD in a platform DSC, and inheriting the PCD as-is (with the
same value) from a package DEC file. The difference is that in the
latter case, the platform doesn't directly choose an access method (here
"dynamic") for the PCD, and then the access method is "deduced".

This deduction is not trivial:

https://edk2-docs.gitbooks.io/edk-ii-build-specification/content/v/release/1.28/8_pre-build_autogen_stage/84_auto-generated_pcd_database_file.html

so I can't guarantee, without checking the build report, that the above
patch would keep the access method "dynamic", for the PCDs in question.

And, in OvmfPkg/PlatformPei, we have PcdSet64S() calls for
PcdPciMmio64Size that are actually reachable in the IA32X64 and X64
builds, dependent on the QEMU command line (they are not reachable in
the IA32 build).

If possible, I wouldn't like to introduce an avenue to trip an
ASSERT_RETURN_ERROR(). If we continue specifying the access method in
the DSC files, just change the default value to 0, then PcdSet64S() is
guaranteed to succeed, *plus* that fact will not be difficult to see.

(It's a different question whether it makes sense for a user to fiddle
with "opt/ovmf/X-PciMmio64Mb" in a CSM-enabled build... But a failed
assertion is usually one of the less graceful behaviors.)

Thanks
Laszlo

Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[Ni, Ray]

> 
> 
> In general I think this approach is viable; at the worst we might have to gate
> the code with a Feature PCD. Let's see what Ray says.
> 
> Thanks
> Laszlo
> 
> > +        Description = L"VirtIO Device";
> > +        break;
> > +      }
> >      }
> > +
> > +    Description = L"Misc Device";
> >      break;
> >    }
> >
> >
Can EfiBootManagerRegisterBootDescriptionHandler() be used to extend the support for VirtIO
in PlatformBootManagerLib?

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

[Ni, Ray]

David,
I am afraid it will cause issues when exposing EfiBootManagerGetBootDescription().
If you check the implementation, this API visits mPlatformBootDescriptionHandlers.
mPlatformBootDescriptionHandlers is modified by another already-exposed API
EfiBootManagerRegisterBootDescriptionHandler().

The *Register* API is to provide a capability to PlatformBootManagerLib to create
boot option description for special/platform-specific boot options.

But the implicit requirement is boot option description can only be retrieved within
BdsDxe driver because only BdsDxe driver links to PlatformBootManagerLib.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of David
> Woodhouse
> Sent: Saturday, June 22, 2019 6:32 AM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH 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>
> ---
>  .../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..dd4d160f31 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
> 
> 
> 

Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[David Woodhouse]

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

On Tue, 2019-06-25 at 01:44 +0000, Ni, Ray wrote:
> Can EfiBootManagerRegisterBootDescriptionHandler() be used to extend the support for VirtIO
> in PlatformBootManagerLib?

Potentially.... although those are handled differently and not prefixed
with "UEFI " (or "Legacy " after my patch series). It looked like they
weren't for block devices, but other special targets.


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

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

[David Woodhouse]

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

On Tue, 2019-06-25 at 02:00 +0000, Ni, Ray wrote:
> David,
> I am afraid it will cause issues when exposing EfiBootManagerGetBootDescription().
> If you check the implementation, this API visits mPlatformBootDescriptionHandlers.
> mPlatformBootDescriptionHandlers is modified by another already-exposed API
> EfiBootManagerRegisterBootDescriptionHandler().
> 
> The *Register* API is to provide a capability to PlatformBootManagerLib to create
> boot option description for special/platform-specific boot options.
> 
> But the implicit requirement is boot option description can only be retrieved within
> BdsDxe driver because only BdsDxe driver links to PlatformBootManagerLib.

Hm, I'm not sure I fully understand the reason why the transitive
dependency isn't OK, but neither is are the "special/platform-specific" 
boot options relevant for the CSM boot. So I'm happy to drop that part
from the exported EfiBootManagerGetBootDescription() function and do it
only in the internal BmGetBootDescription().




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

Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[Ni, Ray]

The *Register* API was invented to handle the situation that platform wants
to have a special name for certain boot options.
I think you can use that.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of David
> Woodhouse
> Sent: Tuesday, June 25, 2019 3:40 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; lersek@redhat.com
> Subject: Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib:
> describe VirtIO devices correctly
> 
> On Tue, 2019-06-25 at 01:44 +0000, Ni, Ray wrote:
> > Can EfiBootManagerRegisterBootDescriptionHandler() be used to extend
> the support for VirtIO
> > in PlatformBootManagerLib?
> 
> Potentially.... although those are handled differently and not prefixed
> with "UEFI " (or "Legacy " after my patch series). It looked like they
> weren't for block devices, but other special targets.
> 
> 
> 

Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[David Woodhouse]

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

On Tue, 2019-06-25 at 08:06 +0000, Ni, Ray wrote:
> The *Register* API was invented to handle the situation that platform wants
> to have a special name for certain boot options.
> I think you can use that.

Except didn't I just agree to stop calling those registered handlers
from the exported EfiBootManagerGetBootDescription() function, because
of the transitive dependencies?

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

Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[Ni, Ray]

David,
That will cause confusion to caller of *GetBootOption().
Because two behaviors will be seen:
1. BdsDxe calls this API to get a platform customized name for a certain option.
2. Another module calls this API to get a "Misc" name for a certain option because
it doesn't link to PlatformBootManagerLib.

Wait a sec, perhaps I am over worried.
If the NULL class library calls *Register* API, then any consumer of *GetBootOption() API
can link that NULL class library so the consistent boot description can be achieved.

But I still need to understand why the *GetBootOption() API is needed.
Because for quite a long time since the MdeModulePkg/Bds was added, there is no
such requirement.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of David
> Woodhouse
> Sent: Tuesday, June 25, 2019 4:29 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; lersek@redhat.com
> Subject: Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib:
> describe VirtIO devices correctly
> 
> On Tue, 2019-06-25 at 08:06 +0000, Ni, Ray wrote:
> > The *Register* API was invented to handle the situation that platform
> wants
> > to have a special name for certain boot options.
> > I think you can use that.
> 
> Except didn't I just agree to stop calling those registered handlers
> from the exported EfiBootManagerGetBootDescription() function, because
> of the transitive dependencies?
> 
> 

Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[David Woodhouse]

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

On Tue, 2019-06-25 at 09:15 +0000, Ni, Ray wrote:
> But I still need to understand why the *GetBootOption() API is needed.
> Because for quite a long time since the MdeModulePkg/Bds was added, there is no
> such requirement.

It's for CSM, because otherwise all the legacy boot targets other than
IDE are just shown as 'Harddisk'.

See patch [5/7] in this thread which uses the GetBootDescription API
that [4/7] exposes.


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

Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[Ni, Ray]

David,
Thanks for pointing that patch.

Now I understand it.
Normally it's the CSM16 code that builds the boot descriptions for legacy boot options
and LegacyBootManagerLib consumes that boot descriptions.

But in your case, LegacyBios driver builds the boot descriptions for VirtIo devices and
LegacyBootManagerLib consumes that boot descriptions.

The flow is like below: (I understand there is no VirtIoBootOptionDescriptionHandler() in
your current patch. But I think we could write such handler and register it through
EfiBootManagerRegisterBootDescriptionHandler() API)

  *module*          *UefiBootManagerLib*
LegacyBios -> GetBootOptionDescription() -> VirtIoBootOptionDescriptionHandler()
BdsDxe -> GetBootOptionDescription() -> VirtIoBootOptionDescriptionHandler()

So instead of routing to *UefiBootManagerLib* GetBootOptionDescription(),
why not you directly call VirtIoBootOptionDescriptionHandler() from LegacyBios?

I understand from functionality perspective, it makes no difference.
But I care about that because it avoids LegacyBios unnecessarily depends on
*UefiBootManagerLib*.

What do you think?


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of David
> Woodhouse
> Sent: Tuesday, June 25, 2019 5:29 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; lersek@redhat.com
> Subject: Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib:
> describe VirtIO devices correctly
> 
> On Tue, 2019-06-25 at 09:15 +0000, Ni, Ray wrote:
> > But I still need to understand why the *GetBootOption() API is needed.
> > Because for quite a long time since the MdeModulePkg/Bds was added,
> there is no
> > such requirement.
> 
> It's for CSM, because otherwise all the legacy boot targets other than
> IDE are just shown as 'Harddisk'.
> 
> See patch [5/7] in this thread which uses the GetBootDescription API
> that [4/7] exposes.
> 
> 
> 

Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

[David Woodhouse]

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

On Tue, 2019-06-25 at 09:56 +0000, Ni, Ray wrote:
> David,
> Thanks for pointing that patch.
> 
> Now I understand it.
> Normally it's the CSM16 code that builds the boot descriptions for legacy boot options
> and LegacyBootManagerLib consumes that boot descriptions.
> 
> But in your case, LegacyBios driver builds the boot descriptions for VirtIo devices and
> LegacyBootManagerLib consumes that boot descriptions.
> 
> The flow is like below: (I understand there is no VirtIoBootOptionDescriptionHandler() in
> your current patch. But I think we could write such handler and register it through
> EfiBootManagerRegisterBootDescriptionHandler() API)
> 
>   *module*          *UefiBootManagerLib*
> LegacyBios -> GetBootOptionDescription() -> VirtIoBootOptionDescriptionHandler()
> BdsDxe -> GetBootOptionDescription() -> VirtIoBootOptionDescriptionHandler()
> 
> So instead of routing to *UefiBootManagerLib* GetBootOptionDescription(),
> why not you directly call VirtIoBootOptionDescriptionHandler() from LegacyBios?
> 
> I understand from functionality perspective, it makes no difference.
> But I care about that because it avoids LegacyBios unnecessarily depends on
> *UefiBootManagerLib*.
> 
> What do you think?

Forget VirtIO, look only at the previous two patches.

    MdeModulePkg/UefiBootManagerLib: export EfiBootManagerGetBootDescription()
    OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription()

Their commit messages (attempt to) set this out fairly clearly.

In BmGetBootDescription() we already have a whole pile of special cases
for things including NVMe, USB, etc. to get descriptive strings that
match a given BlockIo handle.

It's bad enough that that functionality exists in that form already,
rather than being provided in a clean and generic fashion by the block
device driver itself somehow.

I absolutely did not want to reproduce it all, just to get meaningful
strings for the Legacy boot targets from the same block devices. Hence
a tweaking, renaming and exporting the existing BmGetBootDescription()
function so I can use it from LegacyBbs code.

Adding another special case for VirtIO alongside all the other special
cases that were already in BmGetBootDescription (now called
EfiBootManagerGetBootDescription()) is not really the important part
here.


Is there a particular problem with having LegacyBios depend on
UefiBootManagerLib? The code in BmDescription.c is actually fairly
standalone — should we move it out into its *own* library so that it
can be used from both places? That seems like overkill to me, when our
long term plan really ought to be to kill it with fire and let block
device drivers provide their own descriptive strings through a standard
protocol.

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

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

[David Woodhouse]

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

On Tue, 2019-06-25 at 01:50 +0200, Laszlo Ersek wrote:
> (1) Side request -- can you please set the "xfuncname" knob so that the
> diff hunk headers (@@) reflect the section name being modified?
> 
> It's explained at
> <https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>,
> and it can be automated by "BaseTools/Scripts/SetupGit.py".


Oops, sorry. Missed that part; will go set it up now for next time.

> (2) For the patch. I'd prefer
> - setting PcdPciMmio64Base to zero unconditionally, and
> - setting PcdPciMmio64Size unconditionally as well, just to different
> values, dependent on CSM_ENABLE.

I did that.

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