Re: [edk2-devel] [PATCH 5/7] OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription()

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

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™.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
>  OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c       | 46 ++++++++++++++++++++-
>  OvmfPkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf |  1 +
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Csm/LegacyBiosDxe/LegacyBbs.c b/OvmfPkg/Csm/LegacyBiosDxe/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
>  
>  #include "LegacyBiosInterface.h"
>  #include <IndustryStandard/Pci.h>
> +#include <Library/UefiBootManagerLib.h>
> +
> +#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.

>  
>  // Give floppy 3 states
>  // FLOPPY_PRESENT_WITH_MEDIA  = 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.

>  
>      for (Removable = 0; Removable < 2; Removable++) {
>        for (BlockIndex = 0; BlockIndex < NumberBlockIoHandles; BlockIndex++) {
> @@ -370,16 +375,55 @@ LegacyBiosBuildBbs (
>            continue;
>          }
>  
> +        Description = EfiBootManagerGetBootDescription(L"Legacy ", BlockIoHandles[BlockIndex]);
> +
>          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));
> +                 BusNum, DevNum, FuncNum, Description ? Description : L"<No description>"));

(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);
>          );
>  
> +        if (Description != NULL) {
> +          VOID *BbsDescription = 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) >= LEGACY_BBS_BOOT_DESCRIPTION_LENGTH) {
> +		  Description[LEGACY_BBS_BOOT_DESCRIPTION_LENGTH - 1] = 0;

(6) For readability, I'd suggest L'\0'.

> +	  }
> +          UnicodeStrToAsciiStrS (Description, DescriptionA, sizeof (DescriptionA));
> +
> +	  //
> +	  // Copy to low memory and reference from BbsTable
> +	  //



> +          Status = Private->LegacyBios.GetLegacyRegion(
> +                                         &Private->LegacyBios,
> +                                         AsciiStrSize(DescriptionA),
> +                                         (UINTN)0, /* Any region */
> +                                         (UINTN)1, /* Alignment */
> +                                         &BbsDescription
> +                                         );
> +
> +          if (!EFI_ERROR (Status)) {
> +            Status = Private->LegacyBios.CopyLegacyRegion (
> +                                           &Private->LegacyBios,
> +                                           AsciiStrSize(DescriptionA),
> +                                           BbsDescription,
> +                                           DescriptionA
> +                                           );
> +          }
> +          if (!EFI_ERROR (Status)) {
> +            BbsTable[BbsIndex].DescStringSegment = (UINT16) (((UINTN) BbsDescription >> 16) << 12);
> +            BbsTable[BbsIndex].DescStringOffset  = (UINT16) (UINTN) BbsDescription;
> +          }

Hmmm OK there is no way to release memory allocated by
GetLegacyRegion(), in the (theoretical) case when CopyLegacyRegion() fails.

> +
> +          FreePool (Description);
> +        }
> +
>          BbsTable[BbsIndex].Bus                      = BusNum;
>          BbsTable[BbsIndex].Device                   = DevNum;
>          BbsTable[BbsIndex].Function                 = 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
> 

Thanks
Laszlo