On 01/28/15 00:29, Jordan Justen wrote:
> On 2015-01-27 00:17:52, Laszlo Ersek wrote:
>> On 01/26/15 22:32, Jordan Justen wrote:
>>> On 2015-01-24 15:04:52, Laszlo Ersek wrote:
>>>> +EFI_STATUS
>>>> +EFIAPI
>>>> +InstallAllQemuLinkedTables (
>>>> + IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
>>>> + );
>>>
>>> What do you think about moving this to QemuFwCfgLib instead?
>>
>> This crossed my mind earlier, but I don't think it's a good idea. Just
>> because it depends on FwCfg, I don't want to fuse it with the base
>> library. (Same for QemuBootOrderLib.) ("Base" meaning "foundational",
>> not "available to all phases", in this context, clearly.)
>>
>> (1) Main reason is it makes it harder to port the various independent
>> features gradually, and/or to port them selectively even for the longer
>> term.
>>
>> A good negative example is the QemuFwCfgS3Enabled() API. Because it was
>> so small and so easy to implement for OVMF / x86, we fused it with the
>> QemuFwCfgLib interface. When implementing the library class for ARM
>> guests, I had no choice but to implement it too (as "return FALSE")
>> because it had already been part of the interface. The implementation is
>> quite useless, and worse, nothing at all calls it in ArmVirtualizationPkg.
>>
>>> How about:
>>> RETURN_STATUS
>>> EFIAPI
>>> QemuFwCfgInstallAcpiTables (
>>> VOID
>>> );
>>>
>>> Obviously this should just assert if called in SEC or PEI.
>>
>> I can rename the function if you'd like, but I think build-time (ie.
>> interface-level) constraints of an API are superior to runtime asserts.
>> This holds for both different UEFI phases and different architectures.
>>
>> You probably don't like the proliferation of small, QEMU-specific
>> libraries in OVMF. I can appreciate that from an aesthetic POV, but
>> these features are really this fine-grained, and exposing their
>> dependencies on the library class level allows me to port them more
>> flexibly.
>>
>> (2) Another reason is that by making the QemuFwCfgLib lib class more
>> comprehensive, code duplication would worsen. The QemuBootOrderLib and
>> QemuLoaderLib functionality is identical between ARM and x86. The
>> underlying fw_cfg access / transfer methods are different.
>
> They may be different, but looking, I'm wondering why
> OvmfPkg/Library/QemuFwCfgLib doesn't have arm support, rather than
> putting it into a separate module over in
> ArmPlatformPkg/ArmVirtualizationPkg/Library/QemuFwCfgLib.
(a) OvmfPkg/Library/QemuFwCfgLib shares the C-language implementation of
InternalQemuFwCfgReadBytes() between Ia32 and X64, and the underlying
IoReadFifo8() function has platform-dependent assembly implementation.
Whereas ArmPlatformPkg/ArmVirtualizationPkg/Library/QemuFwCfgLib has an
implementation that differs even on the C language level (and no
assembly at all).
The library constructor is also different; the latter depends on
ARM-specific PCDs. It seemed cleaner to break away completely than to
litter the code with MDE_CPU_* macros, different INF sections, and mix
in some assembly too.
(b) OvmfPkg/Library/QemuFwCfgLib supports a large number of UEFI phases
(with different library instances). Since the ARM one depends on PCDs,
and also because ArmVirtualizationPkg needs it only for DXE_DRIVER
modules, the ARM library instance supports only DXE_DRIVER modules.
I already find OvmfPkg/Library/QemuFwCfgLib complicated. It supports 2
INF files (ie. 2 sets of UEFI phases) * 2 assembly subdirs == 4 configs.
I needed something for ARM that supported 1 C file and 1 UEFI phase. It
could have been solved by adding a 3rd INF file to
OvmfPkg/Library/QemuFwCfgLib and splitting up the existent C files even
more ways than they are now. I didn't like it.
> It still may be valid to discuss whether it make sense to add to the
> QemuFwCfgLib interface, but if they are able to be merged into a
> single module, then maybe there would be less worry about code
> duplication.
>
> To throw out another idea ... If it still feels like there should be a
> separation between a library that accesses fw-cfg, and libraries than
> make use of fw-cfg data: How about something like QemuFwCfgIoLib and
> QemuFwCfgLib?
As far as I undertand, that would mean:
- renaming QemuFwCfgLib to QemuFwCfgIoLib,
- fusing QemuBootOrderLib, QemuLoaderLib, and maybe QemuFwCfgS3Enabled
into a new QemuFwCfgLib.
It looks technically doable, but it also appears to be a fair amount of
churn for little benefit. The resultant libraries don't appear cleaner
to me. Part of the new (fused) QemuFwCfgLib would be linked into the BDS
driver (the boot order functionality). Another part would be linked into
AcpiTableDxe (the ACPI download / processing code). The only organizing
principle would be "this stuff depends on fw_cfg".
>> If I fused
>> these all together, I'd either need two full library instances
>> (duplicating QemuBootOrderLib & QemuLoaderLib just for the sake of the
>> different fw_cfg access methods), *or* I'd have to keep these all in one
>> subdirectory somewhere, and use [Sources.AARCH64] and [Sources.X64,
>> Sources.Ia32] sections in the INF file just for the transfer &
>> constructor code. I find those sections unwieldy.
>
> Hmm. I guess you already don't like my idea above. :)
The idea may easily have merit (there's really no way to deny that
completely without actually implementing it), but I don't have more time
to experiment with it than you do. :)
I think that the proposed "library granularity" is clean enough. It
allowed me to work on the refactoring / porting in feature-let sized
sprints. If you'd like to see your idea implemented first, and compare
it against the proposal at hand, then I'll have to shelve this series
until either you or me find the time to code up what you propose.
Thanks,
Laszlo
------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
