> -----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. > > (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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56938): https://edk2.groups.io/g/devel/message/56938 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] -=-=-=-=-=-=-=-=-=-=-=-