[PATCH v3 0/4] OvmfPkg: CSM boot fixes

[PATCH v3 0/4] OvmfPkg: CSM boot fixes

[David Woodhouse]

For v3, leaving out the cosmetic parts that touch code outside OvmfPkg. 
This series is now purely the correctness fixes within OvmfPkg which are 
required to make CSM boots work properly again.

The first two patches allow NVMe and VirtIO disks to be used as Legacy
boot targets, since nobody really uses IDE any more.

The third avoids using QemuVideoDxe when we have CSM, as the INT 10h 
shim installed by QemuVideoDxe conflicts with a real legacy video BIOS
being installed.

Finally, avoid placing PCI BARs above 4GiB. Strictly speaking we only 
need this for PCI devices which might be natively supported by the CSM 
BIOS, like NVMe. Devices with an OpRom already get special-cased to stay 
below 4GiB. But an IncompatiblePciDeviceSupportProtocol implementation 
doesn't get to see the PCI device class; only the vendor/device IDs so 
we can't use it for that purpose to downgrade more selectively. Instead, 
just default to putting everything below 4GiB.


David Woodhouse (4):
  OvmfPkg/LegacyBios: set NumberBbsEntries to the size of BbsTable
  OvmfPkg/LegacyBbs: Add boot entries for VirtIO and NVME devices
  OvmfPkg: Don't build in QemuVideoDxe when we have CSM
  OvmfPkg: don't assign PCI BARs above 4GiB when CSM enabled

 OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c  | 157 ++++++++++++++++++++++++-
 OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c |   7 +-
 OvmfPkg/OvmfPkgIa32.dsc                |   2 +
 OvmfPkg/OvmfPkgIa32.fdf                |   5 +-
 OvmfPkg/OvmfPkgIa32X64.dsc             |   6 +
 OvmfPkg/OvmfPkgIa32X64.fdf             |   5 +-
 OvmfPkg/OvmfPkgX64.dsc                 |   6 +
 OvmfPkg/OvmfPkgX64.fdf                 |   5 +-
 8 files changed, 179 insertions(+), 14 deletions(-)

-- 
2.21.0

[PATCH v3 1/4] 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 05e3ffd2bbb8..5e795bfe6570 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 v3 2/4] 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 6b1dd344f344..5e4c7a249ef1 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 (
     }
   }
 
+  //
+  // 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 v3 3/4] 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    | 5 +++--
 OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
 OvmfPkg/OvmfPkgIa32X64.fdf | 5 +++--
 OvmfPkg/OvmfPkgX64.dsc     | 2 ++
 OvmfPkg/OvmfPkgX64.fdf     | 5 +++--
 6 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 473eaba24613..87716123997a 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -738,7 +738,9 @@ [Components]
   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 14100356b4f6..785affeb90c8 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -305,9 +305,10 @@ [FV.DXEFV]
 INF  OvmfPkg/Csm/BiosThunk/VideoDxe/VideoDxe.inf
 INF  OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
 INF  RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf
-!endif
-
+!else
 INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+!endif
+
 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 73f33b721832..639e33cb285f 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -747,7 +747,9 @@ [Components.X64]
   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 5af3cdc93d20..74407072563b 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -311,9 +311,10 @@ [FV.DXEFV]
 INF  OvmfPkg/Csm/BiosThunk/VideoDxe/VideoDxe.inf
 INF  OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
 INF  RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf
-!endif
-
+!else
 INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+!endif
+
 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 39ac79156529..69a3497c2c9e 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -745,7 +745,9 @@ [Components]
   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 5af3cdc93d20..74407072563b 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -311,9 +311,10 @@ [FV.DXEFV]
 INF  OvmfPkg/Csm/BiosThunk/VideoDxe/VideoDxe.inf
 INF  OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
 INF  RuleOverride=CSM OvmfPkg/Csm/Csm16/Csm16.inf
-!endif
-
+!else
 INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+!endif
+
 INF  OvmfPkg/QemuRamfbDxe/QemuRamfbDxe.inf
 INF  OvmfPkg/VirtioGpuDxe/VirtioGpu.inf
 INF  OvmfPkg/PlatformDxe/Platform.inf
-- 
2.21.0

[PATCH v3 4/4] 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
 OvmfPkg/OvmfPkgX64.dsc     | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 639e33cb285f..ad20531ceb8b 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -543,7 +543,11 @@ [PcdsDynamicDefault]
   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 69a3497c2c9e..0542ac2235b4 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -542,7 +542,11 @@ [PcdsDynamicDefault]
   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 v3 0/4] OvmfPkg: CSM boot fixes

[Laszlo Ersek]

On 06/26/19 13:37, David Woodhouse wrote:
> For v3, leaving out the cosmetic parts that touch code outside OvmfPkg. 
> This series is now purely the correctness fixes within OvmfPkg which are 
> required to make CSM boots work properly again.
> 
> The first two patches allow NVMe and VirtIO disks to be used as Legacy
> boot targets, since nobody really uses IDE any more.
> 
> The third avoids using QemuVideoDxe when we have CSM, as the INT 10h 
> shim installed by QemuVideoDxe conflicts with a real legacy video BIOS
> being installed.
> 
> Finally, avoid placing PCI BARs above 4GiB. Strictly speaking we only 
> need this for PCI devices which might be natively supported by the CSM 
> BIOS, like NVMe. Devices with an OpRom already get special-cased to stay 
> below 4GiB. But an IncompatiblePciDeviceSupportProtocol implementation 
> doesn't get to see the PCI device class; only the vendor/device IDs so 
> we can't use it for that purpose to downgrade more selectively. Instead, 
> just default to putting everything below 4GiB.
> 
> 
> David Woodhouse (4):
>   OvmfPkg/LegacyBios: set NumberBbsEntries to the size of BbsTable
>   OvmfPkg/LegacyBbs: Add boot entries for VirtIO and NVME devices
>   OvmfPkg: Don't build in QemuVideoDxe when we have CSM
>   OvmfPkg: don't assign PCI BARs above 4GiB when CSM enabled
> 
>  OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c  | 157 ++++++++++++++++++++++++-
>  OvmfPkg/Csm/LegacyBiosDxe/LegacyBios.c |   7 +-
>  OvmfPkg/OvmfPkgIa32.dsc                |   2 +
>  OvmfPkg/OvmfPkgIa32.fdf                |   5 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc             |   6 +
>  OvmfPkg/OvmfPkgIa32X64.fdf             |   5 +-
>  OvmfPkg/OvmfPkgX64.dsc                 |   6 +
>  OvmfPkg/OvmfPkgX64.fdf                 |   5 +-
>  8 files changed, 179 insertions(+), 14 deletions(-)
> 

Series pushed as commit range 2f3435c2343f..c7341877f695.

Thank you!
Laszlo