public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: edk2-devel@ml01.01.org
Subject: Re: [PATCH 2/5] ArmVirtPkg: implement FdtPciHostBridgeLib
Date: Wed, 24 Aug 2016 11:01:17 -0400	[thread overview]
Message-ID: <8ba58ec8-9360-4805-c9a5-b9d5c193fdb0@redhat.com> (raw)
In-Reply-To: <1471847752-26574-3-git-send-email-ard.biesheuvel@linaro.org>

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".

> +/**
> +  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.

> +      "%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.

> +    *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.


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

> +
> +  *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.

> +
> +  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.

> +
> +  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.

(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.

> +
> +  //
> +  // 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.

> +  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.

> +  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?

> +#
> +
> +[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


  reply	other threads:[~2016-08-24 15:01 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-22  6:35 [PATCH 0/5] ArmVirtQemu: move to generic PciHostBridgeDxe Ard Biesheuvel
2016-08-22  6:35 ` [PATCH 1/5] ArmVirtPkg/PciHostBridgeDxe: don't set linux, pci-probe-only DT property Ard Biesheuvel
2016-08-23 21:23   ` Laszlo Ersek
2016-08-24  5:30     ` Ard Biesheuvel
2016-08-22  6:35 ` [PATCH 2/5] ArmVirtPkg: implement FdtPciHostBridgeLib Ard Biesheuvel
2016-08-24 15:01   ` Laszlo Ersek [this message]
2016-08-24 15:19     ` Ard Biesheuvel
2016-08-31 13:51       ` Laszlo Ersek
2016-08-31 13:54         ` Ard Biesheuvel
2016-08-22  6:35 ` [PATCH 3/5] ArmVirtPkg/ArmVirtQemu: switch to generic PciHostBridgeDxe Ard Biesheuvel
2016-08-23 18:04   ` Ard Biesheuvel
2016-08-31 12:24     ` Laszlo Ersek
2016-08-22  6:35 ` [PATCH 4/5] ArmVirtPkg/FdtPciHostBridgeLib: add MMIO64 support Ard Biesheuvel
2016-08-31 14:06   ` Laszlo Ersek
2016-08-22  6:35 ` [PATCH 5/5] ArmVirtPkg: remove now unused PciHostBridgeDxe Ard Biesheuvel
2016-08-31 14:11   ` Laszlo Ersek
2016-08-22 11:58 ` [PATCH 0/5] ArmVirtQemu: move to generic PciHostBridgeDxe Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8ba58ec8-9360-4805-c9a5-b9d5c193fdb0@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox