From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 24 Jun 2019 15:46:14 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 29B8887620; Mon, 24 Jun 2019 22:46:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-226.ams2.redhat.com [10.36.116.226]) by smtp.corp.redhat.com (Postfix) with ESMTP id 142C1600CD; Mon, 24 Jun 2019 22:46:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/7] OvmfPkg/LegacyBbs: Add boot entries for VirtIO and NVME devices To: devel@edk2.groups.io, dwmw2@infradead.org Cc: Ray Ni References: <20190621223156.701502-1-dwmw2@infradead.org> <20190621223156.701502-2-dwmw2@infradead.org> From: "Laszlo Ersek" Message-ID: <5e9a1d83-c378-9c71-29eb-da0d45d8aa90@redhat.com> Date: Tue, 25 Jun 2019 00:46:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190621223156.701502-2-dwmw2@infradead.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Mon, 24 Jun 2019 22:46:13 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Acked-by: Laszlo Ersek > --- > 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; > } > > >