From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.3022.1587633564187160006 for ; Thu, 23 Apr 2020 02:19:24 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=SZHYLkL2; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587633563; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KpVtoR8PFEeVW9838Aj6MggTQDKa3zxgIHkSE70C6K8=; b=SZHYLkL2Ie+6metrMEbSfl1ciRpWjSt9jY9zTJX0U/YQq/7mB5vf0VwCiUoLbVgqUUCHw5 DdWZ9jqchIaKK7n9CBGmJLcOz9TiGUsJVUnooJbXJvexgOpdhTy+foNBj/uh7sHi3KnK5i gyDDSUCCU+LC3+23DTFBPB7pqVeZ+jU= 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-60-Zl9oCBqtNT2rm7kYfwuYKQ-1; Thu, 23 Apr 2020 05:19:14 -0400 X-MC-Unique: Zl9oCBqtNT2rm7kYfwuYKQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B1BD7DB21; Thu, 23 Apr 2020 09:19:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-95.ams2.redhat.com [10.36.114.95]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8AFA119D9E; Thu, 23 Apr 2020 09:19:10 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 4/6] Add BhyvePkg, to support the bhyve hypervisor To: devel@edk2.groups.io, rebecca@bsdio.com Cc: Jordan Justen , Ard Biesheuvel , Leif Lindholm , Michael Kinney , Andrew Fish , Peter Grehan References: <20200421030955.114850-1-rebecca@bsdio.com> <20200421030955.114850-5-rebecca@bsdio.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 23 Apr 2020 11:19:09 +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: <20200421030955.114850-5-rebecca@bsdio.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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/21/20 05:09, Rebecca Cran wrote: > BhyvePkg supports the bhyve hypervisor, which is a hypervisor/virtual > machine manager available on FreeBSD, macOS and Illumos. > > Signed-off-by: Rebecca Cran > --- > BhyvePkg/AcpiTables/AcpiTables.inf | 42 + > BhyvePkg/AcpiTables/Dsdt.asl | 1134 +++++++++++ > BhyvePkg/AcpiTables/Facp.aslc | 75 + > BhyvePkg/AcpiTables/Facs.aslc | 79 + > BhyvePkg/AcpiTables/Hpet.aslc | 71 + > BhyvePkg/AcpiTables/Madt.aslc | 144 ++ > BhyvePkg/AcpiTables/Mcfg.aslc | 56 + > BhyvePkg/AcpiTables/Platform.h | 71 + > BhyvePkg/AcpiTables/Spcr.aslc | 62 + > BhyvePkg/AcpiTables/Ssdt.asl | 14 + > BhyvePkg/BhyvePkg.dec | 170 ++ > BhyvePkg/BhyvePkg.fdf.inc | 85 + > BhyvePkg/BhyvePkgX64.dsc | 846 +++++++++ > BhyvePkg/BhyvePkgX64.fdf | 513 +++++ > BhyvePkg/BhyveRfbDxe/BhyveRfbDxe.inf | 67 + > BhyvePkg/BhyveRfbDxe/ComponentName.c | 200 ++ > BhyvePkg/BhyveRfbDxe/Gop.h | 148 ++ > BhyvePkg/BhyveRfbDxe/GopDriver.c | 542 ++++++ > BhyvePkg/BhyveRfbDxe/GopScreen.c | 392 ++++ > BhyvePkg/BhyveRfbDxe/VbeShim.asm | 341 ++++ > BhyvePkg/BhyveRfbDxe/VbeShim.c | 258 +++ > BhyvePkg/BhyveRfbDxe/VbeShim.h | 912 +++++++++ > BhyvePkg/BhyveRfbDxe/VbeShim.sh | 79 + > BhyvePkg/DecomprScratchEnd.fdf.inc | 66 + > BhyvePkg/Include/Library/BhyveFwCtlLib.h | 46 + > .../Library/BhyveFwCtlLib/BhyveFwCtlLib.c | 425 +++++ > .../Library/BhyveFwCtlLib/BhyveFwCtlLib.inf | 40 + > .../PlatformBootManagerLib/BdsPlatform.c | 1660 +++++++++++++++++ > .../PlatformBootManagerLib/BdsPlatform.h | 191 ++ > .../PlatformBootManagerLib.inf | 74 + > .../PlatformBootManagerLib/PlatformData.c | 171 ++ > BhyvePkg/License.txt | 44 + > BhyvePkg/SmbiosPlatformDxe/Bhyve.c | 42 + > .../SmbiosPlatformDxe/SmbiosPlatformDxe.c | 244 +++ > .../SmbiosPlatformDxe/SmbiosPlatformDxe.h | 63 + > .../SmbiosPlatformDxe/SmbiosPlatformDxe.inf | 54 + > BhyvePkg/VarStore.fdf.inc | 115 ++ > Maintainers.txt | 7 + > 38 files changed, 9543 insertions(+) > create mode 100644 BhyvePkg/AcpiTables/AcpiTables.inf > create mode 100644 BhyvePkg/AcpiTables/Dsdt.asl > create mode 100644 BhyvePkg/AcpiTables/Facp.aslc > create mode 100644 BhyvePkg/AcpiTables/Facs.aslc > create mode 100644 BhyvePkg/AcpiTables/Hpet.aslc > create mode 100644 BhyvePkg/AcpiTables/Madt.aslc > create mode 100644 BhyvePkg/AcpiTables/Mcfg.aslc > create mode 100644 BhyvePkg/AcpiTables/Platform.h > create mode 100644 BhyvePkg/AcpiTables/Spcr.aslc > create mode 100644 BhyvePkg/AcpiTables/Ssdt.asl > create mode 100644 BhyvePkg/BhyvePkg.dec > create mode 100644 BhyvePkg/BhyvePkg.fdf.inc > create mode 100644 BhyvePkg/BhyvePkgX64.dsc > create mode 100644 BhyvePkg/BhyvePkgX64.fdf > create mode 100644 BhyvePkg/BhyveRfbDxe/BhyveRfbDxe.inf > create mode 100644 BhyvePkg/BhyveRfbDxe/ComponentName.c > create mode 100644 BhyvePkg/BhyveRfbDxe/Gop.h > create mode 100644 BhyvePkg/BhyveRfbDxe/GopDriver.c > create mode 100644 BhyvePkg/BhyveRfbDxe/GopScreen.c > create mode 100644 BhyvePkg/BhyveRfbDxe/VbeShim.asm > create mode 100644 BhyvePkg/BhyveRfbDxe/VbeShim.c > create mode 100644 BhyvePkg/BhyveRfbDxe/VbeShim.h > create mode 100644 BhyvePkg/BhyveRfbDxe/VbeShim.sh > create mode 100644 BhyvePkg/DecomprScratchEnd.fdf.inc > create mode 100644 BhyvePkg/Include/Library/BhyveFwCtlLib.h > create mode 100644 BhyvePkg/Library/BhyveFwCtlLib/BhyveFwCtlLib.c > create mode 100644 BhyvePkg/Library/BhyveFwCtlLib/BhyveFwCtlLib.inf > create mode 100644 BhyvePkg/Library/PlatformBootManagerLib/BdsPlatform.c > create mode 100644 BhyvePkg/Library/PlatformBootManagerLib/BdsPlatform.h > create mode 100644 BhyvePkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf > create mode 100644 BhyvePkg/Library/PlatformBootManagerLib/PlatformData.c > create mode 100644 BhyvePkg/License.txt > create mode 100644 BhyvePkg/SmbiosPlatformDxe/Bhyve.c > create mode 100644 BhyvePkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c > create mode 100644 BhyvePkg/SmbiosPlatformDxe/SmbiosPlatformDxe.h > create mode 100644 BhyvePkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf > create mode 100644 BhyvePkg/VarStore.fdf.inc > > diff --git a/BhyvePkg/AcpiTables/AcpiTables.inf b/BhyvePkg/AcpiTables/AcpiTables.inf > new file mode 100644 > index 0000000000..d8907e6b56 > --- /dev/null > +++ b/BhyvePkg/AcpiTables/AcpiTables.inf > @@ -0,0 +1,42 @@ > +## @file > +# Component description file for PlatformAcpiTables module. > +# > +# ACPI table data and ASL sources required to boot the platform. > +# > +# Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.
> +# Copyright (c) 2014, Pluribus Networks, Inc.^M > +# > +# SPDX-License-Identifier: BSD-2-Clause > +# > +## (1) The "^M" character sequence seems bogus. (2) If you are contributing this new file in "Pluribus Networks, Inc" colors, then please extend the year to 2020 on that line. Otherwise, if you are contributing this on your own behalf (as work derived from Pluribus Networks's earlier work), then please add your own (C) on top as well, marking the year 2020. My point is that the year is 2020, when this file is being introduced in edk2. Thus, there should be a (C) notice naming the year 2020. [...] > diff --git a/BhyvePkg/AcpiTables/Dsdt.asl b/BhyvePkg/AcpiTables/Dsdt.asl > new file mode 100644 > index 0000000000..a60d001ffd > --- /dev/null > +++ b/BhyvePkg/AcpiTables/Dsdt.asl > @@ -0,0 +1,1134 @@ > +/* > + * Intel ACPI Component Architecture > + * AML Disassembler version 20100528 > + * > + * Disassembly of DSDT.dat, Sat Apr 18 15:41:05 2015 > + * > + * > + * Original Table Header: > + * Signature "DSDT" > + * Length 0x000008FA (2298) > + * Revision 0x02 > + * Checksum 0xC4 > + * OEM ID "BHYVE " > + * OEM Table ID "BVDSDT " > + * OEM Revision 0x00000001 (1) > + * Compiler ID "INTL" > + * Compiler Version 0x20150204 (538247684) > + */ (3) It's fine to include a decompiled DSDT ASL in the git tree; the INF file does reference this file. I'm requesting one of two things, dependent on your *plans* to update this file in the future. (3a) If this file should always be re-generated with "iasl -d" -- i.e., never edited manually --, then please add a comment at the top, to the effect of: generated file -- DO NOT EDIT (3b) If this file is meant as a starting point only, and is supposed to receive manual updates over time, then it needs a (C) notice, and an SPDX-License-Identifier, in my opinion. [...] > diff --git a/BhyvePkg/AcpiTables/Facp.aslc b/BhyvePkg/AcpiTables/Facp.aslc > new file mode 100644 > index 0000000000..73afb4a9aa > --- /dev/null > +++ b/BhyvePkg/AcpiTables/Facp.aslc > @@ -0,0 +1,75 @@ > +/* > + * Copyright (c) 2014, Pluribus Networks, Inc. > + * > + * SPDX-License-Identifier: BSD-2-Clause > + */ (4) Same as (2). [...] > diff --git a/BhyvePkg/AcpiTables/Facs.aslc b/BhyvePkg/AcpiTables/Facs.aslc > new file mode 100644 > index 0000000000..b66d698df4 > --- /dev/null > +++ b/BhyvePkg/AcpiTables/Facs.aslc > @@ -0,0 +1,79 @@ > +/** @file > + FACS Table > + > + Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause > + > +**/ (5) Same as (2). This issue seems to affect the rest of the newly added files; I wouldn't like to point it out repeatedly. Please refresh the copyright notices on all of the new files -- and not just in this patch, but also in the rest of the series. (Obviously, I could be wrong about this; feedback (especially from other stewards) is appreciated.) [...] > diff --git a/BhyvePkg/AcpiTables/Platform.h b/BhyvePkg/AcpiTables/Platform.h > new file mode 100644 > index 0000000000..4c029cc096 > --- /dev/null > +++ b/BhyvePkg/AcpiTables/Platform.h > @@ -0,0 +1,71 @@ > +/** @file > + Platform specific defines for constructing ACPI tables > + > + Copyright (c) 2014, Pluribus Networks, Inc.^M > + Copyright (c) 2012, 2013, Red Hat, Inc. > + Copyright (c) 2008, Intel Corporation. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause > + > +**/ (6) Same as (1) -- bogus "^M". [...] > diff --git a/BhyvePkg/License.txt b/BhyvePkg/License.txt > new file mode 100644 > index 0000000000..d1bdbae60f > --- /dev/null > +++ b/BhyvePkg/License.txt > @@ -0,0 +1,44 @@ > +Copyright (c) 2012, Intel Corporation. All rights reserved. (7) Since you are introducing this package now -- would it make sense to collect all the copyrights from the files being added under BhyvePkg, and collect them here? (This would include the new (C) notices that I'm requesting in this very review.) (8) Please spell out the SPDX identifier here, for the license text that follows below. > + > +Redistribution and use in source and binary forms, with or without > +modification, are permitted provided that the following conditions > +are met: > + > +* Redistributions of source code must retain the above copyright > + notice, this list of conditions and the following disclaimer. > +* Redistributions in binary form must reproduce the above copyright > + notice, this list of conditions and the following disclaimer in > + the documentation and/or other materials provided with the > + distribution. > + > +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS > +FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE > +COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, > +INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, > +BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER > +CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > +LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN > +ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > +POSSIBILITY OF SUCH DAMAGE. The 2-clause BSDL seems to end at this spot. The text that follows below is the MIT license : > + > + > +Permission is hereby granted, free of charge, to any person obtaining a copy > +of this software and associated documentation files (the "Software"), to deal > +in the Software without restriction, including without limitation the rights > +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > +copies of the Software, and to permit persons to whom the Software is > +furnished to do so, subject to the following conditions: > + > +The above copyright notice and this permission notice shall be included in > +all copies or substantial portions of the Software. > + > +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE > +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > +THE SOFTWARE. (9) I think we should delete the MIT license. At this stage, I can't see anything MIT-covered under BhyvePkg. Do you agree? (10) *However*, in the subsequent patches (#5 and #6), you are introducing content (PlatformPei and AcpiPlatformDxe) that is under BSD-2-Clause-Patent. Meaning that BhyvePkg needs a License.txt that's similar to OvmfPkg/License.txt: - it should list *two* licenses, - the license blocks should be separated visually (e.g. a long line of "====="), - both license blocks should have their SPDX identifiers, - the "default" license -- BSD-2-Clause-Patent --should be at the top, - the "non-default" license -- namely BSD-2-Clause -- should be at the bottom, and it should *list* the modules that are covered by it. (11) You should also extend the "Readme.md" file in the project root directory please. Simply search that file for "OvmfPkg/License.txt", and then you'll understand what I'm requesting -- just list "BhyvePkg/License.txt" there, similarly to how OvmfPkg's is. [...] > diff --git a/Maintainers.txt b/Maintainers.txt > index 1733225722..1d1a681216 100644 > --- a/Maintainers.txt > +++ b/Maintainers.txt > @@ -158,6 +158,13 @@ W: https://github.com/tianocore/tianocore.github.io/wiki/BaseTools > M: Bob Feng > M: Liming Gao > > +BhyvePkg > +F: BhyvePkg/ > +W: https://bhyve.org > +M: Rebecca Cran > +R: Peter Grehan > +S: Maintained > + > CryptoPkg > F: CryptoPkg/ > W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg > This hunk looks fine; I highly appreciate it. We'll need Peter to ACK this patch of the series in particular, on the edk2-devel list, please. Thanks! Laszlo