From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.18619.1587076993157118756 for ; Thu, 16 Apr 2020 15:43:13 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Hsp7fp8N; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587076992; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=QApvB9AzLfn1RqDa4lyRUje1okd/HDREEAiTNPqYTN8=; b=Hsp7fp8N9u/oDM4sxGfoppHkNKc2ENKGzmW8z68iE4KB02VnX7o3FZsekJ6RbaY8IIjHtE Rsj7h3uxXIJNvu7iOa7gkP3o00v8nb1s2c280mcxUyYwhN2EzAHW3i1S/lMvgVA1Yzt0XY tS+RTvEFwTjjsaX78H+9msuz4wCYxOg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-431-vwXQ7OVvOm-yWXP0tC-RuA-1; Thu, 16 Apr 2020 18:43:05 -0400 X-MC-Unique: vwXQ7OVvOm-yWXP0tC-RuA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 14E96100DFC3; Thu, 16 Apr 2020 22:43:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-225.ams2.redhat.com [10.36.114.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2AD1E60BE0; Thu, 16 Apr 2020 22:43:01 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 00/13] BhyvePkg - initial patch series for review From: "Laszlo Ersek" To: devel@edk2.groups.io, rebecca@bsdio.com Cc: Jordan Justen , Ard Biesheuvel , "Leif Lindholm (Nuvia address)" , Michael Kinney , Andrew Fish References: <66d11e29-504f-f41f-2a6d-e914d061277a@redhat.com> Message-ID: <7d046df4-86e8-a623-1db9-c94d19a2a814@redhat.com> Date: Fri, 17 Apr 2020 00:43:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <66d11e29-504f-f41f-2a6d-e914d061277a@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: > . 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 >