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 <dw...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42765): https://edk2.groups.io/g/devel/message/42765
Mute This Topic: https://groups.io/mt/32163537/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
