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 >
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#57479): https://edk2.groups.io/g/devel/message/57479 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] -=-=-=-=-=-=-=-=-=-=-=-
