[edk2,2/5] ArmVirtPkg: implement FdtPciHostBridgeLib

Message ID 1471847752-26574-3-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Aug. 22, 2016, 6:35 a.m.
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(+)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Aug. 24, 2016, 3:01 p.m. | #1
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Aug. 24, 2016, 3:19 p.m. | #2
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Aug. 31, 2016, 1:51 p.m. | #3
On 08/24/16 17:19, Ard Biesheuvel wrote:
> On 24 August 2016 at 17:01, Laszlo Ersek <lersek@redhat.com> wrote:

>> On 08/22/16 02:35, Ard Biesheuvel wrote:


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


Sounds good, thanks!

In this case, the IoTranslation variable should become local to
ProcessPciHost() -- and ProcessPciHost() only -- in this patch, plus I
think we should also add

  ASSERT (PcdGet64 (PcdPciIoTranslation) == IoTranslation);

to ProcessPciHost() here, under the DTB_PCI_HOST_RANGE_IO branch.

Furthermore, the zero-assignment of the now-local IoTranslation variable
should probably be moved out from under the
'-Werror=maybe-uninitialized' comment, because it won't be an output
parameter any longer. What do you think?

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Aug. 31, 2016, 1:54 p.m. | #4
On 31 August 2016 at 14:51, Laszlo Ersek <lersek@redhat.com> wrote:
> On 08/24/16 17:19, Ard Biesheuvel wrote:

>> On 24 August 2016 at 17:01, Laszlo Ersek <lersek@redhat.com> wrote:

>>> On 08/22/16 02:35, Ard Biesheuvel wrote:

>

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

>

> Sounds good, thanks!

>

> In this case, the IoTranslation variable should become local to

> ProcessPciHost() -- and ProcessPciHost() only -- in this patch, plus I

> think we should also add

>

>   ASSERT (PcdGet64 (PcdPciIoTranslation) == IoTranslation);

>

> to ProcessPciHost() here, under the DTB_PCI_HOST_RANGE_IO branch.

>

> Furthermore, the zero-assignment of the now-local IoTranslation variable

> should probably be moved out from under the

> '-Werror=maybe-uninitialized' comment, because it won't be an output

> parameter any longer. What do you think?

>


Yes, that was my idea as well. The FdtPciPcdProducerLib will need some
work, but it is ultimately a much cleaner approach.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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
+    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;
+}
+
+
+/**
+  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,
+      "%a: failed to discover PCI host bridge: %s not present\n",
+      __FUNCTION__, Status));
+    *Count = 0;
+    return NULL;
+  }
+
+  PcdSet64 (PcdPciIoTranslation, IoTranslation);
+
+  *Count = 1;
+  RootBridge = AllocateZeroPool (*Count * sizeof *RootBridge);
+
+  RootBridge->Segment     = 0;
+  RootBridge->Supports    = 0;
+  RootBridge->Attributes  = RootBridge->Supports;
+
+  RootBridge->DmaAbove4G  = TRUE;
+
+  RootBridge->AllocationAttributes  = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM |
+                                      EFI_PCI_HOST_BRIDGE_MEM64_DECODE ;
+
+  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;
+
+  //
+  // 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);
+
+  RootBridge->NoExtendedConfigSpace = FALSE;
+
+  RootBridge->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath;
+
+  return RootBridge;
+}
+
+/**
+  Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
+
+  @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);
+}
+
+/**
+  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
+                   );
+  }
+}
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
+  FILE_GUID                      = 05E7AB83-EF8D-482D-80F8-905B73377A15
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = PciHostBridgeLib
+
+#
+# The following information is for reference only and not required by the build
+# tools.
+#
+#  VALID_ARCHITECTURES           = AARCH64
+#
+
+[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