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

[miaoyubo <miaoyubo@huawei.com>]

> -----Original Message-----
> From: miaoyubo
> Sent: Friday, April 3, 2020 4:56 PM
> To: 'Laszlo Ersek' <lersek@redhat.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.
> 
> 
> 
> > -----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.
> >

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