From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.15986.1587070056071930628 for ; Thu, 16 Apr 2020 13:47:36 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Vn94ZrqZ; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587070055; 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=JmICedEHQjD3ZCDN/uRvqBbS3keEi+DwCIz13k4vKnI=; b=Vn94ZrqZCKsaE898W9faMB+mD2/NN7oWjLuFgduSMBqh7zAzUe0G52tthlTc/KvH2XotvG r3hY9gGo55zinLGzVg3Jt2Dsyk9qUS83zfyZAW8kagcOVb8ZnU4MoOjqW7eZsydamXzirv ZRFMR7n6jYnRbcHEqp6Lz9zjsVDue5w= 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-66-bOOMkdKpMvK0IrR9v5hygQ-1; Thu, 16 Apr 2020 16:47:27 -0400 X-MC-Unique: bOOMkdKpMvK0IrR9v5hygQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 71FA91034B5C; Thu, 16 Apr 2020 20:47:25 +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 671137E7D1; Thu, 16 Apr 2020 20:47:23 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 00/13] BhyvePkg - initial patch series for review To: devel@edk2.groups.io, rebecca@bsdio.com Cc: Jordan Justen , Ard Biesheuvel , "Leif Lindholm (Linaro address)" , Michael Kinney , Andrew Fish References: From: "Laszlo Ersek" Message-ID: <66d11e29-504f-f41f-2a6d-e914d061277a@redhat.com> Date: Thu, 16 Apr 2020 22:47:22 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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 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