From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web12.1563.1587749721142471116 for ; Fri, 24 Apr 2020 10:35:21 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=QT4Ds3Ls; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587749720; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=9S6DDiDJ6PJ9HYU5boI5GYEJKu0/Ue1Kr7ahEReyaMc=; b=QT4Ds3LsIIOQOrCzUv6ix3kKnve321+FCPk5bk7XiKozKuFa/hYTp1gf2gW4LeB6oSd4s1 sAyc0MfWR+BQIS6pOYyC73wX/UjRXZBTRIm0GjbBBaM3O5VeaVFHHct/DWLjQ93nBqk/50 kSKsYFoRZ/78PqPvZMXPWPGu152BF7E= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-381-2Ug4gHpGMZ-_eR7SYhlXHA-1; Fri, 24 Apr 2020 13:35:16 -0400 X-MC-Unique: 2Ug4gHpGMZ-_eR7SYhlXHA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2C31287308F; Fri, 24 Apr 2020 17:35:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-159.ams2.redhat.com [10.36.113.159]) by smtp.corp.redhat.com (Postfix) with ESMTP id DB15C5D70C; Fri, 24 Apr 2020 17:35:12 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/2] OvmfPkg: Add DxeResetSystemLibBhyve To: devel@edk2.groups.io, rebecca@bsdio.com, Jordan Justen , Ard Biesheuvel References: <20200423030214.57861-1-rebecca@bsdio.com> <20200423030214.57861-2-rebecca@bsdio.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 24 Apr 2020 19:35:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200423030214.57861-2-rebecca@bsdio.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > --- > 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 > + Copyright (C) 2020, Red Hat, Inc. > + Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#include // BIT13 > + > +#include // CpuDeadLoop() > +#include // IoOr16() > +#include // ResetShutdown() > +#include // 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.
> +# 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 >