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™.
>
> Signed-off-by: David Woodhouse <dw...@infradead.org>
> ---
> 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/LegacyBiosDxe/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
>
> #include "LegacyBiosInterface.h"
> #include <IndustryStandard/Pci.h>
> +#include <Library/UefiBootManagerLib.h>
>
> // Give floppy 3 states
> // FLOPPY_PRESENT_WITH_MEDIA = Floppy controller present and media is
> inserted
> @@ -280,6 +281,8 @@ LegacyBiosBuildBbs (
> UINTN BusNum;
> UINTN DevNum;
> UINTN FuncNum;
> + CHAR16 *Description;
> + CHAR8 AsciiDescription[32];
>
> for (Removable = 0; Removable < 2; Removable++) {
> for (BlockIndex = 0; BlockIndex < NumberBlockIoHandles; BlockIndex++) {
> @@ -372,8 +375,48 @@ LegacyBiosBuildBbs (
> continue;
> }
>
> - DEBUG ((DEBUG_INFO, "Add Legacy Bbs entry for PCI %d/%d/%d\n",
> - BusNum, DevNum, FuncNum));
> + Description = EfiBootManagerGetBootDescription(L"Legacy ",
> BlockIoHandles[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"<No
> description>"));
> +
> + if (Description != 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
<https://bugzilla.tianocore.org/show_bug.cgi?id=607>:
//
// set BbsDescription to suppress incorrect compiler/analyzer warnings
//
BbsDescription = NULL;
Anyway, I don't insist that you add "BbsDescription = NULL;"
immediately, I'm just highlighting a potential build failure.
> +
> + //
> + // Truncate Description and convert to ASCII.
> + //
> + if (StrLen (Description) >= sizeof (AsciiDescription)) {
> + Description[sizeof (AsciiDescription) - 1] = 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 = 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 = Private->LegacyBios.CopyLegacyRegion (
> + &Private->LegacyBios,
> + AsciiStrSize(AsciiDescription),
(6) missing space
> + BbsDescription,
> + AsciiDescription
> + );
> + }
> + if (!EFI_ERROR (Status)) {
> + BbsTable[BbsIndex].DescStringSegment = (UINT16) (((UINTN)
> BbsDescription >> 16) << 12);
> + BbsTable[BbsIndex].DescStringOffset = (UINT16) (UINTN)
> BbsDescription;
> + }
> +
> + FreePool (Description);
> + }
>
> BbsTable[BbsIndex].Bus = BusNum;
> BbsTable[BbsIndex].Device = 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
>
With (1), and (3) through (6), fixed, and with (2) optionally fixed:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42818): https://edk2.groups.io/g/devel/message/42818
Mute This Topic: https://groups.io/mt/32202511/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
