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; Tue, 25 Jun 2019 06:29:16 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C178A23E6C5; Tue, 25 Jun 2019 13:29:05 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-204.ams2.redhat.com [10.36.116.204]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4A0795D717; Tue, 25 Jun 2019 13:29:03 +0000 (UTC) Subject: Re: [PATCH v2 5/7] OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription() To: David Woodhouse , devel@edk2.groups.io Cc: Ray Ni References: <20190625114859.795331-1-dwmw2@infradead.org> <20190625114859.795331-5-dwmw2@infradead.org> From: "Laszlo Ersek" Message-ID: <7f917141-3522-c8f0-2f06-b9d5e52a5b58@redhat.com> Date: Tue, 25 Jun 2019 15:29:02 +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: <20190625114859.795331-5-dwmw2@infradead.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 25 Jun 2019 13:29:15 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 06/25/19 13:48, 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 | 47 ++++++++++++++++++++- > OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf | 1 + > 2 files changed, 46 insertions(+), 2 deletions(-) Please consider including a patch-level changelog (see "git-notes"). > diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/Legacy= BiosDxe/LegacyBbs.c > index 5e4c7a249e..f3cc64f89d 100644 > --- a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c > +++ b/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c > @@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > =20 > #include "LegacyBiosInterface.h" > #include > +#include > =20 > // Give floppy 3 states > // FLOPPY_PRESENT_WITH_MEDIA =3D Floppy controller present and media = is inserted > @@ -280,6 +281,8 @@ LegacyBiosBuildBbs ( > UINTN BusNum; > UINTN DevNum; > UINTN FuncNum; > + CHAR16 *Description; > + CHAR8 AsciiDescription[32]; > =20 > for (Removable =3D 0; Removable < 2; Removable++) { > for (BlockIndex =3D 0; BlockIndex < NumberBlockIoHandles; BlockI= ndex++) { > @@ -372,8 +375,48 @@ LegacyBiosBuildBbs ( > continue; > } > =20 > - DEBUG ((DEBUG_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d\n", > - BusNum, DevNum, FuncNum)); > + Description =3D EfiBootManagerGetBootDescription(L"Legacy ", B= lockIoHandles[BlockIndex]); (1) Missing space character. > + > + DEBUG ((DEBUG_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d: %s= \n", > + BusNum, DevNum, FuncNum, Description ? Description : L"")); > + > + if (Description !=3D NULL) { > + VOID *BbsDescription; (2) You removed the NULL-initialization altogether, and the resultant code remains correct (and it now conforms to the edk2 coding style). However, this pattern tends to tickle bugs in compiler data flow analysis. I expect that at least one toolchain will whine about "BbsDescription" being used without initialization. The toolchain might not see that reaching CopyLegacyRegion() implies success of GetLegacyRegion() implies non-NULL-ity of "BbsDescription". This occurs so frequently that we have some "recommended" comment format, to document spurious variable assignments (that we do only for shutting up compilers). See : // // set BbsDescription to suppress incorrect compiler/analyzer warnings // BbsDescription =3D NULL; Anyway, I don't insist that you add "BbsDescription =3D NULL;" immediately, I'm just highlighting a potential build failure. > + > + // > + // Truncate Description and convert to ASCII. > + // > + if (StrLen (Description) >=3D sizeof (AsciiDescription)) { > + Description[sizeof (AsciiDescription) - 1] =3D L'0'; (3) Sneaky typo. You mean (and I requested) L'\0'. L'0' is different. :) > + } > + UnicodeStrToAsciiStrS (Description, AsciiDescription, sizeof= (AsciiDescription)); > + > + // > + // Copy to low memory and reference from BbsTable > + // > + Status =3D Private->LegacyBios.GetLegacyRegion( (4) missing space > + &Private->LegacyBios, > + AsciiStrSize(AsciiDescription= ), (5) missing space > + (UINTN)0, /* Any region */ > + (UINTN)1, /* Alignment */ > + &BbsDescription > + ); > + > + if (!EFI_ERROR (Status)) { > + Status =3D Private->LegacyBios.CopyLegacyRegion ( > + &Private->LegacyBios, > + AsciiStrSize(AsciiDescripti= on), (6) missing space > + BbsDescription, > + AsciiDescription > + ); > + } > + if (!EFI_ERROR (Status)) { > + BbsTable[BbsIndex].DescStringSegment =3D (UINT16) (((UINTN= ) BbsDescription >> 16) << 12); > + BbsTable[BbsIndex].DescStringOffset =3D (UINT16) (UINTN) = BbsDescription; > + } > + > + FreePool (Description); > + } > =20 > BbsTable[BbsIndex].Bus =3D BusNum; > BbsTable[BbsIndex].Device =3D DevNum; > 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 With (1), and (3) through (6), fixed, and with (2) optionally fixed: Reviewed-by: Laszlo Ersek Thanks Laszlo