> -----Original Message----- > From: miaoyubo > Sent: Friday, April 3, 2020 4:56 PM > To: 'Laszlo Ersek' <ler...@redhat.com>; ard.biesheu...@linaro.org; > l...@nuviainc.com > Cc: devel@edk2.groups.io; Xiexiangyou <xiexiang...@huawei.com> > Subject: RE: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for > Arm. > > > > > -----Original Message----- > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > Sent: Thursday, April 2, 2020 9:52 PM > > To: miaoyubo <miaoy...@huawei.com>; ard.biesheu...@linaro.org; > > l...@nuviainc.com > > Cc: devel@edk2.groups.io; Xiexiangyou <xiexiang...@huawei.com> > > Subject: Re: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots > > for Arm. > > > > Quick review comments only, for now: > > > > On 04/02/20 14:08, Yubo Miao wrote: > > > From: miaoyubo <miaoy...@huawei.com> > > > > > > Add support for extra roots for arm, in this patch, we import the > > > scan for extra root buses therefore the uefi could recognize > > > multiply roots for arm. > > > > > > The logic keeps the same with the logic in > > > "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c" > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: miaoyubo <miaoy...@huawei.com> > > > --- > > > .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 264 +++++++++++++++-- > - > > > .../FdtPciHostBridgeLib.inf | 3 + > > > 2 files changed, 230 insertions(+), 37 deletions(-) > > > > (1) The "Contributed-under:" line is no longer necessary (even > > defined) in the commit message. > > > > (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? > > (3) Can you please provide a pointer to the QEMU-side work? In > > particular, this logic depdens on the "etc/extra-pci-roots" fw_cfg file. > > Where is that file being exposed by qemu-system-aarch64 / "virt"? In > > general, the firmware code is merged after the QEMU work is merged. > > Has the design been accepted for QEMU at least? (So that it make sense > > for us to look at the interfacing ArmVirtPkg code.) > > > > (4) Have you tested booting from PCI devices behind the "extra" root > bridges? > > In particular, have you tested the boot order manipulation via the > > "bootindex" device properties? (OvmfPkg/Library/QemuBootOrderLib is > > already being used by the ArmVirtQemu platform, so I'd expect no > > changes related to boot order filtering / reordering -- but it should > > be tested.) > > > > (5) I think this feature deserves a TianoCore Feature Request. Can you > > please file one at <https://bugzilla.tianocore.org/>? Then the > > bugzilla link should be referenced in the commit message. > > > > (Preferably the bugzilla entry should summarize the present QEMU > > status too, or provide further links to QEMU discussions etc.) > > > > Thanks, > > Laszlo > > > > Thanks for replying! > 1. I see, I would not define it in next patch. > 2. I would factor the same logic parts into a separate library. > 3.The qemu side work is to support pxb-device(which would have extra > roots), the patch has been updated to V4, but "etc/extra-pci-roots" fw_cfg > file work would be done in v5. I would soon push the patch v5 and attach the > link in next edk patch. > 4. Yes, I have tested to boot from the pci devices behind extra roots with > bootindex, it works very well. > 5. It would be done in next patch. > > > > > > > > diff --git > > > a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > > > b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > > > index 496b192d22..706efeb416 100644 > > > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > > > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > > > @@ -14,6 +14,10 @@ > > > #include <Library/MemoryAllocationLib.h> #include > > > <Library/PcdLib.h> #include <Library/UefiBootServicesTableLib.h> > > > +#include <Library/QemuFwCfgLib.h> > > > +#include <Library/PciLib.h> > > > +#include <IndustryStandard/Pci.h> > > > +#include <Library/BaseMemoryLib.h> > > > > > > #include <Protocol/FdtClient.h> > > > #include <Protocol/PciRootBridgeIo.h> @@ -306,7 +310,70 @@ > > > ProcessPciHost ( > > > return Status; > > > } > > > > > > -STATIC PCI_ROOT_BRIDGE mRootBridge; > > > +EFI_STATUS > > > +InitRootBridge ( > > > + IN UINT64 Supports, > > > + IN UINT64 Attributes, > > > + IN UINT64 AllocAttributes, > > > + IN UINT8 RootBusNumber, > > > + IN UINT8 MaxSubBusNumber, > > > + IN PCI_ROOT_BRIDGE_APERTURE *Io, > > > + IN PCI_ROOT_BRIDGE_APERTURE *Mem, > > > + IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, > > > + IN PCI_ROOT_BRIDGE_APERTURE *PMem, > > > + IN PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G, > > > + OUT PCI_ROOT_BRIDGE *RootBus > > > + ) > > Regards, > Miao Regards， Miao -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#57064): https://edk2.groups.io/g/devel/message/57064 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] -=-=-=-=-=-=-=-=-=-=-=-
