Re: [edk2-devel] [PATCH v3 4/6] Add BhyvePkg, to support the bhyve hypervisor

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

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 <rebecca@bsdio.com>
> ---
>  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.<BR>
> +#  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.<BR>
> +
> +  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.<BR>
> +
> +  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 <https://spdx.org/licenses/BSD-2-Clause.html> seems to
end at this spot.

The text that follows below is the MIT license
<https://spdx.org/licenses/MIT.html>:

> +
> +
> +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 <bob.c.feng@intel.com>
>  M: Liming Gao <liming.gao@intel.com>
>
> +BhyvePkg
> +F: BhyvePkg/
> +W: https://bhyve.org
> +M: Rebecca Cran <rebecca@bsdio.com>
> +R: Peter Grehan <grehan@freebsd.org>
> +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