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 16:03:20 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 760A76697A; Mon, 24 Jun 2019 23:03:17 +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 6E8155C234; Mon, 24 Jun 2019 23:03:16 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 5/7] OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription() To: devel@edk2.groups.io, dwmw2@infradead.org Cc: Ray Ni References: <20190621223156.701502-1-dwmw2@infradead.org> <20190621223156.701502-5-dwmw2@infradead.org> From: "Laszlo Ersek" Message-ID: Date: Tue, 25 Jun 2019 01:03:15 +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-5-dwmw2@infradead.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 24 Jun 2019 23:03:20 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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=E2=84=A2. >=20 > Signed-off-by: David Woodhouse > --- > OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c | 46 ++++++++++++++++++++- > OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf | 1 + > 2 files changed, 46 insertions(+), 1 deletion(-) >=20 > diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/Legacy= BiosDxe/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 > =20 > #include "LegacyBiosInterface.h" > #include > +#include > + > +#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. > =20 > // Give floppy 3 states > // FLOPPY_PRESENT_WITH_MEDIA =3D 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. > =20 > for (Removable =3D 0; Removable < 2; Removable++) { > for (BlockIndex =3D 0; BlockIndex < NumberBlockIoHandles; BlockI= ndex++) { > @@ -370,16 +375,55 @@ LegacyBiosBuildBbs ( > continue; > } > =20 > + Description =3D EfiBootManagerGetBootDescription(L"Legacy ", B= lockIoHandles[BlockIndex]); > + > DEBUG_CODE ( > CHAR16 *PathText; > =20 > PathText =3D ConvertDevicePathToText(DevicePath, FALSE, FALS= E); > =20 > 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"")); (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); > ); > =20 > + if (Description !=3D NULL) { > + VOID *BbsDescription =3D 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) >=3D LEGACY_BBS_BOOT_DESCRIPTION_LENGTH) { > + Description[LEGACY_BBS_BOOT_DESCRIPTION_LENGTH - 1] =3D 0; (6) For readability, I'd suggest L'\0'. > + } > + UnicodeStrToAsciiStrS (Description, DescriptionA, sizeof (De= scriptionA)); > + > + // > + // Copy to low memory and reference from BbsTable > + // > + Status =3D Private->LegacyBios.GetLegacyRegion( > + &Private->LegacyBios, > + AsciiStrSize(DescriptionA), > + (UINTN)0, /* Any region */ > + (UINTN)1, /* Alignment */ > + &BbsDescription > + ); > + > + if (!EFI_ERROR (Status)) { > + Status =3D Private->LegacyBios.CopyLegacyRegion ( > + &Private->LegacyBios, > + AsciiStrSize(DescriptionA), > + BbsDescription, > + DescriptionA > + ); > + } > + if (!EFI_ERROR (Status)) { > + BbsTable[BbsIndex].DescStringSegment =3D (UINT16) (((UINTN= ) BbsDescription >> 16) << 12); > + BbsTable[BbsIndex].DescStringOffset =3D (UINT16) (UINTN) = BbsDescription; > + } Hmmm OK there is no way to release memory allocated by GetLegacyRegion(), in the (theoretical) case when CopyLegacyRegion() fail= s. > + > + FreePool (Description); > + } > + > BbsTable[BbsIndex].Bus =3D BusNum; > BbsTable[BbsIndex].Device =3D DevNum; > BbsTable[BbsIndex].Function =3D 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 >=20 Thanks Laszlo