From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A3095803B0 for ; Fri, 10 Mar 2017 19:17:59 -0800 (PST) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D554E85545; Sat, 11 Mar 2017 03:17:59 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-21.phx2.redhat.com [10.3.116.21]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v2B3HvUM014249; Fri, 10 Mar 2017 22:17:58 -0500 To: Jordan Justen , edk2-devel-01 References: <20170223014814.10937-1-lersek@redhat.com> <148879199242.27619.7627489493363959501@jljusten-skl> <3e3242e1-2578-3b14-636e-58d8000b41da@redhat.com> <148913972621.26490.6378343220939156912@jljusten-skl> <0278752e-36a1-4d0b-736f-77a17821a427@redhat.com> Cc: Ard Biesheuvel From: Laszlo Ersek Message-ID: <489879bd-5690-ba73-7cec-7516f56e6166@redhat.com> Date: Sat, 11 Mar 2017 04:17:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <0278752e-36a1-4d0b-736f-77a17821a427@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Sat, 11 Mar 2017 03:17:59 +0000 (UTC) Subject: Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 11 Mar 2017 03:17:59 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/10/17 21:16, Laszlo Ersek wrote: > On 03/10/17 10:55, Jordan Justen wrote: >> On 2017-03-06 10:41:31, Laszlo Ersek wrote: >>> On 03/06/17 10:19, Jordan Justen wrote: >>> >>>> Is there any reason we couldn't add a >>>> pointer within the scatch buffer for reading/writing to? It just >>>> seems to make the interface a bit more generic/intuitive. >>> >>> That's how I initially designed the interface, but while implementing >>> the functions, I realized there was no good use for that feature. >>> >>> The only use case for storing the data read from fw_cfg at a nonzero >>> offset in the scratch buffer, and for writing data to fw_cfg from a >>> nonzero offset in the scratch buffer, would be that you have "something >>> else" at offset zero. What could that thing be? >>> >> >> We discussed this on irc a bit. I agree that we can proceed with your >> current design. >> >> I still think the library interface would be clearer if it had the >> buffer passed to the read/write routines, but I concede that it would >> complicate and grow the code for no other gain than a clearer library >> interface. >> >> I think the helper callback adds to the confusion in the library >> interface, but if you don't have the buffer passed into the read/write >> routines, then you need the scratch buffer, and then the callback makes >> sense. >> >> I think we could have one helper to 'call me back when boot script is >> available', but didn't have the scratch allocation aspect. Of course, >> having torn out all the fw-cfg context, this now seems like something >> that some generic boot script lib might provide. >> >> Then I'd leave it up to the user of the lib to allocate a buffer and >> pass it into the fw-cfg boot script routines to be logged. The library >> would also have to allocate the DMA transfer buffer, which is just one >> way that this would be less efficient. >> >> Once again, I agree this would be less efficient, but I think it >> easier to follow. (Or, at least more intuitive from a library >> interface perspective.) And, once again, I am okay with you proceeding >> with your current design. > > Thank you very much for taking the time to review this and for the > suggested improvements. I'd like to post v2 with the following changes: > > - rename QemuFwCfgS3TransferOwnership to > QemuFwCfgS3CallWhenBootScriptReady > > - rename FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION to > FW_CFG_BOOT_SCRIPT_CALLBACK_FUNCTION > > - similarly, rename Append to BootScriptCallback I'll use plain "Callback" here; "BootScriptCallback" would blow up all of the comment formatting. Thanks Laszlo > > - rename QemuFwCfgS3Xxx to QemuFwCfgS3ScriptXxx > > I wouldn't like to change the following aspects: > > - Indentation of function calls -- I've been consistent about those for > several years, they satisfy the requirements of the coding std, and code > I wrote like that had been accepted even in MdeModulePkg > (PciHostBridgeDxe) and MdePkg (BaseOrderedCollectionRedBlackTreeLib). > > My main argument against *exclusively* using the one-arg-per-line style > is that it wastes a lot of vertical space, especially because I'm > unwilling to write longer than 80char lines. (Those who are willing to > go up to 120 -- that is, 50% more! -- chars per line have it much > easier, because they need to break up a *lot* fewer function calls.) > > - The way the scratch buffer is allocated and propagated. I entirely > agree with you on both characteristics of the v1 set: it is a bit more > efficient, and a bit harder to follow, than the alternative you > describe. I think arguments can be made equally well in favor of either > approach. > > Unfortunately, I'm again out of time (with downstream responsibilities > rearing their ugly heads -- the rebase is done, but more churn is to > follow). I'll be happy if I can squeeze out a v2 with the above > non-intrusive changes early next week. (I wonder if the mental image of > the upcoming chaos is going to stress me enough to post v2 over the > weekend. Sigh.) > > Thank you, > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >