On 04/12/16 12:15, Ard Biesheuvel wrote:
> On 11 April 2016 at 21:14, Laszlo Ersek <[email protected]> wrote:
>> * Patch
>>
>> 8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol
>>
>> also looks mostly good. Some comments:
>>
>> - Please fix a typo in the commit message:
>>
>> s/host provided/host-provided/
>>
>> - Also, the paragraph "This means some PCI related PCDs...", in the
>> commit message, belongs to the next patch
>> ("ArmVirtPkg/VirtFdtDxe: drop PCI host bridge handling"). Please
>> move it over.
>>
>> - The ProcessPciHost() function's leading comment has become stale.
>> Please drop it.
>>
>> - Please do not introduce those global variables just for
>> returning data from ProcessPciHost() to InitializePciHostBridge().
>> Use locals and an appropriate signature for ProcessPciHost(). (You
>> can collect those values in a new structure type too.)
>>
>> This might require you to bring back those explicit
>> zero-assignments, before the "ranges" loop.
>>
>> - Since the MmioBase and MmioSize values are no longer kept in UINT32
>> PCDs (but UINT64 variables), the explicit (UINT64) cast is
>> unnecessary, in InitializePciHostBridge().
>>
>> - The nullity checks in (EFI_ERROR (Status) || Prop == NULL) are
>> redundant, I think, after all of the GetNodeProperty() calls, in
>> ProcessPciHost().
>>
>> - Can you factor out the setting of the /chosen node to a separate
>> function? (Together with the Tmp variable.)
>>
>> - In this (v3-wip) iteration, you also removed the
>>
>> PcdSetBool (PcdPciDisableBusEnumeration, FALSE);
>>
>> call. I almost suggested to reinstate it, but doing so wouldn't be
>> safe actually.
>>
>> Namely, according to the build report, two drivers consume
>> PcdPciDisableBusEnumeration: PciBusDxe and
>> QemuFwCfgAcpiPlatformDxe.
>>
>> The PciBusDxe case is safe, because it is a UEFI driver model
>> driver that depends on protocols produced by PciHostBridgeDxe.
>> However, QemuFwCfgAcpiPlatformDxe is just a DXE_DRIVER, and it
>> consumes PcdPciDisableBusEnumeration first thing in its entry point.
>>
>> This means that you will have to introduce another plugin library
>> (in a separate patch) just for setting PcdPciDisableBusEnumeration,
>> and link it into both PciBusDxe and QemuFwCfgAcpiPlatformDxe.
>>
>> Also, in this plugin library, since the PCD is just a boolean, we
>> cannot use the caching trick (through a special value of the PCD),
>> hence minimally the presence of "pci-host-ecam-generic" will have to
>> be verified every time.
>>
>
> Actually, looking at this again, wouldn't it make sense to have a
> single plugin library that sets both PcdPciDisableBusEnumeration and
> PcdPciExpressBaseAddress, and use it for these two modules and for the
> BaseCachingPciExpressLib?
>
This could be a valid idea, but it requires some care.
- BaseCachingPciExpressLib will have to spell out the dependency on
this additional library class explicitly, otherwise the order of
constructors would not be ensured. In other words, for clients of
BaseCachingPciExpressLib, the new library instance would not be
"plugged", but regularly inherited.
It should also mean that the source code of the
BaseCachingPciExpressLib instance doesn't change (relative to
master), only the [LibraryClasses] section does, in its INF file.
- For modules that need PcdPciDisableBusEnumeration, the plugin lib
instance would have to remain a plugin. I'm unsure about the case
when a module gets the plugin both implicitly and explicitly.
- Should the condition (PcdPciExpressBaseAddress != MAX_UINT64) imply
that PcdPciDisableBusEnumeration has been finalized as well? Probably
so, but this should be documented in the DSC files, near the
MAX_UINT64 default value.
- The above co-caching saves some cycles, and we avoid yet another
library class+instance. That's good. However, we should check how
much potential is there for wasted work.
For a client of PciExpressLib, the waste is only a single
PcdSetBool(), which is negligible.
For a client that needs only PcdPciDisableBusEnumeration, the waste
is: a PcdSet64(), which is negligible, and the fact that we don't
just look up the "pci-host-ecam-generic" node, but also grab its
"reg" property (plus a SwapBytes64() call). I don't think it's bad.
Clearly, the "waste" in the above cases can only be called "waste" if
nothing in the firmware needs that "other" thing later. And even
then, the "waste" is spent only once, due to the caching through
(PcdPciExpressBaseAddress != MAX_UINT64).
So, I think this is worth a shot!
I wonder what a good name could be, for this new plugin lib.
PciPcdProducerLib?
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
