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

[miaoyubo <miaoyubo@huawei.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, April 2, 2020 9:52 PM
> To: miaoyubo <miaoyubo@huawei.com>; ard.biesheuvel@linaro.org;
> leif@nuviainc.com
> Cc: devel@edk2.groups.io; Xiexiangyou <xiexiangyou@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 <miaoyubo@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 <miaoyubo@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