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 <dw...@infradead.org>
> Acked-by: Laszlo Ersek <ler...@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;
> }
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42763): https://edk2.groups.io/g/devel/message/42763
Mute This Topic: https://groups.io/mt/32163534/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
