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 02D84803A6 for ; Fri, 10 Mar 2017 12:16:29 -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 3F7B3C05680F; Fri, 10 Mar 2017 20:16:29 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-26.phx2.redhat.com [10.3.117.26]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v2AKGQUB030846; Fri, 10 Mar 2017 15:16:27 -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> Cc: Ard Biesheuvel From: Laszlo Ersek Message-ID: <0278752e-36a1-4d0b-736f-77a17821a427@redhat.com> Date: Fri, 10 Mar 2017 21:16:25 +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: <148913972621.26490.6378343220939156912@jljusten-skl> 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.32]); Fri, 10 Mar 2017 20:16:29 +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: Fri, 10 Mar 2017 20:16:29 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 - 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