On 01/29/15 10:03, Laszlo Ersek wrote:
> On 01/29/15 09:05, Laszlo Ersek wrote:
>> On 01/29/15 03:50, Jordan Justen wrote:
>>> On 2015-01-28 14:38:00, Laszlo Ersek wrote:
>>>> On 01/28/15 20:01, Jordan Justen wrote:
>>>>> On 2015-01-28 00:50:43, Laszlo Ersek wrote:
>>>>>> 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.
>>>>>
>>>>> I'm thinking arch specific C files, possibly only with
>>>>> InternalQemuFwCfgReadBytes. It does seem like a single implementation
>>>>> of QemuFwCfgLib is possible. Regarding the PCDs, I would say they
>>>>> could be fixed or dynamic depending on the platform. Anyway, I guess
>>>>> this is more of a cleanup issue at this point...
>>>>
>>>> The PCDs are only needed for the memory-mapped fw_cfg registers, and
>>>> they are detected / set from the DTB. x86 uses IO ports.
>>>>
>>>>> Regarding the other issue. How about instead of yet-another-library,
>>>>> we add OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiDxe.inf that only supports
>>>>> fw-cfg?
>>>>
>>>> I assume that in that case, the following two files:
>>>> - OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf (current)
>>>> - OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiDxe.inf (suggested)
>>>>
>>>> would list different "AcpiPlatform.c" files (ie. the "outer shell" of
>>>> the drivers would differ), but both INF files would include Qemu.c from
>>>> that directory (and QemuFwCfgAcpiDxe would only utilize a part of
>>>> Qemu.obj). Then, ARM would include QemuFwCfgAcpiDxe.inf.
>>>>
>>>> I can only speak for myself, but I find several INF files per directory,
>>>> with an overlapping set of [Sources], *much* more confusing than a new
>>>> library (with appropriate dependencies). In fact this is my pet peeve with:
>>>> - OvmfPkg/Library/QemuFwCfgLib/
>>>> - ArmPlatformPkg/MemoryInitPei/
>>>> - ArmPlatformPkg/PlatformPei/
>>>> - MdeModulePkg/Library/SmmLockBoxLib/
>>>>
>>>> Whenever I have to look at these directories (and that's "frequently"),
>>>> my brain melts.
>>>>
>>>> ... What is your criticism against introducing QemuLoaderLib, summarily?
>>>
>>> To add this support to OvmfPkg/AcpiPlatformDxe, you'd need to add 1
>>> .inf and 1 .c file.
>>>
>>> To extract it out, you now have a new library (with a single function)
>>> that is used by two separate drivers.
>>>
>>> It is also conceivable that an OVMF build could use
>>> QemuFwCfgAcpiDxe.inf in some scenario where it only wanted to target
>>> newer QEMU builds.
>>
>> Understood.
>>
>> I'm not convinced, but I'll rework the series with QemuFwCfgAcpiDxe, my
>> conviction notwithstanding.
>
> I started implementing this, and then I found something that speaks
> against QemuFwCfgAcpiDxe.inf.
>
> As you said, "QemuFwCfgAcpiDxe.inf" should be conceivable for both
> ArmVirtualizationQemu.dsc and (potentially) OvmfPkg*.dsc. Accordingly,
> the leading comment on the new INF file should probably say something like
>
> ## @file
> # OVMF ACPI Platform Driver for QEMU virtual machines where the ACPI
> # linker/loader is guaranteed to be available.
>
> The issue here is the following.
>
> qemu-system-i386 and qemu-system-x86_64 have PCI. Correspondingly,
> OvmfPkg*.dsc includes MdeModulePkg/Bus/Pci/PciBusDxe, which performs PCI
> enumeration. The ACPI payload exposed by QEMU for these targets depends
> on the specific PCI resources that PciBusDxe assigns. Therefore
> AcpiPlatformDxe is not dispatched until PCI enumeration completes:
>
> [Depex]
> gEfiAcpiTableProtocolGuid AND gEfiPciEnumerationCompleteProtocolGuid
>
> Please see SVN r16411.
>
> The ARM targets have no PCI (yet). ArmVirtualizationQemu.dsc doesn't
> include either a root bridge driver or PciBusDxe. Accordingly, the
> ARM-specific AcpiPlatformDxe that is in patch 3/3 (which should become
> QemuFwCfgAcpiDxe.inf under OvmfPkg, according to your suggestion) has
> the following depex only:
>
> [Depex]
> gEfiAcpiTableProtocolGuid
>
> Using the driver with such a Depex would be incorrect on any qemu target
> that has PCI (including OVMF). Conversely, using the stricter
>
> [Depex]
> gEfiAcpiTableProtocolGuid AND gEfiPciEnumerationCompleteProtocolGuid
>
> would be incorrect for ARM targets (the driver would never be dispatched).
>
> Due to the different depexes we need separate INF files for ARM and x86
> targets (at least for the time being), despite the QemuFwCfgAcpiDxe
> driver supporting only the linker/loader interface on both.
>
> Yes, yes, I could introduce a new Feature PCD describing PCI
> availability, and based on that, I could optionally use
> gBS->RegisterProtocolNotify() to delay ACPI table installation. I could
> also introduce another build-time define (-D) that would select the
> appropriate Depex in the INF file. I think sharing the same "outer"
> driver code and the INF file between ARM and x86 is not worth this
> terrible gymnastics.
I even implemented that -- the approach with gBS->RegisterProtocolNotify().
However, splitting Qemu.c is unavoidable. The things that I do *not*
need for ARM -- ie. "part A" in
http://thread.gmane.org/gmane.comp.bios.tianocore.devel/12158/focus=12256
-- is not only something we don't *call* on ARM, it doesn't even
*compile* for ARM. The reason being, that code depends on PCDs that are
x86 specific, and make no sense for ARM:
- gPcAtChipsetPkgTokenSpaceGuid.Pcd8259LegacyModeEdgeLevel
- gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
- gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
So let's collect what the above results mean, for QemuFwCfgAcpiDxe:
- we need a new INF file, QemuFwCfgAcpiDxe.inf,
- we need a new C file with the new driver's entry point,
- we must introduce a new Feature PCD, and dependent on it, call
RegisterProtocolNotify(), so that the dependency on PCI enumeration
can be expressed flexibly,
- due to x86-related PCDs in the unneeded code of Qemu.c, we need to
split Qemu.c into two files, which adds yet another C file.
2 new .c files, 1 new .inf file, 1 new PCD, and protocol callback hackery.
Versus, 1 new .c file, 1 new .inf file, and 1 new .h file added by this
patch. Please accept it; I spent the time to explore your alternative,
and it seems inferior.
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
