Re: [edk2-devel] [PATCH 1/2] OvmfPkg: Add DxeResetSystemLibBhyve

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

On 04/23/20 05:02, Rebecca Cran wrote:
> Introduce the DxeResetSystemLibBhyve library to support powering off
> bhyve guests.

(1) Please replace all "DxeResetSystemLibBhyve" references in the patch
(code, commit message, subject line, file names) with
"BaseResetSystemLibBhyve".

The reason is that the library instance you are introducing is suitable
for all firmware phases and module types. We don't perform any action
that would preclude a particular firmware phase. And we also don't have
phase-specific opportunities for performance improvements. And so it
makes sense to make this a BASE library, and support the broadest
collection of module types.

> 
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  OvmfPkg/Include/IndustryStandard/Bhyve.h      |  2 +
>  .../ResetSystemLib/DxeResetShutdownBhyve.c    | 43 +++++++++++++++++++
>  .../ResetSystemLib/DxeResetSystemLibBhyve.inf | 38 ++++++++++++++++
>  3 files changed, 83 insertions(+)
>  create mode 100644 OvmfPkg/Library/ResetSystemLib/DxeResetShutdownBhyve.c
>  create mode 100644 OvmfPkg/Library/ResetSystemLib/DxeResetSystemLibBhyve.inf
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/Bhyve.h b/OvmfPkg/Include/IndustryStandard/Bhyve.h
> index 02ce5587ee..9745d62688 100644
> --- a/OvmfPkg/Include/IndustryStandard/Bhyve.h
> +++ b/OvmfPkg/Include/IndustryStandard/Bhyve.h
> @@ -13,4 +13,6 @@
>  
>  #define BHYVE_ACPI_TIMER_IO_ADDR 0x408
>  
> +#define BHYVE_PM_VALUE 0x408
> +
>  #endif // __BHYVE_H__
> diff --git a/OvmfPkg/Library/ResetSystemLib/DxeResetShutdownBhyve.c b/OvmfPkg/Library/ResetSystemLib/DxeResetShutdownBhyve.c
> new file mode 100644
> index 0000000000..cb30d75ee2
> --- /dev/null
> +++ b/OvmfPkg/Library/ResetSystemLib/DxeResetShutdownBhyve.c
> @@ -0,0 +1,43 @@
> +/** @file
> +  DXE Reset System Library Shutdown API implementation for bhyve.
> +
> +  Copyright (C) 2020, Rebecca Cran <rebecca@bsdio.com>
> +  Copyright (C) 2020, Red Hat, Inc.
> +  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Base.h>                   // BIT13
> +
> +#include <Library/BaseLib.h>        // CpuDeadLoop()
> +#include <Library/IoLib.h>          // IoOr16()
> +#include <Library/ResetSystemLib.h> // ResetShutdown()
> +#include <OvmfPlatforms.h>          // BHYVE_PM_VALUE

(2) Note that you are adding BHYVE_PM_VALUE to
"OvmfPkg/Include/IndustryStandard/Bhyve.h" (very correctly), and
presently, "OvmfPlatforms.h" does not include "Bhyve.h".

Therefore I suggest replacing the above #include with one for
"IndustryStandard/Bhyve.h".

(And then please re-sort the #include directives.)

> +
> +EFI_STATUS
> +EFIAPI
> +DxeResetInit (
> +  IN EFI_HANDLE       ImageHandle,
> +  IN EFI_SYSTEM_TABLE *SystemTable
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> +

(3) A CONSTRUCTOR does not look necessary for this library instance.

> +/**
> +  Calling this function causes the system to enter a power state equivalent
> +  to the ACPI G2/S5 or G3 states.
> +
> +  System shutdown should not return, if it returns, it means the system does
> +  not support shut down reset.
> +**/
> +VOID
> +EFIAPI
> +ResetShutdown (
> +  VOID
> +  )
> +{
> +  IoBitFieldWrite16 (BHYVE_PM_VALUE, 10, 13, 5);
> +  IoOr16 (BHYVE_PM_VALUE, BIT13);
> +  CpuDeadLoop ();
> +}
> diff --git a/OvmfPkg/Library/ResetSystemLib/DxeResetSystemLibBhyve.inf b/OvmfPkg/Library/ResetSystemLib/DxeResetSystemLibBhyve.inf
> new file mode 100644
> index 0000000000..c7923a4755
> --- /dev/null
> +++ b/OvmfPkg/Library/ResetSystemLib/DxeResetSystemLibBhyve.inf
> @@ -0,0 +1,38 @@
> +## @file
> +#  DXE library instance for ResetSystem library class for bhyve

(4) Please update this comment too.

> +#
> +#  Copyright (C) 2020, Red Hat, Inc.
> +#  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent

(5) I think your (C) notice is needed here.

> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.29
> +  BASE_NAME                      = DxeResetSystemLibBhyve
> +  FILE_GUID                      = 88a12688-98f5-44a6-bf4b-7894706a2d11
> +  MODULE_TYPE                    = DXE_DRIVER

(6) Flip this to BASE please.

> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ResetSystemLib|DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION

(7) Please drop the module type restrictions (the part starting with "|").

Thanks!
Laszlo

> +  CONSTRUCTOR                    = DxeResetInit
> +
> +#
> +# The following information is for reference only and not required by the build
> +# tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Sources]
> +  DxeResetShutdownBhyve.c
> +  ResetSystemLib.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  IoLib
> +  TimerLib
>