On 04/08/20 05:58, miaoyubo wrote: >>> (2) This code is way too large for my taste to duplicate between >>> ArmVirtPkg and OvmfPkg. I would strongly prefer if we could factor the >>> logic in OvmfPkg out to a separate library, and use that from both >> consumer sites. >>> > > Where should I put the extracted logic?(Still put in OvmfPkg and direct use > it in ArmVirtPkg or > put them in MdeModulePkg/Include/Library/PciHostBridgeLib.h? If in > PciHostBridgeLib.h, > should I create a new .c file as a common implementation?) > There are some other codes duplicated between FdtPciHostBridgeLib.c and > PciHostBridgeLib.c, > should I extract them?
Good questions, I've kind of expected them. Thanks for asking them. There are at least two ways to approach this. * One would be to move the ArmVirtPkg/Library/FdtPciHostBridgeLib/ stuff under OvmfPkg/Library/PciHostBridgeLib/, have two INF files in the same directory, and separate out exactly those bits of functionality that are shared, using module-internal header files. This is usually a good thing if much of the logic is shared. In this case, I don't like it however, as the ArmVirtPkg lib instance INF file itself lists some dependencies that come from "ArmPkg.dec" and "ArmVirtPkg.dec": - PciPcdProducerLib class - PcdPciMmio32Translation, PcdPciMmio64Translation, PcdPciIoTranslation - gFdtClientProtocolGuid So we'd either have to move those into OvmfPkg, or else make OvmfPkg content depend on ArmVirtPkg.dec / ArmPkg.dec. None of those are good ideas. * Therefore, please go the more winding route, and introduce a new lib class, and lib instance, under OvmfPkg. For the lib class name, I suggest "PciHostBridgeUtilityLib". Files: OvmfPkg/OvmfPkg.dec OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c Then both OvmfPkg's and ArmVirtPkg's PciHostBridgeLib instances can consume this. I'd prefer four patches: - #1: OvmfPkg patch. Introduce the new lib class and lib instance, with functionality that the ArmVirtPkg and OvmfPkg PciHostBridgeLib instances already share. Regarding the OvmfPkg/Library/PciHostBridgeLib instace, you can at once move code from that lib instance to the new OvmfPkg/Library/PciHostBridgeUtilityLib, in this patch. - #2: ArmVirtPkg patch. Elminate the currently duplicated code, and put the newly extracted code to use, through consuming the new lib class. - #3: OvmfPkg patch. Move more code from PciHostBridgeLib to PciHostBridgeUtilityLib: code that you will need to use in the next patch, for enabling multiple root bridges in ArmVirtPkg. -#4: ArmVirtPkg patch. Consume the code exposed in patch#3 for enabling the feature. In theory, the code extraction / movement described in patches #1 and #3 could be done in a single unified step, at the start of the series. However, we certainly want to keep #2 and #4 separate (#2 is refactoring, #4 is feature enablement). And then I prefer keeping #1 and #3 separate too -- while both move code from the same old lib instance to the same new lib instance, their motivations are different. The ultimate goal is of course just the one "share as much code as possible between ArmVirtPkg and OvmfPkg", but, when I run "git-blame" on the new lib class header, I'd like to understand why exactly any given function prototype was declared there. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#57062): https://edk2.groups.io/g/devel/message/57062 Mute This Topic: https://groups.io/mt/72723351/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
