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

["Laszlo Ersek" <lersek@redhat.com>]

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;
>  }
>  
>  
>