Topics

OvmfPkg: Adding support for bhyve as OvmfPkg/Bhyve

Rebecca Cran
 

I've finished the work to move BhyvePkg into OvmfPkg/Bhyve, and the changes are available at https://github.com/bcran/edk2/commit/09cfc08b50992ed3f76459611e8a928b74ed8c8a .

Since it's such a large patch (over 400KB, but the vast majority of it is existing changes from previously-reviewed patches), I'm posting a link to the changes here in case there are any fundamental issues with it before I post it for review.

I've run PatchCheck.py and the only issue it flagged is that the 'license' on VbeShim.h is wrong, and that's because the header is non-standard:

//
// THIS FILE WAS GENERATED BY "VbeShim.sh". DO NOT EDIT.
//


--

Rebecca Cran

Laszlo Ersek
 

Hi Rebecca,

On 06/26/20 17:38, Rebecca Cran wrote:
I've finished the work to move BhyvePkg into OvmfPkg/Bhyve, and the
changes are available at
https://github.com/bcran/edk2/commit/09cfc08b50992ed3f76459611e8a928b74ed8c8a
.

Since it's such a large patch (over 400KB, but the vast majority of it
is existing changes from previously-reviewed patches), I'm posting a
link to the changes here in case there are any fundamental issues with
it before I post it for review.
I'll look at the patches on the list. (I don't believe in reviewing
before reviewing. In case I need to make comments for what I find in
your repo, I could only make them without any context to quote.)

I've run PatchCheck.py and the only issue it flagged is that the
'license' on VbeShim.h is wrong, and that's because the header is
non-standard:

//
// THIS FILE WAS GENERATED BY "VbeShim.sh". DO NOT EDIT.
//

This likely comes from BaseTools commit a4cfb842fca9
("BaseTools/PatchCheck.py: Add LicenseCheck", 2020-06-12).

One approach would be to remove "VbeShim.h" from the tracked files under
OvmfPkg, replacing it with a PREBUILD command in the OVMF DSC files.
(Then Bhyve could do the same.)

However, the generator, namely "VbeShim.sh", is not written in Python,
but in (POSIX) shell, and so it can't be called from PREBUILD (I think
it would break OVMF builds on Windows).

I don't know what to tell you, other than the blanket license
enforcement from commit a4cfb842fca9 is likely wrong.

The generated include file *must* be a ".h" file, otherwise the INF file
reference won't be able to trigger an incremental build, if I understand
correctly. So replacing the ".h" suffix with something else, such as
".genh" (for "generated header") won't work, I believe.

Modifying the printf invocations in the generator script to also output
a license header would not be right either, IMO. A license tag makes no
sense (I think) without a copyright (C) statement. And what copyright
(C) notice should we put on a generated file?

Furthermore, although "ReadMe.rst" in the project root states

"""
Contributions of code put into the public domain can also be accepted.
"""

I don't see how the license check implemented in commit a4cfb842fca9
would accommodate a public domain contribution. (I think it would be
fine to place the the generated header file in the public domain, *if*
(a) we could express that somehow (is there an SPDX tag for that?),
*and* (b) if that would eliminate the need for a (C) notice / authorship
mark.)

Note that this generator use case is not unique to QemuVideoDxe; see for
example commit 1e9d6b0f98b5 ("OvmfPkg/OvmfXen: Creating an ELF header",
2019-08-21).

I've now filed a bug for BaseTools:

https://bugzilla.tianocore.org/show_bug.cgi?id=2833

Once that bug is solved -- that is, once we standardize a tag for
marking generated source files as such --, we can update "VbeShim.sh" to
produce the tag, in "VbeShim.h". Then OvmfPkg/Bhyve can do the same.

Thanks
Laszlo

Rebecca Cran
 

On 7/2/20 3:27 AM, Laszlo Ersek wrote:

I'll look at the patches on the list. (I don't believe in reviewing
before reviewing. In case I need to make comments for what I find in
your repo, I could only make them without any context to quote.)
Thanks, I'll wait for the changes to PatchCheck.py to land, then send the bhyve patch to the list. I think at this point I've updated all the licenses to be BSD+Patent with the exception of the generated file.


--
Rebecca Cran

Laszlo Ersek
 

On 07/03/20 05:13, Rebecca Cran wrote:
On 7/2/20 3:27 AM, Laszlo Ersek wrote:

I'll look at the patches on the list. (I don't believe in reviewing
before reviewing. In case I need to make comments for what I find in
your repo, I could only make them without any context to quote.)
Thanks, I'll wait for the changes to PatchCheck.py to land, then send
the bhyve patch to the list. I think at this point I've updated all the
licenses to be BSD+Patent with the exception of the generated file.
Sounds great!
Laszlo