Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Add BaseResetSystemLibBhyve.inf

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

On 05/04/20 04:18, Rebecca Cran wrote:
> Introduce BaseResetSystemLibBhyve.inf, to support powering off
> bhyve guests.
>
> 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>
> ---
> Laszlo: I couldn't find your email specifying specific changes to
> make,

It's archived here:

http://mid.mail-archive.com/f1dacfcb-fd50-6e58-965b-656181d3d6d2@redhat.com
https://edk2.groups.io/g/devel/message/58076


(1) The ".inf" should be stripped from the subject line.

> so I hope I've caught all of them.
>
>  OvmfPkg/Include/IndustryStandard/Bhyve.h      |  2 +
>  .../ResetSystemLib/BaseResetShutdownBhyve.c   | 34 ++++++++++++++++
>  .../BaseResetSystemLibBhyve.inf               | 40 +++++++++++++++++++
>  3 files changed, 76 insertions(+)
>  create mode 100644 OvmfPkg/Library/ResetSystemLib/BaseResetShutdownBhyve.c
>  create mode 100644 OvmfPkg/Library/ResetSystemLib/BaseResetSystemLibBhyve.inf
>
> diff --git a/OvmfPkg/Include/IndustryStandard/Bhyve.h b/OvmfPkg/Include/IndustryStandard/Bhyve.h
> index 02ce5587ee0f..ab8f2b0729b4 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_REG 0x404
> +
>  #endif // __BHYVE_H__
> diff --git a/OvmfPkg/Library/ResetSystemLib/BaseResetShutdownBhyve.c b/OvmfPkg/Library/ResetSystemLib/BaseResetShutdownBhyve.c
> new file mode 100644
> index 000000000000..156e5ad0d797
> --- /dev/null
> +++ b/OvmfPkg/Library/ResetSystemLib/BaseResetShutdownBhyve.c
> @@ -0,0 +1,34 @@
> +/** @file
> +  Base 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_REG

(2) Even though <OvmfPlatforms.h> does include
<IndustryStandard/Bhyve.h> now, from commit 91dee771fc0d ("OvmfPkg: Add
bhyve support into AcpiTimerLib", 2020-04-30), there's still no reason
for this library instance to prefer the more general <OvmfPlatforms.h>
to <IndustryStandard/Bhyve.h>.

> +
> +/**
> +  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_REG, 10, 13, 5);
> +  IoOr16 (BHYVE_PM_REG, BIT13);
> +  CpuDeadLoop ();
> +}
> diff --git a/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLibBhyve.inf b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLibBhyve.inf
> new file mode 100644
> index 000000000000..33e4e2689e57
> --- /dev/null
> +++ b/OvmfPkg/Library/ResetSystemLib/BaseResetSystemLibBhyve.inf
> @@ -0,0 +1,40 @@
> +## @file
> +#  Base library instance for ResetSystem library class for bhyve
> +#
> +#  Copyright (C) 2020, Rebecca Cran <rebecca@bsdio.com>
> +#  Copyright (C) 2020, Red Hat, Inc.
> +#  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.29
> +  BASE_NAME                      = BaseResetSystemLibBhyve
> +  FILE_GUID                      = 5c71b08f-0ade-4607-8b9d-946c2757fee8
> +  MODULE_TYPE                    = DXE_DRIVER

(3) Should be BASE.


I've fixed up the above three warts for you:

[lersek@redhat.com: MODULE_TYPE: replace DXE_DRIVER with BASE]
[lersek@redhat.com: replace <OvmfPlatforms.h> with <IndustryStandard/Bhyve.h>]
[lersek@redhat.com: strip ".inf" from subject line]

With those:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(I also build-tested the library after my fixups.)

github.com pull request: https://github.com/tianocore/edk2/pull/580

Commit: f159102a130fac9b416418eb9b6fa35b63426ca5

Thanks,
Laszlo

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