On 24 August 2016 at 17:01, Laszlo Ersek <ler...@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.biesheu...@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
