Re: [PATCH 2/5] ArmVirtPkg: implement FdtPciHostBridgeLib

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 24 August 2016 at 17:01, Laszlo Ersek <lersek@redhat.com> wrote:
> On 08/22/16 02:35, Ard Biesheuvel wrote:
>> Implement PciHostBridgeLib for DT platforms that expose a PCI root bridge
>> via a pci-host-ecam-generic DT node. The DT parsing logic is copied from
>> the PciHostBridgeDxe implementation in ArmVirtPkg, with the one notable
>> difference that we don't set the various legacy PCI attributes for IDE
>> and VGA I/O ranges.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c   | 400 ++++++++++++++++++++
>>  ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf |  56 +++
>>  2 files changed, 456 insertions(+)
>>
>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> new file mode 100644
>> index 000000000000..887ddb01f586
>> --- /dev/null
>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>> @@ -0,0 +1,400 @@
>> +/** @file
>> +  PCI Host Bridge Library instance for pci-ecam-generic DT nodes
>> +
>> +  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>> +
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php.
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
>> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +#include <PiDxe.h>
>> +#include <Library/PciHostBridgeLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/DevicePathLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +
>> +#include <Protocol/FdtClient.h>
>> +#include <Protocol/PciRootBridgeIo.h>
>> +#include <Protocol/PciHostBridgeResourceAllocation.h>
>> +
>> +#pragma pack(1)
>> +typedef struct {
>> +  ACPI_HID_DEVICE_PATH     AcpiDevicePath;
>> +  EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
>> +} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
>> +#pragma pack ()
>> +
>> +STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = {
>> +  {
>> +    {
>> +      ACPI_DEVICE_PATH,
>> +      ACPI_DP,
>> +      {
>> +        (UINT8) (sizeof(ACPI_HID_DEVICE_PATH)),
>> +        (UINT8) ((sizeof(ACPI_HID_DEVICE_PATH)) >> 8)
>> +      }
>> +    },
>> +    EISA_PNP_ID(0x0A08), // PCI Express
>
> Differs from 0x0A03, but this change is valid -- not only because we
> have PCI Express here, but also because QEMU's ACPI generator produces
> PNP0A08 as _HID (and PNP0A03 only as _CID) for PCI0, in acpi_dsdt_add_pci().
>
>> +    0
>> +  },
>> +
>> +  {
>> +    END_DEVICE_PATH_TYPE,
>> +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> +    {
>> +      END_DEVICE_PATH_LENGTH,
>> +      0
>> +    }
>> +  }
>> +};
>> +
>> +GLOBAL_REMOVE_IF_UNREFERENCED
>> +CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
>> +  L"Mem", L"I/O", L"Bus"
>> +};
>> +
>> +//
>> +// We expect the "ranges" property of "pci-host-ecam-generic" to consist of
>> +// records like this.
>> +//
>> +#pragma pack (1)
>> +typedef struct {
>> +  UINT32 Type;
>> +  UINT64 ChildBase;
>> +  UINT64 CpuBase;
>> +  UINT64 Size;
>> +} DTB_PCI_HOST_RANGE_RECORD;
>> +#pragma pack ()
>> +
>> +#define DTB_PCI_HOST_RANGE_RELOCATABLE  BIT31
>> +#define DTB_PCI_HOST_RANGE_PREFETCHABLE BIT30
>> +#define DTB_PCI_HOST_RANGE_ALIASED      BIT29
>> +#define DTB_PCI_HOST_RANGE_MMIO32       BIT25
>> +#define DTB_PCI_HOST_RANGE_MMIO64       (BIT25 | BIT24)
>> +#define DTB_PCI_HOST_RANGE_IO           BIT24
>> +#define DTB_PCI_HOST_RANGE_TYPEMASK     (BIT31 | BIT30 | BIT29 | BIT25 | BIT24)
>> +
>> +STATIC
>> +EFI_STATUS
>> +ProcessPciHost (
>> +  OUT  UINT64    *IoBase,
>> +  OUT  UINT64    *IoSize,
>> +  OUT  UINT64    *IoTranslation,
>> +  OUT  UINT64    *MmioBase,
>> +  OUT  UINT64    *MmioSize,
>> +  OUT  UINT64    *MmioTranslation,
>> +  OUT  UINT32    *BusMin,
>> +  OUT  UINT32    *BusMax
>> +  )
>> +{
>> +  FDT_CLIENT_PROTOCOL         *FdtClient;
>> +  INT32                       Node;
>> +  UINT64                      ConfigBase, ConfigSize;
>> +  CONST VOID                  *Prop;
>> +  UINT32                      Len;
>> +  UINT32                      RecordIdx;
>> +  EFI_STATUS                  Status;
>> +
>> +  //
>> +  // The following output arguments are initialized only in
>> +  // order to suppress '-Werror=maybe-uninitialized' warnings
>> +  // *incorrectly* emitted by some gcc versions.
>> +  //
>> +  *IoBase = 0;
>> +  *IoTranslation = 0;
>> +  *MmioBase = 0;
>> +  *MmioTranslation = 0;
>> +  *BusMin = 0;
>> +  *BusMax = 0;
>> +
>> +  //
>> +  // *IoSize and *MmioSize are initialized to zero because the logic below
>> +  // requires it. However, since they are also affected by the issue reported
>> +  // above, they are initialized early.
>> +  //
>> +  *IoSize = 0;
>> +  *MmioSize = 0;
>> +
>> +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
>> +                  (VOID **)&FdtClient);
>> +  ASSERT_EFI_ERROR (Status);
>> +
>> +  Status = FdtClient->FindCompatibleNode (FdtClient, "pci-host-ecam-generic",
>> +                        &Node);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_INFO,
>> +      "%a: No 'pci-host-ecam-generic' compatible DT node found\n",
>> +      __FUNCTION__));
>> +    return EFI_NOT_FOUND;
>> +  }
>> +
>> +  DEBUG_CODE (
>> +    INT32 Tmp;
>> +
>> +    //
>> +    // A DT can legally describe multiple PCI host bridges, but we are not
>> +    // equipped to deal with that. So assert that there is only one.
>> +    //
>> +    Status = FdtClient->FindNextCompatibleNode (FdtClient,
>> +                          "pci-host-ecam-generic", Node, &Tmp);
>> +    ASSERT (Status == EFI_NOT_FOUND);
>> +  );
>> +
>> +  Status = FdtClient->GetNodeProperty (FdtClient, Node, "reg", &Prop, &Len);
>> +  if (EFI_ERROR (Status) || Len != 2 * sizeof (UINT64)) {
>> +    DEBUG ((EFI_D_ERROR, "%a: 'reg' property not found or invalid\n",
>> +      __FUNCTION__));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  //
>> +  // Fetch the ECAM window.
>> +  //
>> +  ConfigBase = SwapBytes64 (((CONST UINT64 *)Prop)[0]);
>> +  ConfigSize = SwapBytes64 (((CONST UINT64 *)Prop)[1]);
>> +
>> +  //
>> +  // Fetch the bus range (note: inclusive).
>> +  //
>> +  Status = FdtClient->GetNodeProperty (FdtClient, Node, "bus-range", &Prop,
>> +                        &Len);
>> +  if (EFI_ERROR (Status) || Len != 2 * sizeof (UINT32)) {
>> +    DEBUG ((EFI_D_ERROR, "%a: 'bus-range' not found or invalid\n",
>> +      __FUNCTION__));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +  *BusMin = SwapBytes32 (((CONST UINT32 *)Prop)[0]);
>> +  *BusMax = SwapBytes32 (((CONST UINT32 *)Prop)[1]);
>> +
>> +  //
>> +  // Sanity check: the config space must accommodate all 4K register bytes of
>> +  // all 8 functions of all 32 devices of all buses.
>> +  //
>> +  if (*BusMax < *BusMin || *BusMax - *BusMin == MAX_UINT32 ||
>> +      DivU64x32 (ConfigSize, SIZE_4KB * 8 * 32) < *BusMax - *BusMin + 1) {
>> +    DEBUG ((EFI_D_ERROR, "%a: invalid 'bus-range' and/or 'reg'\n",
>> +      __FUNCTION__));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  //
>> +  // Iterate over "ranges".
>> +  //
>> +  Status = FdtClient->GetNodeProperty (FdtClient, Node, "ranges", &Prop, &Len);
>> +  if (EFI_ERROR (Status) || Len == 0 ||
>> +      Len % sizeof (DTB_PCI_HOST_RANGE_RECORD) != 0) {
>> +    DEBUG ((EFI_D_ERROR, "%a: 'ranges' not found or invalid\n", __FUNCTION__));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  for (RecordIdx = 0; RecordIdx < Len / sizeof (DTB_PCI_HOST_RANGE_RECORD);
>> +       ++RecordIdx) {
>> +    CONST DTB_PCI_HOST_RANGE_RECORD *Record;
>> +
>> +    Record = (CONST DTB_PCI_HOST_RANGE_RECORD *)Prop + RecordIdx;
>> +    switch (SwapBytes32 (Record->Type) & DTB_PCI_HOST_RANGE_TYPEMASK) {
>> +    case DTB_PCI_HOST_RANGE_IO:
>> +      *IoBase = SwapBytes64 (Record->ChildBase);
>> +      *IoSize = SwapBytes64 (Record->Size);
>> +      *IoTranslation = SwapBytes64 (Record->CpuBase) - *IoBase;
>> +      break;
>> +
>> +    case DTB_PCI_HOST_RANGE_MMIO32:
>> +      *MmioBase = SwapBytes64 (Record->ChildBase);
>> +      *MmioSize = SwapBytes64 (Record->Size);
>> +      *MmioTranslation = SwapBytes64 (Record->CpuBase) - *MmioBase;
>> +
>> +      if (*MmioBase > MAX_UINT32 || *MmioSize > MAX_UINT32 ||
>> +          *MmioBase + *MmioSize > SIZE_4GB) {
>> +        DEBUG ((EFI_D_ERROR, "%a: MMIO32 space invalid\n", __FUNCTION__));
>> +        return EFI_PROTOCOL_ERROR;
>> +      }
>> +
>> +      if (*MmioTranslation != 0) {
>> +        DEBUG ((EFI_D_ERROR, "%a: unsupported nonzero MMIO32 translation "
>> +          "0x%Lx\n", __FUNCTION__, *MmioTranslation));
>> +        return EFI_UNSUPPORTED;
>> +      }
>> +
>> +      break;
>> +    }
>> +  }
>> +  if (*IoSize == 0 || *MmioSize == 0) {
>> +    DEBUG ((EFI_D_ERROR, "%a: %a space empty\n", __FUNCTION__,
>> +      (*IoSize == 0) ? "IO" : "MMIO32"));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  //
>> +  // The dynamic PCD PcdPciExpressBaseAddress should have already been set,
>> +  // and should match the value we found in the DT node.
>> +  //
>> +  ASSERT (PcdGet64 (PcdPciExpressBaseAddress) == ConfigBase);
>> +
>> +  DEBUG ((EFI_D_INFO, "%a: Config[0x%Lx+0x%Lx) Bus[0x%x..0x%x] "
>> +    "Io[0x%Lx+0x%Lx)@0x%Lx Mem[0x%Lx+0x%Lx)@0x%Lx\n", __FUNCTION__, ConfigBase,
>> +    ConfigSize, *BusMin, *BusMax, *IoBase, *IoSize, *IoTranslation, *MmioBase,
>> +    *MmioSize, *MmioTranslation));
>> +  return EFI_SUCCESS;
>> +}
>
> Okay, the above seems to be preserved identically from
> "ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c".
>

Yes. That is why I decided to remove the linux,pci-probe-only hack first.

>> +/**
>> +  Return all the root bridge instances in an array.
>> +
>> +  @param Count  Return the count of root bridge instances.
>> +
>> +  @return All the root bridge instances in an array.
>> +          The array should be passed into PciHostBridgeFreeRootBridges()
>> +          when it's not used.
>> +**/
>> +PCI_ROOT_BRIDGE *
>> +EFIAPI
>> +PciHostBridgeGetRootBridges (
>> +  UINTN *Count
>> +  )
>> +{
>> +  PCI_ROOT_BRIDGE     *RootBridge;
>> +  UINT64              IoBase, IoSize, IoTranslation;
>> +  UINT64              Mmio32Base, Mmio32Size, Mmio32Translation;
>> +  UINT32              BusMin, BusMax;
>> +  EFI_STATUS          Status;
>> +
>> +  if (PcdGet64 (PcdPciExpressBaseAddress) == 0) {
>> +    DEBUG ((EFI_D_INFO, "%a: PCI host bridge not present\n", __FUNCTION__));
>> +
>> +    *Count = 0;
>> +    return NULL;
>> +  }
>> +
>> +  Status = ProcessPciHost (&IoBase, &IoSize, &IoTranslation, &Mmio32Base,
>> +             &Mmio32Size, &Mmio32Translation, &BusMin, &BusMax);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_INFO,
>
> I propose to turn this into EFI_D_ERROR. At this point
> FdtPciPcdProducerLib has located "pci-host-ecam-generic" and set
> PcdPciExpressBaseAddress to a nonzero value, so any failure to process
> the FDT node correctly should be reported on the ERROR level.
>

Agreed.

>> +      "%a: failed to discover PCI host bridge: %s not present\n",
>> +      __FUNCTION__, Status));
>
> The "%s" format spec takes a CHAR16* argument, but Status has type
> EFI_STATUS.
>
> I would replace
>
>   %s not present
>
> with
>
>   %r
>
> in the format string.
>

Ah yes, I fudged that. Will fix.

>> +    *Count = 0;
>> +    return NULL;
>> +  }
>> +
>> +  PcdSet64 (PcdPciIoTranslation, IoTranslation);
>
> I agree this is necessary, but it's not in the right place, plus as-is,
> it is not sufficient.
>
> In <https://tianocore.acgmultimedia.com/show_bug.cgi?id=65#c0> I wrote
> -- and I'm slightly reformatting it here, because the formatting got
> lost in the GitHub -> BZ migration --:
>
>     The main customization in ArmVirtPkg/PciHostBridgeDxe is that it
>     emulates IO port accesses with an MMIO range, whose base address is
>     parsed from the DTB.
>
>     The core PciHostBridgeDxe driver delegates the IO port access
>     implementation to EFI_CPU_IO2_PROTOCOL. @ardbiesheuvel recently
>     implemented ArmPkg/Drivers/ArmPciCpuIo2Dxe (in commit 3dece14)
>     which provides this protocol, backed by the same kind of
>     translation as described above. The base address comes from
>     gArmTokenSpaceGuid.PcdPciIoTranslation.
>
>     Therefore:
>
>     (1) We should extend our ArmVirtPkg/Library/FdtPciPcdProducerLib
>         plugin library to locate the IO translation offset in the DTB,
>         and store it in the above PCD.
>
>     (2) We should include ArmPciCpuIo2Dxe in the ArmVirtQemu builds,
>         plugging the above library into it.
>
>     (3) We should implement a PciHostBridgeLib instance, and plug it
>         into the core PciHostBridgeDxe driver.
>
>         We should create one PCI_ROOT_BRIDGE object, populating it with
>         the FTD Client code we are currently using in
>         ArmVirtPkg/PciHostBridgeDxe.
>
>     (4) Extra benefit: the core PciHostBridgeDxe driver supports 64-bit
>         MMIO, we just have to parse those values from the DTB as well.
>
> Steps (2) through (4) are implemented by this series, but I don't think
> the above PcdSet64() call satisfies (1). What guarantees that by the
> time ArmPciCpuIo2Dxe looks at it, PcdPciIoTranslation will be set?
>
> ... Especially because PciHostBridgeDxe.inf has a DEPEX on
> gEfiCpuIo2ProtocolGuid, so the PcdSet64() above is bound to run *after*
> ArmPciCpuIo2Dxe is dispatched?
>
> Hmmmm... Are you making the argument that the PCD is set *between*
> ArmPciCpuIo2Dxe's dispatch and actual use of the PCD? That is:
>
> - ArmPciCpuIo2Dxe is dispatched
> - PciHostBridgeDxe is dispatched
> - we set the PCD in this lib (as part of PciHostBridgeDxe's startup)
> - (much later) something makes a PCI IO or MEM access that causes
>   PciHostBridgeDxe to talk to ArmPciCpuIo2Dxe
> - ArmPciCpuIo2Dxe consumes the PCD
>
> I think this is a valid argument to make -- I've even checked:
> ArmPciCpuIo2Dxe indeed performs a PcdGet64() on every CpuIoServiceRead()
> / CpuIoServiceWrite(), and none in its entry point --, but it has to be
> explained in detail.
>
> This borders on a hack -- because ArmPciCpuIo2Dxe is not actually ready
> for use when its entry point exits with success --, but I think we can
> allow it, both ArmPciCpuIo2Dxe and this lib being platform specific, as
> long as we document it profusely.
>
> So please describe this quirk in the commit message, and add a large
> comment right above the PcdSet64() call.
>
> (Also, did you test this series with std VGA, on TCG? That will actually
> put the IO translation to use.)
>
> The alternative to the commit msg addition / code comment would be my
> suggestion in (1), that is, to extend FdtPciPcdProducerLib with setting
> PcdPciIoTranslation, and to link it into ArmPciCpuIo2Dxe via NULL
> library resolution. I guess you don't like that, which is fine, but then
> please add the docs. Thanks.
>

Actually, I prefer your original suggestion, to add it to
FdtPciPcdProducerLib. That way, we no longer have to set any PCDs in
this code, which is much cleaner imo

>
> Independent request (just because I referenced the edk2 BZ above):
> please add something like this to all of the commit messages:
>
> Ref: https://tianocore.acgmultimedia.com/show_bug.cgi?id=65
>

Sure

>> +
>> +  *Count = 1;
>> +  RootBridge = AllocateZeroPool (*Count * sizeof *RootBridge);
>
> Please check for memory allocation failure here, and return in the usual
> way if it fails.
>
> Alternatively, you can do the exact same thing as with the device path:
> give this singleton root bridge object static storage duration, and make
> PciHostBridgeFreeRootBridges() a no-op.
>

For count == 1, that makes sense of course, and we currently don't
deal with anything else. I am not sure how likely that is to change,
though? I think it has been suggested to allow PCI pass through of
non-DMA coherent PCI devices using a separate PCI host, but I am not
sure if that is ever going to materialize, especially since host
systems that have an SMMU wired to their PCI rc (which I suppose is a
requirement for PCI pass through in real-world scenarios) should be
able to perform coherent DMA

>> +
>> +  RootBridge->Segment     = 0;
>> +  RootBridge->Supports    = 0;
>> +  RootBridge->Attributes  = RootBridge->Supports;
>
> Hmmm, not so sure about this. This change breaks away from what we
> currently have in "ArmVirtPkg/PciHostBridgeDxe/PciRootBridgeIo.c",
> function RootBridgeConstructor().
>
> The attributes are documented in the UEFI spec (v2.6), "13.2 PCI Root
> Bridge I/O Protocol". We do support IO emulation, so I think at least
> some of those attributes should be preserved. Honestly, given that we
> intend to support std VGA on at least TCG, I would keep all the current
> attributes, except the IDE ones. Hmmm, I'd also drop
> EFI_PCI_ATTRIBUTE_VGA_MEMORY. So, I'd keep:
>
>   EFI_PCI_ATTRIBUTE_ISA_IO_16 |
>   EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO |
>   EFI_PCI_ATTRIBUTE_VGA_IO_16  |
>   EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16
>
> According to your commit message, these were dropped deliberately. Can
> you please elaborate why we shouldn't keep the above four flags, despite
> wanting to support stdvga on TCG? For now I think we should preserve
> them, and the commit message should be updated.
>

I simply thought we didn't need any of them, but I am happy to keep
all of them, or just the ones above.

>> +
>> +  RootBridge->DmaAbove4G  = TRUE;
>
> OK, I think.
>
> ... Here you skip setting NoExtendedConfigSpace explicitly to FALSE. I
> think that's fine (due to AllocateZeroPool() above, or else due to the
> automatic zero-init of the (proposed) static variable), functionally
> speaking. OK.
>
> Same for ResourceAssigned -- defaults to FALSE, good.
>
>> +
>> +  RootBridge->AllocationAttributes  = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM |
>> +                                      EFI_PCI_HOST_BRIDGE_MEM64_DECODE ;
>
> Here the EFI_PCI_HOST_BRIDGE_MEM64_DECODE bit belongs to patch #4 ("add
> MMIO64 support"), not here, in my opinion. It should depend on whether
> the FDT provides a 64-bit MMIO range.
>

OK, will fix that

> (Also, gratuitous whitespace before the semicolon.)
>
>> +
>> +  RootBridge->Bus.Base              = BusMin;
>> +  RootBridge->Bus.Limit             = BusMax;
>> +  RootBridge->Io.Base               = IoBase;
>> +  RootBridge->Io.Limit              = IoBase + IoSize - 1;
>> +  RootBridge->Mem.Base              = Mmio32Base;
>> +  RootBridge->Mem.Limit             = Mmio32Base + Mmio32Size - 1;
>> +  RootBridge->MemAbove4G.Base       = 0x100000000ULL;
>> +  RootBridge->MemAbove4G.Limit      = 0xFFFFFFFF;
>
> This is valid for disabling the 64-bit MMIO range (temporarily), but
> it's not consistent with how the prefetchable stuff is disabled below
> (with MAX_UINT64 and 0). I suspect this is going to be modified in patch
> #4 anyway, but I suggest to keep the consistency here.
>

Actually, I fixed that up but folded the hunk into the wrong patch.

>> +
>> +  //
>> +  // No separate ranges for prefetchable and non-prefetchable BARs
>> +  //
>> +  RootBridge->PMem.Base             = MAX_UINT64;
>> +  RootBridge->PMem.Limit            = 0;
>> +  RootBridge->PMemAbove4G.Base      = MAX_UINT64;
>> +  RootBridge->PMemAbove4G.Limit     = 0;
>> +
>> +  ASSERT (FixedPcdGet64 (PcdPciMmio32Translation) == 0);
>> +  ASSERT (FixedPcdGet64 (PcdPciMmio64Translation) == 0);
>
> OK.
>
>> +
>> +  RootBridge->NoExtendedConfigSpace = FALSE;
>
> Ah! So here it is. How about moving it up to where the structure
> definition would suggest it? Not too important, of course.
>
>> +
>> +  RootBridge->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath;
>> +
>> +  return RootBridge;
>> +}
>
> Looks good.
>
>> +
>> +/**
>> +  Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
>
> This line is too long. Doesn't justify a repost on its own, but if you
> repost for other reasons, it would be nice to wrap this.
>
>> +
>> +  @param Bridges The root bridge instances array.
>> +  @param Count   The count of the array.
>> +**/
>> +VOID
>> +EFIAPI
>> +PciHostBridgeFreeRootBridges (
>> +  PCI_ROOT_BRIDGE *Bridges,
>> +  UINTN           Count
>> +  )
>> +{
>> +  FreePool (Bridges);
>> +}
>
> Good if we stick with AllocateZeroPool() -- augmented with error
> checking --; otherwise, if you opt for the static object, this should
> become an empty function.
>
>> +
>> +/**
>> +  Inform the platform that the resource conflict happens.
>> +
>> +  @param HostBridgeHandle Handle of the Host Bridge.
>> +  @param Configuration    Pointer to PCI I/O and PCI memory resource
>> +                          descriptors. The Configuration contains the resources
>> +                          for all the root bridges. The resource for each root
>> +                          bridge is terminated with END descriptor and an
>> +                          additional END is appended indicating the end of the
>> +                          entire resources. The resource descriptor field
>> +                          values follow the description in
>> +                          EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL
>> +                          .SubmitResources().
>> +**/
>> +VOID
>> +EFIAPI
>> +PciHostBridgeResourceConflict (
>> +  EFI_HANDLE                        HostBridgeHandle,
>> +  VOID                              *Configuration
>> +  )
>> +{
>> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
>> +  UINTN                             RootBridgeIndex;
>> +  DEBUG ((EFI_D_ERROR, "PciHostBridge: Resource conflict happens!\n"));
>> +
>> +  RootBridgeIndex = 0;
>> +  Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
>> +  while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
>> +    DEBUG ((EFI_D_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
>> +    for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
>> +      ASSERT (Descriptor->ResType <
>> +              (sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr) /
>> +               sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr[0])
>> +               )
>> +              );
>> +      DEBUG ((EFI_D_ERROR, " %s: Length/Alignment = 0x%lx / 0x%lx\n",
>> +              mPciHostBridgeLibAcpiAddressSpaceTypeStr[Descriptor->ResType],
>> +              Descriptor->AddrLen, Descriptor->AddrRangeMax
>> +              ));
>> +      if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
>> +        DEBUG ((EFI_D_ERROR, "     Granularity/SpecificFlag = %ld / %02x%s\n",
>> +                Descriptor->AddrSpaceGranularity, Descriptor->SpecificFlag,
>> +                ((Descriptor->SpecificFlag &
>> +                  EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE
>> +                  ) != 0) ? L" (Prefetchable)" : L""
>> +                ));
>> +      }
>> +    }
>> +    //
>> +    // Skip the END descriptor for root bridge
>> +    //
>> +    ASSERT (Descriptor->Desc == ACPI_END_TAG_DESCRIPTOR);
>> +    Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)(
>> +                   (EFI_ACPI_END_TAG_DESCRIPTOR *)Descriptor + 1
>> +                   );
>> +  }
>> +}
>
> Okay, this function is directly copied from
> "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c".
>
>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>> new file mode 100644
>> index 000000000000..16beef0c2425
>> --- /dev/null
>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
>> @@ -0,0 +1,56 @@
>> +## @file
>> +#  PCI Host Bridge Library instance for pci-ecam-generic DT nodes
>> +#
>> +#  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>> +#
>> +#  This program and the accompanying materials are licensed and made available
>> +#  under the terms and conditions of the BSD License which accompanies this
>> +#  distribution. The full text of the license may be found at
>> +#  http://opensource.org/licenses/bsd-license.php
>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> +#  IMPLIED.
>> +#
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
>> +  BASE_NAME                      = AmdStyxPciHostBridgeLib
>
> The BASE_NAME is wrong.
>

Oops

>> +  FILE_GUID                      = 05E7AB83-EF8D-482D-80F8-905B73377A15
>> +  MODULE_TYPE                    = BASE
>
> I think this should be DXE_DRIVER, for consistency. The parameter list
> of the entry point function is irrelevant here, but in a BASE type
> library, we are supposed to use RETURN_STATUS, not EFI_STATUS (although
> those types and their values are identical). The C file added above uses
> EFI_STATUS in a bunch of places, so I'd make the module type DXE_DRIVER,
> for consistency.
>

Sure

>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = PciHostBridgeLib
>> +
>> +#
>> +# The following information is for reference only and not required by the build
>> +# tools.
>> +#
>> +#  VALID_ARCHITECTURES           = AARCH64
>
> Not to be used in the ARM build of ArmVirtQemu?
>

Actually, it should but I haven't tried.

>> +#
>> +
>> +[Sources]
>> +  FdtPciHostBridgeLib.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  ArmPkg/ArmPkg.dec
>> +  ArmVirtPkg/ArmVirtPkg.dec
>> +
>> +[LibraryClasses]
>> +  DebugLib
>> +  DevicePathLib
>> +  MemoryAllocationLib
>> +  PciPcdProducerLib
>> +
>> +[FixedPcd]
>> +  gArmTokenSpaceGuid.PcdPciMmio32Translation
>> +  gArmTokenSpaceGuid.PcdPciMmio64Translation
>> +
>> +[Pcd]
>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> +  gArmTokenSpaceGuid.PcdPciIoTranslation
>> +
>> +[Depex]
>> +  gFdtClientProtocolGuid
>>
>
> Thanks!
> Laszlo