Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

[Laszlo Ersek <lersek@redhat.com>]

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
>