Re: [edk2-devel] [PATCH 00/13] BhyvePkg - initial patch series for review

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

argh, used the wrong email address for CC'ing Leif. Resending...

On 04/16/20 22:47, Laszlo Ersek wrote:
> Hi Rebecca,
> 
> On 04/16/20 01:09, Rebecca Cran wrote:
>> This is the bhyve patch series for initial review.
>> I know there are a few formatting issues, but thought
>> it might be good to get some feedback to make sure I'm
>> on the right track with adding this new package/platform.
> 
> Thanks for posting this early.
> 
> First, some rudimentary feedback.
> 
> Regarding the BhyvePkg patches, I'm not familiar with the platform, and
> I definitely won't have time to review *those* patches, even for general
> sanity, or coding style. I recommend / request:
> 
> (1) Please try to run the BhyvePkg code through the ECC tool, and see if
> ECC is OK with the coding style. (In some cases ECC is *way* too strict
> in my opinion, so please use common sense, and/or ask for feedback on
> the list, with specific code examples and questions.)
> 
> (2) Regarding platform code correctness, if someone from the bhyve
> community can help you and review your code, that's welcome. We've had
> fruitful examples for such collaboration, between xen-devel and
> edk2-devel subscribers. But, I won't really "insist" on this -- I just
> propose it.
> 
> (3) Please use SPDX license tags in the new files, rather than
> open-coded license blocks. Reference:
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1373>. Note that this
> particular request of mine is only formal (it targets the representation
> of the licenses, not the license terms themselves).
> 
> (4) Furthermore (asking under a separate bullet point), if you can,
> please contribute the new files under the "BSD-2-Clause-Patent" license,
> not the 2-clause BSDL. (See "# License Details" in "Readme.md", and
> again TianoCore#1373.)
> 
> If you must (or want to) stick with the 2-clause BSDL, then please list
> the components with that license in "Readme.md", near the existent
> "exceptions".
> 
> (5) Please make sure that the new files use CRLF line terminators
> consistently.
> 
> (6) Please add a block to "Maintainers.txt" that lists the new package,
> and designates you as "M:". It would be nice to have at least an "R:"
> person too, e.g. from the *BSD / bhyve communities. (You'll need a
> reviewer for your future patches too -- I guess some of the stewards
> could alternate and help out with that, but that should be a stop-gap
> solution, not the rule. Adding BhyvePkg to edk2 should be justified by
> at least two bhyve users stepping up, for reviewing each other's
> BhyvePkg patches.)
> 
> With the above addressed, I reckon I'm OK with ACKing the BhyvePkg
> patches. I hope at least one other steward can approve the BhyvePkg
> patches, like that. Obviously feedback and corrections to my above
> requests is highly welcome.
> 
> 
> Regarding the OvmfPkg patches, I'll go through them (later) with a finer
> granularity. In advance, I can already tell you that I wouldn't like
> bhyve-oriented changes added to either OvmfPkg/PlatformPei (patch#9) or
> to OvmfPkg/AcpiPlatformDxe (patch#10). Both of these modules are already
> extremely complex (aka "brittle"), they're super important on QEMU; and
> not only am I worried about regressions, but I'm strongly opposed to any
> coding restrictions/limitations that bhyve code might present in those
> modules for future QEMU features/bugfixes.
> 
> So, regarding these two modules, please do create copies of them under
> BhyvePkg, add the bhyve special sauce there, and eliminate everything
> from them that you do not need on bhyve.
> 
> These are modules where I definitely opt for razor sharp separation --
> and code duplication, if need be --, because both modules are very
> tightly coupled with the hypervisor platform, and historically, both
> modules have caused *lots* of headache. Xen code in these modules is
> already a maintenance problem for me (but the Xen code is thankfully on
> its way to its own platform, see TianoCore#2122).
> 
> Thanks!
> Laszlo
>