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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#57476): https://edk2.groups.io/g/devel/message/57476 Mute This Topic: https://groups.io/mt/73045166/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
