Re: [PATCH v2 5/7] OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription()

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

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 <dwmw2@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 <lersek@redhat.com>

Thanks
Laszlo