Re: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for Arm.

["Laszlo Ersek" <lersek@redhat.com>]

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